Revert 72877 - Make BookmarkModelVerifier wait for favicons to load before extracting them

Reason for revert: Favicon sync test is still flaky on the buildbots.

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

TBR=rsimha@chromium.org
Review URL: http://codereview.chromium.org/6359017

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

3 files changed, 8 insertions, 50 deletions

diff --git a/chrome/test/live_sync/bookmark_model_verifier.cc b/chrome/test/live_sync/bookmark_model_verifier.cc
index 1301c02..ec0e87d 100644
--- a/chrome/test/live_sync/bookmark_model_verifier.cc
+++ b/chrome/test/live_sync/bookmark_model_verifier.cc
@@ -9,7 +9,6 @@
 
 #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"
@@ -33,13 +32,6 @@ 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,
@@ -114,43 +106,16 @@ 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);
-    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);
+    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);
   }
   ret_val = ret_val && (!iterator_b.has_next());
   return ret_val;
 }
 
-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) {
+bool BookmarkModelVerifier::FaviconsMatch(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 4f160d4..44f6c17 100644
--- a/chrome/test/live_sync/bookmark_model_verifier.h
+++ b/chrome/test/live_sync/bookmark_model_verifier.h
@@ -38,17 +38,9 @@ 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 FaviconBitmapsMatch(const SkBitmap& bitmap_a,
-                                  const SkBitmap& bitmap_b);
+  static bool FaviconsMatch(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 18510c1..7255c9f 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,8 +123,9 @@ 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,
-                       SC_AddFirstBMWithFavicon) {
+                       FAILS_SC_AddFirstBMWithFavicon) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllModelsMatchVerifier());