[NTP Tiles] Fetch missing MostLikely tiles from a Google server

[NTP Tiles] Fetch missing MostLikely tiles from a Google server

This CL extends the IconCacher to support caching also for MostLikely
tiles (via the prefetching functionality of LargeIconService). The new
feature is guarded behind an experimental feature (for both Android and
iOS).

BUG=536988
Review-Url: https://codereview.chromium.org/2866033002
Cr-Commit-Position: refs/heads/master@{#471365}
Committed: https://chromium.googlesource.com/chromium/src/+/43f24f8751ec2d4d056623977fb49adc6dce18ae

[jkrcal, 2017-05-08 18:31:58]

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

[jkrcal, 2017-05-08 18:31:58]

Peter, could you PTAL at components/favicon_base/?

Chris, I still want to add some unit-tests into IconCacherTest, tomorrow. In the
mean-time I am happy to hear some preliminary feedback (not needed).

[sfiera, 2017-05-09 08:53:36]

Looks good, other than the missing tests :)

https://codereview.chromium.org/2866033002/diff/20001/components/favicon_base...
File components/favicon_base/favicon_types.cc (right):

https://codereview.chromium.org/2866033002/diff/20001/components/favicon_base...
components/favicon_base/favicon_types.cc:44:
fallback_icon_style->is_default_background_color);
I find this condition hard to read. What about:

  if (bitmap.is_valid())
    return false;
  if (fallback is valid)
    return false;
  return true;

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher.h (right):

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher.h:16: // Does not provide any way to get a
fetched favicon; use the FaviconService for
…or LargeIconService…?

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:161: base::Unretained(this), page_url,
icon_available),
Why Unretained()? I don't see anything that guarantees this object exists as
long as the LargeIconService.

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:176:
->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
Does this method also add the icon to the local cache? As long as the name may
be, that's not clear from it, and I would assume a Get…() method to be just an
accessor.

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:179: base::Unretained(this), page_url,
icon_available));
Ditto

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:183: const GURL& page_url,
Unused

[pkotwicz, 2017-05-09 17:40:34]

https://codereview.chromium.org/2866033002/diff/20001/components/favicon_base...
File components/favicon_base/favicon_types.cc (right):

https://codereview.chromium.org/2866033002/diff/20001/components/favicon_base...
components/favicon_base/favicon_types.cc:44:
fallback_icon_style->is_default_background_color);
It would be nice if FallbackIconStyle had a FallbackIconStyle::IsDefault()
method.

Implementing FallbackIconStyle::IsDefault() will be simpler once
crbug.com/712475 is addressed. I have started addressing crbug.com/712475 (super
easy to do :) ). I am currently blocked on hearing from treib@ as to whether
instant uses chrome-search://large-icon or chrome-search://fallback-icon

[jkrcal, 2017-05-10 17:26:03]

Thanks! PTAL, again.

The unit-tests should be complete, now.

https://codereview.chromium.org/2866033002/diff/20001/components/favicon_base...
File components/favicon_base/favicon_types.cc (right):

https://codereview.chromium.org/2866033002/diff/20001/components/favicon_base...
components/favicon_base/favicon_types.cc:44:
fallback_icon_style->is_default_background_color);
On 2017/05/09 08:53:36, sfiera wrote:
> I find this condition hard to read. What about:
> 
>   if (bitmap.is_valid())
>     return false;
>   if (fallback is valid)
>     return false;
>   return true;

Done.

https://codereview.chromium.org/2866033002/diff/20001/components/favicon_base...
components/favicon_base/favicon_types.cc:44:
fallback_icon_style->is_default_background_color);
On 2017/05/09 17:40:34, pkotwicz wrote:
> It would be nice if FallbackIconStyle had a FallbackIconStyle::IsDefault()
> method.
> 
> Implementing FallbackIconStyle::IsDefault() will be simpler once
> crbug.com/712475 is addressed. I have started addressing crbug.com/712475
(super
> easy to do :) ). I am currently blocked on hearing from treib@ as to whether
> instant uses chrome-search://large-icon or chrome-search://fallback-icon

So far, I went for the suggestion of sfiera@.
Can we please keep your proposal for a follow-up CL? 
(such as the one that introduces IsDefault())

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher.h (right):

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher.h:16: // Does not provide any way to get a
fetched favicon; use the FaviconService for
On 2017/05/09 08:53:36, sfiera wrote:
> …or LargeIconService…?

Done.

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:161: base::Unretained(this), page_url,
icon_available),
On 2017/05/09 08:53:36, sfiera wrote:
> Why Unretained()? I don't see anything that guarantees this object exists as
> long as the LargeIconService.

Huh, right. I copied that from ContentSuggestionService that DependsOn
LargeIconService (which guarantees shorter lifetime).

Not the case for this cacher. Fixed using the |weak_ptr_factory_|.

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:176:
->GetLargeIconOrFallbackStyleFromGoogleServerSkippingLocalCache(
On 2017/05/09 08:53:36, sfiera wrote:
> Does this method also add the icon to the local cache? As long as the name may
> be, that's not clear from it, and I would assume a Get…() method to be just an
> accessor.

Yes, it does. I see your point, I add a TODO for that.

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:179: base::Unretained(this), page_url,
icon_available));
On 2017/05/09 08:53:36, sfiera wrote:
> Ditto

Done.

https://codereview.chromium.org/2866033002/diff/20001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:183: const GURL& page_url,
On 2017/05/09 08:53:36, sfiera wrote:
> Unused

Done.

[jkrcal, 2017-05-10 17:53:30]

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

[commit-bot: I haz the power, 2017-05-10 17:54:55]

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-05-10 21:18:04]

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

[commit-bot: I haz the power, 2017-05-10 21:18:05]

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_...)

[sfiera, 2017-05-11 10:34:41]

LGTM

https://codereview.chromium.org/2866033002/diff/40001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl_unittest.cc (right):

https://codereview.chromium.org/2866033002/diff/40001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl_unittest.cc:386: image_fetcher_unused_(
The difference between image_fetcher_ and image_fetcher_unused_ is confusing.
Could they be fetcher_for_large_icon_service_, fetcher_for_icon_cacher_?

https://codereview.chromium.org/2866033002/diff/40001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl_unittest.cc:387:
base::MakeUnique<::testing::NiceMock<MockImageFetcher>>()) {}
If this one is supposed to be unused, StrictMock<>?

[jkrcal, 2017-05-11 16:08:31]

Thanks!

Peter, could you please take another look?

https://codereview.chromium.org/2866033002/diff/40001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl_unittest.cc (right):

https://codereview.chromium.org/2866033002/diff/40001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl_unittest.cc:386: image_fetcher_unused_(
On 2017/05/11 10:34:41, sfiera wrote:
> The difference between image_fetcher_ and image_fetcher_unused_ is confusing.
> Could they be fetcher_for_large_icon_service_, fetcher_for_icon_cacher_?

Done.

https://codereview.chromium.org/2866033002/diff/40001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl_unittest.cc:387:
base::MakeUnique<::testing::NiceMock<MockImageFetcher>>()) {}
On 2017/05/11 10:34:41, sfiera wrote:
> If this one is supposed to be unused, StrictMock<>?

Done. Had to add a few uninteresting expectations in the constructor (that are
called from the constructor of the cacher).

[pkotwicz, 2017-05-12 07:40:39]

https://codereview.chromium.org/2866033002/diff/60001/components/favicon_base...
File components/favicon_base/favicon_types.cc (right):

https://codereview.chromium.org/2866033002/diff/60001/components/favicon_base...
components/favicon_base/favicon_types.cc:41: bool LargeIconResult::is_empty()
const {
I think that the implementation should be in the caller. i.e.
IconCacherImpl::OnGetLargeIconOrFallbackStyleFinished()

This function isn't really checking for empty. It wants to know whether:
1) LargeIconResult uses a fallback style
2) The fallback style which is used is the default fallback style

(LargeIconService should never return a LargeIconResult with an invalid
FaviconRawBitmapResult)

LargeIconResult::is_default() would be a better name for the function 

For the sake of clarity my preference is for the implementation to be in the
caller for now

[jkrcal, 2017-05-12 11:49:03]

Thanks!

Peter, can you please still look at the last remaining file in favicon?

https://codereview.chromium.org/2866033002/diff/60001/components/favicon_base...
File components/favicon_base/favicon_types.cc (right):

https://codereview.chromium.org/2866033002/diff/60001/components/favicon_base...
components/favicon_base/favicon_types.cc:41: bool LargeIconResult::is_empty()
const {
On 2017/05/12 07:40:38, pkotwicz wrote:
> I think that the implementation should be in the caller. i.e.
> IconCacherImpl::OnGetLargeIconOrFallbackStyleFinished()
> 
> This function isn't really checking for empty. It wants to know whether:
> 1) LargeIconResult uses a fallback style
> 2) The fallback style which is used is the default fallback style
> 
> (LargeIconService should never return a LargeIconResult with an invalid
> FaviconRawBitmapResult)
> 
> LargeIconResult::is_default() would be a better name for the function 
> 
> For the sake of clarity my preference is for the implementation to be in the
> caller for now

The reason I put it here because something similar is done in
ContentSuggestionsService::OnGetFaviconFromCacheFinished() and I want to have
clean code and avoid duplicating such helper functions.

Anyway, I think it more belongs to the LargeIconService (because it is the
service that in the end decides under which conditions the image can get
fetched).

So I follow your preference, put it into the caller, now and postpone the
potential move of this helper into LargeIconService for a later CL.

[jkrcal, 2017-05-12 11:50:37]

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

[commit-bot: I haz the power, 2017-05-12 11:50:47]

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-05-12 12:44:21]

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

[commit-bot: I haz the power, 2017-05-12 12:44:22]

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

[pkotwicz, 2017-05-12 15:40:04]

LGTM

[pkotwicz, 2017-05-12 15:40:19]

https://codereview.chromium.org/2866033002/diff/80001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2866033002/diff/80001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:50: return
result.fallback_icon_style->is_default_background_color;
Nit:

return result.fallback_icon_style &&
result.fallback_icon_style->is_default_background_color

[jkrcal, 2017-05-12 17:08:59]

Thanks!

https://codereview.chromium.org/2866033002/diff/80001/components/ntp_tiles/ic...
File components/ntp_tiles/icon_cacher_impl.cc (right):

https://codereview.chromium.org/2866033002/diff/80001/components/ntp_tiles/ic...
components/ntp_tiles/icon_cacher_impl.cc:50: return
result.fallback_icon_style->is_default_background_color;
On 2017/05/12 15:40:18, pkotwicz wrote:
> Nit:
> 
> return result.fallback_icon_style &&
> result.fallback_icon_style->is_default_background_color

Even though the logic is simple, I prefer it separate. 
(I would rather tend to add a comment to the first false case).

[jkrcal, 2017-05-12 17:09:14]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-05-12 17:09:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org
Link to the patchset: https://codereview.chromium.org/2866033002/#ps80001
(title: "Comments #3")

[commit-bot: I haz the power, 2017-05-12 17:10:31]

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-05-12 18:01:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494608954064170,
"parent_rev": "af0999af43d835424ed067f9e512ed1b15c5054f", "commit_rev":
"43f24f8751ec2d4d056623977fb49adc6dce18ae"}

[commit-bot: I haz the power, 2017-05-12 18:06:31]

Description was changed from

==========
[NTP Tiles] Fetch missing MostLikely tiles from a Google server

This CL extends the IconCacher to support caching also for MostLikely
tiles (via the prefetching functionality of LargeIconService). The new
feature is guarded behind an experimental feature (for both Android and
iOS).

BUG=536988
==========

to

==========
[NTP Tiles] Fetch missing MostLikely tiles from a Google server

This CL extends the IconCacher to support caching also for MostLikely
tiles (via the prefetching functionality of LargeIconService). The new
feature is guarded behind an experimental feature (for both Android and
iOS).

BUG=536988

Review-Url: https://codereview.chromium.org/2866033002
Cr-Commit-Position: refs/heads/master@{#471365}
Committed:
https://chromium.googlesource.com/chromium/src/+/43f24f8751ec2d4d056623977fb4...
==========

[commit-bot: I haz the power, 2017-05-12 18:06:32]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/43f24f8751ec2d4d056623977fb4...