Add support for favicon scale factor in WebUI

chrome/browser/ui/webui/favicon_source.cc

Index: chrome/browser/ui/webui/favicon_source.cc
diff --git a/chrome/browser/ui/webui/favicon_source.cc b/chrome/browser/ui/webui/favicon_source.cc
index 5016affcf605ce35d98d072ede4d3de4926878b7..959259dc09ca478127770fb7ae9d0e9e84c323c5 100644
--- a/chrome/browser/ui/webui/favicon_source.cc
+++ b/chrome/browser/ui/webui/favicon_source.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/webui/web_ui_util.h"
 #include "chrome/common/url_constants.h"
 #include "grit/locale_settings.h"
 #include "grit/ui_resources.h"
@@ -51,27 +52,44 @@ void FaviconSource::StartDataRequest(const std::string& path,
     return;
   }
 
+  ui::ScaleFactor scale_factor = ui::SCALE_FACTOR_NONE;
+
   FaviconService::Handle handle;
-  if (path.size() > 8 && path.substr(0, 8) == "iconurl/") {
+  if (path.size() > 8 && (path.substr(0, 8) == "iconurl/" ||
+       path.substr(0, 8) == "iconurl@")) {

[pkotwicz, 2012/09/17 19:32:06]

Can you put " (path.substr(0, 8) == iconurl/ ||" on a separate line for clarity?

[kevers, 2012/09/17 21:13:02]

Done.

+    size_t prefix_length = 8;
+    // Optional scale factor appended to iconurl, which may be @1x or @2x.
+    if (path.at(7) == '@') {
+        size_t slash = path.find("/");
+        std::string scale_str = path.substr(8, slash - 8);
+        web_ui_util::ParseScaleFactor(scale_str, &scale_factor);
+        prefix_length = slash + 1;
+    }
     // TODO : Change GetFavicon to support combination of IconType.
     handle = favicon_service->GetRawFavicon(
-        GURL(path.substr(8)),
+        GURL(path.substr(prefix_length)),
         history::FAVICON,
         gfx::kFaviconSize,
-        ui::SCALE_FACTOR_100P,
+        scale_factor,
         &cancelable_consumer_,
         base::Bind(&FaviconSource::OnFaviconDataAvailable,
                    base::Unretained(this)));
   } else {
     GURL url;
-
     if (path.size() > 5 && path.substr(0, 5) == "size/") {
       size_t slash = path.find("/", 5);
+      size_t scale_delimiter = path.find("@", 5);
       std::string size = path.substr(5, slash - 5);
       int pixel_size = atoi(size.c_str());
       CHECK(pixel_size == 32 || pixel_size == 16) <<
           "only 32x32 and 16x16 icons are supported";
-      request_size_map_[request_id] = pixel_size;
+      // Optional scale factor.
+      if (scale_delimiter != path.npos && scale_delimiter < slash) {

[pkotwicz, 2012/09/17 19:32:06]

Can you use std::string::npos instead?

[kevers, 2012/09/17 21:13:02]

Done.

+        std::string scale_str = path.substr(scale_delimiter + 1,
+                                            slash - scale_delimiter - 1);
+        web_ui_util::ParseScaleFactor(scale_str, &scale_factor);
+      }
+      request_size_map_[request_id] = IconSize(pixel_size, scale_factor);

[pkotwicz, 2012/09/17 19:32:06]

Can you use std::map::insert() and thus avoid needing to define the empty
constructor for IconSize() ?

[kevers, 2012/09/17 21:13:02]

Using insert does not appear to eliminate the need for an empty constructor and
the [] operator has a much cleaner syntax.

[Evan Stade, 2012/09/18 13:54:05]

is it possible to delete the map (therefore fixing my todo :) and instead bind
the struct to cancelable consumer in SetClientData? (this also requires making
the struct hold the request id)

[kevers, 2012/09/18 17:31:03]

Done.

       url = GURL(path.substr(slash + 1));
     } else {
       // URL requests prefixed with "origin/" are converted to a form with an
@@ -91,7 +109,7 @@ void FaviconSource::StartDataRequest(const std::string& path,
         url = GURL(path);
       }
 
-      request_size_map_[request_id] = 16;
+      request_size_map_[request_id] = IconSize(16, scale_factor);
     }
 
     // Intercept requests for prepopulated pages.
@@ -102,7 +120,7 @@ void FaviconSource::StartDataRequest(const std::string& path,
         SendResponse(request_id,
             ResourceBundle::GetSharedInstance().LoadDataResourceBytes(
                 history::kPrepopulatedPages[i].favicon_id,
-                ui::SCALE_FACTOR_100P));
+                scale_factor));
         return;
       }
     }
@@ -115,7 +133,7 @@ void FaviconSource::StartDataRequest(const std::string& path,
             icon_types_,
             gfx::kFaviconSize,
             &cancelable_consumer_),
-        ui::SCALE_FACTOR_100P,
+        scale_factor,
         base::Bind(&FaviconSource::OnFaviconDataAvailable,
                    base::Unretained(this)));
   }
@@ -154,18 +172,20 @@ void FaviconSource::OnFaviconDataAvailable(
 
 void FaviconSource::SendDefaultResponse(int request_id) {
   base::RefCountedMemory* bytes = NULL;
-  if (request_size_map_[request_id] == 32) {
+  ui::ScaleFactor scale = request_size_map_[request_id].scale_factor;

[pkotwicz, 2012/09/17 19:32:06]

Nit: can you call this variable scale_factor?
In general whenever I see a variable named scale I think it is a float.

[kevers, 2012/09/17 21:13:02]

Done.

+
+  if (request_size_map_[request_id].pixel_size == 32) {
     if (!default_favicon_large_.get()) {
       default_favicon_large_ =
           ResourceBundle::GetSharedInstance().LoadDataResourceBytes(
-              IDR_DEFAULT_LARGE_FAVICON, ui::SCALE_FACTOR_100P);
+              IDR_DEFAULT_LARGE_FAVICON, scale);
     }
     bytes = default_favicon_large_;
   } else {
     if (!default_favicon_.get()) {
       default_favicon_ =
           ResourceBundle::GetSharedInstance().LoadDataResourceBytes(
-              IDR_DEFAULT_FAVICON, ui::SCALE_FACTOR_100P);
+              IDR_DEFAULT_FAVICON, scale);
     }
     bytes = default_favicon_;
   }