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

[Icons NTP] Allow chrome-search:// large-icon and fallback-icon hosts to use <view_id>/<restricted_id> instead of URL.

For chrome-search://thumbnail on NTP we don't allow direct URL queries,
but only allow <view_id>/<restricted_id>. We're adding the same features
to chrome-search://big-icon and chrome-search://fallback-icon . For the
latter we can actually enable both direct URL and ID's (since URL is only
parsed to extract first letter in domain name) -- but we don't need this
feature right now.

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

[huangs, 2015-03-17 20:44:08]

huangs@chromium.org changed reviewers:
+ beaudoin@chromium.org, jhawkins@chromium.org, rogerm@chromium.org

[huangs, 2015-03-17 20:44:08]

This CL has prerequesite https://codereview.chromium.org/1016833003/
(chrome::kChromeUILargeIconHost), otherwise pretty much independent.

PTAL.

[James Hawkins, 2015-03-17 21:46:41]

This CL needs tests.

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:1383: if (url.host() ==
chrome::kChromeUIFaviconHost)
Shouldn't these all be else if since the hosts are mutually exclusive?

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
chrome/renderer/searchbox/searchbox.cc:120: bool
GetRestrictedIDFromLargeIconUrl(int render_view_id,
nit: Document methods and params.

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
chrome/renderer/searchbox/searchbox.h:64: bool
GenerateLargeIconURLFromTransientURL(const GURL& transient_url,
It seems like having three similarly named methods is less preferable than
having one method which is configurable (say with a parameter).  What do you
think?

[huangs, 2015-03-18 21:43:10]

Updated, PTAL.

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:1383: if (url.host() ==
chrome::kChromeUIFaviconHost)
I thought you don't need else if returning?

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
chrome/renderer/searchbox/searchbox.cc:120: bool
GetRestrictedIDFromLargeIconUrl(int render_view_id,
On 2015/03/17 21:46:41, James Hawkins wrote:
> nit: Document methods and params.

Done.

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/1017853002/diff/1/chrome/renderer/searchbox/s...
chrome/renderer/searchbox/searchbox.h:64: bool
GenerateLargeIconURLFromTransientURL(const GURL& transient_url,
I think we can even do better: Yank all these out into a separate class!

There's high pressure to have the feature though.  I'd rather defer cleanup
later.  Please see crbug.com/468320 to track the refactoring.

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

https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.cc:169: return true;
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.

https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.cc:306: return true;
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]

Responded.

https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.cc:169: return true;
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.

https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.cc:306: return true;
Same as above.

[beaudoin, 2015-03-19 02:30:18]

On 2015/03/19 02:23:30, huangs wrote:
> Responded.
> 
>
https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
> File chrome/renderer/searchbox/searchbox.cc (right):
> 
>
https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
> chrome/renderer/searchbox/searchbox.cc:169: return true;
> 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.
> 
>
https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
> chrome/renderer/searchbox/searchbox.cc:306: return true;
> Same as above.

I would typically hesitate to sign-off on promises of future refactors but this
time I'll let it fly because:
1) You sit behind me so I'll remind you every day until it's done.
2) I trust you'll live in shame until you're name is no longer attach to this.
;)

LGTM if James is also OK with it.

[James Hawkins, 2015-03-19 19:15:16]

On 2015/03/19 02:30:18, beaudoin wrote:
> On 2015/03/19 02:23:30, huangs wrote:
> > Responded.
> > 
> >
>
https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
> > File chrome/renderer/searchbox/searchbox.cc (right):
> > 
> >
>
https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
> > chrome/renderer/searchbox/searchbox.cc:169: return true;
> > 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.
> > 
> >
>
https://codereview.chromium.org/1017853002/diff/20001/chrome/renderer/searchb...
> > chrome/renderer/searchbox/searchbox.cc:306: return true;
> > Same as above.
> 
> I would typically hesitate to sign-off on promises of future refactors but
this
> time I'll let it fly because:
> 1) You sit behind me so I'll remind you every day until it's done.
> 2) I trust you'll live in shame until you're name is no longer attach to this.
> ;)
> 
> LGTM if James is also OK with it.

We should not knowingly introduce poorly factored code for the sake of getting
functionality in faster.  Remember the boy scout rule: always leave it cleaner
than you found it.  In this case I'm OK with leaving it as clean as it was
before, but not worse.

[huangs, 2015-03-20 02:58:55]

Refactored and added many more unit tests, PTAL.  Details:

- favicon & thumb, and the new big-icon & fallback-icon flows are unified.
- Added helper class SearchBox::IconURLHelper so SearchBox's involvement is
decoupled, allowing testing.
- Parsing of view_id/rid is purified (no more parameter injection for checking).

Behavioral change:
- Flow with favicon is more permissive: even if parsing fails, it will return
true and with stub "chrome://favicon".

In fact it will be logical to extract all this parsing functionality into a new
file (ImageSourceType + IconURLHelper + the internal:: stuff in searchbox.cc). 
However, do in this will cannibalize SearchBox's unit test.

I'd rather save further restructuring for later; we really need to get the code
in and I've already spent too much time refactoring this.

[James Hawkins, 2015-03-20 16:26:44]

Looks a lot cleaner, thanks.

LGTM with nit.

https://codereview.chromium.org/1017853002/diff/40001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/1017853002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.h:88: // We do to prevent search providers
from abusing image URLs and deduce
nit: Please clean up the grammar at the beginning of this sentence, "We do to
prevent..."

[huangs, 2015-03-20 17:32:37]

Thanks!  Don't worry I'm not off the hook yet for further cleanup per
crbug.com/468320 (where I'll break the new stuff off to a separate class).

https://codereview.chromium.org/1017853002/diff/40001/chrome/renderer/searchb...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/1017853002/diff/40001/chrome/renderer/searchb...
chrome/renderer/searchbox/searchbox.h:88: // We do to prevent search providers
from abusing image URLs and deduce
On 2015/03/20 16:26:43, James Hawkins wrote:
> nit: Please clean up the grammar at the beginning of this sentence, "We do to
> prevent..."

Done.

[huangs, 2015-03-20 20:48:35]

The CQ bit was checked by huangs@chromium.org

[huangs, 2015-03-20 20:48:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
beaudoin@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/1017853002/#ps100001
(title: "Sync.")

[commit-bot: I haz the power, 2015-03-20 20:50:01]

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

[commit-bot: I haz the power, 2015-03-20 21:30:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-20 21:31:09]

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