Do not show "Add to Home Screen" info bar if WebAPK is installed

Do not show "Add to Home Screen" info bar if WebAPK is installed

Non-WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135
Committed: https://crrev.com/cda82fec90f5262fbd162bb2433c5059bca1e7ba
Cr-Commit-Position: refs/heads/master@{#404453}

[pkotwicz, 2016-07-06 03:08:00]

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

[pkotwicz, 2016-07-06 03:08:01]

Xi can you please take a look?

[Xi Han, 2016-07-06 13:29:58]

lgtm

[pkotwicz, 2016-07-06 14:49:14]

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

[pkotwicz, 2016-07-06 14:49:15]

dominickn@ can you please take a look?

I promise that I will stop mucking in app_banner_data_fetcher.cc till you finish
your refactoring (after this CL)

[dominickn, 2016-07-07 00:50:58]

https://codereview.chromium.org/2124703003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2124703003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:439:
public static boolean isWebApkInstalled(String url) {
Why is this a different check to the one in WebappRegistry? Shouldn't we ideally
have one way to tell if a WebAPK is installed?

https://codereview.chromium.org/2124703003/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/2124703003/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:78: const
GURL& start_url) {
You should explicitly document here that this method will return false if
WebAPKs are not enabled, and return false if a web app is installed, but not as
a WebAPK.

[pkotwicz, 2016-07-07 16:10:20]

Description was changed from

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

BUG=622135
==========

to

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

Non WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135
==========

[pkotwicz, 2016-07-07 17:57:09]

Description was changed from

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

Non WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135
==========

to

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

Non-WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135
==========

[pkotwicz, 2016-07-07 18:03:46]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-07-07 18:12:44]

dominickn@ can you please take another look?

https://codereview.chromium.org/2124703003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2124703003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:439:
public static boolean isWebApkInstalled(String url) {
ShortcutHelper#isWebApkInstalled() is different than
WebappRegistry#isWebApkInstalled() because:

ShortcutHelper#isWebApkInstalled() checks whether a WebAPK is installed given a
URL
WebappRegistry#isWebApkInstalled() checks whether a WebAPK is installed given an
Android package name.

The logic for checking whether a WebAPK is installed in
ShortcutHelper#isWebApkInstalled() is pretty standard. Its the same logic as in
WebappLauncherActivity#isValidWebApk()

https://codereview.chromium.org/2124703003/diff/1/chrome/browser/android/bann...
File chrome/browser/android/banners/app_banner_data_fetcher_android.cc (right):

https://codereview.chromium.org/2124703003/diff/1/chrome/browser/android/bann...
chrome/browser/android/banners/app_banner_data_fetcher_android.cc:78: const
GURL& start_url) {
I have added some comments. Let me know if my comments can be improved.

[dominickn, 2016-07-08 01:34:37]

lgtm

https://codereview.chromium.org/2124703003/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2124703003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:439:
public static boolean isWebApkInstalled(String url) {
On 2016/07/07 18:12:43, pkotwicz wrote:
> ShortcutHelper#isWebApkInstalled() is different than
> WebappRegistry#isWebApkInstalled() because:
> 
> ShortcutHelper#isWebApkInstalled() checks whether a WebAPK is installed given
a
> URL
> WebappRegistry#isWebApkInstalled() checks whether a WebAPK is installed given
an
> Android package name.
> 
> The logic for checking whether a WebAPK is installed in
> ShortcutHelper#isWebApkInstalled() is pretty standard. Its the same logic as
in
> WebappLauncherActivity#isValidWebApk()

Thanks for the explanation.

[pkotwicz, 2016-07-08 14:47:00]

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

[pkotwicz, 2016-07-08 14:47:00]

dfalcantara@ for OWNERS for ShortcutHelper.java

[gone, 2016-07-08 16:31:09]

OWNERS lgtm

[pkotwicz, 2016-07-08 17:23:25]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-07-08 17:23:26]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org
Link to the patchset: https://codereview.chromium.org/2124703003/#ps40001
(title: "Merge branch 'master' into webapk_app_banner_already_installed")

[commit-bot: I haz the power, 2016-07-08 17:24:06]

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, 2016-07-08 18:58:03]

Description was changed from

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

Non-WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135
==========

to

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

Non-WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135
==========

[commit-bot: I haz the power, 2016-07-08 18:58:04]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-07-08 18:58:52]

Description was changed from

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

Non-WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135
==========

to

==========
Do not show "Add to Home Screen" info bar if WebAPK is installed

Non-WebAPK web apps rely on a property in site preferences to determine
whether the web app has been installed. If a user clears Chrome's data
via Settings->Apps->Chrome->Storage->"Clear Data" site preferences are
cleared.
This CL adds a way for determining whether a WebAPK is installed which
works even after a user clears Chrome's data.

BUG=622135

Committed: https://crrev.com/cda82fec90f5262fbd162bb2433c5059bca1e7ba
Cr-Commit-Position: refs/heads/master@{#404453}
==========

[commit-bot: I haz the power, 2016-07-08 18:58:53]

Patchset 2 (id:??) landed as
https://crrev.com/cda82fec90f5262fbd162bb2433c5059bca1e7ba
Cr-Commit-Position: refs/heads/master@{#404453}