Extend LargeIconService to fetch missing favicons from a Google server

Extend LargeIconService to fetch missing favicons from a 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 LargeIconService with an API to fetch favicons from
Google servers (i.e. without actually visiting the site). The actual client code is left outside the scope of this CL, but the new API is
designed to be used only in cases where a large enough icon is not
available.

The design doc for this change is here: go/favicon-pe-google-server

BUG=644102

[jkrcal, 2017-02-09 20:34:02]

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

[jkrcal, 2017-02-09 20:34:03]

Peter, Mikel, this is the overhauled version of the FaviconService CL.

Could you please take a look to give me quick feedback? 
Tomorrow, I'll update the design doc and send it out to you.

Peter:
 - We agreed that I rename the function to make it clear that it downloads
favicon from a Google server. I did not do it yet to keep the CL minimal, at the
moment. I will do it in a later patch set.
 - Compared to the previous CL, this one also includes integration of v2 favicon
server. Both versions are guarded by field trial features.

[pkotwicz, 2017-02-10 05:32:24]

I'll take a look at your CL on Friday

[jkrcal, 2017-02-10 16:46:50]

On 2017/02/10 05:32:24, pkotwicz wrote:
> I'll take a look at your CL on Friday

A brief d-doc: go/favicon-pe-google-server

[pkotwicz, 2017-02-10 20:44:34]

A few initial comments.

I tried out your CL.
I deleted the favicon database file and launched Chrome and visited
chrome://history. All of the icons were blank for 2 - 5 seconds. Then the icons
showed up

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:48:
"faviconV2?url=%s&type=APPLE_TOUCH&size=%d&min_size=%d&max_size=%d";
Doesn't chrome want to prioritize FAVICONS over APPLE_TOUCH and TOUCH_ICONS?

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:229: callback_.Run(*result_);
If we go and fetch results from the Google server shouldn't we call the
callback_ with those results?

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:266: GURL
LargeIconWorker::GetIconUrlForGoogleServer2() const {
Different bits of UI want differently sized favicons. For instance, bookmarks on
Android want favicons of size 16x16dp (48x48px on a Nexus 5). The new tab tiles
want favicons of size 48x48dp (144x144px on a Nexus 5).

If a user:
Visits the bookmarks page
then
Visits the new tab page

It is possible that the favicons fetched and put into the database for the
bookmarks page are too small of the new tab page. Thus when the user visits the
new tab page the user will still see crappy icons despite LargeIconService
having fetched icons from the Google server  for the bookmarks page

I suggest hard coding the minimum size and the maximum size for icons fetched
from the Google server. The size fetched from the server will be unrelated to
the size requested by the caller of LargeIconService

Perhaps hard code
min size to 16 dp
and max size to 48 dp

This means that the server needs to be smarter. I don't know if there is a way
to indicate to the server
- to prefer a larger size
- return a smaller size if available

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:286: base::Unretained(this)));
Don't use base::Unretained(). LargeIconWorker is ref counted. Using
base::Unretained() makes this code crash

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:301: base::Unretained(this),
GURL(icon_url), image));
Don't use base::Unretained(). LargeIconWorker is ref counted. Using
base::Unretained() makes this code crash

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:307: SkBitmap icon =
image.AsBitmap();
I think that it would be better to store the non-resized icon in the database.
(Perhaps we want to resize if the icon is too big) A non-resized icon will
resize more nicely to the other sizes that you want.

You should however return to the callback a resized version of the icon

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:361: // icon.
You should use CancelableTaskTracker::NewTrackedTaskId() to get the task id and
use the callback in the output parameter to determine whether the operation has
been canceled.

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:362: return
favicon_service_->GetLargestRawFaviconForPageURL(
You can't use GetLargestRawFaviconForPageURL() here because
GetLargestRawFaviconForPageURL() sometimes returns no results at all when there
is data in the database. This function should not under any circumstance
overwrite valid data in the database.

[jkrcal, 2017-02-13 14:39:45]

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

[jkrcal, 2017-02-13 14:39:46]

Thanks, Peter! PTAL, again!

Note it is WIP (one function is kept there and only commented out before we
clarify how it should behave w.r.t. callback_).

Scott, PTAL!
The related design doc is go/favicon-pe-google-server

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:48:
"faviconV2?url=%s&type=APPLE_TOUCH&size=%d&min_size=%d&max_size=%d";
On 2017/02/10 20:44:34, pkotwicz wrote:
> Doesn't chrome want to prioritize FAVICONS over APPLE_TOUCH and TOUCH_ICONS?

The code for FaviconService also prioritizes APPLE_TOUCH, see the comment of
GetRawFaviconForPageURL():
  If favicon bitmaps for several IconTypes are available, the
  favicon bitmap is chosen in the priority of TOUCH_PRECOMPOSED_ICON,
  TOUCH_ICON and FAVICON.

Do you think we should change that? 

Anyway, prioritizing FAVICONS over APPLE_TOUCH is currently not supported by the
server. If we agree this is the way to go, can we post-pone it for a later CL?
(when the support on the server is added).

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:229: callback_.Run(*result_);
On 2017/02/10 20:44:34, pkotwicz wrote:
> If we go and fetch results from the Google server shouldn't we call the
> callback_ with those results?

This is conceptually a UX question to me. 

The way it is implemented now it allows the user to:
 - get the results (in this case fallback style) quickly so that the coloured
tile can be rendered quickly,
 - listen to HistoryService's icon updates and crossfade in the UI the true icon
if available any time later

If we wait for the response from the Google server, it may take a very long time
before the callback is called, especially on bad 2g networks. This way, the UI 
  - either leaves the icon area blank and, after the long delay, crossfade the
icon/colored-tile when it obtains the data from the callback;
  - or render a gray tile before calling the function and then, after the long
delay, cross-fade the icon/colored-tile when it obtains the data from the
callback.

Both solutions seem to be slightly worse to me than the current solution. I
haven't consulted with any UX expert, though.

WDYT?

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:266: GURL
LargeIconWorker::GetIconUrlForGoogleServer2() const {
On 2017/02/10 20:44:34, pkotwicz wrote:
> Different bits of UI want differently sized favicons. For instance, bookmarks
on
> Android want favicons of size 16x16dp (48x48px on a Nexus 5). The new tab
tiles
> want favicons of size 48x48dp (144x144px on a Nexus 5).
> 
> If a user:
> Visits the bookmarks page
> then
> Visits the new tab page
> 
> It is possible that the favicons fetched and put into the database for the
> bookmarks page are too small of the new tab page. Thus when the user visits
the
> new tab page the user will still see crappy icons despite LargeIconService
> having fetched icons from the Google server  for the bookmarks page
> 
> I suggest hard coding the minimum size and the maximum size for icons fetched
> from the Google server. The size fetched from the server will be unrelated to
> the size requested by the caller of LargeIconService
> 
> Perhaps hard code
> min size to 16 dp
> and max size to 48 dp
> 
> This means that the server needs to be smarter. I don't know if there is a way
> to indicate to the server
> - to prefer a larger size
> - return a smaller size if available

Good point! I've fixed min, max, and desired sizes in pixels as constants. I do
not think it is appropriate at the moment to compute these sizes from dpi sizes
and from the scale factor of the device. The reason is that icons >= 48px are
still not very wide-spread so we should be very careful about imposing larger
min_size constraints.

The server is already smart about it:
 - it never returns anything out of range of [min, max]
 - if there are images of size >= desired_size, it returns the smallest of them
(i.e. with size closest to desired_size)
 - otherwise, if there are images of size < desired_size, it returns the largest
of them.

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:286: base::Unretained(this)));
On 2017/02/10 20:44:34, pkotwicz wrote:
> Don't use base::Unretained(). LargeIconWorker is ref counted. Using
> base::Unretained() makes this code crash

Done.

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:301: base::Unretained(this),
GURL(icon_url), image));
On 2017/02/10 20:44:34, pkotwicz wrote:
> Don't use base::Unretained(). LargeIconWorker is ref counted. Using
> base::Unretained() makes this code crash

Done.

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:307: SkBitmap icon =
image.AsBitmap();
On 2017/02/10 20:44:34, pkotwicz wrote:
> I think that it would be better to store the non-resized icon in the database.
> (Perhaps we want to resize if the icon is too big) A non-resized icon will
> resize more nicely to the other sizes that you want.
> 
> You should however return to the callback a resized version of the icon

Done.

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:361: // icon.
On 2017/02/10 20:44:34, pkotwicz wrote:
> You should use CancelableTaskTracker::NewTrackedTaskId() to get the task id
and
> use the callback in the output parameter to determine whether the operation
has
> been canceled.

Well, I noticed this function would give me what I need here. I got scared off
by the comment:
  // Note. This function is used to address some special 
  // cancelation requirement in existing code. You SHOULD NOT need 
  // this function in new code.

I do not want to use deprecated functions in new code. WDYT?

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:362: return
favicon_service_->GetLargestRawFaviconForPageURL(
On 2017/02/10 20:44:34, pkotwicz wrote:
> You can't use GetLargestRawFaviconForPageURL() here because
> GetLargestRawFaviconForPageURL() sometimes returns no results at all when
there
> is data in the database. This function should not under any circumstance
> overwrite valid data in the database.

I am fine with using GetRawFaviconForPageURL() and filtering here.

I am not fine with it prioritizing exact match of type in sorting the results.
If I ask for a 120px icon with type TOUCH_ICON | TOUCH_ICON_PRECOMPOSED |
FAVICON, I would much prefer to get a FAVICON of size 96px over a TOUCH_ICON of
size 60px on the first position. Would you be fine with changing that logic that
orders results from the database (in some later CL)?

[jkrcal, 2017-02-13 14:40:34]

Description was changed from

==========
Extend LargeIconService to fetch missing favicons from a 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 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.

At the moment, the client needs to be an observer of the history
service to find out that the newly fetched favicon is available in the
cache. Making this notification available through the large icon service
API is a topic for a further CL.

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

to

==========
Extend LargeIconService to fetch missing favicons from a 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 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.

The design doc for this change is here: go/favicon-pe-google-server

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

[pkotwicz, 2017-02-13 21:33:32]

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:229: callback_.Run(*result_);
Given how you are planning on using this, I'd rather go back to the
implementation in https://codereview.chromium.org/2347173002/ I'd rather change
LargeIconService::GetLargeIconOrFallbackStyle() once many users of
LargeIconService::GetLargeIconOrFallbackStyle() download icons from the Google
server (my preference)

You could also pass two callbacks to
LargeIconService::GetLargeIconOrFallbackStyle(). One for when the favicon fetch
is completed and one for when the Google server fetch has completed

It is weird for LargeIconService::GetLargeIconOrFallbackStyle()
- to do fetching which is unrelated to the value passed to the callback.
- to keep on doing work after the callback has been called

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:266: GURL
LargeIconWorker::GetIconUrlForGoogleServer2() const {
Thank you for pointing this out. I didn't see the desired size parameter in the
server URL

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:361: // icon.
I didn't see that comment. Looks like the author of the comment (introduced in
https://codereview.chromium.org/11414041) ignored it
https://chromiumcodereview.appspot.com/11441006

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:362: return
favicon_service_->GetLargestRawFaviconForPageURL(
I have forgotten about the filtering in
ThumbnailDatabase::GetIconMappingsForPageURL(). Yes, we should change the logic
to
1) Filter based on the best size
then
2) Filter so that we have just one icon type

Historic note: the logic in HistoryBackend and ThumbnailDatabase was written to
handle TOUCH_ICON | TOUCH_ICON_PRECOMPOSED not TOUCH_ICON |
TOUCH_ICON_PRECOMPOSED | FAVICON

[jkrcal, 2017-02-27 17:31:19]

Peter, this is again WIP, not working properly, yet. Please shout if you have
conceptual problems with the direction this is heading to.

In the discussion with Scott, we converged to a "GoogleFetcher" API for fetching
from the Google server that you can access only after you try to fetch it from
local cache.

Actually, 
 - this would hide the new functionality quite a bit, and
 - this approach does not safe us against race conditions (especially since such
an API cannot guarantee that the application stores the GoogleFetcher obtained
from the cache query and uses it later). 
Thus, I decided to go for a more robust way: a new function in the service that
only does the fetch-from-Google:
 1) It spells it out clearly in its name that it skips the local cache.
 2) It nicely separates the code for fetching from local cache from the code for
fetching from Google.
 3) It should check there is nothing in the cache after the fetch, while writing
the new icon (not implemented yet).

No matter which way we do the API in the end, I think 2) and 3) is useful.

WDYT?

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:229: callback_.Run(*result_);
On 2017/02/13 21:33:31, pkotwicz wrote:
> Given how you are planning on using this, I'd rather go back to the
> implementation in https://codereview.chromium.org/2347173002/ I'd rather
change
> LargeIconService::GetLargeIconOrFallbackStyle() once many users of
> LargeIconService::GetLargeIconOrFallbackStyle() download icons from the Google
> server (my preference)
> 
> You could also pass two callbacks to
> LargeIconService::GetLargeIconOrFallbackStyle(). One for when the favicon
fetch
> is completed and one for when the Google server fetch has completed
> 
> It is weird for LargeIconService::GetLargeIconOrFallbackStyle()
> - to do fetching which is unrelated to the value passed to the callback.
> - to keep on doing work after the callback has been called

Okay, the new functionality now does not get triggered by using the previously
existing function.

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:362: return
favicon_service_->GetLargestRawFaviconForPageURL(
On 2017/02/13 21:33:31, pkotwicz wrote:
> I have forgotten about the filtering in
> ThumbnailDatabase::GetIconMappingsForPageURL(). Yes, we should change the
logic
> to
> 1) Filter based on the best size
> then
> 2) Filter so that we have just one icon type
> 
> Historic note: the logic in HistoryBackend and ThumbnailDatabase was written
to
> handle TOUCH_ICON | TOUCH_ICON_PRECOMPOSED not TOUCH_ICON |
> TOUCH_ICON_PRECOMPOSED | FAVICON

Can we leave that for a further CL?

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

https://codereview.chromium.org/2685173002/diff/40001/components/favicon/core...
components/favicon/core/large_icon_service.cc:398: // TODO(jkrcal): never
overwrite favicons. Fix in this CL!
This still definitely needs to get addressed in this CL: I need to make sure we
never overwrite anything. 

Related to my question in the design-doc:
https://docs.google.com/a/google.com/document/d/1rYLo_24nax7w5qEyJRpToaR9ykeW...
Can we write the icons if there is a FAVICON but no TOUCH_ICON?

[pkotwicz, 2017-02-28 01:51:57]

Your approach looks reasonable

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
File components/favicon/core/large_icon_service.cc (right):

https://codereview.chromium.org/2685173002/diff/1/components/favicon/core/lar...
components/favicon/core/large_icon_service.cc:362: return
favicon_service_->GetLargestRawFaviconForPageURL(
Doing this in a follow up CL is OK

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

https://codereview.chromium.org/2685173002/diff/60001/components/favicon/core...
components/favicon/core/large_icon_service.cc:89:
LargeIconWorkerBase::LargeIconWorkerBase(
I am not much of a fan of LargeIconWorkerBase. We can put all the variables that
each implementation needs in the respective LargeIconWorkers

Can we make PrepareLargeIconResult() a function in the anonymous namespace.
Perhaps:
std::unique_ptr<favicon_base::LargeIconResult> CreateLargeIconResult(const
scoped_refptr<base::RefCountedMemory>& bitmap_data, bool create_bitmap) {}

https://codereview.chromium.org/2685173002/diff/60001/components/favicon/core...
components/favicon/core/large_icon_service.cc:213: const
favicon_base::FaviconRawBitmapResult& bitmap_result) {
Can this function be in the anonymous namespace and not in LargeIconCacheWorker.
Functions called on a background thread tend to be very good candidates to be
loose functions in the anonymous namespace.

[jkrcal, 2017-03-01 09:32:06]

Peter, no need to look, yet.
Just handling over the current state of the CL to Mikel as I am sick.

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

https://codereview.chromium.org/2685173002/diff/60001/components/favicon/core...
components/favicon/core/large_icon_service.cc:89:
LargeIconWorkerBase::LargeIconWorkerBase(
On 2017/02/28 01:51:56, pkotwicz wrote:
> I am not much of a fan of LargeIconWorkerBase. We can put all the variables
that
> each implementation needs in the respective LargeIconWorkers
> 
> Can we make PrepareLargeIconResult() a function in the anonymous namespace.
> Perhaps:
> std::unique_ptr<favicon_base::LargeIconResult> CreateLargeIconResult(const
> scoped_refptr<base::RefCountedMemory>& bitmap_data, bool create_bitmap) {}

Done.

https://codereview.chromium.org/2685173002/diff/60001/components/favicon/core...
components/favicon/core/large_icon_service.cc:213: const
favicon_base::FaviconRawBitmapResult& bitmap_result) {
On 2017/02/28 01:51:56, pkotwicz wrote:
> Can this function be in the anonymous namespace and not in
LargeIconCacheWorker.
> Functions called on a background thread tend to be very good candidates to be
> loose functions in the anonymous namespace.

I would like to keep this function in the class as it corresponds to the flow of
functions that are called.
I've moved the Resize* functions out of the classes as these feel more like
helper functions.

[mastiz, 2017-03-01 16:06:40]

Since Jan is OOO, I patched this CL and continued working:
https://codereview.chromium.org/2721363002

Please review that one instead, as per jan's request.

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

Description was changed from

==========
Extend LargeIconService to fetch missing favicons from a 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 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.

The design doc for this change is here: go/favicon-pe-google-server

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

to

==========
Extend LargeIconService to fetch missing favicons from a 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 LargeIconService with an API to fetch favicons from
Google servers (i.e. without actually visiting the site). The actual client code
is left outside the scope of this CL, but the new API is
designed to be used only in cases where a large enough icon is not
available.

The design doc for this change is here: go/favicon-pe-google-server

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

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

mastiz@chromium.org changed reviewers:
+ jkrcal@chromium.org
- mastiz@chromium.org, sky@chromium.org