[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out part 1/3

[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out part 1/3

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with two shortcuts with the same icon on their homescreen:
- one for a WebAPK
- one for a webapp

BUG=696132
Review-Url: https://codereview.chromium.org/2733543002
Cr-Commit-Position: refs/heads/master@{#457193}
Committed: https://chromium.googlesource.com/chromium/src/+/67dd4e7e82a41fef1e90f489f4b5e91ea5179c66

[pkotwicz, 2017-03-03 03:36:05]

Description was changed from

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with both a WebAPK shortcut and a webapp shortcut on their
homescreen.

BUG=696132
==========

to

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with both a WebAPK shortcut and a webapp shortcut on their
homescreen for the same start URL.

BUG=696132
==========

[pkotwicz, 2017-03-03 03:37:16]

Description was changed from

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with both a WebAPK shortcut and a webapp shortcut on their
homescreen for the same start URL.

BUG=696132
==========

to

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with two shortcuts with the same icon on their homescreen:
- one for a WebAPK
- one for a webapp

BUG=696132
==========

[pkotwicz, 2017-03-03 03:41:10]

Description was changed from

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with two shortcuts with the same icon on their homescreen:
- one for a WebAPK
- one for a webapp

BUG=696132
==========

to

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out part
1/3

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with two shortcuts with the same icon on their homescreen:
- one for a WebAPK
- one for a webapp

BUG=696132
==========

[pkotwicz, 2017-03-03 03:41:10]

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

[pkotwicz, 2017-03-03 03:41:22]

Dominick, can you please take a look?

[pkotwicz, 2017-03-03 03:41:27]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-03-03 03:41:37]

Patchset #1 (id:20001) has been deleted

[dominickn, 2017-03-05 23:52:40]

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:131:
private void notify(int result) {
Can you call this variable webApkInstallResult, or add the @annotation to show
that the result variable should be a WebApkInstallResult?

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:408:
Java_AppBannerInfoBarDelegateAndroid_showWebApkInstallFailureToast(env);
If the WebAPK install still might succeed, why show the toast? Won't it be
confusing if the toast shows and then the install succeeds afterwards?

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_install_service.h (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:31: INSTALLED = 0,
SUCCESS ("Install" is redundant)

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:32: INSTALL_FAILED = 1,
FAILURE

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:37:
INSTALL_PROBABLY_FAILED = 2
TIMEOUT (you can add new result values in future for different "probable
failures")

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:308: return
Java_WebApkInstaller_updateAsyncFromNative(env, java_ref_,
Can updates also use WebApkInstallResult instead of a boolean for consistency?
Then they can also just call OnResult(result)

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:344: void
WebApkInstaller::OnSuccess() {
Can you remove this method and just call OnResult directly?

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:631: void
WebApkInstaller::OnFailure() {
Can you remove this method and call OnResult directly?

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_update_manager.cc (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_manager.cc:36: bool success =
(result == WebApkInstallResult::INSTALLED);
Can we get rid of the logging here now?

That makes this cleaner (removes the double conditional for just one).

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_manager.cc:120:
WebApkInstallResult::INSTALL_FAILED, ""));
Is it worth having a separate result case here to designate that another install
for this WebAPK is in progress?

[pkotwicz, 2017-03-12 21:11:49]

Dominick, can you please take another look? This CL now depends on
https://codereview.chromium.org/2746723002/

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:131:
private void notify(int result) {
The java_cpp_enum.py script does not seem to support the @ annotation. Renamed
the variable to webApkInstallResult

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:408:
Java_AppBannerInfoBarDelegateAndroid_showWebApkInstallFailureToast(env);
It would also be confusing if the install fails and the user gets no feedback at
all.

Personally, I prefer if the user is happily surprised that the WebAPK did in
fact succeed rather than the user being confused about why the infobar
disappeared with no explanation

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_install_service.h (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:37:
INSTALL_PROBABLY_FAILED = 2
I prefer keeping this as INSTALL_PROBABLY_FAILED. In the case of some of the
time outs during the install flow (e.g. WebApkInstaller::SendRequest()) we know
for sure that the install failed.

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:308: return
Java_WebApkInstaller_updateAsyncFromNative(env, java_ref_,
I have addressed this concern in https://codereview.chromium.org/2746723002/
This CL now depends on that CL

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_update_manager.cc (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_manager.cc:36: bool success =
(result == WebApkInstallResult::INSTALLED);
On 2017/03/05 23:52:40, dominickn wrote:
> Can we get rid of the logging here now?
> 
> That makes this cleaner (removes the double conditional for just one).

Done.

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_update_manager.cc:120:
WebApkInstallResult::INSTALL_FAILED, ""));
I don't think so. An update being already in progress should never occur because
there can't be multiple simultaneously running instances of a WebAPK

In the install case, IsInstallInProgress() is called quite a bit before the
actual installation

[dominickn, 2017-03-13 03:22:24]

Thanks, this is getting pretty close now.

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:131:
private void notify(int result) {
On 2017/03/12 21:11:48, pkotwicz wrote:
> The java_cpp_enum.py script does not seem to support the @ annotation. Renamed
> the variable to webApkInstallResult

Hmm, I've definitely seen this supported. Did you fully qualify the enum name?
E.g. look at
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...,
which uses
https://cs.chromium.org/chromium/src/chrome/browser/ntp_snippets/ntp_snippets...

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:408:
Java_AppBannerInfoBarDelegateAndroid_showWebApkInstallFailureToast(env);
On 2017/03/12 21:11:48, pkotwicz wrote:
> It would also be confusing if the install fails and the user gets no feedback
at
> all.
> 
> Personally, I prefer if the user is happily surprised that the WebAPK did in
> fact succeed rather than the user being confused about why the infobar
> disappeared with no explanation

We should probably check in with UI/PM on this. Given the timeout is 3 minutes
long, it's quite possible that the user has already navigated off the page
(killing the infobar), or scrolled around (hiding it) by the time we hit this.

I think the confusion of being told that the install failed and then having the
WebAPK end up mysteriously in your All Apps is worse than the WebAPK infobar
disappearing.

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_install_service.h (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:37:
INSTALL_PROBABLY_FAILED = 2
On 2017/03/12 21:11:48, pkotwicz wrote:
> I prefer keeping this as INSTALL_PROBABLY_FAILED. In the case of some of the
> time outs during the install flow (e.g. WebApkInstaller::SendRequest()) we
know
> for sure that the install failed.

I don't quite understand your reasoning - WebApkInstaller::SendRequest uses the
FAILURE code?

The comment here explicitly says that the install timed out, so why not call it
TIMEOUT?

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:330:
finish_callback_.Run(result, relax_updates_, webapk_package_);
Nit: this should call weak_ptr_factory_.InvalidateWeakPtrs() first to make sure
no callbacks will be invoked.

[pkotwicz, 2017-03-14 22:38:57]

Dominick, can you please take another look?

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:131:
private void notify(int result) {
Thank you for the pointer! I switched the code to use the annotation

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:408:
Java_AppBannerInfoBarDelegateAndroid_showWebApkInstallFailureToast(env);
We discussed this at the weekly meeting and decided on not showing a toast

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_install_service.h (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:37:
INSTALL_PROBABLY_FAILED = 2
I prefer keeping the PROBABLY_FAILURE because we want to send the
PROBABLY_FAILURE status for some but not all timeouts. In particular, we want to
send FAILURE instead of PROBABLY_FAILURE when the |WebApkInstaller::timer_|
fires

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:330:
finish_callback_.Run(result, relax_updates_, webapk_package_);
Doesn't the destructor call WeakPtrFactory::InvalidateWeakPtrs() implicitly?

[dominickn, 2017-03-14 23:25:19]

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_install_service.h (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:37:
INSTALL_PROBABLY_FAILED = 2
On 2017/03/14 22:38:57, pkotwicz wrote:
> I prefer keeping the PROBABLY_FAILURE because we want to send the
> PROBABLY_FAILURE status for some but not all timeouts. In particular, we want
to
> send FAILURE instead of PROBABLY_FAILURE when the |WebApkInstaller::timer_|
> fires

I don't want to block on this. Please rename it to PROBABLE_FAILURE (not
PROBABLY) and we'll leave it there.

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:330:
finish_callback_.Run(result, relax_updates_, webapk_package_);
On 2017/03/14 22:38:57, pkotwicz wrote:
> Doesn't the destructor call WeakPtrFactory::InvalidateWeakPtrs() implicitly?

Yes, but finish_callback_ could potentially be bound to something that triggers
one of the callbacks, or it could do some blocking work on a different thread
that allows one of the weak pointer calls to squeeze in before the destructor is
run. Invalidating the weak pointers first here makes sure none of that will
happen.

https://codereview.chromium.org/2733543002/diff/110001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2733543002/diff/110001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:404: //
Else: the WebAPK install timed out. Don't add shortcut because the install
This comment is a bit weirdly placed now. Can you move it above the conditional
and change it to say something like:

If the install didn't definitely fail, we don't add a shortcut. This could
happen if Play was busy with another install and this one is still queued (and
hence might succeed in the future).

[pkotwicz, 2017-03-14 23:43:29]

Dominick, can you please take another look?

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_install_service.h (right):

https://codereview.chromium.org/2733543002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_install_service.h:37:
INSTALL_PROBABLY_FAILED = 2
Thanks!

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2733543002/diff/90001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:330:
finish_callback_.Run(result, relax_updates_, webapk_package_);
Fair enough

https://codereview.chromium.org/2733543002/diff/110001/chrome/browser/android...
File chrome/browser/android/banners/app_banner_infobar_delegate_android.cc
(right):

https://codereview.chromium.org/2733543002/diff/110001/chrome/browser/android...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:404: //
Else: the WebAPK install timed out. Don't add shortcut because the install
On 2017/03/14 23:25:19, dominickn wrote:
> This comment is a bit weirdly placed now. Can you move it above the
conditional
> and change it to say something like:
> 
> If the install didn't definitely fail, we don't add a shortcut. This could
> happen if Play was busy with another install and this one is still queued (and
> hence might succeed in the future).

Done.

[dominickn, 2017-03-14 23:46:57]

lgtm, thanks

[pkotwicz, 2017-03-14 23:48:45]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-03-14 23:49:48]

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-15 00:00:01]

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

[commit-bot: I haz the power, 2017-03-15 00:00:02]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2017-03-15 15:34:40]

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

[pkotwicz, 2017-03-15 15:34:41]

Dan can you please take a look at the changes in:

chrome/android/BUILD.gn
chrome/android/java/strings/android_chrome_strings.grd

[gone, 2017-03-15 20:31:32]

those files lgtm

[pkotwicz, 2017-03-15 20:32:03]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-03-15 20:33:17]

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-15 20:39:10]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1489609923818940,
"parent_rev": "4984a107a57e2f0939a1497b0307158abca41957", "commit_rev":
"67dd4e7e82a41fef1e90f489f4b5e91ea5179c66"}

[commit-bot: I haz the power, 2017-03-15 20:39:49]

Description was changed from

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out part
1/3

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with two shortcuts with the same icon on their homescreen:
- one for a WebAPK
- one for a webapp

BUG=696132
==========

to

==========
[Android:WebAPK] Don't add webapp to homescreen if WebAPK install times out part
1/3

Chrome considers a install as "failed" if Google Play does not finish the
install within 3 minutes. It is possible for the install to take more time
if Google Play is busy with another install. When an install fails we add a
webapp shortcut to the homescreen.

This CL changes the logic so that a webapp shortcut is not added to the
homescreen if the WebAPK install times out. It would be confusing for the
user to end up with two shortcuts with the same icon on their homescreen:
- one for a WebAPK
- one for a webapp

BUG=696132

Review-Url: https://codereview.chromium.org/2733543002
Cr-Commit-Position: refs/heads/master@{#457193}
Committed:
https://chromium.googlesource.com/chromium/src/+/67dd4e7e82a41fef1e90f489f4b5...
==========

[commit-bot: I haz the power, 2017-03-15 20:39:50]

Committed patchset #6 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/67dd4e7e82a41fef1e90f489f4b5...