[Icons NTP] Enable Large Icon URL storage and image fetching (Touch Icons only), behind flag.

[Icons NTP] Enable Large Icon URL storage and image fetching (Touch Icons only), behind flag.

This works with crrev.com/1007563004 so URLs for Big Icons (currently only
Apple Touch Icon) are saved into Favicon database, and the icons get fetched
and stored. The feature is hidden behind flag "IconNTP/Enabled". rogerm@ is
working on bounding storage size of large icons.

Also doing some refactoring on FaviconHandler: existing has some ambiguity
on "handler type" and "icon type".

BUG=467712

[huangs, 2015-03-17 19:44:30]

huangs@chromium.org changed reviewers:
+ mathp@chromium.org, sdefresne@chromium.org

[huangs, 2015-03-17 19:44:31]

PTAL.

sdefresne@: The entire flow.
mathp@: Usage of flags to control feature.

Thanks!

[Roger McFarlane (Chromium), 2015-03-17 20:09:58]

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

[Roger McFarlane (Chromium), 2015-03-17 20:10:24]

lgtm

[Mathieu, 2015-03-17 20:13:28]

field trial code lgtm, sadly I don't have enough background on favicon fetching
to review the rest.

[beaudoin, 2015-03-17 20:57:52]

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

[beaudoin, 2015-03-17 20:57:52]

LGTM

[sdefresne, 2015-03-18 09:53:12]

lgtm

[huangs, 2015-03-18 14:50:07]

The CQ bit was checked by huangs@chromium.org

[commit-bot: I haz the power, 2015-03-18 14:50:22]

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

[commit-bot: I haz the power, 2015-03-18 15:48:16]

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

[commit-bot: I haz the power, 2015-03-18 15:48:54]

Committed patchset #1 (id:1)

[sky, 2015-05-06 19:29:04]

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

[sky, 2015-05-06 19:29:05]

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:48: bool IsIconNTPEnabled() {
Will this ever be used on android?

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
File components/favicon/core/browser/favicon_handler.h (right):

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
components/favicon/core/browser/favicon_handler.h:89: static int
GetIconTypesFromHandlerType(Type icon_type);
Why is this public?

[huangs, 2015-05-06 19:59:14]

Replied. Is this CL causing problem somewhere?

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:48: bool IsIconNTPEnabled() {
Currently it isn't, and likely Android will do things differently but we have to
verify that.

Should we wrap this under !defined(OS_ANDROID) then, and do something similar to
download_largest_icon in line 59?

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
File components/favicon/core/browser/favicon_handler.h (right):

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
components/favicon/core/browser/favicon_handler.h:89: static int
GetIconTypesFromHandlerType(Type icon_type);
An oversight from my part.  Move to private?

[sky, 2015-05-06 20:13:54]

Your patch isn't causing problems. Roger asked me to review
https://codereview.chromium.org/1119163003/ and I handed see the LARGE type and
dug up this patch.

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:71: if (IsIconNTPEnabled())
My concern is more of here. If IsIconNTPEnabled is true on android it would mean
downloading icons twice. I think this should be an else (with a comment).
Otherwise we may end up downloading the same set of icons twice.

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
File components/favicon/core/browser/favicon_handler.h (right):

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
components/favicon/core/browser/favicon_handler.h:89: static int
GetIconTypesFromHandlerType(Type icon_type);
On 2015/05/06 19:59:14, huangs wrote:
> An oversight from my part.  Move to private?

Yes please.

[huangs, 2015-05-06 20:33:55]

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:71: if (IsIconNTPEnabled())
rogerm@ has considered this, and has a CL to make this cleaner:
https://crrev.com/1118073002/

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
File components/favicon/core/browser/favicon_handler.h (right):

https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
components/favicon/core/browser/favicon_handler.h:89: static int
GetIconTypesFromHandlerType(Type icon_type);
On 2015/05/06 20:13:54, sky wrote:
> On 2015/05/06 19:59:14, huangs wrote:
> > An oversight from my part.  Move to private?
> 
> Yes please.

Acknowledged.

[sky, 2015-05-06 20:34:36]

handed? Wow. I mean 'and I searched for LARGE type and came across your patch.

On Wed, May 6, 2015 at 1:13 PM,  <sky@chromium.org> wrote:
> Your patch isn't causing problems. Roger asked me to review
> https://codereview.chromium.org/1119163003/ and I handed see the LARGE type
> and
> dug up this patch.
>
>
>
https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
> File chrome/browser/favicon/favicon_tab_helper.cc (right):
>
>
https://codereview.chromium.org/1010293002/diff/1/chrome/browser/favicon/favi...
> chrome/browser/favicon/favicon_tab_helper.cc:71: if (IsIconNTPEnabled())
> My concern is more of here. If IsIconNTPEnabled is true on android it
> would mean downloading icons twice. I think this should be an else (with
> a comment). Otherwise we may end up downloading the same set of icons
> twice.
>
>
https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
> File components/favicon/core/browser/favicon_handler.h (right):
>
>
https://codereview.chromium.org/1010293002/diff/1/components/favicon/core/bro...
> components/favicon/core/browser/favicon_handler.h:89: static int
> GetIconTypesFromHandlerType(Type icon_type);
> On 2015/05/06 19:59:14, huangs wrote:
>>
>> An oversight from my part.  Move to private?
>
>
> Yes please.
>
> https://codereview.chromium.org/1010293002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.