Support badge icon in WebAPK update components

Support badge icon in WebAPK update components

This CL adds badge icon to WebApkUpdateManager,
WebApkUpdateDataFetcher, WebApkInstallService, and WebApkInstaller.
Follow-up CLs will update shell APK's AndroidManifest.xml.

Documentation "WebAPK update scenarios" will be updated after this CL
takes effect.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/1ebMBNhqRew/_6hAQKAfBwAJ

BUG=715158
Review-Url: https://codereview.chromium.org/2921623004
Cr-Commit-Position: refs/heads/master@{#479089}
Committed: https://chromium.googlesource.com/chromium/src/+/49a24dab4bd0966d7efeb27d307f598bebca9da7

[F, 2017-06-05 19:53:23]

Patchset #1 (id:1) has been deleted

[F, 2017-06-05 20:10:45]

Patchset #1 (id:20001) has been deleted

[F, 2017-06-05 20:16:20]

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

[commit-bot: I haz the power, 2017-06-05 20:17:01]

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-06-06 01:36:25]

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

[commit-bot: I haz the power, 2017-06-06 01:36:26]

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

[F, 2017-06-06 15:35:02]

Patchset #1 (id:40001) has been deleted

[F, 2017-06-06 20:20:16]

Patchset #1 (id:60001) has been deleted

[F, 2017-06-06 20:20:24]

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

[commit-bot: I haz the power, 2017-06-06 20:20:55]

Dry run: CQ is trying da patch.

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

[F, 2017-06-06 20:53:16]

Description was changed from

==========
Support badge icon in WebAPK update components

BUG=
==========

to

==========
Support badge icon in WebAPK update components

This CL adds badge icon to WebApkUpdateManager,
WebApkUpdateDataFetcher, WebApkInstallService, and WebApkInstaller.
Follow-up CLs will update shell APK's AndroidManifest.xml.

Documentation "WebAPK update scenarios" will be updated after this CL
takes effect.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/1ebMBNhqRew/_6hAQKAf...

BUG=715158
==========

[F, 2017-06-06 20:53:48]

Hi Peter, PTAL. Thanks!

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

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

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

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-07 19:06:44]

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

[pkotwicz, 2017-06-07 21:22:24]

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java
(right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java:88:
mBestIconMurmur2Hash =
fetchedInfo.iconUrlToMurmur2HashMap().get(primaryIconUrl);
Should |mBestIconMurmur2Hash| be renamed to |mPrimaryIconMurmur2Hash| ?

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java:100:
public String bestIconMurmur2Hash() {
This function should be renamed?

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:666:
* - Web Manifest is updated to refer to different icon.
Maybe change this point to:
"A new icon URL is added to the Web Manifest. InstallableManager selects the new
icon as the primary icon."

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:671:
fetchedData.iconUrlToMurmur2HashMap.clear();
I think that the test would be clearer if you did not clear the HashMap. You
don't need to remove the old primary icon from the HashMap.

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:704:
fetchedData.iconUrlToMurmur2HashMap.clear();
I think that this test would also be clearer if you don't clear the HashMap

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:712:
* Test than an upgrade is not requested if:
Nit: than -> that

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer_unittest.cc:205: new
TestWebApkInstaller(browser_context_, info, SkBitmap(), SkBitmap());
The WebApkInstaller should use a non-empty primary icon. It should be impossible
for an empty primary icon to be passed in.

I am suggesting this because we may want the behavior in
WebApkInstallerTest.BuildWebApkProtoWhenManifestIsObsolete() to be different if
we "officially support" passing an empty primary icon to WebApkInstaller.

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_update_data_fetcher.cc (right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:187:
profile->GetRequestContext(), info_.best_primary_icon_url,
Shouldn't this be |best_badge_icon_url|?

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:203: const
std::string& badge_icon_murmur2_hash) {
Can we use |info_|, |primary_icon_| and |badge_icon_| so that the function has
less parameters?

[F, 2017-06-08 17:36:46]

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

[commit-bot: I haz the power, 2017-06-08 17:37:11]

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-06-08 19:15:34]

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

[commit-bot: I haz the power, 2017-06-08 19:15:35]

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

[F, 2017-06-08 20:00:19]

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

[F, 2017-06-08 20:00:39]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-06-08 20:00:48]

Dry run: CQ is trying da patch.

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

[F, 2017-06-08 20:01:13]

Thanks Peter! PTAL

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java
(right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java:88:
mBestIconMurmur2Hash =
fetchedInfo.iconUrlToMurmur2HashMap().get(primaryIconUrl);
On 2017/06/07 21:22:23, pkotwicz wrote:
> Should |mBestIconMurmur2Hash| be renamed to |mPrimaryIconMurmur2Hash| ?

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkUpdateDataFetcherTest.java:100:
public String bestIconMurmur2Hash() {
On 2017/06/07 21:22:23, pkotwicz wrote:
> This function should be renamed?

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:666:
* - Web Manifest is updated to refer to different icon.
On 2017/06/07 21:22:23, pkotwicz wrote:
> Maybe change this point to:
> "A new icon URL is added to the Web Manifest. InstallableManager selects the
new
> icon as the primary icon."

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:671:
fetchedData.iconUrlToMurmur2HashMap.clear();
On 2017/06/07 21:22:23, pkotwicz wrote:
> I think that the test would be clearer if you did not clear the HashMap. You
> don't need to remove the old primary icon from the HashMap.

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:704:
fetchedData.iconUrlToMurmur2HashMap.clear();
On 2017/06/07 21:22:23, pkotwicz wrote:
> I think that this test would also be clearer if you don't clear the HashMap

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:712:
* Test than an upgrade is not requested if:
On 2017/06/07 21:22:23, pkotwicz wrote:
> Nit: than -> that

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer_unittest.cc:205: new
TestWebApkInstaller(browser_context_, info, SkBitmap(), SkBitmap());
On 2017/06/07 21:22:23, pkotwicz wrote:
> The WebApkInstaller should use a non-empty primary icon. It should be
impossible
> for an empty primary icon to be passed in.
> 
> I am suggesting this because we may want the behavior in
> WebApkInstallerTest.BuildWebApkProtoWhenManifestIsObsolete() to be different
if
> we "officially support" passing an empty primary icon to WebApkInstaller.

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_update_data_fetcher.cc (right):

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:187:
profile->GetRequestContext(), info_.best_primary_icon_url,
On 2017/06/07 21:22:23, pkotwicz wrote:
> Shouldn't this be |best_badge_icon_url|?

Done.

https://codereview.chromium.org/2921623004/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:203: const
std::string& badge_icon_murmur2_hash) {
On 2017/06/07 21:22:23, pkotwicz wrote:
> Can we use |info_|, |primary_icon_| and |badge_icon_| so that the function has
> less parameters?

Done.

[commit-bot: I haz the power, 2017-06-08 20:46:45]

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

[commit-bot: I haz the power, 2017-06-08 20:46:46]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-06-09 23:36:50]

lgtm

[F, 2017-06-12 14:27:24]

zpeng@chromium.org changed reviewers:
+ yfriedman@chromium.org

[F, 2017-06-12 14:27:24]

Thanks Peter!

Hi Yaron, PTAL. Thanks!

[Yaron, 2017-06-12 18:49:02]

https://codereview.chromium.org/2921623004/diff/120001/chrome/browser/android...
File chrome/browser/android/webapk/webapk_update_data_fetcher.cc (right):

https://codereview.chromium.org/2921623004/diff/120001/chrome/browser/android...
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:201:
OnWebManifestNotWebApkCompatible();
not sure we should really abort everything if just the badge icon is missing.
It's unpredictable behaviour and could lead to devs accidentally screwing their
site up for installability (as opposed to the badge icon simply not working).

[F, 2017-06-13 14:51:14]

Thanks Peter & Yaron! PTAL

https://codereview.chromium.org/2921623004/diff/120001/chrome/browser/android...
File chrome/browser/android/webapk/webapk_update_data_fetcher.cc (right):

https://codereview.chromium.org/2921623004/diff/120001/chrome/browser/android...
chrome/browser/android/webapk/webapk_update_data_fetcher.cc:201:
OnWebManifestNotWebApkCompatible();
On 2017/06/12 18:49:02, Yaron wrote:
> not sure we should really abort everything if just the badge icon is missing.
> It's unpredictable behaviour and could lead to devs accidentally screwing
their
> site up for installability (as opposed to the badge icon simply not working).

For clarification, we do not abort if the badge icon is missing. The badge is
still optional. A website developer is free to add or remove badge icons without
losing installability.

However, here we abort because something is really wrong with Clank.
did_fetch_badge_icon being true means Clank has already fetched the badge icon
and the badge icon has already passed all icon validity checks; and
badge_icon_murmur2_hash being empty means either Clank's http cache encounters
an error or the badge icon is empty.

Currently if the WebApkInstaller encounters a similar error, it would also
abort.

WDUT?

[Yaron, 2017-06-13 17:37:24]

lgtm - thanks for explanation. I think my issue is actually with
installablemanager then but that's a separate point. you're right that this
should be a failure

[F, 2017-06-13 17:57:45]

The CQ bit was checked by zpeng@chromium.org

[F, 2017-06-13 17:57:51]

The CQ bit was unchecked by zpeng@chromium.org

[F, 2017-06-13 18:05:36]

The CQ bit was checked by zpeng@chromium.org

[F, 2017-06-13 18:05:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/2921623004/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-13 18:05:59]

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-06-13 19:05:07]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497377136391760,
"parent_rev": "59a1748c0349df8961742469156763529134c634", "commit_rev":
"49a24dab4bd0966d7efeb27d307f598bebca9da7"}

[commit-bot: I haz the power, 2017-06-13 19:05:16]

Description was changed from

==========
Support badge icon in WebAPK update components

This CL adds badge icon to WebApkUpdateManager,
WebApkUpdateDataFetcher, WebApkInstallService, and WebApkInstaller.
Follow-up CLs will update shell APK's AndroidManifest.xml.

Documentation "WebAPK update scenarios" will be updated after this CL
takes effect.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/1ebMBNhqRew/_6hAQKAf...

BUG=715158
==========

to

==========
Support badge icon in WebAPK update components

This CL adds badge icon to WebApkUpdateManager,
WebApkUpdateDataFetcher, WebApkInstallService, and WebApkInstaller.
Follow-up CLs will update shell APK's AndroidManifest.xml.

Documentation "WebAPK update scenarios" will be updated after this CL
takes effect.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/1ebMBNhqRew/_6hAQKAf...

BUG=715158

Review-Url: https://codereview.chromium.org/2921623004
Cr-Commit-Position: refs/heads/master@{#479089}
Committed:
https://chromium.googlesource.com/chromium/src/+/49a24dab4bd0966d7efeb27d307f...
==========

[commit-bot: I haz the power, 2017-06-13 19:05:20]

Committed patchset #3 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/49a24dab4bd0966d7efeb27d307f...