Remove tile stats for tiles that are no longer showing up

With trending tiles, users could click on some trending tiles and
then leave the score in tile store forever.
This CL will clean them up if the tile doesn't show up.
We are doing tile stats cleaning in sorting stage is to save the cost
of doing another tree traversal to get all the tile IDs.

BUG=1096224

Change-Id: Iee421bd24d382234730adda4baea48fd2bf6015c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490555
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#820133}

components/query_tiles/internal/tile_manager.cc

@@ -168,8 +168,10 @@ class TileManagerImpl : public TileManager {
       tile_stats_group_ = std::move(loaded_groups[kTileStatsGroup]);
       // prevent the stats group from being deleted.
       loaded_groups.erase(kTileStatsGroup);
-      if (tile_group_)
-        SortTiles(&tile_group_->tiles, &tile_stats_group_->tile_stats);
+      if (tile_group_) {
+        SortTilesAndClearUnusedStats(&tile_group_->tiles,
+                                     &tile_stats_group_->tile_stats);
+      }
     }
 
     // Deletes other groups.
@@ -253,7 +255,7 @@ class TileManagerImpl : public TileManager {
     // Find the parent tile first. If it cannot be found, that's fine as the
     // old tile score will be used.
     std::vector<std::unique_ptr<Tile>>* tiles = &tile_group_->tiles;
-    if (parent_tile_id.has_value()) {
+    if (parent_tile_id) {
       for (const auto& tile : tile_group_->tiles) {
         if (tile->id == parent_tile_id.value()) {
           tiles = &tile->sub_tiles;
@@ -261,7 +263,7 @@ class TileManagerImpl : public TileManager {
         }
       }
     }
-    // Now check if a tile has the same query text.
+    // Now check if a sub tile has the same query text.
     for (const auto& tile : *tiles) {
       if (query_text == base::UTF8ToUTF16(tile->query_text)) {
         OnTileClicked(tile->id);

components/query_tiles/internal/tile_utils.cc

@@ -23,10 +23,9 @@ struct TileComparator {
   std::map<std::string, double> tile_score_map;
 };
 
-}  // namespace
-
 void SortTiles(std::vector<std::unique_ptr<Tile>>* tiles,
-               std::map<std::string, TileStats>* tile_stats) {
+               std::map<std::string, TileStats>* tile_stats,
+               std::map<std::string, double>* score_map) {
   if (!tiles || tiles->empty())
     return;
 
@@ -36,7 +35,6 @@ void SortTiles(std::vector<std::unique_ptr<Tile>>* tiles,
   base::Time now_time = base::Time::Now();
   TileStats last_tile_stats(now_time, last_score);
   size_t new_tile_index = 0;
-  std::map<std::string, double> score_map;
   // Find any tiles that don't have scores, and add new entries for them.
   for (size_t i = 0; i < tiles->size(); ++i) {
     auto iter = tile_stats->find((*tiles)[i]->id);
@@ -66,7 +64,7 @@ void SortTiles(std::vector<std::unique_ptr<Tile>>* tiles,
       }
       for (size_t j = new_tile_index; j < i; ++j) {
         tile_stats->emplace((*tiles)[j]->id, new_stats);
-        score_map.emplace((*tiles)[j]->id, min_score);
+        score_map->emplace((*tiles)[j]->id, min_score);
       }
     }
     // Move |new_tile_index| to the next one that might not have
@@ -74,20 +72,39 @@ void SortTiles(std::vector<std::unique_ptr<Tile>>* tiles,
     new_tile_index = i + 1;
     last_score = new_score;
     last_tile_stats = iter->second;
-    score_map.emplace((*tiles)[i]->id, last_score);
+    score_map->emplace((*tiles)[i]->id, last_score);
   }
   // Fill the new tiles at the end with 0 score.
   if (new_tile_index < tiles->size()) {
     TileStats new_stats(now_time, 0);
     for (size_t j = new_tile_index; j < tiles->size(); ++j) {
       tile_stats->emplace((*tiles)[j]->id, new_stats);
-      score_map.emplace((*tiles)[j]->id, 0);
+      score_map->emplace((*tiles)[j]->id, 0);
     }
   }
   // Sort the tiles in descending order.
-  std::sort(tiles->begin(), tiles->end(), TileComparator(score_map));
+  std::sort(tiles->begin(), tiles->end(), TileComparator(*score_map));
   for (auto& tile : *tiles)
-    SortTiles(&tile->sub_tiles, tile_stats);
+    SortTiles(&tile->sub_tiles, tile_stats, score_map);
+}
+
+}  // namespace
+
+void SortTilesAndClearUnusedStats(
+    std::vector<std::unique_ptr<Tile>>* tiles,
+    std::map<std::string, TileStats>* tile_stats) {
+  if (!tiles || tiles->empty())
+    return;
+  std::map<std::string, double> score_map;
+  SortTiles(tiles, tile_stats, &score_map);
+  auto iter = tile_stats->begin();
+  while (iter != tile_stats->end()) {
+    if (score_map.find(iter->first) == score_map.end()) {
+      iter = tile_stats->erase(iter);
+    } else {
+      iter++;
+    }
+  }
 }
 
 double CalculateTileScore(const TileStats& tile_stats,

components/query_tiles/internal/tile_utils.h

@@ -15,7 +15,8 @@ namespace query_tiles {
 
 // Function to sort a vector of tiles based on their score in |tile_stats|. If
 // a tile ID doesn't exists in |tile_stats|, a new entry will be created and
-// a score will be calculated.
+// a score will be calculated. If a tile ID in |tile_stats| doesn't show up in
+// |tiles|, it will be removed.
 // To calculate scores for new tiles, ordering from the server response will
 // be taken into consideration. As the server has already ordered tiles
 // according to their importance.
@@ -30,8 +31,8 @@ namespace query_tiles {
 // will result in (0.5, 0.5, 0.7, 0). And for new tiles at the front, they are
 // guaranteed a minimum score. So that if all the other tiles haven't been
 // clicked for a while, it will have a chance to be placed at the front.
-void SortTiles(std::vector<std::unique_ptr<Tile>>* tiles,
-               std::map<std::string, TileStats>* tile_stats);
+void SortTilesAndClearUnusedStats(std::vector<std::unique_ptr<Tile>>* tiles,
+                                  std::map<std::string, TileStats>* tile_stats);
 
 // Calculates the current tile score based on |current_time|. Tile score will
 // decay over time.

components/query_tiles/internal/tile_utils_unittest.cc

@@ -12,11 +12,23 @@
 namespace query_tiles {
 namespace {
 
+// Tests that nothing happens when sorting an empty TileGroup.
+TEST(TileUtilsTest, SortEmptyTileGroup) {
+  TileGroup group;
+  std::map<std::string, TileStats> tile_stats;
+  tile_stats["guid-1-3"] = TileStats(group.last_updated_ts, 0.7);
+  tile_stats["guid-1-4"] = TileStats(group.last_updated_ts, 0.4);
+
+  SortTilesAndClearUnusedStats(&group.tiles, &tile_stats);
+  EXPECT_EQ(tile_stats["guid-1-3"].score, 0.7);
+  EXPECT_EQ(tile_stats["guid-1-4"].score, 0.4);
+}
+
 TEST(TileUtilsTest, Sort) {
   TileGroup group;
   test::ResetTestGroup(&group);
 
-  SortTiles(&group.tiles, &group.tile_stats);
+  SortTilesAndClearUnusedStats(&group.tiles, &group.tile_stats);
   EXPECT_EQ(group.tiles[0]->id, "guid-1-3");
   EXPECT_EQ(group.tiles[1]->id, "guid-1-1");
   EXPECT_EQ(group.tiles[2]->id, "guid-1-2");
@@ -32,7 +44,7 @@ TEST(TileUtilsTest, SortWithEmptytile_stats) {
 
   std::map<std::string, TileStats> tile_stats;
 
-  SortTiles(&group.tiles, &tile_stats);
+  SortTilesAndClearUnusedStats(&group.tiles, &tile_stats);
   EXPECT_EQ(group.tiles[0]->id, "guid-1-1");
   EXPECT_EQ(group.tiles[1]->id, "guid-1-2");
   EXPECT_EQ(group.tiles[2]->id, "guid-1-3");
@@ -51,7 +63,7 @@ TEST(TileUtilsTest, SortWithNewTilesAtTheFront) {
   tile_stats["guid-1-4"] = TileStats(group.last_updated_ts, 0.4);
   tile_stats["guid-2-2"] = TileStats(group.last_updated_ts, 0.6);
 
-  SortTiles(&group.tiles, &tile_stats);
+  SortTilesAndClearUnusedStats(&group.tiles, &tile_stats);
   EXPECT_EQ(group.tiles[0]->id, "guid-1-1");
   EXPECT_EQ(group.tiles[1]->id, "guid-1-2");
   EXPECT_EQ(group.tiles[2]->id, "guid-1-3");
@@ -77,7 +89,7 @@ TEST(TileUtilsTest, SortWithNewTilesAtTheEnd) {
   tile_stats["guid-1-2"] = TileStats(group.last_updated_ts, 0.2);
   tile_stats["guid-2-1"] = TileStats(group.last_updated_ts, 0.3);
 
-  SortTiles(&group.tiles, &tile_stats);
+  SortTilesAndClearUnusedStats(&group.tiles, &tile_stats);
   EXPECT_EQ(group.tiles[0]->id, "guid-1-1");
   EXPECT_EQ(group.tiles[1]->id, "guid-1-2");
   EXPECT_EQ(group.tiles[2]->id, "guid-1-3");
@@ -96,7 +108,7 @@ TEST(TileUtilsTest, SortWithNewTilesInTheMiddle) {
   tile_stats["guid-1-1"] = TileStats(group.last_updated_ts, 0.5);
   tile_stats["guid-1-3"] = TileStats(group.last_updated_ts, 0.7);
 
-  SortTiles(&group.tiles, &tile_stats);
+  SortTilesAndClearUnusedStats(&group.tiles, &tile_stats);
   EXPECT_EQ(group.tiles[0]->id, "guid-1-3");
   EXPECT_EQ(group.tiles[1]->id, "guid-1-1");
   EXPECT_EQ(group.tiles[2]->id, "guid-1-2");
@@ -104,6 +116,25 @@ TEST(TileUtilsTest, SortWithNewTilesInTheMiddle) {
   EXPECT_EQ(tile_stats["guid-1-2"].last_clicked_time, group.last_updated_ts);
 }
 
+// Test the case that stats for unused tiles are cleared.
+TEST(TileUtilsTest, UnusedTilesCleared) {
+  TileGroup group;
+  test::ResetTestGroup(&group);
+  std::string unsed_tile_id = "guid-x";
+
+  std::map<std::string, TileStats> tile_stats;
+  tile_stats["guid-1-1"] = TileStats(group.last_updated_ts, 0.5);
+  tile_stats["guid-1-3"] = TileStats(group.last_updated_ts, 0.7);
+  // Stats for a tile that is no longer used.
+  tile_stats[unsed_tile_id] = TileStats(group.last_updated_ts, 0.1);
+
+  SortTilesAndClearUnusedStats(&group.tiles, &tile_stats);
+  EXPECT_EQ(group.tiles[0]->id, "guid-1-3");
+  EXPECT_EQ(group.tiles[1]->id, "guid-1-1");
+  EXPECT_EQ(group.tiles[2]->id, "guid-1-2");
+  EXPECT_TRUE(tile_stats.find(unsed_tile_id) == tile_stats.end());
+}
+
 TEST(TileUtilsTest, CalculateTileScore) {
   base::Time now_time = base::Time::Now();
   EXPECT_EQ(CalculateTileScore(TileStats(now_time, 0.7), now_time), 0.7);