[Icons NTP] Add LargeIconUrlParser.

chrome/common/favicon/large_icon_url_parser.cc

+// Copyright 2015 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/common/favicon/large_icon_url_parser.h"
+
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
+#include "third_party/skia/include/utils/SkParse.h"
+#include "ui/gfx/favicon_size.h"
+
+namespace chrome {
+
+LargeIconUrlParser::LargeIconUrlParser() : size_in_pixels_(48) {
+}
+
+LargeIconUrlParser::~LargeIconUrlParser() {
+}
+
+bool LargeIconUrlParser::Parse(const std::string& path) {
+  if (path.empty())
+    return false;
+
+  size_t slash = path.find("/", 0);

[Roger McFarlane (Chromium), 2015/03/17 20:21:41]

A comment that the path you expect to be given actually starts AFTER the initial
slash in the URL path.

Otherwise, I would expect that for

   chrome-search://large_icon/48/...

The path you would be receiving on would be /48/...

[huangs, 2015/03/18 18:01:48]

Added comment in .h file and here.

+  if (slash == std::string::npos)
+    return false;
+  std::string size_str = path.substr(0, slash);

[Roger McFarlane (Chromium), 2015/03/17 20:21:41]

micro-optimization:

  StringPiece size_str(path.c_str(), slash);

this better handles malicious input.

For the expected valid inputs on platforms where string has
small-string-optimization enabled, it's more or less equivalent.

[huangs, 2015/03/18 18:01:48]

Done, made Parse() take StringPiece(), and added crbug.com/468320 to refactor
sibling classes.

+  if (!size_str.empty() && !base::StringToInt(size_str, &size_in_pixels_))
+    return false;
+
+  // Need to store the index of the URL field, so Instant Extended can translate
+  // large icon URLs using advanced parameters.
+  // Example:
+  //   "chrome-search://large-icon/48/<renderer-id>/<most-visited-id>"
+  // would be translated to:
+  //   "chrome-search://large-icon/48/<most-visited-item-with-given-id>".
+  path_index_ = slash + 1;
+  url_string_ = path.substr(path_index_);

[beaudoin, 2015/03/17 23:59:14]

url_string_ is a bit misleading here, since it's not necessarily a URL? If I
understand the comment above it's more like a |resource_str| which can be either
of the form <renderer-id>/<most-visited-id> or <url> but this method doesn't
test the validity of the resource_str. Correct? If so, please make the comment
clearer and rename the variable.

[huangs, 2015/03/18 18:01:48]

I'd prefer to to this as a separate refactoring task, since the sibling parsers
and callers should be changed as well.  Added crbug.com/468320 to track this.

[beaudoin, 2015/03/18 18:35:29]

Acknowledged.

+  return true;
+}
+
+}  // namespace chrome