Use a more common and easily parsed format for chrome-search://ntpicon

Instead of using custom dir/like/things, let's use the more standard:

  ?param1=val1&param2=val2

Because it allows us to use URLSearchParams[1] in JS:

  url.searchParams.set('param1', 'val1');

And net::QueryIterator in C++:

  for (net::QueryIterator it(url); !it.IsAtEnd(); it.Advance()) {
    // use it.GetKey() and it.GetValue()
  }

R=kristipark@chromium.org
BUG=953962

[1] https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams

Change-Id: I8159e0f6d70b35bc803e9e1064c67ee27a93244b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1574690
Auto-Submit: Dan Beam <dbeam@chromium.org>
Reviewed-by: Kristi Park <kristipark@chromium.org>
Commit-Queue: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652623}

chrome/browser/resources/local_ntp/most_visited_single.js

@@ -490,9 +490,8 @@ var addTile = function(args) {
     }
 
     data.tid = data.rid;
-    // Use a dark icon if dark mode is enabled. Keep value in sync with
-    // NtpIconSource.
-    data.dark = args.darkMode ? 'dark/' : '';
+    // Use a dark icon if dark mode is enabled.
+    data.dark = args.darkMode;
     if (!data.faviconUrl) {
       data.faviconUrl = 'chrome-search://favicon/size/16@' +
           window.devicePixelRatio + 'x/' + data.renderViewId + '/' + data.tid;
@@ -746,8 +745,11 @@ function renderMaterialDesignTile(data) {
     // Set title and alt to empty so screen readers won't say the image name.
     fi.title = '';
     fi.alt = '';
-    fi.src = 'chrome-search://ntpicon/size/24@' + window.devicePixelRatio +
-        'x/' + data.dark + data.url;
+    const url = new URL('chrome-search://ntpicon/');
+    url.searchParams.set('dark', data.dark);
+    url.searchParams.set('size', '24@' + window.devicePixelRatio + 'x');
+    url.searchParams.set('url', data.url);
+    fi.src = url.toString();
     loadedCounter += 1;
     fi.addEventListener('load', function(ev) {
       // Store the type for a potential later navigation.

chrome/browser/search/ntp_icon_source.cc

@@ -13,6 +13,7 @@
 #include "base/callback.h"
 #include "base/hash/sha1.h"
 #include "base/memory/ref_counted_memory.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "build/build_config.h"
@@ -24,6 +25,7 @@
 #include "chrome/browser/search/instant_io_context.h"
 #include "chrome/browser/search/suggestions/suggestions_service_factory.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/common/webui_url_constants.h"
 #include "chrome/grit/platform_locale_settings.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/favicon/core/fallback_url_util.h"
@@ -36,6 +38,7 @@
 #include "components/suggestions/suggestions_service.h"
 #include "content/public/browser/storage_partition.h"
 #include "net/base/escape.h"
+#include "net/base/url_util.h"
 #include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ -63,11 +66,14 @@ const char kImageFetcherUmaClientName[] = "NtpIconSource";
 // changing the algorithm in RenderIconBitmap() that guarantees contrast.
 constexpr SkColor kFallbackIconLetterColor = SK_ColorWHITE;
 
-// Delimiter in the url that looks for the size specification.
-const char kSizeParameter[] = "size/";
+// Whether or not the requested icon is being rendered on a dark background.
+const char kDarkModeParameter[] = "dark";
 
-// Delimiter in the url for dark mode specification.
-const char kDarkModeParameter[] = "dark/";
+// The requested size of the icon.
+const char kSizeParameter[] = "size";
+
+// The URL for which to create an icon.
+const char kUrlParameter[] = "url";
 
 // Size of the icon background (gray circle), in dp.
 const int kIconSizeDip = 48;
@@ -104,68 +110,48 @@ float GetMaxDeviceScaleFactor() {
   return favicon_scales.back();
 }
 
-// Returns true if |search| is a substring of |path| which starts at
-// |start_index|.
-bool HasSubstringAt(const std::string& path,
-                    size_t start_index,
-                    const std::string& search) {
-  return path.compare(start_index, search.length(), search) == 0;
-}
-
 // Parses the path after chrome-search://ntpicon/. Example path is
-// "size/24@2x/https://cnn.com".
+// "?size=24@2x&url=https%3A%2F%2Fcnn.com"
 const ParsedNtpIconPath ParseNtpIconPath(const std::string& path) {
   ParsedNtpIconPath parsed;
   parsed.url = GURL();
   parsed.size_in_dip = gfx::kFaviconSize;
-  parsed.device_scale_factor = 1.0f;
 
   if (path.empty())
     return parsed;
 
-  // Size specification has to be present.
-  size_t parsed_index = 0;
-  if (!HasSubstringAt(path, parsed_index, kSizeParameter))
-    return parsed;
-
-  parsed_index += strlen(kSizeParameter);
-  size_t slash = path.find("/", parsed_index);
-  if (slash == std::string::npos)
-    return parsed;
-
-  // Parse the size spec (e.g. "24@2x")
-  size_t scale_delimiter = path.find("@", parsed_index);
-  std::string size_str =
-      path.substr(parsed_index, scale_delimiter - parsed_index);
-  std::string scale_str =
-      path.substr(scale_delimiter + 1, slash - scale_delimiter - 1);
-
-  int size_in_dip = 0;
-  if (!base::StringToInt(size_str, &size_in_dip))
-    return parsed;
-  parsed.size_in_dip = std::min(size_in_dip, kMaxIconSizeDip);
-
-  if (!scale_str.empty()) {
-    float scale_factor = 0.0;
-    webui::ParseScaleFactor(scale_str, &scale_factor);
-    // Do not exceed the maximum scale factor for the device.
-    parsed.device_scale_factor =
-        std::min(scale_factor, GetMaxDeviceScaleFactor());
-  }
-
-  parsed_index = slash + 1;
-
-  // Parse the dark mode spec (e.g. "dark"). If present, render a dark mode
-  // icon.
-  if (HasSubstringAt(path, parsed_index, kDarkModeParameter)) {
-    parsed.is_dark_mode = true;
-    slash = path.find("/", parsed_index);
-    if (slash == std::string::npos)
-      return parsed;
-    parsed_index = slash + 1;
+  // NOTE(dbeam): can't start with an empty GURL() and use ReplaceComponents()
+  // because it's not allowed for invalid URLs.
+  GURL request = GURL(base::StrCat({chrome::kChromeSearchScheme, "://",
+                                    chrome::kChromeUINewTabIconHost}))
+                     .Resolve(path);
+
+  for (net::QueryIterator it(request); !it.IsAtEnd(); it.Advance()) {
+    std::string key = it.GetKey();
+    if (key == kDarkModeParameter) {
+      parsed.is_dark_mode = it.GetValue() == "true";
+    } else if (key == kSizeParameter) {
+      std::vector<std::string> pieces =
+          base::SplitString(it.GetUnescapedValue(), "@", base::TRIM_WHITESPACE,
+                            base::SPLIT_WANT_NONEMPTY);
+      if (pieces.empty() || pieces.size() > 2)
+        continue;
+      int size_in_dip = 0;
+      if (!base::StringToInt(pieces[0], &size_in_dip))
+        continue;
+      parsed.size_in_dip = std::min(size_in_dip, kMaxIconSizeDip);
+      if (pieces.size() > 1) {
+        float scale_factor = 0.0;
+        webui::ParseScaleFactor(pieces[1], &scale_factor);
+        // Do not exceed the maximum scale factor for the device.
+        parsed.device_scale_factor =
+            std::min(scale_factor, GetMaxDeviceScaleFactor());
+      }
+    } else if (key == kUrlParameter) {
+      parsed.url = GURL(it.GetUnescapedValue());
+    }
   }
 
-  parsed.url = GURL(path.substr(parsed_index));
   return parsed;
 }
 

chrome/browser/ui/search/local_ntp_browsertest_base.cc

@@ -14,7 +14,7 @@ namespace {
 
 // Delimiter in the Most Visited icon URL that indicates a dark icon. Keep value
 // in sync with NtpIconSource.
-const char kMVIconDarkParameter[] = "/dark/";
+const char kMVIconDarkParameter[] = "dark=true";
 
 }  // namespace