Make BookmarkModelVerifier wait for favicon load during GetFavicon

The test case for bookmark favicon sync was failing in its verification
step because GetFavicon sometimes returned a blank favicon for a URL.
This was because GetFavicon can be asynchronous if the favicon of a
bookmark node is not already loaded.

This patch modifies BookmarkModelVerifier::FaviconsMatch and wraps
BookmarkModel::GetFavicon with an additional wait on favicon load,
but only if a favicon was set for that particular URL.

BUG=69694
TEST=sync_integration_tests

Review URL: http://codereview.chromium.org/6760033

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@80540 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 76 insertions, 28 deletions

diff --git a/chrome/test/live_sync/bookmark_model_verifier.cc b/chrome/test/live_sync/bookmark_model_verifier.cc
index 3a34a93..c8e0158 100644
--- a/chrome/test/live_sync/bookmark_model_verifier.cc
+++ b/chrome/test/live_sync/bookmark_model_verifier.cc
@@ -66,9 +66,8 @@ class FaviconLoadObserver : public BookmarkModelObserver {
 
 }
 
-// static
 bool BookmarkModelVerifier::NodesMatch(const BookmarkNode* node_a,
-                                       const BookmarkNode* node_b) {
+                                       const BookmarkNode* node_b) const {
   if (node_a == NULL || node_b == NULL)
     return node_a == node_b;
   if (node_a->is_folder() != node_b->is_folder()) {
@@ -95,9 +94,8 @@ bool BookmarkModelVerifier::NodesMatch(const BookmarkNode* node_a,
   return true;
 }
 
-// static
 bool BookmarkModelVerifier::ModelsMatch(BookmarkModel* model_a,
-                                        BookmarkModel* model_b) {
+                                        BookmarkModel* model_b) const {
   bool ret_val = true;
   ui::TreeNodeIterator<const BookmarkNode> iterator_a(model_a->root_node());
   ui::TreeNodeIterator<const BookmarkNode> iterator_b(model_b->root_node());
@@ -106,16 +104,27 @@ bool BookmarkModelVerifier::ModelsMatch(BookmarkModel* model_a,
     EXPECT_TRUE(iterator_b.has_next());
     const BookmarkNode* node_b = iterator_b.Next();
     ret_val = ret_val && NodesMatch(node_a, node_b);
-    const SkBitmap& bitmap_a = model_a->GetFavicon(node_a);
-    const SkBitmap& bitmap_b = model_b->GetFavicon(node_b);
-    ret_val = ret_val && FaviconsMatch(bitmap_a, bitmap_b);
+    if (node_a->type() != BookmarkNode::URL ||
+        node_b->type() != BookmarkNode::URL)
+      continue;
+    ret_val = ret_val && FaviconsMatch(model_a, model_b, node_a, node_b);
   }
   ret_val = ret_val && (!iterator_b.has_next());
   return ret_val;
 }
 
-bool BookmarkModelVerifier::FaviconsMatch(const SkBitmap& bitmap_a,
-                                          const SkBitmap& bitmap_b) {
+bool BookmarkModelVerifier::FaviconsMatch(BookmarkModel* model_a,
+                                          BookmarkModel* model_b,
+                                          const BookmarkNode* node_a,
+                                          const BookmarkNode* node_b) const {
+  const SkBitmap& bitmap_a = GetFavicon(model_a, node_a);
+  const SkBitmap& bitmap_b = GetFavicon(model_b, node_b);
+  return FaviconBitmapsMatch(bitmap_a, bitmap_b);
+}
+
+bool BookmarkModelVerifier::FaviconBitmapsMatch(
+    const SkBitmap& bitmap_a,
+    const SkBitmap& bitmap_b) const {
   if (bitmap_a.getSize() == 0U && bitmap_a.getSize() == 0U)
     return true;
   if ((bitmap_a.getSize() != bitmap_b.getSize()) ||
@@ -141,7 +150,8 @@ bool BookmarkModelVerifier::FaviconsMatch(const SkBitmap& bitmap_a,
   }
 }
 
-bool BookmarkModelVerifier::ContainsDuplicateBookmarks(BookmarkModel* model) {
+bool BookmarkModelVerifier::ContainsDuplicateBookmarks(
+    BookmarkModel* model) const {
   ui::TreeNodeIterator<const BookmarkNode> iterator(model->root_node());
   while (iterator.has_next()) {
     const BookmarkNode* node = iterator.Next();
@@ -162,11 +172,10 @@ bool BookmarkModelVerifier::ContainsDuplicateBookmarks(BookmarkModel* model) {
   return false;
 }
 
-// static
 int BookmarkModelVerifier::CountNodesWithTitlesMatching(
     BookmarkModel* model,
     BookmarkNode::Type node_type,
-    const string16& title) {
+    const string16& title) const {
   ui::TreeNodeIterator<const BookmarkNode> iterator(model->root_node());
   // Walk through the model tree looking for bookmark nodes of node type
   // |node_type| whose titles match |title|.
@@ -263,6 +272,7 @@ void BookmarkModelVerifier::SetFavicon(
     BookmarkModel* model,
     const BookmarkNode* node,
     const std::vector<unsigned char>& icon_bytes_vector) {
+  urls_with_favicons_.insert(node->GetURL());
   if (use_verifier_model_) {
     const BookmarkNode* v_node = NULL;
     FindNodeInVerifier(model, node, &v_node);
@@ -277,6 +287,25 @@ void BookmarkModelVerifier::SetFavicon(
   observer.WaitForFaviconLoad();
 }
 
+const SkBitmap& BookmarkModelVerifier::GetFavicon(
+    BookmarkModel* model,
+    const BookmarkNode* node) const {
+  // If a favicon wasn't explicitly set for a particular URL, simply return its
+  // blank favicon.
+  if (urls_with_favicons_.find(node->GetURL()) == urls_with_favicons_.end()) {
+    return node->favicon();
+  }
+  // If a favicon was explicitly set, we may need to wait for it to be loaded
+  // via BookmarkModel::GetFavIcon(), which is an asynchronous operation.
+  if (!node->is_favicon_loaded()) {
+    FaviconLoadObserver observer(model, node);
+    model->GetFavicon(node);
+    observer.WaitForFaviconLoad();
+  }
+  EXPECT_TRUE(node->is_favicon_loaded());
+  return node->favicon();
+}
+
 void BookmarkModelVerifier::Move(BookmarkModel* model,
                                  const BookmarkNode* node,
                                  const BookmarkNode* new_parent,
diff --git a/chrome/test/live_sync/bookmark_model_verifier.h b/chrome/test/live_sync/bookmark_model_verifier.h
index a4ac993..e8dd49e 100644
--- a/chrome/test/live_sync/bookmark_model_verifier.h
+++ b/chrome/test/live_sync/bookmark_model_verifier.h
@@ -6,6 +6,7 @@
 #define CHROME_TEST_LIVE_SYNC_BOOKMARK_MODEL_VERIFIER_H_
 #pragma once
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -31,16 +32,24 @@ class BookmarkModelVerifier {
   // Checks if the hierarchies in |model_a| and |model_b| are equivalent in
   // terms of the data model and favicon. Returns true if they both match.
   // Note: Some peripheral fields like creation times are allowed to mismatch.
-  static bool ModelsMatch(BookmarkModel* model_a,
-                          BookmarkModel* model_b) WARN_UNUSED_RESULT;
+  bool ModelsMatch(BookmarkModel* model_a,
+                   BookmarkModel* model_b) const WARN_UNUSED_RESULT;
 
   // Checks if |model| contains any instances of two bookmarks with the same URL
   // under the same parent folder. Returns true if even one instance is found.
-  static bool ContainsDuplicateBookmarks(BookmarkModel* model);
+  bool ContainsDuplicateBookmarks(BookmarkModel* model) const;
+
+  // Checks if the favicon in |node_a| from |model_a| matches that of |node_b|
+  // from |model_b|. Returns true if they match.
+  bool FaviconsMatch(BookmarkModel* model_a,
+                     BookmarkModel* model_b,
+                     const BookmarkNode* node_a,
+                     const BookmarkNode* node_b) const;
 
   // Checks if the favicon data in |bitmap_a| and |bitmap_b| are equivalent.
   // Returns true if they match.
-  static bool FaviconsMatch(const SkBitmap& bitmap_a, const SkBitmap& bitmap_b);
+  bool FaviconBitmapsMatch(const SkBitmap& bitmap_a,
+                           const SkBitmap& bitmap_b) const;
 
   // Adds the same bookmark to |model| and |verifier_model_|. See
   // BookmarkModel::AddURL for details.
@@ -70,6 +79,10 @@ class BookmarkModelVerifier {
                   const BookmarkNode* node,
                   const std::vector<unsigned char>& icon_bytes_vector);
 
+  // Gets the favicon associated with |node| in |model|.
+  const SkBitmap& GetFavicon(BookmarkModel* model,
+                             const BookmarkNode* node) const;
+
   // Moves the same node to the same position in both |model| and
   // |verifier_model_|. See BookmarkModel::Move for details.
   void Move(BookmarkModel* model,
@@ -100,8 +113,8 @@ class BookmarkModelVerifier {
 
   // Does a deep comparison of BookmarkNode fields in |model_a| and |model_b|.
   // Returns true if they are all equal.
-  static bool NodesMatch(
-      const BookmarkNode* model_a, const BookmarkNode* model_b);
+  bool NodesMatch(const BookmarkNode* model_a,
+                  const BookmarkNode* model_b) const;
 
   bool use_verifier_model() const { return use_verifier_model_; }
 
@@ -111,9 +124,9 @@ class BookmarkModelVerifier {
 
   // Returns the number of nodes of node type |node_type| in |model| whose
   // titles match the string |title|.
-  static int CountNodesWithTitlesMatching(BookmarkModel* model,
-                                          BookmarkNode::Type node_type,
-                                          const string16& title);
+  int CountNodesWithTitlesMatching(BookmarkModel* model,
+                                   BookmarkNode::Type node_type,
+                                   const string16& title) const;
 
  private:
   // A pointer to the BookmarkModel object within the verifier_profile_ object
@@ -124,6 +137,12 @@ class BookmarkModelVerifier {
   // verifier model or not.
   bool use_verifier_model_;
 
+  // A collection of URLs for which we have added favicons. Since loading a
+  // favicon is an asynchronous operation and doesn't necessarily invoke a
+  // callback, this collection is used to determine if we must wait for a URL's
+  // favicon to load or not.
+  std::set<GURL> urls_with_favicons_;
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkModelVerifier);
 };
 
diff --git a/chrome/test/live_sync/live_bookmarks_sync_test.cc b/chrome/test/live_sync/live_bookmarks_sync_test.cc
index 23e219c..8934181 100644
--- a/chrome/test/live_sync/live_bookmarks_sync_test.cc
+++ b/chrome/test/live_sync/live_bookmarks_sync_test.cc
@@ -182,8 +182,8 @@ bool LiveBookmarksSyncTest::ModelMatchesVerifier(int profile) {
                << "DisableVerifier(). Use ModelsMatch() instead.";
     return false;
   }
-  return BookmarkModelVerifier::ModelsMatch(
-      GetVerifierBookmarkModel(), GetBookmarkModel(profile));
+  return verifier_helper_->ModelsMatch(GetVerifierBookmarkModel(),
+                                       GetBookmarkModel(profile));
 }
 
 bool LiveBookmarksSyncTest::AllModelsMatchVerifier() {
@@ -202,8 +202,8 @@ bool LiveBookmarksSyncTest::AllModelsMatchVerifier() {
 }
 
 bool LiveBookmarksSyncTest::ModelsMatch(int profile_a, int profile_b) {
-  return BookmarkModelVerifier::ModelsMatch(
-      GetBookmarkModel(profile_a), GetBookmarkModel(profile_b));
+  return verifier_helper_->ModelsMatch(GetBookmarkModel(profile_a),
+                                       GetBookmarkModel(profile_b));
 }
 
 bool LiveBookmarksSyncTest::AllModelsMatch() {
@@ -217,7 +217,7 @@ bool LiveBookmarksSyncTest::AllModelsMatch() {
 }
 
 bool LiveBookmarksSyncTest::ContainsDuplicateBookmarks(int profile) {
-  return BookmarkModelVerifier::ContainsDuplicateBookmarks(
+  return verifier_helper_->ContainsDuplicateBookmarks(
       GetBookmarkModel(profile));
 }
 
diff --git a/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc b/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
index 7fed5b4..81cbeb9 100644
--- a/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
+++ b/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
@@ -123,14 +123,14 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
 }
 
 // Test Scribe ID - 370489.
-// TODO(rsimha): Enable after http://crbug.com/69694 is fixed.
 IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
-                       FAILS_SC_AddFirstBMWithFavicon) {
+                       SC_AddFirstBMWithFavicon) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllModelsMatchVerifier());
 
   const BookmarkNode* bookmark = AddURL(0, kGenericURLTitle, GURL(kGenericURL));
   ASSERT_TRUE(bookmark != NULL);
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
   SetFavicon(0, bookmark, GenericFavicon());
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
   ASSERT_TRUE(AllModelsMatchVerifier());