Replace trending tiles after reaching impression threshold

This CL will allow the server to send more trending tiles to the client, and
client can cache those tiles to replace existing trending tiles if they are
not being interacted after some time.
By default, 2 trending tiles will be shown. If a trending tile is shown twice,
it will be removed if no activity is recorded.

Bug: 1154456
Change-Id: I7bf304b56b86e92ad51446bf82427e24dbd810d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2568224Reviewed-by: Xing Liu <xingliu@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833419}

components/query_tiles/internal/tile_config.cc

@@ -53,6 +53,11 @@ constexpr char kTileScoreDecayLambdaKey[] = "tile_score_decay_lambda";
 constexpr char kMinimumScoreForNewFrontTilesKey[] =
     "min_score_for_new_front_tiles";
 
+constexpr char kNumTrendingTilesKey[] = "num_trending_tiles_to_display";
+
+constexpr char kMaxTrendingTileImpressionsKey[] =
+    "max_trending_tile_impressions";
+
 // Default expire duration.
 constexpr int kDefaultExpireDurationInSeconds = 48 * 60 * 60;  // 2 days.
 
@@ -80,6 +85,12 @@ constexpr double kDefaultTileScoreDecayLambda = -0.099;
 // clicked for 2 days.
 constexpr double kDefaultMinimumTileScoreForNewFrontTiles = 0.9;
 
+// Default number of trending tiles to be displayed at the same time.
+constexpr int kDefaultNumTrendingTilesToDisplay = 2;
+
+// Default number of impressions a trending tile to be displayed .
+constexpr int kDefaultMaxTrendingTileImpressions = 2;
+
 namespace {
 
 // For testing. Json string for single tier experiment tag.
@@ -94,6 +105,7 @@ const GURL BuildGetQueryTileURL(const GURL& base_url, const char* path) {
   replacements.SetPathStr(path);
   return base_url.ReplaceComponents(replacements);
 }
+
 }  // namespace
 
 // static
@@ -198,4 +210,18 @@ double TileConfig::GetMinimumScoreForNewFrontTiles() {
       kDefaultMinimumTileScoreForNewFrontTiles);
 }
 
+// static
+int TileConfig::GetNumTrendingTilesToDisplay() {
+  return base::GetFieldTrialParamByFeatureAsInt(
+      features::kQueryTiles, kNumTrendingTilesKey,
+      kDefaultNumTrendingTilesToDisplay);
+}
+
+// static
+int TileConfig::GetMaxTrendingTileImpressions() {
+  return base::GetFieldTrialParamByFeatureAsInt(
+      features::kQueryTiles, kMaxTrendingTileImpressionsKey,
+      kDefaultMaxTrendingTileImpressions);
+}
+
 }  // namespace query_tiles

components/query_tiles/internal/tile_config.h

@@ -54,6 +54,12 @@ extern const char kTileScoreDecayLambdaKey[];
 // front of others.
 extern const char kMinimumScoreForNewFrontTilesKey[];
 
+// Finch parameter key for number of trending tiles to display.
+extern const char kNumTrendingTilesKey[];
+
+// Finch parameter key for max number of trending tile impressions.
+extern const char kMaxTrendingTileImpressionsKey[];
+
 class TileConfig {
  public:
   // Gets the URL for the Query Tiles server.
@@ -97,6 +103,12 @@ class TileConfig {
 
   // Get the minimum scrore for newly showing tiles that are in front of others.
   static double GetMinimumScoreForNewFrontTiles();
+
+  // Get the number of trending tiles to be displayed at the same time.
+  static int GetNumTrendingTilesToDisplay();
+
+  // Get the maximum number of impressions for a trending tile to be displayed.
+  static int GetMaxTrendingTileImpressions();
 };
 
 }  // namespace query_tiles

components/query_tiles/internal/tile_group.cc

@@ -4,9 +4,11 @@
 
 #include "components/query_tiles/internal/tile_group.h"
 
+#include <set>
 #include <sstream>
 #include <utility>
 
+#include "components/query_tiles/internal/tile_iterator.h"
 #include "components/query_tiles/internal/tile_utils.h"
 
 namespace query_tiles {
@@ -27,6 +29,17 @@ void DeepCopyGroup(const TileGroup& input, TileGroup* output) {
   output->tile_stats = input.tile_stats;
 }
 
+// Removes |id| from |id_set|. Returns true if |id| is found, or false
+// otherwise.
+bool RemoveIdFromSet(std::set<std::string>* id_set, const std::string& id) {
+  const auto it = id_set->find(id);
+  if (it != id_set->end()) {
+    id_set->erase(it);
+    return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 TileGroup::TileGroup() = default;
@@ -76,4 +89,36 @@ std::string TileGroup::DebugString() {
   return out.str();
 }
 
+void TileGroup::RemoveTiles(const std::vector<std::string>& tile_ids) {
+  std::set<std::string> id_set(tile_ids.begin(), tile_ids.end());
+  std::queue<Tile*> tile_queue;
+  // Check if there are top level tiles to be removed.
+  for (auto iter = tiles.begin(); iter != tiles.end();) {
+    if (RemoveIdFromSet(&id_set, (*iter)->id)) {
+      iter = tiles.erase(iter);
+      if (id_set.empty())
+        return;
+    } else {
+      tile_queue.push(iter->get());
+      ++iter;
+    }
+  }
+
+  // Recursively check if there are sub tiles to be removed.
+  while (!tile_queue.empty()) {
+    Tile* tile = tile_queue.front();
+    tile_queue.pop();
+    for (auto it = tile->sub_tiles.begin(); it != tile->sub_tiles.end();) {
+      if (RemoveIdFromSet(&id_set, (*it)->id)) {
+        it = tile->sub_tiles.erase(it);
+        if (id_set.empty())
+          return;
+      } else {
+        tile_queue.push(it->get());
+        ++it;
+      }
+    }
+  }
+}
+
 }  // namespace query_tiles

components/query_tiles/internal/tile_group.h

@@ -32,6 +32,9 @@ struct TileGroup {
   // Called when a tile was clicked, need to recalculate |tile_stats|.
   void OnTileClicked(const std::string& tile_id);
 
+  // Remove a tile from |tiles| given by its ID.
+  void RemoveTiles(const std::vector<std::string>& tile_ids);
+
   // Unique id for the group.
   std::string id;
 

components/query_tiles/internal/tile_group_unittest.cc

@@ -81,6 +81,38 @@ TEST(TileGroupTest, OnTileClicked) {
   EXPECT_EQ(group.tile_stats["guid-1-2"].score, 1.5);
 }
 
+TEST(TileGroupTest, RemoveTiles) {
+  TileGroup group;
+  test::ResetTestGroup(&group);
+  EXPECT_EQ(group.tiles.size(), 3u);
+  EXPECT_EQ(group.tiles[2]->id, "guid-1-3");
+  EXPECT_FALSE(group.tiles[2]->sub_tiles.empty());
+
+  std::vector<std::string> tiles_to_remove;
+  tiles_to_remove.emplace_back("guid-1-1");
+  group.RemoveTiles(tiles_to_remove);
+  EXPECT_EQ(group.tiles.size(), 2u);
+  EXPECT_EQ(group.tiles[0]->id, "guid-1-2");
+  EXPECT_EQ(group.tiles[1]->id, "guid-1-3");
+
+  test::ResetTestGroup(&group);
+  tiles_to_remove.clear();
+  tiles_to_remove.emplace_back("guid-1-4");
+  group.RemoveTiles(tiles_to_remove);
+  EXPECT_EQ(group.tiles.size(), 3u);
+  EXPECT_EQ(group.tiles[2]->id, "guid-1-3");
+  EXPECT_TRUE(group.tiles[2]->sub_tiles.empty());
+
+  // Remove 2 tiles.
+  test::ResetTestGroup(&group);
+  tiles_to_remove.emplace_back("guid-1-1");
+  group.RemoveTiles(tiles_to_remove);
+  EXPECT_EQ(group.tiles.size(), 2u);
+  EXPECT_EQ(group.tiles[0]->id, "guid-1-2");
+  EXPECT_EQ(group.tiles[1]->id, "guid-1-3");
+  EXPECT_TRUE(group.tiles[1]->sub_tiles.empty());
+}
+
 }  // namespace
 
 }  // namespace query_tiles

components/query_tiles/internal/tile_manager.cc

@@ -28,6 +28,45 @@ namespace {
 // A special tile group for tile stats.
 constexpr char kTileStatsGroup[] = "tile_stats";
 
+// Class for handling trending tiles. It checks whether a
+// trending tile should be displayed, or removed. Construct
+// a new instance each time when the caller has a list of
+// trending tiles to be displayed.
+class TrendingTilesHandler {
+ public:
+  // Map between tile ID and tile impression.
+  using ImpressionMap = std::map<std::string, int>;
+  explicit TrendingTilesHandler(ImpressionMap* impression_map)
+      : impression_map_(impression_map) {}
+
+  // Add a new trending tile for display, returns true if
+  // allowed, or returns false otherwise.
+  bool AddTrendingTileForDisplay(const std::string& tile_id) {
+    if (base::FeatureList::IsEnabled(
+            features::kQueryTilesRemoveTrendingTilesAfterInactivity) &&
+        (*impression_map_)[tile_id] >=
+            TileConfig::GetMaxTrendingTileImpressions()) {
+      tiles_to_remove_.push_back(tile_id);
+      return false;
+    }
+
+    if (++trending_tiles_count_ > TileConfig::GetNumTrendingTilesToDisplay())
+      return false;
+
+    ++(*impression_map_)[tile_id];
+    return true;
+  }
+
+  const std::vector<std::string>& tiles_to_remove() const {
+    return tiles_to_remove_;
+  }
+
+ private:
+  std::vector<std::string> tiles_to_remove_;
+  int trending_tiles_count_ = 0;
+  ImpressionMap* impression_map_;
+};
+
 class TileManagerImpl : public TileManager {
  public:
   TileManagerImpl(std::unique_ptr<TileStore> store,
@@ -68,10 +107,19 @@ class TileManagerImpl : public TileManager {
     }
 
     std::vector<Tile> tiles;
-    for (const auto& tile : tile_group_->tiles)
+    TrendingTilesHandler handler(&tile_impressions_);
+    for (const auto& tile : tile_group_->tiles) {
+      if (IsTrendingTile(tile->id)) {
+        // Return a limited number of trending tiles, filter out the rest.
+        if (!handler.AddTrendingTileForDisplay(tile->id))
+          continue;
+      }
       tiles.emplace_back(*tile.get());
+    }
+
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::BindOnce(std::move(callback), std::move(tiles)));
+    RemoveTiles(handler.tiles_to_remove());
   }
 
   void GetTile(const std::string& tile_id, TileCallback callback) override {
@@ -87,10 +135,26 @@ class TileManagerImpl : public TileManager {
         }
       }
     }
-
     auto result_tile = result ? base::make_optional(*result) : base::nullopt;
+    TrendingTilesHandler handler(&tile_impressions_);
+    if (result_tile.has_value()) {
+      std::vector<std::unique_ptr<Tile>> tiles_to_display;
+      for (auto& tile : result_tile->sub_tiles) {
+        if (IsTrendingTile(tile->id)) {
+          // Return a limited number of trending subtiles, filter out the rest.
+          if (!handler.AddTrendingTileForDisplay(tile->id))
+            continue;
+        }
+        // Moving the tile to a temporary vector first, and will then
+        // use the temporary vector to replace |result_tile->sub_tiles|.
+        tiles_to_display.emplace_back(std::move(tile));
+      }
+      result_tile->sub_tiles = std::move(tiles_to_display);
+    }
+
+    RemoveTiles(handler.tiles_to_remove());
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(std::move(callback), result_tile));
+        FROM_HERE, base::BindOnce(std::move(callback), std::move(result_tile)));
   }
 
   TileGroupStatus PurgeDb() override {
@@ -245,6 +309,8 @@ class TileManagerImpl : public TileManager {
     // It's fine if |tile_stats_group_| is not saved, so no callback needs to
     // be passed to Update().
     store_->Update(kTileStatsGroup, *tile_stats_group_, base::DoNothing());
+
+    tile_impressions_.erase(tile_id);
   }
 
   void OnQuerySelected(const base::Optional<std::string>& parent_tile_id,
@@ -272,6 +338,17 @@ class TileManagerImpl : public TileManager {
     }
   }
 
+  void RemoveTiles(const std::vector<std::string>& tile_ids) {
+    if (!tile_group_)
+      return;
+
+    if (tile_ids.empty())
+      return;
+
+    tile_group_->RemoveTiles(tile_ids);
+    store_->Update(tile_group_->id, *tile_group_, base::DoNothing());
+  }
+
   // Indicates if the db is fully initialized, rejects calls if not.
   bool initialized_;
 
@@ -293,6 +370,10 @@ class TileManagerImpl : public TileManager {
   // the same language.
   std::string accept_languages_;
 
+  // Map tracking how many times tiles are requested.
+  // TODO(qinmin): move this to |tile_stats_group_|.
+  std::map<std::string, int> tile_impressions_;
+
   base::WeakPtrFactory<TileManagerImpl> weak_ptr_factory_{this};
 };
 

components/query_tiles/internal/tile_manager_unittest.cc

@@ -8,10 +8,12 @@
 
 #include "base/bind.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_clock.h"
 #include "base/test/task_environment.h"
 #include "components/query_tiles/internal/tile_config.h"
 #include "components/query_tiles/internal/tile_store.h"
+#include "components/query_tiles/switches.h"
 #include "components/query_tiles/test/test_utils.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -166,6 +168,15 @@ class TileManagerTest : public testing::Test {
     std::move(closure).Run();
   }
 
+  void OnTileClicked(const std::string& tile_id) {
+    EXPECT_CALL(*tile_store(), Update(_, _, _))
+        .WillOnce(Invoke([](const std::string& id, const TileGroup& group,
+                            MockTileStore::UpdateCallback callback) {
+          std::move(callback).Run(true);
+        }));
+    manager()->OnTileClicked(tile_id);
+  }
+
  protected:
   TileManager* manager() { return manager_.get(); }
   MockTileStore* tile_store() { return tile_store_; }
@@ -346,6 +357,101 @@ TEST_F(TileManagerTest, GetTileGroup) {
   EXPECT_TRUE(test::AreTileGroupsIdentical(*actual, expected));
 }
 
+// Check that the right number of trending tiles are returned.
+TEST_F(TileManagerTest, GetTilesWithTrendingTiles) {
+  EXPECT_CALL(*tile_store(), Update(_, _, _))
+      .WillOnce(Invoke([](const std::string& id, const TileGroup& group,
+                          MockTileStore::UpdateCallback callback) {
+        std::move(callback).Run(true);
+      }));
+  EXPECT_CALL(*tile_store(), Delete(_, _)).Times(0);
+  Init(TileGroupStatus::kNoTiles);
+
+  std::vector<std::unique_ptr<Tile>> tiles_to_save =
+      test::GetTestTrendingTileList();
+
+  std::vector<Tile> expected;
+  expected.emplace_back(*tiles_to_save[0].get());
+  expected.emplace_back(*tiles_to_save[1].get());
+
+  SaveTiles(std::move(tiles_to_save), TileGroupStatus::kSuccess);
+  GetTiles(std::move(expected));
+}
+
+// Check that the right number of trending subtiles are returned.
+TEST_F(TileManagerTest, GetSingleTileWithTrendingSubTiles) {
+  EXPECT_CALL(*tile_store(), Update(_, _, _))
+      .WillOnce(Invoke([](const std::string& id, const TileGroup& group,
+                          MockTileStore::UpdateCallback callback) {
+        std::move(callback).Run(true);
+      }));
+  EXPECT_CALL(*tile_store(), Delete(_, _)).Times(0);
+  Init(TileGroupStatus::kNoTiles);
+
+  auto parent_tile = std::make_unique<Tile>();
+  parent_tile->id = "parent";
+
+  parent_tile->sub_tiles = test::GetTestTrendingTileList();
+
+  base::Optional<Tile> parent_tile2 = base::make_optional(*parent_tile.get());
+  parent_tile2->sub_tiles.pop_back();
+
+  std::vector<std::unique_ptr<Tile>> tiles_to_save;
+  tiles_to_save.emplace_back(std::move(parent_tile));
+
+  SaveTiles(std::move(tiles_to_save), TileGroupStatus::kSuccess);
+  GetSingleTile("parent", std::move(parent_tile2));
+}
+
+// Check that trending tiles get removed after inactivity.
+TEST_F(TileManagerTest, TrendingTilesRemovedAfterInactivity) {
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      features::kQueryTilesRemoveTrendingTilesAfterInactivity);
+  EXPECT_CALL(*tile_store(), Update(_, _, _))
+      .WillOnce(Invoke([](const std::string& id, const TileGroup& group,
+                          MockTileStore::UpdateCallback callback) {
+        std::move(callback).Run(true);
+      }));
+  EXPECT_CALL(*tile_store(), Delete(_, _)).Times(0);
+  Init(TileGroupStatus::kNoTiles);
+
+  std::vector<std::unique_ptr<Tile>> tiles_to_save =
+      test::GetTestTrendingTileList();
+
+  std::vector<Tile> expected;
+  expected.emplace_back(*tiles_to_save[0].get());
+  expected.emplace_back(*tiles_to_save[1].get());
+  Tile trending_3 = *tiles_to_save[2].get();
+
+  SaveTiles(std::move(tiles_to_save), TileGroupStatus::kSuccess);
+  GetTiles(expected);
+
+  // Click the 2nd tile.
+  OnTileClicked("trending_2");
+  GetTiles(expected);
+
+  // The first tile will be removed due to inactivity and the third tile
+  // will be returned.
+  expected.erase(expected.begin());
+  expected.emplace_back(std::move(trending_3));
+  EXPECT_CALL(*tile_store(), Update(_, _, _))
+      .WillOnce(Invoke([](const std::string& id, const TileGroup& group,
+                          MockTileStore::UpdateCallback callback) {
+        std::move(callback).Run(true);
+      }));
+  GetTiles(expected);
+
+  // The 2nd tile will be removed due to inactivity.
+  expected.erase(expected.begin());
+  EXPECT_CALL(*tile_store(), Update(_, _, _))
+      .WillOnce(Invoke([](const std::string& id, const TileGroup& group,
+                          MockTileStore::UpdateCallback callback) {
+        std::move(callback).Run(true);
+      }));
+  GetTiles(expected);
+}
+
 }  // namespace
 
 }  // namespace query_tiles

components/query_tiles/internal/tile_utils.cc

@@ -5,6 +5,7 @@
 #include <algorithm>
 #include <limits>
 
+#include "base/strings/string_util.h"
 #include "components/query_tiles/internal/tile_config.h"
 #include "components/query_tiles/internal/tile_utils.h"
 
@@ -116,4 +117,8 @@ double CalculateTileScore(const TileStats& tile_stats,
              (current_time - tile_stats.last_clicked_time).InDaysFloored());
 }
 
+bool IsTrendingTile(const std::string& tile_id) {
+  return base::StartsWith(tile_id, "trending_");
+}
+
 }  // namespace query_tiles

components/query_tiles/internal/tile_utils.h

@@ -38,6 +38,9 @@ void SortTilesAndClearUnusedStats(std::vector<std::unique_ptr<Tile>>* tiles,
 // decay over time.
 double CalculateTileScore(const TileStats& tile_stats, base::Time current_time);
 
+// Checks whether a tile ID is for trending tile.
+bool IsTrendingTile(const std::string& tile_id);
+
 }  // namespace query_tiles
 
 #endif  // COMPONENTS_QUERY_TILES_INTERNAL_TILE_UTILS_H_

components/query_tiles/internal/tile_utils_unittest.cc

@@ -146,6 +146,13 @@ TEST(TileUtilsTest, CalculateTileScore) {
             exp(-0.099));
 }
 
+TEST(TileUtilsTest, IsTrendingTile) {
+  EXPECT_TRUE(IsTrendingTile("trending_news"));
+  EXPECT_FALSE(IsTrendingTile("Trending_news"));
+  EXPECT_FALSE(IsTrendingTile("trendingnews"));
+  EXPECT_FALSE(IsTrendingTile("news"));
+}
+
 }  // namespace
 
 }  // namespace query_tiles

components/query_tiles/switches.cc

@@ -18,6 +18,9 @@ const base::Feature kQueryTilesEnableQueryEditing{
     "QueryTilesEnableQueryEditing", base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kQueryTilesLocalOrdering{"QueryTilesLocalOrdering",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kQueryTilesRemoveTrendingTilesAfterInactivity{
+    "QueryTilesRemoveTrendingAfterInactivity",
+    base::FEATURE_DISABLED_BY_DEFAULT};
 
 bool IsEnabledQueryTilesInOmnibox() {
   return base::FeatureList::IsEnabled(features::kQueryTilesGeoFilter) &&

components/query_tiles/switches.h

@@ -34,6 +34,10 @@ extern const base::Feature kQueryTilesEnableQueryEditing;
 // based on local user interactions.
 extern const base::Feature kQueryTilesLocalOrdering;
 
+// Feature flag to determine whether trending tiles should disapear after
+// some time of inactivity.
+extern const base::Feature kQueryTilesRemoveTrendingTilesAfterInactivity;
+
 // Helper function to determine whether query tiles should be shown on omnibox.
 bool IsEnabledQueryTilesInOmnibox();
 

components/query_tiles/test/test_utils.cc

@@ -65,6 +65,23 @@ void ResetTestGroup(TileGroup* group) {
   group->tile_stats["guid-3-1"] = TileStats(group->last_updated_ts, 0.5);
 }
 
+std::vector<std::unique_ptr<Tile>> GetTestTrendingTileList() {
+  auto trending_tile1 = std::make_unique<Tile>();
+  trending_tile1->id = "trending_1";
+
+  auto trending_tile2 = std::make_unique<Tile>();
+  trending_tile2->id = "trending_2";
+
+  auto trending_tile3 = std::make_unique<Tile>();
+  trending_tile3->id = "trending_3";
+
+  std::vector<std::unique_ptr<Tile>> trending_tile_list;
+  trending_tile_list.emplace_back(std::move(trending_tile1));
+  trending_tile_list.emplace_back(std::move(trending_tile2));
+  trending_tile_list.emplace_back(std::move(trending_tile3));
+  return trending_tile_list;
+}
+
 bool AreTileGroupsIdentical(const TileGroup& lhs, const TileGroup& rhs) {
   if (lhs != rhs)
     return false;

components/query_tiles/test/test_utils.h

@@ -22,6 +22,9 @@ void ResetTestGroup(TileGroup* group);
 // Build and reset the Tile for test usage.
 void ResetTestEntry(Tile* entry);
 
+// Return a list of trending tiles for test.
+std::vector<std::unique_ptr<Tile>> GetTestTrendingTileList();
+
 // Returns true if all data in two TileGroups are identical.
 bool AreTileGroupsIdentical(const TileGroup& lhs, const TileGroup& rhs);