Retrieve favicons from history as NavigationEntries are converted from TabNav...

chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc

Index: chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
===================================================================
--- chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc	(revision 70486)
+++ chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc	(working copy)
@@ -7,15 +7,53 @@
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/history/history.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/renderer_host/test/test_render_view_host.h"
 #include "chrome/browser/tab_contents/navigation_controller.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/tab_contents/test_tab_contents.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/test/testing_profile.h"
+#include "gfx/codec/png_codec.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
+namespace {
+
+static const int kIconWidth = 16;
+static const int kIconHeight = 16;
+
+void MakeTestSkBitmap(int w, int h, SkBitmap* bmp) {

[sky, 2011/01/06 17:21:56]

See the code in top_sites_unittest CreateBitmap that does this. It's simpler.

+  bmp->setConfig(SkBitmap::kARGB_8888_Config, w, h);
+  bmp->allocPixels();
+
+  uint32_t* src_data = bmp->getAddr32(0, 0);
+  for (int i = 0; i < w * h; i++) {
+    src_data[i] = SkPreMultiplyARGB(i % 255, i % 250, i % 245, i % 240);
+  }
+}
+
+}  // namespace
+
+class FaviconDelegate : public menus::MenuModel::Delegate {

[sky, 2011/01/06 17:21:56]

Keep this in the anonymous namespace.

+ public:
+  explicit FaviconDelegate(MessageLoop* loop) : loop_(loop) {

[sky, 2011/01/06 17:21:56]

Does this really need to cache the loop? Can't it just quit the current loop?

+    DCHECK(loop);
+  }
+
+  void OnIconChanged(int model_index) {
+    loop_->Quit();
+  }
+
+ private:
+  MessageLoop* loop_;
+  DISALLOW_COPY_AND_ASSIGN(FaviconDelegate);
+};
+
 class BackFwdMenuModelTest : public RenderViewHostTestHarness {
  public:
   void ValidateModel(BackForwardMenuModel* model, int history_items,
@@ -420,3 +458,61 @@
   EXPECT_EQ(1, back_model->GetIndexOfNextChapterStop(2, false));
   EXPECT_EQ(-1, back_model->GetIndexOfNextChapterStop(1, false));
 }
+
+TEST_F(BackFwdMenuModelTest, FaviconLoadTest) {

[sky, 2011/01/06 17:21:56]

Description?

+  profile()->CreateHistoryService(true, false);
+  profile()->CreateFaviconService();
+  Browser browser_(Browser::TYPE_NORMAL, profile());

[sky, 2011/01/06 17:21:56]

browser_ -> browser

+  FaviconDelegate favicon_delegate(MessageLoop::current());
+
+  scoped_ptr<BackForwardMenuModel> back_model(new BackForwardMenuModel(
+    &browser_, BackForwardMenuModel::BACKWARD_MENU));
+  back_model->set_test_tab_contents(controller().tab_contents());
+  back_model->SetDelegate(&favicon_delegate);
+
+  SkBitmap new_icon;
+  MakeTestSkBitmap(kIconWidth, kIconHeight, &new_icon);
+  std::vector<unsigned char> icon_data;
+  gfx::PNGCodec::EncodeBGRASkBitmap(new_icon, false, &icon_data);
+
+  GURL url1 = GURL("http://www.a.com/1");
+  GURL url2 = GURL("http://www.a.com/2");
+  GURL url1_favicon("http://www.a.com/1/favicon.ico");
+
+  NavigateAndCommit(url1);
+  // Navigate to a new URL so that url1 will be in the BackForwardMenuModel.
+  NavigateAndCommit(url2);
+
+  // Set the desired favicon for url1.
+  profile()->GetHistoryService(Profile::EXPLICIT_ACCESS)->AddPage(url1,
+      history::SOURCE_BROWSED);
+  profile()->GetFaviconService(Profile::EXPLICIT_ACCESS)->SetFavicon(url1,
+      url1_favicon, icon_data);
+  MessageLoop::current()->RunAllPending();

[sky, 2011/01/06 17:21:56]

Why the RunAllPending here?

+
+  // Will return the current icon (default) but start an anync call
+  // to retrieve the favicon from the favicon service.
+  SkBitmap default_icon;
+  back_model->GetIconAt(0, &default_icon);
+
+  // Make the favicon service run GetFavIconForURL,
+  // FaviconDelegate.OnIconChanged will be called.
+  MessageLoop::current()->Run();

[sky, 2011/01/06 17:21:56]

Shouldn't have an EXPECT that the delegate was called?

+
+  SkBitmap valid_icon;
+  // This time we will get the new favicon returned.
+  back_model->GetIconAt(0, &valid_icon);
+  SkAutoLockPixels a(new_icon);
+  SkAutoLockPixels b(valid_icon);
+  SkAutoLockPixels c(default_icon);
+  // Verify we did not get the default favicon.
+  EXPECT_NE(0, memcmp(default_icon.getPixels(), valid_icon.getPixels(),
+               default_icon.getSize()));
+  // Verify we did get the expected favicon.
+  EXPECT_EQ(0, memcmp(new_icon.getPixels(), valid_icon.getPixels(),
+              new_icon.getSize()));
+  browser_.CloseAllTabs();
+  // This is required to prevent the message loop from hanging.
+  profile()->DestroyHistoryService();
+}
+