ntp4: fix dominant color bug

this bug manifested itself in 2 ways:
1) most visited tiles frequently didn't have a color stripe
2) bookmark tiles frequently had the wrong color

the bug stems from the fact the Most Visited and Bookmark ID sets were overlapping (integers 0-7 for Most Visited, and integers starting at 0 counting up for bookmarks). I changed it to use a string (specifically, the DOM ID of the relevant tile contents) as the key.

BUG=none
TEST=manual

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

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

6 files changed, 53 insertions, 75 deletions

diff --git a/chrome/browser/resources/ntp4/apps_page.js b/chrome/browser/resources/ntp4/apps_page.js
index c8e7ce9..9ba9dc0 100644
--- a/chrome/browser/resources/ntp4/apps_page.js
+++ b/chrome/browser/resources/ntp4/apps_page.js
@@ -207,8 +207,7 @@ cr.define('ntp4', function() {
         stripeDiv.className = 'color-stripe';
         imgDiv.appendChild(stripeDiv);
 
-        chrome.send('getAppIconDominantColor',
-            [this.appData_.icon_small, 'ntp4.setAppFaviconDominantColor']);
+        chrome.send('getAppIconDominantColor', [this.id]);
       } else {
         appImgContainer.addEventListener('click', this.onClick_.bind(this));
         appContents.appendChild(appImgContainer);
@@ -709,25 +708,12 @@ cr.define('ntp4', function() {
     $(id).setupNotification_(notification);
   };
 
-  /**
-   * Set the dominant color for an app tile.  This is the callback method
-   * from a request made when the tile was created.
-   * @param {number} id The ID of the tile.
-   * @param {string} color The color represented as a CSS string.
-   */
-  function setAppFaviconDominantColor(id, color) {
-    var tile = $(id);
-    if (tile)
-      tile.stripeColor = color;
-  };
-
   return {
     APP_LAUNCH: APP_LAUNCH,
     appNotificationChanged: appNotificationChanged,
     AppsPage: AppsPage,
     appsPrefChangeCallback: appsPrefChangeCallback,
     launchAppAfterEnable: launchAppAfterEnable,
-    setAppFaviconDominantColor: setAppFaviconDominantColor
   };
 });
 
diff --git a/chrome/browser/resources/ntp4/bookmarks_page.js b/chrome/browser/resources/ntp4/bookmarks_page.js
index bfc50370..bfe7e8b 100644
--- a/chrome/browser/resources/ntp4/bookmarks_page.js
+++ b/chrome/browser/resources/ntp4/bookmarks_page.js
@@ -60,8 +60,7 @@ cr.define('ntp4', function() {
       var faviconUrl;
       if (this.data.url) {
         faviconUrl = 'chrome://favicon/size/16/' + this.data.url;
-        chrome.send('getFaviconDominantColor',
-                    [faviconUrl, id, 'ntp4.setBookmarksFaviconDominantColor']);
+        chrome.send('getFaviconDominantColor', [faviconUrl, this.id]);
       } else {
         faviconUrl = 'chrome://theme/IDR_BOOKMARK_BAR_FOLDER';
         // TODO(csilv): Should we vary this color by platform?
@@ -447,11 +446,11 @@ cr.define('ntp4', function() {
   /**
    * Set the dominant color for a bookmark tile.  This is the callback method
    * from a request made when the tile was created.
-   * @param {number} id The numeric ID of the bookmark tile.
+   * @param {string} id The ID of the bookmark tile.
    * @param {string} color The color represented as a CSS string.
    */
   function setBookmarksFaviconDominantColor(id, color) {
-    var tile = $('bookmark_tile_' + id);
+    var tile = $(id);
     if (tile)
       tile.stripeColor = color;
   };
@@ -459,7 +458,6 @@ cr.define('ntp4', function() {
   return {
     BookmarksPage: BookmarksPage,
     initBookmarkChevron: initBookmarkChevron,
-    setBookmarksFaviconDominantColor: setBookmarksFaviconDominantColor
   };
 });
 
diff --git a/chrome/browser/resources/ntp4/most_visited_page.js b/chrome/browser/resources/ntp4/most_visited_page.js
index 317bc83..794abe2 100644
--- a/chrome/browser/resources/ntp4/most_visited_page.js
+++ b/chrome/browser/resources/ntp4/most_visited_page.js
@@ -94,12 +94,10 @@ cr.define('ntp4', function() {
           'chrome://favicon/size/16/' + data.url;
       faviconDiv.style.backgroundImage = url(faviconUrl);
       faviconDiv.dir = data.direction;
-      if (data.faviconDominantColor) {
-        this.setStripeColor(data.faviconDominantColor);
-      } else {
-        chrome.send('getFaviconDominantColor',
-            [faviconUrl, id, 'ntp4.setMostVisitedFaviconDominantColor']);
-      }
+      if (data.faviconDominantColor)
+        this.stripeColor = data.faviconDominantColor;
+      else
+        chrome.send('getFaviconDominantColor', [faviconUrl, this.id]);
 
       var title = this.querySelector('.title');
       title.textContent = data.title;
@@ -118,7 +116,7 @@ cr.define('ntp4', function() {
      * Sets the color of the favicon dominant color bar.
      * @param {string} color The css-parsable value for the color.
      */
-    setStripeColor: function(color) {
+    set stripeColor(color) {
       this.querySelector('.color-stripe').style.backgroundColor = color;
     },
 
@@ -414,15 +412,8 @@ cr.define('ntp4', function() {
     return oldData;
   };
 
-  function setMostVisitedFaviconDominantColor(id, color) {
-    var tile = $('most-visited-tile-' + id);
-    if (tile)
-      tile.setStripeColor(color);
-  };
-
   return {
     MostVisitedPage: MostVisitedPage,
     refreshData: refreshData,
-    setMostVisitedFaviconDominantColor: setMostVisitedFaviconDominantColor,
   };
 });
diff --git a/chrome/browser/resources/ntp4/new_tab.js b/chrome/browser/resources/ntp4/new_tab.js
index 6927a69..0bd56a3 100644
--- a/chrome/browser/resources/ntp4/new_tab.js
+++ b/chrome/browser/resources/ntp4/new_tab.js
@@ -793,6 +793,19 @@ cr.define('ntp4', function() {
     bookmarksPage.bookmarkNodeRemoved(id, removeInfo);
   };
 
+  /**
+   * Set the dominant color for a node. This will be called in response to
+   * getFaviconDominantColor. The node represented by |id| better have a setter
+   * for stripeColor.
+   * @param {string} id The ID of a node.
+   * @param {string} color The color represented as a CSS string.
+   */
+  function setStripeColor(id, color) {
+    var node = $(id);
+    if (node)
+      node.stripeColor = color;
+  };
+
   // Return an object with all the exports
   return {
     appAdded: appAdded,
@@ -818,6 +831,7 @@ cr.define('ntp4', function() {
     setBookmarksData: setBookmarksData,
     setMostVisitedPages: setMostVisitedPages,
     setRecentlyClosedTabs: setRecentlyClosedTabs,
+    setStripeColor: setStripeColor,
     showNotification: showNotification,
     themeChanged: themeChanged,
     updateOfflineEnabledApps: updateOfflineEnabledApps
diff --git a/chrome/browser/ui/webui/ntp/favicon_webui_handler.cc b/chrome/browser/ui/webui/ntp/favicon_webui_handler.cc
index e3eb124..4627690 100644
--- a/chrome/browser/ui/webui/ntp/favicon_webui_handler.cc
+++ b/chrome/browser/ui/webui/ntp/favicon_webui_handler.cc
@@ -40,7 +40,8 @@ class ExtensionIconColorManager : public ExtensionIconManager {
 };
 
 FaviconWebUIHandler::FaviconWebUIHandler()
-    : app_icon_color_manager_(new ExtensionIconColorManager(this)) {
+    : id_(0),
+      app_icon_color_manager_(new ExtensionIconColorManager(this)) {
 }
 
 FaviconWebUIHandler::~FaviconWebUIHandler() {
@@ -61,12 +62,9 @@ void FaviconWebUIHandler::HandleGetFaviconDominantColor(const ListValue* args) {
       "path is " << path;
   path = path.substr(arraysize("chrome://favicon/size/16/") - 1);
 
-  double id;
-  CHECK(args->GetDouble(1, &id));
-
-  std::string callback_name;
-  CHECK(args->GetString(2, &callback_name));
-  callbacks_map_[static_cast<int>(id)] = callback_name;
+  std::string dom_id;
+  CHECK(args->GetString(1, &dom_id));
+  dom_id_map_[id_] = dom_id;
 
   FaviconService* favicon_service =
       Profile::FromWebUI(web_ui_)->GetFaviconService(Profile::EXPLICIT_ACCESS);
@@ -78,7 +76,7 @@ void FaviconWebUIHandler::HandleGetFaviconDominantColor(const ListValue* args) {
       history::FAVICON,
       &consumer_,
       NewCallback(this, &FaviconWebUIHandler::OnFaviconDataAvailable));
-  consumer_.SetClientData(favicon_service, handle, static_cast<int>(id));
+  consumer_.SetClientData(favicon_service, handle, id_++);
 }
 
 void FaviconWebUIHandler::OnFaviconDataAvailable(
@@ -87,21 +85,22 @@ void FaviconWebUIHandler::OnFaviconDataAvailable(
   FaviconService* favicon_service =
       Profile::FromWebUI(web_ui_)->GetFaviconService(Profile::EXPLICIT_ACCESS);
   int id = consumer_.GetClientData(favicon_service, request_handle);
-  base::FundamentalValue id_value(id);
   scoped_ptr<StringValue> color_value;
 
   if (favicon.is_valid()) {
     // TODO(estade): cache the response
-    color_value.reset(GetDominantColor(favicon.image_data));
+    color_value.reset(GetDominantColorCssString(favicon.image_data));
   } else {
     color_value.reset(new StringValue("#919191"));
   }
 
-  web_ui_->CallJavascriptFunction(callbacks_map_[id], id_value, *color_value);
-  callbacks_map_.erase(id);
+  web_ui_->CallJavascriptFunction("ntp4.setStripeColor",
+                                  StringValue(dom_id_map_[id]),
+                                  *color_value);
+  dom_id_map_.erase(id);
 }
 
-StringValue* FaviconWebUIHandler::GetDominantColor(
+StringValue* FaviconWebUIHandler::GetDominantColorCssString(
     scoped_refptr<RefCountedMemory> png) {
   color_utils::GridSampler sampler;
   SkColor color = color_utils::CalculateKMeanColorOfPNG(png, 100, 665, sampler);
@@ -114,22 +113,8 @@ StringValue* FaviconWebUIHandler::GetDominantColor(
 
 void FaviconWebUIHandler::HandleGetAppIconDominantColor(
     const ListValue* args) {
-  std::string path;
-  CHECK(args->GetString(0, &path));
-  GURL gurl(path);
-  DCHECK(StartsWithASCII(path, chrome::kChromeUIExtensionIconURL, false)) <<
-      "path is " << path;
-
-  std::string callback_name;
-  CHECK(args->GetString(1, &callback_name));
-
-  // See ExtensionIconSource::ParseData
-  std::string path_lower = StringToLowerASCII(gurl.path());
-  std::vector<std::string> path_parts;
-  base::SplitString(path_lower, '/', &path_parts);
-  CHECK(path_parts.size() > 1);
-  std::string extension_id = path_parts.at(1);
-  app_callbacks_map_[extension_id] = callback_name;
+  std::string extension_id;
+  CHECK(args->GetString(0, &extension_id));
 
   ExtensionService* extension_service =
       Profile::FromWebUI(web_ui_)->GetExtensionService();
@@ -148,9 +133,7 @@ void FaviconWebUIHandler::NotifyAppIconReady(const std::string& extension_id) {
   CHECK(gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, true, &bits));
   scoped_refptr<RefCountedStaticMemory> bits_mem(
       new RefCountedStaticMemory(&bits.front(), bits.size()));
-  scoped_ptr<StringValue> color_value(GetDominantColor(bits_mem));
-  StringValue extension_id_value(extension_id);
+  scoped_ptr<StringValue> color_value(GetDominantColorCssString(bits_mem));
   web_ui_->CallJavascriptFunction(
-      app_callbacks_map_[extension_id], extension_id_value, *color_value);
-  app_callbacks_map_.erase(extension_id);
+      "ntp4.setStripeColor", StringValue(extension_id), *color_value);
 }
diff --git a/chrome/browser/ui/webui/ntp/favicon_webui_handler.h b/chrome/browser/ui/webui/ntp/favicon_webui_handler.h
index 2e9430a..0393c10 100644
--- a/chrome/browser/ui/webui/ntp/favicon_webui_handler.h
+++ b/chrome/browser/ui/webui/ntp/favicon_webui_handler.h
@@ -30,7 +30,12 @@ class FaviconWebUIHandler : public WebUIMessageHandler {
   // WebUIMessageHandler
   virtual void RegisterMessages() OVERRIDE;
 
+  // Called from the JS to get the dominant color of a favicon. The first
+  // argument is a favicon URL, the second is the ID of the DOM node that is
+  // asking for it.
   void HandleGetFaviconDominantColor(const base::ListValue* args);
+
+  // As above, but for an app tile. The sole argument is the extension ID.
   void HandleGetAppIconDominantColor(const base::ListValue* args);
 
   // Callback getting signal that an app icon is loaded.
@@ -40,21 +45,22 @@ class FaviconWebUIHandler : public WebUIMessageHandler {
   // Called when favicon data is available from the history backend.
   void OnFaviconDataAvailable(FaviconService::Handle request_handle,
                               history::FaviconData favicon);
-  base::StringValue* GetDominantColor(scoped_refptr<RefCountedMemory> png);
+  base::StringValue* GetDominantColorCssString(
+      scoped_refptr<RefCountedMemory> png);
 
   CancelableRequestConsumerTSimple<int> consumer_;
 
+  // Map from request ID to DOM ID so we can make the appropriate callback when
+  // the favicon request comes back. This map exists because
+  // CancelableRequestConsumerTSimple only takes POD keys.
+  std::map<int, std::string> dom_id_map_;
+  // A counter to track ID numbers as we use them.
+  int id_;
+
   // Raw PNG representation of the favicon to show when the favicon
   // database doesn't have a favicon for a webpage.
   scoped_refptr<RefCountedMemory> default_favicon_;
 
-  // A mapping of favicon ID to callback names for requests that are
-  // in-progress.
-  std::map<int, std::string> callbacks_map_;
-
-  // A mapping of extension ID to callback name for app requests in progress.
-  std::map<std::string, std::string> app_callbacks_map_;
-
   // Manage retrieval of icons from apps.
   scoped_ptr<ExtensionIconColorManager> app_icon_color_manager_;