[Content suggestions] Add a function to the service API to fetch favicons

[Content suggestions] Add an interface to fetch favicons

This CL is a first step towards fetching publishers favicons in c++,
using the new API function of LargeIconService. The purpose of this CL
is to divide follow-up tasks into smaller chunks.

In follow-up CLs, I will:
 - implement the actual functionality in ContentSuggestionsService,
 - create a trial feature to experiment with the new code path,
 - use the code on the Java side.

BUG=692975
Review-Url: https://codereview.chromium.org/2781583002
Cr-Commit-Position: refs/heads/master@{#460320}
Committed: https://chromium.googlesource.com/chromium/src/+/10004604d31dfc0134b334d946f7f62550b2b6da

[jkrcal, 2017-03-27 16:44:22]

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

[commit-bot: I haz the power, 2017-03-27 16:45:15]

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

[jkrcal, 2017-03-27 16:45:55]

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

[jkrcal, 2017-03-27 16:45:55]

Bernhard, could you PTAL?

[jkrcal, 2017-03-27 16:46:42]

Description was changed from

==========
[Content suggestions] Add an interface to fetch favicons

This CL is a first step towards fetching publishers favicons in c++,
using the new API function of LargeIconService.

In follow-up CLs, I will:
 - implement the actual functionality in ContentSuggestionsService,
 - create a trial feature to experiment with the new code path,
 - use the code on the Java side.

BUG=692975
==========

to

==========
[Content suggestions] Add an interface to fetch favicons

This CL is a first step towards fetching publishers favicons in c++,
using the new API function of LargeIconService. The purpose of this CL
is to divide follow-up tasks into smaller chunks.

In follow-up CLs, I will:
 - implement the actual functionality in ContentSuggestionsService,
 - create a trial feature to experiment with the new code path,
 - use the code on the Java side.

BUG=692975
==========

[commit-bot: I haz the power, 2017-03-27 17:10:36]

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

[commit-bot: I haz the power, 2017-03-27 17:10:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jkrcal, 2017-03-27 17:40:18]

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

[jkrcal, 2017-03-27 17:40:19]

Michael, could you PTAL at FakeSuggestionsSource?

(Maybe it would be more in the spirit of this minimalistic preparation CL, not
to implement the fake function and keep its body empty. Should I do that?)

[Bernhard Bauer, 2017-03-28 10:11:05]

lgtm

https://codereview.chromium.org/2781583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2781583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:89:
void fetchSuggestionFavicon(SnippetArticle suggestion, int minimumSizeInPixel,
Nit: The usual way to name unit variables is to just append an abbreviation for
the unit (durationMs, sizeMb), so in this case minimumSizePx (px for physical
pixels, dp for resolution-independent pixels).

[Michael van Ouwerkerk, 2017-03-28 11:23:56]

And what Bernhard said.

https://codereview.chromium.org/2781583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2781583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:82:
* @param minimumSizeInPixel Minimal required size, if only a smaller favicon is
available, null
nit: 'a null Bitmap'

https://codereview.chromium.org/2781583002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2781583002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:242:
base::android::ScopedJavaGlobalRef<jobject> callback(j_callback);
There's a using statement for ScopedJavaGlobalRef, please replace all usages in
this file with ScopedJavaGlobalRef.

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java
(right):

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java:35:
private final Map<String, Bitmap> mFavicons = new HashMap<>();
Please document what the String represents.

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java:81:
* Sets the bitmap to be returned when the favicon is requested for a snippet
with that id.
We're trying to move away from this use of the word "snippet", correct? Then
maybe use "suggestion" here and in the documentation for setThumbnailForId.

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java:159:
callback.onResult(mFavicons.get(suggestion.mIdWithinCategory));
The documentation for fetchSuggestionFavicon explicitly states the callback is
never called synchronously, can you implement that in this fake also?

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:127: // Fetches the
favicon from local cache (if larger than
"large than", or "larger than or equal to"?

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:129: // cache) and returns
the resuls in the callback. If that suggestion doesn't
s/resuls/results/

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:130: // exist or the fetch
fails, the callback gets an empty image. The callback
In the Java docs, it says we may get a null Bitmap, but here it says we may get
an empty image. I think I'd prefer the null case, it's more explicit.

[Michael van Ouwerkerk, 2017-03-28 11:24:35]

On 2017/03/27 17:40:19, jkrcal wrote:
> Michael, could you PTAL at FakeSuggestionsSource?
> 
> (Maybe it would be more in the spirit of this minimalistic preparation CL, not
> to implement the fake function and keep its body empty. Should I do that?)

Either way is fine, it's a small thing. Just keep it as you've done it already.

[jkrcal, 2017-03-28 13:12:53]

Thanks for the detailed review! Michael, PTAL again.

https://codereview.chromium.org/2781583002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
(right):

https://codereview.chromium.org/2781583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:82:
* @param minimumSizeInPixel Minimal required size, if only a smaller favicon is
available, null
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> nit: 'a null Bitmap'

Done.

https://codereview.chromium.org/2781583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java:89:
void fetchSuggestionFavicon(SnippetArticle suggestion, int minimumSizeInPixel,
On 2017/03/28 10:11:05, Bernhard Bauer wrote:
> Nit: The usual way to name unit variables is to just append an abbreviation
for
> the unit (durationMs, sizeMb), so in this case minimumSizePx (px for physical
> pixels, dp for resolution-independent pixels).

Done.

https://codereview.chromium.org/2781583002/diff/20001/chrome/browser/android/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2781583002/diff/20001/chrome/browser/android/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:242:
base::android::ScopedJavaGlobalRef<jobject> callback(j_callback);
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> There's a using statement for ScopedJavaGlobalRef, please replace all usages
in
> this file with ScopedJavaGlobalRef.

Done (for much more).

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java
(right):

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java:35:
private final Map<String, Bitmap> mFavicons = new HashMap<>();
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> Please document what the String represents.

Done.

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java:81:
* Sets the bitmap to be returned when the favicon is requested for a snippet
with that id.
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> We're trying to move away from this use of the word "snippet", correct? Then
> maybe use "suggestion" here and in the documentation for setThumbnailForId.

Done.

https://codereview.chromium.org/2781583002/diff/20001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/suggestions/FakeSuggestionsSource.java:159:
callback.onResult(mFavicons.get(suggestion.mIdWithinCategory));
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> The documentation for fetchSuggestionFavicon explicitly states the callback is
> never called synchronously, can you implement that in this fake also?

Done.

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:127: // Fetches the
favicon from local cache (if larger than
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> "large than", or "larger than or equal to"?

Done.

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:129: // cache) and returns
the resuls in the callback. If that suggestion doesn't
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> s/resuls/results/

Done.

https://codereview.chromium.org/2781583002/diff/20001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:130: // exist or the fetch
fails, the callback gets an empty image. The callback
On 2017/03/28 11:23:56, Michael van Ouwerkerk wrote:
> In the Java docs, it says we may get a null Bitmap, but here it says we may
get
> an empty image. I think I'd prefer the null case, it's more explicit.

The difference between c++ and java makes sense. "Being empty" is a standard
notion for a passed-by-reference gfx::Image:
https://cs.chromium.org/chromium/src/ui/gfx/image/image.h?sq=package:chromium...

[jkrcal, 2017-03-28 13:13:02]

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

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

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

[Michael van Ouwerkerk, 2017-03-28 13:21:34]

Thanks! lgtm

[commit-bot: I haz the power, 2017-03-28 13:37:46]

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

[commit-bot: I haz the power, 2017-03-28 13:37:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jkrcal, 2017-03-28 13:52:19]

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

[commit-bot: I haz the power, 2017-03-28 13:52:46]

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-28 14:17:08]

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

[commit-bot: I haz the power, 2017-03-28 14:17:09]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[jkrcal, 2017-03-28 15:13:33]

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

[commit-bot: I haz the power, 2017-03-28 15:14:04]

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-28 16:15:42]

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

[commit-bot: I haz the power, 2017-03-28 16:15:43]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-03-29 07:39:54]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-03-29 07:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
mvanouwerkerk@chromium.org
Link to the patchset: https://codereview.chromium.org/2781583002/#ps80001
(title: "Fix compilation #2")

[commit-bot: I haz the power, 2017-03-29 07:40:19]

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-29 07:44:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490773194724910,
"parent_rev": "8b97a74f58c8b45a85de5c5521a58a54a0935700", "commit_rev":
"10004604d31dfc0134b334d946f7f62550b2b6da"}

[commit-bot: I haz the power, 2017-03-29 07:45:44]

Description was changed from

==========
[Content suggestions] Add an interface to fetch favicons

This CL is a first step towards fetching publishers favicons in c++,
using the new API function of LargeIconService. The purpose of this CL
is to divide follow-up tasks into smaller chunks.

In follow-up CLs, I will:
 - implement the actual functionality in ContentSuggestionsService,
 - create a trial feature to experiment with the new code path,
 - use the code on the Java side.

BUG=692975
==========

to

==========
[Content suggestions] Add an interface to fetch favicons

This CL is a first step towards fetching publishers favicons in c++,
using the new API function of LargeIconService. The purpose of this CL
is to divide follow-up tasks into smaller chunks.

In follow-up CLs, I will:
 - implement the actual functionality in ContentSuggestionsService,
 - create a trial feature to experiment with the new code path,
 - use the code on the Java side.

BUG=692975

Review-Url: https://codereview.chromium.org/2781583002
Cr-Commit-Position: refs/heads/master@{#460320}
Committed:
https://chromium.googlesource.com/chromium/src/+/10004604d31dfc0134b334d946f7...
==========

[commit-bot: I haz the power, 2017-03-29 07:45:46]

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