[Icons NTP] Allow chrome-search:// large-icon and fallback-icon hosts to use <view_id>/<restricted_…

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 9adce8536714bf97658937ee385048a46db5c780..03c3d89f807354dc0929d29395b882d97eb96447 100644
--- a/chrome/renderer/searchbox/searchbox.cc
+++ b/chrome/renderer/searchbox/searchbox.cc
@@ -10,7 +10,9 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/favicon/fallback_icon_url_parser.h"
 #include "chrome/common/favicon/favicon_url_parser.h"
+#include "chrome/common/favicon/large_icon_url_parser.h"
 #include "chrome/common/omnibox_focus_state.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
@@ -115,6 +117,59 @@ bool GetRestrictedIDFromFaviconUrl(int render_view_id,
   return true;
 }
 
+// Similar to GetRestrictedIDFromFaviconUrl(), but for large icons.
+bool GetRestrictedIDFromLargeIconUrl(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);
+
+  LargeIconUrlParser parser;
+  if (!parser.Parse(raw_path))
+    return false;
+
+  *favicon_params = raw_path.substr(0, parser.path_index());
+
+  std::string id_part = raw_path.substr(parser.path_index());
+  InstantRestrictedID id;
+  if (!GetInstantRestrictedIDFromPath(render_view_id, id_part, &id))
+    return true;
+
+  *rid = id;
+  return true;
+}
+
+// Similar to GetRestrictedIDFromFaviconUrl(), but for fallback icons.
+bool GetRestrictedIDFromFallbackIconUrl(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::ParsedFallbackIconPath parser;
+  if (!parser.Parse(raw_path))
+    return false;
+
+  *favicon_params = raw_path.substr(0, parser.path_index());
+
+  std::string id_part = raw_path.substr(parser.path_index());
+  InstantRestrictedID id;
+  if (!GetInstantRestrictedIDFromPath(render_view_id, id_part, &id))
+    return true;
+
+  *rid = id;
+  return true;

[beaudoin, 2015/03/19 02:17:35]

This looks like three copies of the same code with a different parser. Couldn't
you extract that to a helper function and just pass in a different parser to
each?

I'm fine doing the more involved clean-up you propose later, but for the moment
I'd be happy if each method just called something like:
  return GetRestrictedIDFromUrl(render_view_id, url,
chrome::ParsedFallbackIconPath(), favicon_params, rid);

Looking at the code below, I don't even think you need these three flavors, you
can just instantiate a different parser in the three Generate*FromTransientURL.

[huangs, 2015/03/19 02:23:30]

I have grand plans to refactor these.  Triplicating code like a n00b is to
mitigate risk; the diff is add-only, I won't have to change how existing code
works, so easier to review and less chance of merge issues.  crbug.com/468320
states my plans to pay back the debt.

+}
+
+
 // Parses a thumbnail |url| and fills in |id| with the InstantRestrictedID
 // obtained from the |url|. |render_view_id| is the ID of the associated
 // RenderView.
@@ -210,6 +265,47 @@ bool SearchBox::GenerateFaviconURLFromTransientURL(const GURL& transient_url,
   return true;
 }
 
+bool SearchBox::GenerateLargeIconURLFromTransientURL(const GURL& transient_url,
+                                                     GURL* url) const {
+  std::string favicon_params;
+  InstantRestrictedID rid = -1;
+  bool success = internal::GetRestrictedIDFromLargeIconUrl(
+      render_view()->GetRoutingID(), transient_url, &favicon_params, &rid);
+  if (!success)
+    return false;
+
+  InstantMostVisitedItem item;
+  std::string item_url;
+  if (rid != -1 && GetMostVisitedItemWithID(rid, &item))
+    item_url = item.url.spec();
+
+  *url = GURL(base::StringPrintf("chrome-search://large-icon/%s%s",
+                                 favicon_params.c_str(),
+                                 item_url.c_str()));
+  return true;
+}
+
+bool SearchBox::GenerateFallbackIconURLFromTransientURL(
+    const GURL& transient_url,
+    GURL* url) const {
+  std::string favicon_params;
+  InstantRestrictedID rid = -1;
+  bool success = internal::GetRestrictedIDFromFallbackIconUrl(
+      render_view()->GetRoutingID(), transient_url, &favicon_params, &rid);
+  if (!success)
+    return false;
+
+  InstantMostVisitedItem item;
+  std::string item_url;
+  if (rid != -1 && GetMostVisitedItemWithID(rid, &item))
+    item_url = item.url.spec();
+
+  *url = GURL(base::StringPrintf("chrome-search://fallback-icon/%s%s",
+                                 favicon_params.c_str(),
+                                 item_url.c_str()));
+  return true;

[beaudoin, 2015/03/19 02:17:35]

Same here, I'd be fine with an
internal::GenerateURLFromTransientURL(transient_url, parser, endpoint) that
these three methods would call.

[huangs, 2015/03/19 02:23:30]

Same as above.

+}
+
 bool SearchBox::GenerateThumbnailURLFromTransientURL(const GURL& transient_url,
                                                      GURL* url) const {
   InstantRestrictedID rid = 0;