Supporting high dpi favicons in Instant Extended.

chrome/renderer/searchbox/searchbox.cc

Index: chrome/renderer/searchbox/searchbox.cc
diff --git a/chrome/renderer/searchbox/searchbox.cc b/chrome/renderer/searchbox/searchbox.cc
index 041688957012c03af8599285462b561a5504404a..eea84202448631189c2225435e8cb21339065ab4 100644
--- a/chrome/renderer/searchbox/searchbox.cc
+++ b/chrome/renderer/searchbox/searchbox.cc
@@ -10,6 +10,8 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/favicon_types.h"
+#include "chrome/common/favicon_url_parser.h"
 #include "chrome/common/omnibox_focus_state.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
@@ -32,6 +34,20 @@ const size_t kMaxInstantAutocompleteResultItemCacheSize = 100;
 
 namespace internal {  // for testing
 
+bool GetInstantRestrictedIDFromPath(int render_view_id,
+                                    const std::string& path,
+                                    InstantRestrictedID* id) {
+  // Check that the path is of Most visited item ID form.
+  std::vector<std::string> tokens;
+  if (Tokenize(path, "/", &tokens) != 2)
+    return false;
+
+  int view_id = 0;
+  if (!base::StringToInt(tokens[0], &view_id) || view_id != render_view_id)
+    return false;
+  return base::StringToInt(tokens[1], id);
+}
+
 // Parses |url| and fills in |id| with the InstantRestrictedID obtained from the
 // |url|. |render_view_id| is the ID of the associated RenderView.
 //
@@ -47,15 +63,7 @@ bool GetInstantRestrictedIDFromURL(int render_view_id,
   // Strip leading path.
   std::string path = url.path().substr(1);
 
-  // Check that the path is of Most visited item ID form.
-  std::vector<std::string> tokens;
-  if (Tokenize(path, "/", &tokens) != 2)
-    return false;
-
-  int view_id = 0;
-  if (!base::StringToInt(tokens[0], &view_id) || view_id != render_view_id)
-    return false;
-  return base::StringToInt(tokens[1], id);
+  return internal::GetInstantRestrictedIDFromPath(render_view_id, path, id);
 }
 
 }  // namespace internal
@@ -207,18 +215,53 @@ bool SearchBox::GenerateThumbnailURLFromTransientURL(const GURL& transient_url,
 
 bool SearchBox::GenerateFaviconURLFromTransientURL(const GURL& transient_url,

[samarth, 2013/06/14 21:54:46]

Please update the docs in the .h for this.

[pedro (no code reviews), 2013/06/18 22:35:13]

Done.

                                                    GURL* url) const {
-  InstantRestrictedID rid = 0;
-  if (!internal::GetInstantRestrictedIDFromURL(render_view()->GetRoutingID(),
-                                               transient_url, &rid)) {
-    return false;
+  // TODO(pedrosimonetti): DO NOT SUBMIT

[samarth, 2013/06/14 21:54:46]

Take out debugging info

[pedro (no code reviews), 2013/06/18 22:35:13]

Done. I left debugging info in there to make it easier to test while the CL is
being reviewed. That's why I've used "DO NOT SUBMIT". Log messages removed
anyway.

+  LOG(ERROR) << "~~~~~~~~~~~~~ ::WillSendRequest " << transient_url.spec();
+
+  // Strip leading slash.
+  std::string raw_path = transient_url.path().substr(1);

[samarth, 2013/06/14 21:54:46]

Don't just assume: please add checks that the path is non-empty and that the
first character is /. Probably makes sense to be a DCHECK.

[pedro (no code reviews), 2013/06/18 22:35:13]

Done.

+  bool is_icon_url = false;
+  std::string favicon_url;
+  int size_in_dip = 16;

[samarth, 2013/06/14 21:54:46]

This should be set by ParseFaviconPath, no? Don't define a new constant either
(either initialize to 0/-1 or some common constant).

[pedro (no code reviews), 2013/06/18 22:35:13]

I'm using the struct now.

+  ui::ScaleFactor scale_factor = ui::SCALE_FACTOR_100P;
+  std::string params = "";
+
+  bool success = chrome::ParseFaviconPath(raw_path, false, chrome::FAVICON,
+      &is_icon_url, &favicon_url, &size_in_dip, &scale_factor, &params);

[samarth, 2013/06/14 21:54:46]

Just return false here if success is not true. Or better yet, just wrap this
call in an if. I don't see the need to rewrite to chrome-search://invalid.

[samarth, 2013/06/14 21:54:46]

If you follow my comment from the parse and return how much of the URL string
was parsed, the rest of this function will be more straightforward.  You just
need to pass the remainder of the string to the helper function to get a URL and
then join the first part of the string with the second part.

[pedro (no code reviews), 2013/06/18 22:35:13]

Done.

[pedro (no code reviews), 2013/06/18 22:35:13]

Done.

+  // Now that the favicon URL has been parsed, update |raw_path| with
+  // |favicon_url| which is expected to be the id of a Most Visited
+  // item in Instant Extended.
+  raw_path = favicon_url;
+
+  // Try to convert the |raw_path| to int, which is expected to be the id
+  // of a Most Visited item, and then check if there is a Most Visited item
+  // with that id.
+  InstantRestrictedID id;
+  InstantMostVisitedItem item;
+  bool hasId = internal::GetInstantRestrictedIDFromPath(
+      render_view()->GetRoutingID(), raw_path, &id);
+
+  if (success &&
+      hasId &&
+      GetMostVisitedItemWithID(id, &item)) {
+    // Translate the resource URL replacing the id with the URL of the Most
+    // Visited item associated with it. The |params| variable will ensure
+    // that advanced favicon parameters are preserved, allowing high dpi
+    // favicons to be served from chrome-search schema.
+    GURL item_url = item.url;
+    std::string translated_url = transient_url.GetOrigin().spec() + params +
+        // TODO(pedrosimonetti): Do we have to URL-escape the |item_url|?
+        item_url.spec();
+    *url = GURL(translated_url);
+    // TODO(pedrosimonetti): DO NOT SUBMIT
+    LOG(ERROR) << "OK: " << translated_url;
+    return true;
+  } else {
+    *url = GURL(chrome::kChromeSearchInvalidRequestUrl);
+    // TODO(pedrosimonetti): DO NOT SUBMIT
+    LOG(ERROR) << "FAIL: " << transient_url.spec();
+    return true;
   }
-
-  GURL most_visited_item_url(GetURLForMostVisitedItem(rid));
-  if (most_visited_item_url.is_empty())
-    return false;
-  *url = GURL(base::StringPrintf("chrome-search://favicon/%s",
-                                 most_visited_item_url.spec().c_str()));
-  return true;
 }
 
 bool SearchBox::OnMessageReceived(const IPC::Message& message) {