[Upboarding]: Complete GetTiles flow.

Keep GetTiles as async call but return the copies of tile data.

Bug: 1066550
Change-Id: I0b2eddd7063c5f6ded269c4223cd3e67da2aedf8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2161479
Commit-Queue: Hesen Zhang <hesen@chromium.org>
Auto-Submit: Hesen Zhang <hesen@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763534}

chrome/browser/upboarding/query_tiles/android/tile_provider_bridge.cc

@@ -23,37 +23,37 @@ namespace upboarding {
 namespace {
 const char kTileProviderBridgeKey[] = "tile_provider_bridge";
 
-ScopedJavaLocalRef<jobject> createJavaTileAndMaybeAddToList(
+ScopedJavaLocalRef<jobject> CreateJavaTileAndMaybeAddToList(
     JNIEnv* env,
     ScopedJavaLocalRef<jobject> jlist,
-    Tile* tile) {
+    const Tile& tile) {
   ScopedJavaLocalRef<jobject> jchildren =
       Java_TileProviderBridge_createList(env);
 
-  for (const auto& subtile : tile->sub_tiles)
-    createJavaTileAndMaybeAddToList(env, jchildren, subtile.get());
+  for (const auto& subtile : tile.sub_tiles)
+    CreateJavaTileAndMaybeAddToList(env, jchildren, *subtile.get());
 
   return Java_TileProviderBridge_createTileAndMaybeAddToList(
-      env, jlist, ConvertUTF8ToJavaString(env, tile->id),
-      ConvertUTF8ToJavaString(env, tile->display_text),
-      ConvertUTF8ToJavaString(env, tile->accessibility_text),
-      ConvertUTF8ToJavaString(env, tile->query_text), jchildren);
+      env, jlist, ConvertUTF8ToJavaString(env, tile.id),
+      ConvertUTF8ToJavaString(env, tile.display_text),
+      ConvertUTF8ToJavaString(env, tile.accessibility_text),
+      ConvertUTF8ToJavaString(env, tile.query_text), jchildren);
 }
 
-ScopedJavaLocalRef<jobject> createJavaTiles(JNIEnv* env,
-                                            const std::vector<Tile*>& tiles) {
+ScopedJavaLocalRef<jobject> CreateJavaTiles(JNIEnv* env,
+                                            std::vector<Tile> tiles) {
   ScopedJavaLocalRef<jobject> jlist = Java_TileProviderBridge_createList(env);
 
-  for (Tile* tile : tiles)
-    createJavaTileAndMaybeAddToList(env, jlist, tile);
+  for (const auto& tile : tiles)
+    CreateJavaTileAndMaybeAddToList(env, jlist, tile);
 
   return jlist;
 }
 
 void RunGetTilesCallback(const JavaRef<jobject>& j_callback,
-                         const std::vector<Tile*>& tiles) {
+                         std::vector<Tile> tiles) {
   JNIEnv* env = AttachCurrentThread();
-  RunObjectCallbackAndroid(j_callback, createJavaTiles(env, tiles));
+  RunObjectCallbackAndroid(j_callback, CreateJavaTiles(env, std::move(tiles)));
 }
 
 void RunGeVisualsCallback(const JavaRef<jobject>& j_callback,

chrome/browser/upboarding/query_tiles/internal/BUILD.gn

@@ -52,6 +52,7 @@ source_set("unit_tests") {
     "proto_conversion_unittest.cc",
     "tile_fetcher_unittest.cc",
     "tile_group_unittest.cc",
+    "tile_manager_unittest.cc",
     "tile_store_unittest.cc",
   ]
 

chrome/browser/upboarding/query_tiles/internal/tile_manager.cc

@@ -9,6 +9,8 @@
 #include "base/bind.h"
 #include "base/guid.h"
 #include "base/memory/weak_ptr.h"
+#include "base/task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/upboarding/query_tiles/internal/tile_manager.h"
 
@@ -55,17 +57,14 @@ class TileManagerImpl : public TileManager {
                                   std::move(group), std::move(callback)));
   }
 
-  void GetTiles(std::vector<Tile*>* tiles) override {
-    DCHECK(tiles);
-
-    if (!initialized_)
-      return;
-
-    tiles->clear();
+  void GetTiles(GetTilesCallback callback) override {
+    std::vector<Tile> tiles;
     if (tile_group_ && ValidateGroup(tile_group_.get())) {
       for (const auto& tile : tile_group_->tiles)
-        tiles->emplace_back(tile.get());
+        tiles.emplace_back(*tile.get());
     }
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), std::move(tiles)));
   }
 
   void OnTileStoreInitialized(
@@ -110,7 +109,8 @@ class TileManagerImpl : public TileManager {
     std::move(callback).Run(status);
   }
 
-  // Returns true if the group is not expired and the locale matches OS setting.
+  // Returns true if the group is not expired and the locale matches OS
+  // setting.
   bool ValidateGroup(const TileGroup* group) const {
     return clock_->Now() - group->last_updated_ts < config_->expire_duration &&
            group->locale == config_->locale;

chrome/browser/upboarding/query_tiles/internal/tile_manager.h

@@ -24,6 +24,7 @@ class TileManager {
  public:
   using TileStore = Store<TileGroup>;
   using TileGroupStatusCallback = base::OnceCallback<void(TileGroupStatus)>;
+  using GetTilesCallback = base::OnceCallback<void(std::vector<Tile>)>;
 
   // Creates the instance.
   static std::unique_ptr<TileManager> Create(
@@ -36,7 +37,7 @@ class TileManager {
   virtual void Init(TileGroupStatusCallback callback) = 0;
 
   // Returns tiles to the caller.
-  virtual void GetTiles(std::vector<Tile*>* tiles) = 0;
+  virtual void GetTiles(GetTilesCallback callback) = 0;
 
   // Save the query tiles into database.
   virtual void SaveTiles(std::vector<std::unique_ptr<Tile>> top_level_tiles,

chrome/browser/upboarding/query_tiles/internal/tile_manager_unittest.cc

@@ -85,10 +85,17 @@ class TileManagerTest : public testing::Test {
 
   // Run GetTiles call from manager_, compare the |expected| to the actual
   // returned tiles.
-  void GetTiles(std::vector<Tile*> expected) {
-    std::vector<Tile*> actual;
-    manager()->GetTiles(&actual);
-    EXPECT_TRUE(test::AreTilesIdentical(expected, actual));
+  void GetTiles(base::RepeatingClosure closure, std::vector<Tile> expected) {
+    manager()->GetTiles(base::BindOnce(
+        &TileManagerTest::OnTilesReturned, base::Unretained(this),
+        std::move(closure), std::move(expected)));
+  }
+
+  void OnTilesReturned(base::RepeatingClosure closure,
+                       std::vector<Tile> expected,
+                       std::vector<Tile> tiles) {
+    EXPECT_TRUE(test::AreTilesIdentical(expected, tiles));
+    std::move(closure).Run();
   }
 
  protected:
@@ -105,6 +112,9 @@ class TileManagerTest : public testing::Test {
   base::SimpleTestClock clock_;
 };
 
+// TODO(hesen): Add a test where we request tiles before the initialize
+// callback from the DB.
+
 TEST_F(TileManagerTest, InitAndLoadWithDbOperationFailed) {
   EXPECT_CALL(*tile_store(), InitAndLoad(_))
       .WillOnce(Invoke([](base::OnceCallback<void(
@@ -114,7 +124,7 @@ TEST_F(TileManagerTest, InitAndLoadWithDbOperationFailed) {
 
   base::RunLoop loop;
   Init(loop.QuitClosure(), TileGroupStatus::kFailureDbOperation);
-  GetTiles(std::vector<Tile*>() /*expect an empty result*/);
+  GetTiles(loop.QuitClosure(), std::vector<Tile>() /*expect an empty result*/);
   loop.Run();
 }
 
@@ -127,7 +137,7 @@ TEST_F(TileManagerTest, InitWithEmptyDb) {
 
   base::RunLoop loop;
   Init(loop.QuitClosure(), TileGroupStatus::kSuccess);
-  GetTiles(std::vector<Tile*>() /*expect an empty result*/);
+  GetTiles(loop.QuitClosure(), std::vector<Tile>() /*expect an empty result*/);
   loop.Run();
 }
 
@@ -148,7 +158,7 @@ TEST_F(TileManagerTest, InitAndLoadWithLocaleNotMatch) {
 
   base::RunLoop loop;
   Init(loop.QuitClosure(), TileGroupStatus::kInvalidGroup);
-  GetTiles(std::vector<Tile*>() /*expect an empty result*/);
+  GetTiles(loop.QuitClosure(), std::vector<Tile>() /*expect an empty result*/);
   loop.Run();
 }
 
@@ -170,18 +180,18 @@ TEST_F(TileManagerTest, InitAndLoadWithExpiredGroup) {
 
   base::RunLoop loop;
   Init(loop.QuitClosure(), TileGroupStatus::kInvalidGroup);
-  GetTiles(std::vector<Tile*>() /*expect an empty result*/);
+  GetTiles(loop.QuitClosure(), std::vector<Tile>() /*expect an empty result*/);
   loop.Run();
 }
 
 TEST_F(TileManagerTest, InitAndLoadSuccess) {
   auto input_group = std::make_unique<TileGroup>();
   test::ResetTestGroup(input_group.get());
-  std::vector<Tile*> expected;
+  std::vector<Tile> expected;
   input_group->last_updated_ts =
       clock()->Now() - base::TimeDelta::FromMinutes(5);
   for (const auto& tile : input_group->tiles)
-    expected.emplace_back(tile.get());
+    expected.emplace_back(*tile.get());
 
   MockTileStore::KeysAndEntries input;
   input[input_group->id] = std::move(input_group);
@@ -196,7 +206,7 @@ TEST_F(TileManagerTest, InitAndLoadSuccess) {
 
   base::RunLoop loop;
   Init(loop.QuitClosure(), TileGroupStatus::kSuccess);
-  GetTiles(expected);
+  GetTiles(loop.QuitClosure(), expected);
   loop.Run();
 }
 
@@ -221,7 +231,7 @@ TEST_F(TileManagerTest, SaveTilesWhenUnintialized) {
 
   SaveTiles(std::move(tiles_to_save), loop.QuitClosure(),
             TileGroupStatus::kUninitialized);
-  GetTiles(std::vector<Tile*>() /*expect an empty result*/);
+  GetTiles(loop.QuitClosure(), std::vector<Tile>() /*expect an empty result*/);
 
   loop.Run();
 }
@@ -251,7 +261,7 @@ TEST_F(TileManagerTest, SaveTilesFailed) {
 
   SaveTiles(std::move(tiles_to_save), loop.QuitClosure(),
             TileGroupStatus::kFailureDbOperation);
-  GetTiles(std::vector<Tile*>() /*expect an empty result*/);
+  GetTiles(loop.QuitClosure(), std::vector<Tile>() /*expect an empty result*/);
 
   loop.Run();
 }
@@ -280,12 +290,12 @@ TEST_F(TileManagerTest, SaveTilesSuccess) {
   test::ResetTestEntry(expected_tile.get());
   std::vector<std::unique_ptr<Tile>> tiles_to_save;
   tiles_to_save.emplace_back(std::move(tile_to_save));
-  std::vector<Tile*> expected;
-  expected.emplace_back(expected_tile.get());
+  std::vector<Tile> expected;
+  expected.emplace_back(*expected_tile.get());
 
   SaveTiles(std::move(tiles_to_save), loop.QuitClosure(),
             TileGroupStatus::kSuccess);
-  GetTiles(std::move(expected));
+  GetTiles(loop.QuitClosure(), std::move(expected));
   loop.Run();
 }
 
@@ -326,12 +336,12 @@ TEST_F(TileManagerTest, SaveTilesAndReplaceOldGroupSuccess) {
 
   auto expected_tile = std::make_unique<Tile>();
   test::ResetTestEntry(expected_tile.get());
-  std::vector<Tile*> expected;
-  expected.emplace_back(std::move(expected_tile.get()));
+  std::vector<Tile> expected;
+  expected.emplace_back(std::move(*expected_tile.get()));
 
   SaveTiles(std::move(tiles_to_save), loop.QuitClosure(),
             TileGroupStatus::kSuccess);
-  GetTiles(std::move(expected));
+  GetTiles(loop.QuitClosure(), std::move(expected));
   loop.Run();
 }
 

chrome/browser/upboarding/query_tiles/internal/tile_service_impl.cc

@@ -21,7 +21,7 @@ TileServiceImpl::TileServiceImpl(std::unique_ptr<ImageLoader> image_loader,
 TileServiceImpl::~TileServiceImpl() = default;
 
 void TileServiceImpl::GetQueryTiles(GetTilesCallback callback) {
-  NOTIMPLEMENTED();
+  tile_manager_->GetTiles(std::move(callback));
 }
 
 void TileServiceImpl::GetVisuals(const std::string& tile_id,

chrome/browser/upboarding/query_tiles/test/test_utils.cc

@@ -161,16 +161,27 @@ bool AreTilesIdentical(const Tile& lhs, const Tile& rhs) {
 }
 
 bool AreTilesIdentical(std::vector<Tile*> lhs, std::vector<Tile*> rhs) {
+  std::vector<Tile> lhs_copy, rhs_copy;
+  for (auto* tile : lhs)
+    lhs_copy.emplace_back(*tile);
+  for (auto* tile : rhs)
+    rhs_copy.emplace_back(*tile);
+  return AreTilesIdentical(std::move(lhs_copy), std::move(rhs_copy));
+}
+
+bool AreTilesIdentical(std::vector<Tile> lhs, std::vector<Tile> rhs) {
   if (lhs.size() != rhs.size())
     return false;
 
-  auto entry_comparator = [](Tile* a, Tile* b) { return a->id < b->id; };
+  auto entry_comparator = [](const Tile& a, const Tile& b) {
+    return a.id < b.id;
+  };
 
   std::sort(lhs.begin(), lhs.end(), entry_comparator);
   std::sort(rhs.begin(), rhs.end(), entry_comparator);
 
   for (size_t i = 0; i < lhs.size(); i++) {
-    if (*lhs[i] != *rhs[i])
+    if (!AreTilesIdentical(lhs[i], rhs[i]))
       return false;
   }
 

chrome/browser/upboarding/query_tiles/test/test_utils.h

@@ -38,6 +38,9 @@ bool AreTilesIdentical(const Tile& lhs, const Tile& rhs);
 // Returns true if all data in two lists of Tile are identical.
 bool AreTilesIdentical(std::vector<Tile*> lhs, std::vector<Tile*> rhs);
 
+// Returns true if all data in two lists of Tile are identical.
+bool AreTilesIdentical(std::vector<Tile> lhs, std::vector<Tile> rhs);
+
 }  // namespace test
 }  // namespace upboarding
 

chrome/browser/upboarding/query_tiles/tile_service.h

@@ -19,7 +19,8 @@ class Image;
 
 namespace upboarding {
 
-using GetTilesCallback = base::OnceCallback<void(const std::vector<Tile*>&)>;
+using TileList = std::vector<Tile>;
+using GetTilesCallback = base::OnceCallback<void(TileList)>;
 using VisualsCallback = base::OnceCallback<void(const gfx::Image&)>;
 
 // The central class on chrome client responsible for fetching, storing,