Make BookmarkModelVerifier wait for favicons to load before extracting them

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 the method that compares favicons, and takes care of
cases where the favicon has not yet loaded by adding a conditional wait.

BUG=69694
TEST=sync_integration_tests

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

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

3 files changed, 50 insertions, 8 deletions

diff --git a/chrome/test/live_sync/bookmark_model_verifier.cc b/chrome/test/live_sync/bookmark_model_verifier.cc
index ec0e87d..1301c02 100644
--- a/chrome/test/live_sync/bookmark_model_verifier.cc
+++ b/chrome/test/live_sync/bookmark_model_verifier.cc
@@ -9,6 +9,7 @@
 
 #include "base/rand_util.h"
 #include "base/string_number_conversions.h"
+#include "base/test/test_timeouts.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/browser/bookmarks/bookmark_model_observer.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
@@ -32,6 +33,13 @@ class FaviconLoadObserver : public BookmarkModelObserver {
     model_->RemoveObserver(this);
   }
   void WaitForFaviconLoad() { ui_test_utils::RunMessageLoop(); }
+  void WaitForFaviconLoadWithTimeout(int64 timeout_ms) {
+    MessageLoopForUI::current()->PostDelayedTask(FROM_HERE,
+                                                 new MessageLoop::QuitTask,
+                                                 timeout_ms);
+
+    ui_test_utils::RunMessageLoop();
+  }
   virtual void Loaded(BookmarkModel* model) {}
   virtual void BookmarkNodeMoved(BookmarkModel* model,
                                  const BookmarkNode* old_parent,
@@ -106,16 +114,43 @@ 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) {
+  // BookmarkModel::GetFavIcon() can be an asynchronous operation if a bookmark
+  // node's favicon has not yet loaded.
+  // We wait for TestTimeouts::tiny_timeout_ms() because the favicon is local
+  // and expected to load "almost immediately", if in fact one is present.
+  // See http://crbug.com/69694.
+  if (!node_a->is_favicon_loaded()) {
+    FaviconLoadObserver observer_a(model_a, node_a);
+    model_a->GetFavIcon(node_a);
+    observer_a.WaitForFaviconLoadWithTimeout(TestTimeouts::tiny_timeout_ms());
+  }
+
+  if (!node_b->is_favicon_loaded()) {
+    FaviconLoadObserver observer_b(model_b, node_b);
+    model_b->GetFavIcon(node_b);
+    observer_b.WaitForFaviconLoadWithTimeout(TestTimeouts::tiny_timeout_ms());
+  }
+
+  const SkBitmap& bitmap_a = model_a->GetFavIcon(node_a);
+  const SkBitmap& bitmap_b = model_b->GetFavIcon(node_b);
+  return FaviconBitmapsMatch(bitmap_a, bitmap_b);
+}
+
+bool BookmarkModelVerifier::FaviconBitmapsMatch(const SkBitmap& bitmap_a,
+                                                const SkBitmap& bitmap_b) {
   if (bitmap_a.getSize() == 0U && bitmap_a.getSize() == 0U)
     return true;
   if ((bitmap_a.getSize() != bitmap_b.getSize()) ||
diff --git a/chrome/test/live_sync/bookmark_model_verifier.h b/chrome/test/live_sync/bookmark_model_verifier.h
index 44f6c17..4f160d4 100644
--- a/chrome/test/live_sync/bookmark_model_verifier.h
+++ b/chrome/test/live_sync/bookmark_model_verifier.h
@@ -38,9 +38,17 @@ class BookmarkModelVerifier {
   // under the same parent folder. Returns true if even one instance is found.
   static bool ContainsDuplicateBookmarks(BookmarkModel* model);
 
+  // Checks if the favicon in |node_a| from |model_a| matches that of |node_b|
+  // from |model_b|. Returns true if they match.
+  static bool FaviconsMatch(BookmarkModel* model_a,
+                            BookmarkModel* model_b,
+                            const BookmarkNode* node_a,
+                            const BookmarkNode* node_b);
+
   // 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);
+  static bool FaviconBitmapsMatch(const SkBitmap& bitmap_a,
+                                  const SkBitmap& bitmap_b);
 
   // Adds the same bookmark to |model| and |verifier_model_|. See
   // BookmarkModel::AddURL for details.
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 7255c9f..18510c1 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,9 +123,8 @@ 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());