Supporting high dpi favicons in Instant Extended.

chrome/renderer/searchbox/searchbox.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/renderer/searchbox/searchbox.h"
 
 #include <string>
 
 #include "base/strings/string_number_conversions.h"
 #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"
 #include "chrome/renderer/searchbox/searchbox_extension.h"
 #include "content/public/renderer/render_view.h"
 #include "googleurl/src/gurl.h"
 #include "grit/renderer_resources.h"
 #include "net/base/escape.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebView.h"
 #include "ui/base/resource/resource_bundle.h"
 
 namespace {
 
 // Size of the results cache.
 const size_t kMaxInstantAutocompleteResultItemCacheSize = 100;
 
 }  // namespace
 
 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.
 //
 // Valid |url| forms:

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

Update this comment since this is only expected to be used for
chrome-search://thumb

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

Done.

 // chrome-search://favicon/<view_id>/<restricted_id>
 // chrome-search://thumb/<view_id>/<restricted_id>
 //
 // If the |url| is valid, returns true and fills in |id| with restricted_id
 // value. If the |url| is invalid, returns false and |id| is not set.
 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
 
 SearchBox::SearchBox(content::RenderView* render_view)
     : content::RenderViewObserver(render_view),
       content::RenderViewObserverTracker<SearchBox>(render_view),
       verbatim_(false),
       query_is_restricted_(false),
       selection_start_(0),
@@ skipping 132 matching lines @@
   GURL most_visited_item_url(GetURLForMostVisitedItem(rid));
   if (most_visited_item_url.is_empty())
     return false;
   *url = GURL(base::StringPrintf("chrome-search://thumb/%s",
                                  most_visited_item_url.spec().c_str()));
   return true;
 }
 
 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);
+
+  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.

   }
-
-  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) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(SearchBox, message)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxChange, OnChange)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxSubmit, OnSubmit)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxCancel, OnCancel)
     IPC_MESSAGE_HANDLER(ChromeViewMsg_SearchBoxPopupResize, OnPopupResize)
@@ skipping 288 matching lines @@
   InstantMostVisitedItem item;
   return GetMostVisitedItemWithID(item_id, &item) ? item.url : GURL();
 }
 
 void SearchBox::OnToggleVoiceSearch() {
   if (render_view()->GetWebView() && render_view()->GetWebView()->mainFrame()) {
     extensions_v8::SearchBoxExtension::DispatchToggleVoiceSearch(
         render_view()->GetWebView()->mainFrame());
   }
 }