Extend LargeIconService to fetch missing favicons from a Google server

Extend LargeIconService to fetch missing favicons from a Google server

(Based on original work by jkrcal@)

Design Doc: go/favicon-pe-google-server

Showing good large icons independently of the local favicon cache is
important for synced content or for content suggested from the
server-side (such as tiles and suggestions on New Tab Page on Android).

This CL extends the service by a dedicated API that allows clients to
fetch favicons/largeicons from Google server. No actual use of the
functionality is introduced in this patch.

BUG=644102
Review-Url: https://codereview.chromium.org/2721363002
Cr-Commit-Position: refs/heads/master@{#459392}
Committed: https://chromium.googlesource.com/chromium/src/+/bf4de3ff1c10a7ca6b1c4ca00451165f18fa1eef

[mastiz, 2017-03-01 16:04:17]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 16:04:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mastiz, 2017-03-01 16:05:45]

mastiz@chromium.org changed reviewers:
+ jkrcal@chromium.org, pkotwicz@chromium.org

[mastiz, 2017-03-01 16:05:45]

Picking up jan's work since he's OOO (original CL was
https://codereview.chromium.org/2685173002/, now trimmed down to its simplest
form).

[commit-bot: I haz the power, 2017-03-01 16:10:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-01 16:10:50]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[mastiz, 2017-03-02 10:03:45]

Description was changed from

==========
Extend LargeIconService to fetch missing favicons from a Google server

(Based on original work by jkrcal@)

Showing good large icons independently of the local favicon cache is
important for synced content or for content suggested from the
server-side (such as tiles and suggestions on New Tab Page on Android).

This CL extends the service by a feature that if the service fails to
find a large enough icon, it returns a fallback icon style and then it
downloads the missing large icons from one of Google favicon servers
and stores in the local favicon cache. The client needs to be an observer of the
history service to find out that the newly fetched favicon is available in the
cache.

Design Doc: go/favicon-pe-google-server

BUG=644102
==========

to

==========
Extend LargeIconService to fetch missing favicons from a Google server

(Based on original work by jkrcal@)

Design Doc: go/favicon-pe-google-server

Showing good large icons independently of the local favicon cache is
important for synced content or for content suggested from the
server-side (such as tiles and suggestions on New Tab Page on Android).

This CL extends the service by a dedicated API that allows clients to
fetch favicons/largeicons from Google server. No actual use of the
functionality is introduced in this patch.

BUG=644102
==========

[mastiz, 2017-03-02 10:05:32]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 10:05:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 10:11:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 10:11:41]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-03-02 10:41:30]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 10:41:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 10:48:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 10:48:29]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[mastiz, 2017-03-03 14:29:58]

Friendly ping, thanks!

[mastiz, 2017-03-03 14:30:01]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 14:30:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 14:37:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 14:37:40]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pkotwicz, 2017-03-03 22:33:25]

Your CL looks mostly good. It is also much smaller than I expected (which makes
me a happy camper). Sorry for the code review delay. I was trying to get some
code in by yesterday's deadline

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:32: using
image_fetcher::ImageFetcher;
Do you need this using statement? Personally, I am only in favor using the
"using" keyword if it is for a class which is used a lot. (My personal
preference probably differs from the Google style guide) There is a single use
of ImageFetcher in this file

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:34: // Type APPLE_TOUCH includes
TOUCH_ICONs as well as FAVICONs as fallback.
Can you please mention in the comment whether TOUCH_ICON includes
apple-touch-icon-precomposed too?

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:37:
"faviconV2?url=%s&type=APPLE_TOUCH&size=%d&min_size=%d&max_size=%d";
I tested this URL in the browser. Is it possible to customize the response in
the case that the server has no data? It looks like a 16x16 icon with a picture
of the earth is returned if the server could not find any data

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:42: int min_source_size_in_pixel)
{
Don't you want to hard code the minimum size requested from the server?

If there are two callers to
LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache()
First callers minimum_size = 64
Second caller minimum_size = 16
The contents of the favicon database for page_url will be affected by the
minimum size specified in

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:202: // available and use it
instead of |icon_url|.
Please add a TODO for getting the icon type too

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:266: // Do not retry if there is a
previous cache miss recorded for |icon_url|.
How about: "Do not do download if there is a previous cache miss ..."

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
File components/favicon/core/large_icon_service.h (right):

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.h:71: // sync their history with
Google servers.
Has this feature had a privacy review?
What do other features such a omnibox suggestions do when a user is not logged
into Google?

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1749: 
Given that |page_url| is currently a substring of |icon_url| I would get rid of
the |icon_url| parameter and use |page_url| as the |icon_url|.

If you keep the |icon_url| parameter you need to check that there is nothing in
the database for |icon_url| too

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1754:
thumbnail_db_->SetFaviconOutOfDate(icon_id);
Don't you need to run ScheduleCommit()?

The ScheduleCommit() call will be a no-op because SetFavicons() already calls
it. However, it is weird for us to count on the ScheduleCommit() called by
SetFavicons() to schedule the commit for
ThumbnailDatabase::SetFaviconOutOfDate() as well

[mastiz, 2017-03-06 10:25:33]

Thanks, PTAL!

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:32: using
image_fetcher::ImageFetcher;
On 2017/03/03 22:33:25, pkotwicz wrote:
> Do you need this using statement? Personally, I am only in favor using the
> "using" keyword if it is for a class which is used a lot. (My personal
> preference probably differs from the Google style guide) There is a single use
> of ImageFetcher in this file

Removed. I'm also not a big fan of using declarations unless there are
sufficient references.

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:34: // Type APPLE_TOUCH includes
TOUCH_ICONs as well as FAVICONs as fallback.
On 2017/03/03 22:33:25, pkotwicz wrote:
> Can you please mention in the comment whether TOUCH_ICON includes
> apple-touch-icon-precomposed too?

Done. I also added a TODO because preference towards touch icons is questionable
outside iOS.

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:37:
"faviconV2?url=%s&type=APPLE_TOUCH&size=%d&min_size=%d&max_size=%d";
On 2017/03/03 22:33:25, pkotwicz wrote:
> I tested this URL in the browser. Is it possible to customize the response in
> the case that the server has no data? It looks like a 16x16 icon with a
picture
> of the earth is returned if the server could not find any data

jkrcal@ will talk to bing to disable this on the server-side. We'll need a final
confirmation from the server owners that the URL we're using is fine.

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:42: int min_source_size_in_pixel)
{
On 2017/03/03 22:33:25, pkotwicz wrote:
> Don't you want to hard code the minimum size requested from the server?
> 
> If there are two callers to
>
LargeIconService::GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache()
> First callers minimum_size = 64
> Second caller minimum_size = 16
> The contents of the favicon database for page_url will be affected by the
> minimum size specified in

We actually have clients with different requirements, so hardcoding is not
possible (unless we hardcode a very small number, leading to unnecessary
downloads).

What do you think is the actual issue?
a) That the first URL returned a 404 that will affect the second? --> this is
not possible because the 404 cache remembers the exact URL including size
restricts.

b) That the second caller will get a big icon? --> this is going to happen
anyway because we request the largest one available
(kGoogleServerV2DesiredSizeInPixel).

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:202: // available and use it
instead of |icon_url|.
On 2017/03/03 22:33:25, pkotwicz wrote:
> Please add a TODO for getting the icon type too

What would we use this for? I thought we agreed TOUCH_ICON would always be used
(to avoid mangling with sync), marking them as expired such that they will be
replaced by future visits to the page.

This would presumably require some custom header param so it feels a bit wrong.
I'll anyway get the conversation started with bing.

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:266: // Do not retry if there is a
previous cache miss recorded for |icon_url|.
On 2017/03/03 22:33:25, pkotwicz wrote:
> How about: "Do not do download if there is a previous cache miss ..."

Done.

[pkotwicz, 2017-03-08 05:38:16]

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:42: int min_source_size_in_pixel)
{
I see your point now

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:202: // available and use it
instead of |icon_url|.
I suggested the TODO because always storing as a TOUCH_ICON is a hack because
the icon may not be a touch icon.

We should at the very least have a comment saying that this is a hack and why
the TOUCH_ICON type was chosen (because unlike the FAVICON type there is no
requirement for the size of the icons of type TOUCH_ICON in the database)

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1742: void
HistoryBackend::SetExpiredFaviconsIfNoneKnown(
You should add a test testing that this method does in fact not overwrite
existing data

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1749: 
Ping on this comment

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
File components/favicon/core/large_icon_service.h (right):

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:67: // FaviconService.
The boolean in the callback is not really accurate. The callback might return
true even if the icon was not stored because there is already a favicon stored
for |page_url| in the favicon database.

Should the callback be void(gfx::Image image) instead? I think that callers care
mostly about the fetched gfx::Image.

[mastiz, 2017-03-08 16:37:34]

PTAL, modulo missing tests, thanks!

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/favicon/core...
components/favicon/core/large_icon_service.cc:202: // available and use it
instead of |icon_url|.
On 2017/03/08 05:38:16, pkotwicz wrote:
> I suggested the TODO because always storing as a TOUCH_ICON is a hack because
> the icon may not be a touch icon.
> 
> We should at the very least have a comment saying that this is a hack and why
> the TOUCH_ICON type was chosen (because unlike the FAVICON type there is no
> requirement for the size of the icons of type TOUCH_ICON in the database)

Extended the TODO with this part and clarified the current hackiness.

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1742: void
HistoryBackend::SetExpiredFaviconsIfNoneKnown(
On 2017/03/08 05:38:16, pkotwicz wrote:
> You should add a test testing that this method does in fact not overwrite
> existing data

Will do, I'll add the tests once the rest looks OK, to avoid back and forth with
signature changes.

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1749: 
On 2017/03/08 05:38:16, pkotwicz wrote:
> Ping on this comment

I think it's rather hacky to make such assumption at this layer, conditioned to
the fact that this function is only used by LargeIconService. There's also the
inconsistency of WasUnableToDownload, which we use the icon URL for.

Wrt to checking |icon_url| in the DB, I'm not convinced this is needed. Do you
suggest the function name might be misleading, and that future clients would
assume |icon_url| is also checked? We could also rename to ...IfNoMappingKnown.
WDYT?

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1754:
thumbnail_db_->SetFaviconOutOfDate(icon_id);
On 2017/03/03 22:33:25, pkotwicz wrote:
> Don't you need to run ScheduleCommit()?
> 
> The ScheduleCommit() call will be a no-op because SetFavicons() already calls
> it. However, it is weird for us to count on the ScheduleCommit() called by
> SetFavicons() to schedule the commit for
> ThumbnailDatabase::SetFaviconOutOfDate() as well

Done, added the redundant ScheduleCommit() for clarity (will be a no-op).

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
File components/favicon/core/large_icon_service.h (right):

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:67: // FaviconService.
On 2017/03/08 05:38:16, pkotwicz wrote:
> The boolean in the callback is not really accurate. The callback might return
> true even if the icon was not stored because there is already a favicon stored
> for |page_url| in the favicon database.
> 
> Should the callback be void(gfx::Image image) instead? I think that callers
care
> mostly about the fetched gfx::Image.

Fixed to return a proper value for the bool. I want to avoid returning the image
because this adds complexity to the API (min original size, resampling needs,
etc.) for something we'll likely not need (otherwise we can change the code
later).

[pkotwicz, 2017-03-09 02:25:57]

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1749: 
The sync code uses |page_url| if |icon_url| is missing as well.
(BookmarkChangeProcessor::SetBookmarkFavicon()). I think that the simpler API
makes using |page_url| as the |icon_url| worth it.

Although, I expect that you will only write one or two tests for this function,
a simpler API also means a smaller test surface.

Yes, I am concerned that future clients would assume that |icon_url| is also
checked.

[mastiz, 2017-03-15 16:58:10]

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1749: 
On 2017/03/09 02:25:57, pkotwicz wrote:
> The sync code uses |page_url| if |icon_url| is missing as well.
> (BookmarkChangeProcessor::SetBookmarkFavicon()). I think that the simpler API
> makes using |page_url| as the |icon_url| worth it.
> 
> Although, I expect that you will only write one or two tests for this
function,
> a simpler API also means a smaller test surface.
> 
> Yes, I am concerned that future clients would assume that |icon_url| is also
> checked.

The server-side guys have implemented including the icon URL as part of the
response. How do you think we should proceed with this?

I suggest we do the parsing of this field in a follow-up CL (after the latest
version of the server has been rolled out) WDYT?

Even if you're fine with this, I assume we might need to find a better name for
this function?

[mastiz, 2017-03-15 17:00:38]

mastiz@chromium.org changed reviewers:
+ zodiac@gmail.com

[mastiz, 2017-03-15 17:00:39]

treib@: can you please take a look at all ImageFetcher-related changes? Thx!

I can split this to a dedicated patch if you prefer.

[pkotwicz, 2017-03-16 00:50:45]

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1749: 
In this case, this function should check that there isn't any data for both the
page URL and the icon URL

The function name is reasonable. sky@ might have a suggestion for a better name.
He is the OWNER of history/

[mastiz, 2017-03-21 12:06:46]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 12:06:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mastiz, 2017-03-21 12:09:17]

PTAL, thx!

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/2721363002/diff/80001/components/history/core...
components/history/core/browser/history_backend.cc:1749: 
On 2017/03/16 00:50:45, pkotwicz wrote:
> In this case, this function should check that there isn't any data for both
the
> page URL and the icon URL
> 
> The function name is reasonable. sky@ might have a suggestion for a better
name.
> He is the OWNER of history/

This change has landed in the form of a dedicated patch:
https://codereview.chromium.org/2750313002

[mastiz, 2017-03-21 12:09:28]

mastiz@chromium.org changed reviewers:
- zodiac@gmail.com

[commit-bot: I haz the power, 2017-03-21 12:14:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 12:14:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-03-21 13:40:14]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 13:40:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 13:51:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 13:51:09]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-03-21 14:28:38]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 14:29:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 14:37:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 14:37:29]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-03-21 16:51:33]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 16:51:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 17:01:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 17:01:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pkotwicz, 2017-03-21 18:20:30]

LGTM with nits

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
File components/favicon/core/large_icon_service.h (right):

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:58: // favicon server and stores
the result in the FaviconService cache
Nit: cache -> database

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:60: // favicon cache contains an
icon for |page_url|, so clients are encouraged to
cache -> database

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:60: // favicon cache contains an
icon for |page_url|, so clients are encouraged to
cache -> database

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:63: // A minimum size
|min_source_size_in_pixel| can be specified as constraint.
"as constraint" -> "as a constraint"

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:67: // FaviconService.
I am skeptical of the usefulness of the bool callback value but if you think it
is useful ok

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
File components/favicon/core/favicon_service.h (right):

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
components/favicon/core/favicon_service.h:192: // Same as SetFavicons with three
differences:
Nit: SetFavicons -> SetFavicons()

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
File components/favicon/core/large_icon_service_unittest.cc (right):

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
components/favicon/core/large_icon_service_unittest.cc:277:
StartOrQueueNetworkRequest(expected_server_url,
I don't think that it is worth testing the id parameter passed to
StartOrQueueNetworkRequest()

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
components/favicon/core/large_icon_service_unittest.cc:323: "&max_size=256")))
Can you make WasUnableToDownloadFavicon() return true for all URLs?

[pkotwicz, 2017-03-21 18:20:34]

LGTM with nits

[mastiz, 2017-03-22 09:46:51]

mastiz@chromium.org changed reviewers:
+ sdefresne@chromium.org

[mastiz, 2017-03-22 09:46:53]

sdefresne@: please review changes in:
ios/
components/data_use_measurement/

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
File components/favicon/core/large_icon_service.h (right):

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:58: // favicon server and stores
the result in the FaviconService cache
On 2017/03/21 18:20:29, pkotwicz wrote:
> Nit: cache -> database

Done. I'm a bit surprised that, besides comments, you don't suggest renaming the
function.

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:60: // favicon cache contains an
icon for |page_url|, so clients are encouraged to
On 2017/03/21 18:20:29, pkotwicz wrote:
> cache -> database

Done.

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:60: // favicon cache contains an
icon for |page_url|, so clients are encouraged to
On 2017/03/21 18:20:29, pkotwicz wrote:
> cache -> database

Done.

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:63: // A minimum size
|min_source_size_in_pixel| can be specified as constraint.
On 2017/03/21 18:20:29, pkotwicz wrote:
> "as constraint" -> "as a constraint"

Done.

https://codereview.chromium.org/2721363002/diff/100001/components/favicon/cor...
components/favicon/core/large_icon_service.h:67: // FaviconService.
On 2017/03/21 18:20:29, pkotwicz wrote:
> I am skeptical of the usefulness of the bool callback value but if you think
it
> is useful ok

We can change it later if it turns out to the useless. For now, I anticipate it
will be used, and meanwhile it's useful for testing too.

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
File components/favicon/core/favicon_service.h (right):

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
components/favicon/core/favicon_service.h:192: // Same as SetFavicons with three
differences:
On 2017/03/21 18:20:29, pkotwicz wrote:
> Nit: SetFavicons -> SetFavicons()

Done.

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
File components/favicon/core/large_icon_service_unittest.cc (right):

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
components/favicon/core/large_icon_service_unittest.cc:277:
StartOrQueueNetworkRequest(expected_server_url,
On 2017/03/21 18:20:30, pkotwicz wrote:
> I don't think that it is worth testing the id parameter passed to
> StartOrQueueNetworkRequest()

Done.

https://codereview.chromium.org/2721363002/diff/300001/components/favicon/cor...
components/favicon/core/large_icon_service_unittest.cc:323: "&max_size=256")))
On 2017/03/21 18:20:30, pkotwicz wrote:
> Can you make WasUnableToDownloadFavicon() return true for all URLs?

Leaving unmodified because that would cause tests to pass if
WasUnableToDownload() was called with |page_url| instead of |icon_url|, which is
a likely mistake. 

We need to verify that the URL is equal to the one in
ShouldReportUnavailableIfFetchFromServerFails. Tests could be rearranged to do
this differently but I think this is a good enough and simple solution.

[mastiz, 2017-03-22 09:47:04]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 09:47:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 09:55:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 09:55:14]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2017-03-22 11:46:04]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 11:46:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 11:54:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 11:54:41]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-03-22 12:49:28]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 12:49:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 12:59:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 12:59:34]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2017-03-22 13:27:04]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 13:27:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 13:59:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 13:59:26]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2017-03-23 08:38:34]

sdefresne@: friendly ping, thanks!

[jkrcal, 2017-03-23 08:58:22]

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
components/favicon/core/large_icon_service.cc:41: kGoogleServerV2RequestFormat,
page_url.spec().c_str(),
Can we remove query & fragment from the url?
 (to minimize the range of URLs we ask the server to index)

It would be better if we had a uniform treatment for canonization of page URLs
across favicon_service but this would be a way longer shot.

[sdefresne, 2017-03-23 10:24:42]

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
File components/favicon/core/BUILD.gn (right):

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
components/favicon/core/BUILD.gn:29: public_deps = [
I don't think this whole public_deps is correct. It looks like we generally do
not declare public_deps on //base or other targets just because there are
#include of the target in headers.

It looks like they are used either when trying to masquerade a collection of
targets appear like a single target (usually in templates) or when one of the
depended-on target has public_configs.

At least, it appears to be incorrect for //base (it is not part of
//components/favicon/core and does not export public_configs), and probably for
the other targets too for the same reason. I think they should stay in deps and
instead the dependencies fixed in the client code.

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
File components/favicon/core/favicon_service_impl.cc (right):

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
components/favicon/core/favicon_service_impl.cc:240: gfx::ImageSkia image_skia =
image.AsImageSkia();
This code looks like it is a copy-n-paste version of SetFavicons() except for
the history_service method that is called. Can you avoid duplicating the code?
It looks like this create a std::vector<SkBitmap> from a gfx::Image and does not
access "this", so can probably be extracted to an helper method.

[mastiz, 2017-03-23 12:03:23]

sdefresne@: PTAL, thanks!

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
File components/favicon/core/BUILD.gn (right):

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
components/favicon/core/BUILD.gn:29: public_deps = [
On 2017/03/23 10:24:41, sdefresne wrote:
> I don't think this whole public_deps is correct. It looks like we generally do
> not declare public_deps on //base or other targets just because there are
> #include of the target in headers.
> 
> It looks like they are used either when trying to masquerade a collection of
> targets appear like a single target (usually in templates) or when one of the
> depended-on target has public_configs.
> 
> At least, it appears to be incorrect for //base (it is not part of
> //components/favicon/core and does not export public_configs), and probably
for
> the other targets too for the same reason. I think they should stay in deps
and
> instead the dependencies fixed in the client code.

Done.

Although not many, there are certainly examples of //base in public_deps,
introduced by senior engineers, e.g.:
https://cs.chromium.org/chromium/src/gin/BUILD.gn?type=cs&q=public_deps&l=98

From the official documentation
(https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/referen...):

"Generally if you are writing a target B and you include C's headers as part of
B's public headers, or targets depending on B should consider B and C to be part
of a unit, you should use public_deps instead of deps."

My interpretation is that the first condition is sufficient (i.e. "you include
C's headers as part of B's public headers"), to use public_deps.

Having said that, while searching for information, I came across one of your
patches:
https://r882-11cdd11fc1da-maruel-dot-chromiumcodereview.appspot.com/2539143002/

Which suggests you've thought this through, so addressed your suggestion.

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
File components/favicon/core/favicon_service_impl.cc (right):

https://codereview.chromium.org/2721363002/diff/380001/components/favicon/cor...
components/favicon/core/favicon_service_impl.cc:240: gfx::ImageSkia image_skia =
image.AsImageSkia();
On 2017/03/23 10:24:41, sdefresne wrote:
> This code looks like it is a copy-n-paste version of SetFavicons() except for
> the history_service method that is called. Can you avoid duplicating the code?
> It looks like this create a std::vector<SkBitmap> from a gfx::Image and does
not
> access "this", so can probably be extracted to an helper method.

Done, thanks!

[pkotwicz, 2017-03-23 14:35:03]

https://codereview.chromium.org/2721363002/diff/400001/components/favicon/cor...
File components/favicon/core/favicon_service_impl.cc (right):

https://codereview.chromium.org/2721363002/diff/400001/components/favicon/cor...
components/favicon/core/favicon_service_impl.cc:39: std::vector<SkBitmap>
GetFaviconBitmapsToSave(const gfx::Image& image) {
Maybe rename this method to: ExtractSkBitmapsToStore()

[mastiz, 2017-03-23 15:28:17]

PTAL!

Addressed comment, and one extra change:

I got a bit paranoid about URLs sent to the servers, and although this is
something we wanted to polish in follow-up CLs, I prefer if the first version is
more careful, so I added code (and tests) to verify scheme and trim various URL
components.

https://codereview.chromium.org/2721363002/diff/400001/components/favicon/cor...
File components/favicon/core/favicon_service_impl.cc (right):

https://codereview.chromium.org/2721363002/diff/400001/components/favicon/cor...
components/favicon/core/favicon_service_impl.cc:39: std::vector<SkBitmap>
GetFaviconBitmapsToSave(const gfx::Image& image) {
On 2017/03/23 14:35:02, pkotwicz wrote:
> Maybe rename this method to: ExtractSkBitmapsToStore()

Done.

[sdefresne, 2017-03-23 16:29:09]

Remove components/image_fetcher/content/#DEPS#?

[mastiz, 2017-03-23 16:36:01]

On 2017/03/23 16:29:09, sdefresne wrote:
> Remove components/image_fetcher/content/#DEPS#?

Done, sorry.

[sdefresne, 2017-03-23 16:40:02]

lgtm

[mastiz, 2017-03-23 16:42:15]

On 2017/03/23 16:36:01, mastiz wrote:
> On 2017/03/23 16:29:09, sdefresne wrote:
> > Remove components/image_fetcher/content/#DEPS#?
> 
> Done, sorry.

Wait, there's a few other changes that need to be reverted, please hold on.

[mastiz, 2017-03-23 16:45:01]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 16:45:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 17:12:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 17:12:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mastiz, 2017-03-23 19:31:32]

On 2017/03/23 16:42:15, mastiz wrote:
> On 2017/03/23 16:36:01, mastiz wrote:
> > On 2017/03/23 16:29:09, sdefresne wrote:
> > > Remove components/image_fetcher/content/#DEPS#?
> > 
> > Done, sorry.
> 
> Wait, there's a few other changes that need to be reverted, please hold on.

I believe the last version is in a sane state, sorry about the confusion.

pkotwicz@: do you want to take a final look, specially the URL trimming? Thx!

[mastiz, 2017-03-23 19:33:31]

mastiz@chromium.org changed reviewers:
+ asvitkine@chromium.org

[mastiz, 2017-03-23 19:33:33]

asvitkine@: please take a look at histograms.xml, thx!

[Alexei Svitkine (slow), 2017-03-23 19:45:50]

lgtm

[mastiz, 2017-03-24 08:55:24]

jkrcal@: friendly ping, thx!

I added yet another URL param as per request by bingwang@: fallback_opts=TYPE.
Please sanity check.

I think the patch should be ready to land unless there are further comments.

[jkrcal, 2017-03-24 09:04:06]

Still lgtm, thanks!

[mastiz, 2017-03-24 10:14:15]

The CQ bit was checked by mastiz@chromium.org

[mastiz, 2017-03-24 10:14:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, sdefresne@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2721363002/#ps500001
(title: "Add fallback_opts=TYPE")

[commit-bot: I haz the power, 2017-03-24 10:14:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 11:44:50]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1490350455824690,
"parent_rev": "8cf5cdc8b86d8370d9b56eb54b91b2e6d1d51cc5", "commit_rev":
"bf4de3ff1c10a7ca6b1c4ca00451165f18fa1eef"}

[commit-bot: I haz the power, 2017-03-24 11:45:39]

Description was changed from

==========
Extend LargeIconService to fetch missing favicons from a Google server

(Based on original work by jkrcal@)

Design Doc: go/favicon-pe-google-server

Showing good large icons independently of the local favicon cache is
important for synced content or for content suggested from the
server-side (such as tiles and suggestions on New Tab Page on Android).

This CL extends the service by a dedicated API that allows clients to
fetch favicons/largeicons from Google server. No actual use of the
functionality is introduced in this patch.

BUG=644102
==========

to

==========
Extend LargeIconService to fetch missing favicons from a Google server

(Based on original work by jkrcal@)

Design Doc: go/favicon-pe-google-server

Showing good large icons independently of the local favicon cache is
important for synced content or for content suggested from the
server-side (such as tiles and suggestions on New Tab Page on Android).

This CL extends the service by a dedicated API that allows clients to
fetch favicons/largeicons from Google server. No actual use of the
functionality is introduced in this patch.

BUG=644102

Review-Url: https://codereview.chromium.org/2721363002
Cr-Commit-Position: refs/heads/master@{#459392}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf4de3ff1c10a7ca6b1c4ca00451...
==========

[commit-bot: I haz the power, 2017-03-24 11:45:41]

Committed patchset #26 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/bf4de3ff1c10a7ca6b1c4ca00451...