Add webapp homescreen shortcut when WebAPK install fails

Add webapp homescreen shortcut when WebAPK install fails

In the unlikely case that the WebAPK install fails, create a webapp homescreen
shortcut instead. This way, the "Add to Homescreen" menu item in the app menu
will "do something" for all users.

BUG=696132
Review-Url: https://codereview.chromium.org/2721203004
Cr-Commit-Position: refs/heads/master@{#454210}
Committed: https://chromium.googlesource.com/chromium/src/+/2b338ea073b62cbd75b0a9971a3e126c81847f35

[pkotwicz, 2017-03-02 01:33:49]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-03-02 01:37:49]

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

[pkotwicz, 2017-03-02 01:37:50]

Dominick, can you please take a look?

https://codereview.chromium.org/2721203004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java
(left):

https://codereview.chromium.org/2721203004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java:202:
Toast.LENGTH_SHORT);
I am purposefully not deleting the resource. I am going to bring this function
back in a CL that I will post in a few hours.

We want to display this toast if the request to Google Play to install the
WebAPK times out. If the play request times out it is possible that the WebAPK
install is delayed (e.g. because play is updating) We want to avoid the
situation of a user having two homescreen shortcuts. A WebAPK shortcut and a
webapp shortcut

[dominickn, 2017-03-02 01:56:50]

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

https://codereview.chromium.org/2721203004/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378: if
(!success) {
I wonder whether it's cleaner to have a success_callback and failure_callback
argument to ShortcutHelper::InstallWebApkWithSkBitmap(). That's a lot more
plumbing, but it means we don't have to plumb the boolean back through, and have
custom failure logic.

Thoughts? I don't feel awfully strongly.

https://codereview.chromium.org/2721203004/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:392:
content::WebContents* web_contents =
This would be a bit cleaner as:

if (!infobar())
  return;

content::WebContents* web_contents = ...
...
infobar()->RemoveSelf();

[pkotwicz, 2017-03-02 04:09:42]

Dominick, can you please take another look?

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

https://codereview.chromium.org/2721203004/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378: if
(!success) {
I'd rather not. Especially since I am going to changing the boolean to be a
tristate in an upcoming CL in order to deal with the "WebAPK was probably not
installed case"

https://codereview.chromium.org/2721203004/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:392:
content::WebContents* web_contents =
On 2017/03/02 01:56:50, dominickn wrote:
> This would be a bit cleaner as:
> 
> if (!infobar())
>   return;
> 
> content::WebContents* web_contents = ...
> ...
> infobar()->RemoveSelf();

Done.

[dominickn, 2017-03-02 04:14:49]

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

https://codereview.chromium.org/2721203004/diff/20001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:378: if
(!success) {
On 2017/03/02 04:09:42, pkotwicz wrote:
> I'd rather not. Especially since I am going to changing the boolean to be a
> tristate in an upcoming CL in order to deal with the "WebAPK was probably not
> installed case"

Acknowledged.

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

https://codereview.chromium.org/2721203004/diff/40001/chrome/browser/android/...
chrome/browser/android/banners/app_banner_infobar_delegate_android.cc:392: if
(infobar())
Pretty sure it should be if (!infobar())

[pkotwicz, 2017-03-02 04:34:40]

Dominick, can you please take another look?

[dominickn, 2017-03-02 04:35:20]

lgtm, thanks

[gone, 2017-03-02 05:00:13]

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

[gone, 2017-03-02 05:00:14]

drive-by OWNERs rs lgtm for infobars/

[pkotwicz, 2017-03-02 05:24:21]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-03-02 05:25:33]

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-02 08:20:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488432261969790,
"parent_rev": "eecb9420efdea1d6c4e882a85e1de607ed317781", "commit_rev":
"2b338ea073b62cbd75b0a9971a3e126c81847f35"}

[commit-bot: I haz the power, 2017-03-02 08:21:26]

Description was changed from

==========
Add webapp homescreen shortcut when WebAPK install fails

In the unlikely case that the WebAPK install fails, create a webapp homescreen
shortcut instead. This way, the "Add to Homescreen" menu item in the app menu
will "do something" for all users.

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

to

==========
Add webapp homescreen shortcut when WebAPK install fails

In the unlikely case that the WebAPK install fails, create a webapp homescreen
shortcut instead. This way, the "Add to Homescreen" menu item in the app menu
will "do something" for all users.

BUG=696132

Review-Url: https://codereview.chromium.org/2721203004
Cr-Commit-Position: refs/heads/master@{#454210}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b338ea073b62cbd75b0a9971a3e...
==========

[commit-bot: I haz the power, 2017-03-02 08:21:28]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2b338ea073b62cbd75b0a9971a3e...