[Icons NTP] Add LargeIconUrlParser.

[Icons NTP] Add LargeIconUrlParser.

LargeIconUrlParser is the parser for the path portion of the (upcoming)
chrome-search://large-icon end point. This CL adds the code, but only
the unit test is active.

BUG=467712
Committed: https://crrev.com/915a250403597745e30309705d9cad7ac895fba6
Cr-Commit-Position: refs/heads/master@{#321244}

[huangs, 2015-03-17 19:54:07]

huangs@chromium.org changed reviewers:
+ thakis@chromium.org

[huangs, 2015-03-17 19:54:07]

PTAL.

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

rogerm@chromium.org changed reviewers:
+ rogerm@chromium.org

[Roger McFarlane (Chromium), 2015-03-17 20:21:42]

https://chromiumcodereview.appspot.com/1014853004/diff/1/chrome/common/favico...
File chrome/common/favicon/large_icon_url_parser.cc (right):

https://chromiumcodereview.appspot.com/1014853004/diff/1/chrome/common/favico...
chrome/common/favicon/large_icon_url_parser.cc:26: size_t slash = path.find("/",
0);
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/...

https://chromiumcodereview.appspot.com/1014853004/diff/1/chrome/common/favico...
chrome/common/favicon/large_icon_url_parser.cc:29: std::string size_str =
path.substr(0, slash);
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.

https://chromiumcodereview.appspot.com/1014853004/diff/1/chrome/common/favico...
File chrome/common/favicon/large_icon_url_parser.h (right):

https://chromiumcodereview.appspot.com/1014853004/diff/1/chrome/common/favico...
chrome/common/favicon/large_icon_url_parser.h:15: class LargeIconUrlParser {
comment?

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

beaudoin@chromium.org changed reviewers:
+ beaudoin@chromium.org

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

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
File chrome/common/favicon/large_icon_url_parser.cc (right):

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.cc:40: url_string_ =
path.substr(path_index_);
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.

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
File chrome/common/favicon/large_icon_url_parser.h (right):

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.h:34: // The page URL string of the
fallback icon is requested for.
My natural language parser fails on this comment. :)

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

Updated, PTAL.

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
File chrome/common/favicon/large_icon_url_parser.cc (right):

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.cc:26: size_t slash = path.find("/",
0);
Added comment in .h file and here.

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.cc:29: std::string size_str =
path.substr(0, slash);
Done, made Parse() take StringPiece(), and added crbug.com/468320 to refactor
sibling classes.

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.cc:40: url_string_ =
path.substr(path_index_);
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.

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
File chrome/common/favicon/large_icon_url_parser.h (right):

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.h:15: class LargeIconUrlParser {
On 2015/03/17 20:21:41, Roger McFarlane (Chromium) wrote:
> comment?

Done.

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.h:34: // The page URL string of the
fallback icon is requested for.
Done. Also s/fallback/large/g.

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

LGTM

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
File chrome/common/favicon/large_icon_url_parser.cc (right):

https://codereview.chromium.org/1014853004/diff/1/chrome/common/favicon/large...
chrome/common/favicon/large_icon_url_parser.cc:40: url_string_ =
path.substr(path_index_);
On 2015/03/18 18:01:48, huangs wrote:
> 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.

Acknowledged.

[huangs, 2015-03-18 18:42:09]

huangs@chromium.org changed reviewers:
+ thestig@chromium.org
- thakis@chromium.org

[huangs, 2015-03-18 18:42:39]

OWNER review to thestig@ (thakis@ seems unavaiable), PTAL.  Thanks!

[Lei Zhang, 2015-03-18 18:45:03]

Who will call into this code? Does it belong in chrome/common?

[huangs, 2015-03-18 18:59:24]

This will be called by LargeIconSource
(https://codereview.chromium.org/1016833003/) and SearchBox
(https://codereview.chromium.org/1017853002/).  I'm following the pattern as
favicon_url_parser*.

Integrated prototype for reference: https://codereview.chromium.org/1010783002/

[Lei Zhang, 2015-03-18 19:00:45]

lgtm with some nits.

It looks to me c/c/favicon should really be somewhere in c/b instead, but that's
outside the scope of this CL.

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
File chrome/common/favicon/large_icon_url_parser.h (right):

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser.h:10: #include
"base/gtest_prod_util.h"
not needed?

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser.h:14: namespace chrome {
We've decided to stop using namespace chrome. Please do not add more.

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser.h:34: 
nit: no blank line here. "git cl lint" can probably tell you that.

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
File chrome/common/favicon/large_icon_url_parser_unittest.cc (right):

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser_unittest.cc:52: const char*
test_cases[] = {
const char* const

[Lei Zhang, 2015-03-18 19:02:18]

On 2015/03/18 19:00:45, Lei Zhang wrote:
> It looks to me c/c/favicon should really be somewhere in c/b instead, but
that's
> outside the scope of this CL.
> 

... and scratch that. I see it's used in c/r. @_@

[huangs, 2015-03-18 20:18:04]

Thanks! Committing soon.

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
File chrome/common/favicon/large_icon_url_parser.h (right):

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser.h:10: #include
"base/gtest_prod_util.h"
On 2015/03/18 19:00:45, Lei Zhang wrote:
> not needed?

Done.

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser.h:14: namespace chrome {
On 2015/03/18 19:00:45, Lei Zhang wrote:
> We've decided to stop using namespace chrome. Please do not add more.

Done.

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser.h:34: 
On 2015/03/18 19:00:45, Lei Zhang wrote:
> nit: no blank line here. "git cl lint" can probably tell you that.

Done.

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
File chrome/common/favicon/large_icon_url_parser_unittest.cc (right):

https://codereview.chromium.org/1014853004/diff/20001/chrome/common/favicon/l...
chrome/common/favicon/large_icon_url_parser_unittest.cc:52: const char*
test_cases[] = {
On 2015/03/18 19:00:45, Lei Zhang wrote:
> const char* const

Done.

[huangs, 2015-03-18 22:30:37]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-18 22:30:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, beaudoin@chromium.org
Link to the patchset: https://codereview.chromium.org/1014853004/#ps100001
(title: "Update comment for test case.")

[commit-bot: I haz the power, 2015-03-18 22:32:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1014853004/100001

[commit-bot: I haz the power, 2015-03-19 00:07:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-19 00:08:22]

Patchset 6 (id:??) landed as
https://crrev.com/915a250403597745e30309705d9cad7ac895fba6
Cr-Commit-Position: refs/heads/master@{#321244}