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 1d873975e601c4e1e8fe4a90e35ef3ee56358c63..a46efa6730f291288d67adaaa1ae1c02d8877bb4 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,53 @@ 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);
+}
+
+bool ParseRestrictedFaviconUrl(int render_view_id,
+                               const GURL& url,
+                               std::string* favicon_params,
+                               InstantRestrictedID* rid) {
+  // Strip leading slash.
+  std::string raw_path = url.path();
+  DCHECK_GT(raw_path.length(), (size_t) 0);
+  DCHECK_EQ(raw_path[0], '/');
+  raw_path = raw_path.substr(1);
+
+  chrome::ParsedFaviconPath parsed;
+  if (!chrome::ParseFaviconPath(raw_path, chrome::FAVICON, &parsed))
+    return false;
+
+  // In Instand Extended, the part of the favicon URL which is supposed to

[samarth, 2013/06/20 00:31:29]

No need to keep mentioning Instant Extended (especially in this file, which is
all about Instant Extended).  Just say: "The part of ..." and likewise
elsewhere.

[pedro (no code reviews), 2013/06/20 22:43:45]

Done.

+  // contain the URL from which the favicon is being requested actually
+  // contains a pair in the format "<view_id>/<restricted_id>". If the favicon
+  // URL is not in the expected format then the execution must be stopped,
+  // returning |false|.
+  std::string id_part = raw_path.substr(parsed.path_index);
+  InstantRestrictedID id;
+  if (!GetInstantRestrictedIDFromPath(render_view_id, id_part, &id))
+    return false;
+
+  // The piece of the URL which details the favicon parameters should be
+  // returned so Instant Extended is able to reconstuct the URL, replacing
+  // the restricted_id with the actuall URL from which the favicon is being

[samarth, 2013/06/20 00:31:29]

sp. actuall

[pedro (no code reviews), 2013/06/20 22:43:45]

Done.

+  // requested.
+  *favicon_params = raw_path.substr(0, parsed.path_index);
+  *rid = id;
+  return true;
+}
+
 // Parses |url| and fills in |id| with the InstantRestrictedID obtained from the
 // |url|. |render_view_id| is the ID of the associated RenderView.
 //
@@ -44,18 +93,13 @@ namespace internal {  // for testing
 bool GetInstantRestrictedIDFromURL(int render_view_id,

[samarth, 2013/06/20 00:31:29]

Please rename this function and the one above to be parallel.  Something like
GetRestrictedIDFrom{Favicon|Thumb}URL or the like.

[pedro (no code reviews), 2013/06/20 22:43:45]

Done.

                                    const GURL& url,
                                    InstantRestrictedID* id) {
-  // Strip leading path.
-  std::string path = url.path().substr(1);
+  // Strip leading slash.
+  std::string path = url.path();
+  DCHECK_GT(path.length(), (size_t) 0);
+  DCHECK_EQ(path[0], '/');
+  path = 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
@@ -208,17 +252,20 @@ bool SearchBox::GenerateThumbnailURLFromTransientURL(const GURL& transient_url,
 
 bool SearchBox::GenerateFaviconURLFromTransientURL(const GURL& transient_url,
                                                    GURL* url) const {
-  InstantRestrictedID rid = 0;
-  if (!internal::GetInstantRestrictedIDFromURL(render_view()->GetRoutingID(),
-                                               transient_url, &rid)) {
-    return false;
-  }
+  std::string favicon_params;
+  InstantRestrictedID rid;
+  bool success = internal::ParseRestrictedFaviconUrl(

[samarth, 2013/06/20 00:31:29]

If the favicon URL parser can't parse this URL, we should just return false here
(which means, we won't try to rewrite the URL).

[pedro (no code reviews), 2013/06/20 22:43:45]

Done.

+      render_view()->GetRoutingID(), transient_url, &favicon_params, &rid);
 
-  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()));
+  InstantMostVisitedItem item;
+  if (success && GetMostVisitedItemWithID(rid, &item)) {
+    GURL item_url = item.url;
+    *url = GURL(base::StringPrintf("chrome-search://favicon/%s%s",
+                                   favicon_params.c_str(),
+                                   item_url.spec().c_str()));
+  } else {
+    *url = GURL("chrome-search://favicon/");

[samarth, 2013/06/20 00:31:29]

This should keep favicon_params part of the string.  That way, you get the 2x
version of the default favicon when appropriate.  (I would just construct the
URL in one place and make sure the item_url string is empty in the cases where
we could convert it properly.)

[pedro (no code reviews), 2013/06/20 22:43:45]

Done.

+  }
   return true;
 }