[Android WebAPKs] Don't navigate WebAPK when launching it from launcher

[Android WebAPKs] Don't navigate WebAPK when launching it from launcher

This CL navigates WebAPK's Web Contents:
- when WebAPK is opened for the first time
- when WebAPK is opened from a deep link and it is already running

In particular, this CL stops navigating an already open WebAPK if a user reopens the WebAPK from the launcher.

BUG=702027
Review-Url: https://codereview.chromium.org/2758193002
Cr-Commit-Position: refs/heads/master@{#459799}
Committed: https://chromium.googlesource.com/chromium/src/+/0e44ebc7dcf7c343d45694b97b2edc455f1a6256

[pkotwicz, 2017-03-20 02:26:38]

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

[pkotwicz, 2017-03-20 02:26:39]

Dominick, can you please take a look?

[dominickn, 2017-03-20 04:00:10]

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:153:
boolean shouldLoadUrlIfAlreadyRunning = true;
The null url parameter + this new boolean is really magic and feels very
fragile.

This may be a naive question, but can't we just store a flag in
WebApkActivity#onNewIntent() indicating we've already loaded a URL and are
already running (hence don't load anything)? Or can we just check the currently
loaded URL and the incoming URL and if they're the same we no-op? That's the
main use case here right? Or do we still want to allow EXTRA_URL to be set and
respected in some cases for WebAPKs?

Otherwise, I really don't like this implementation - there are new special cases
everywhere, and it feels like something that's easy to get wrong when adding new
intents into WebAPKs.

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/webapk/s...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java
(right):

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:170:
* Returns the start URL from the Android ManifesReturns the URL that the browser
should
Something went wrong with this comment here. Plus the semi-duplication between
getOverrideUrll() and getStartUrl() is strange.

[pkotwicz, 2017-03-20 22:55:45]

Patchset #6 (id:10010) has been deleted

[pkotwicz, 2017-03-20 22:58:38]

Dominick, can you please take another look?

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:153:
boolean shouldLoadUrlIfAlreadyRunning = true;
There are two situations where we hit WebApkActivity#onNewIntent()

Situation #1:
  Launching WebAPK from launcher. In this case, we don't want to navigate the
WebAPK if it is already open
Situation #2:
  Launching WebAPK from a deep link. In this case, we want to navigate the
WebAPK even if it is already open

I have changed the CL to make MainActivity.java add an extra on the intent to
indicate whether the WebAPK should navigate if it is already open

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/webapk/s...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java
(right):

https://codereview.chromium.org/2758193002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:170:
* Returns the start URL from the Android ManifesReturns the URL that the browser
should
Fixed. Thank you very much for catching this

[dominickn, 2017-03-21 02:09:33]

This explicit behaviour is much nicer, thanks.

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java
(right):

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:693:
packageInfo.packageName, "", false, ShortcutSource.UNKNOWN);
Nit: add /* forceNavigation */ after the boolean parameter

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java
(right):

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:34:
private boolean mShouldNavigateIfAlreadyRunning;
I would call this mForceNavigation;

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:76:
String webApkPackageName, String url, boolean navigateIfAlreadyRunning, int
source) {
Nit: I would put the boolean last.

Nit: I would call the parameter forceNavigation

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInfo.java:183:
public boolean shouldNavigateIfAlreadyRunning() {
shouldForceNavigation()

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java
(right):

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappInfo.java:201:
public boolean shouldNavigateIfAlreadyRunning() {
shouldForceNavigation()

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:254:
return WebApkInfo.create(WEBAPK_ID, "", false, manifestData.scopeUrl,
/* forceNavigation */

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/webapk/...
File
chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java
(right):

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/webapk/...
chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:25:
"org.chromium.chrome.browser.webapk_navigate_if_already_open";
FORCE_NAVIGATION and webapk_force_navigation

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/webapk/...
File
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java
(right):

https://codereview.chromium.org/2758193002/diff/110001/chrome/android/webapk/...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:98:
boolean navigateIfAlreadyOpen = (overrideUrl != null);
Inline this in the putExtra call (but leave the comment).

[pkotwicz, 2017-03-24 00:57:38]

Dominick, can you please take another look?

[dominickn, 2017-03-24 01:57:36]

lgtm, thanks

[pkotwicz, 2017-03-26 23:05:13]

Patchset #8 (id:150001) has been deleted

[pkotwicz, 2017-03-26 23:05:28]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-03-26 23:05:28]

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

[commit-bot: I haz the power, 2017-03-26 23:05:45]

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-27 00:55:55]

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

[commit-bot: I haz the power, 2017-03-27 00:55:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pkotwicz, 2017-03-27 15:13:24]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2017-03-27 15:13:46]

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-27 16:25:54]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1490627604583910,
"parent_rev": "f51dc18c55c7af0bac195c1b507398c5f408f0bf", "commit_rev":
"0e44ebc7dcf7c343d45694b97b2edc455f1a6256"}

[commit-bot: I haz the power, 2017-03-27 16:26:38]

Description was changed from

==========
[Android WebAPKs] Don't navigate WebAPK when launching it from launcher

This CL navigates WebAPK's Web Contents:
- when WebAPK is opened for the first time
- when WebAPK is opened from a deep link and it is already running

In particular, this CL stops navigating an already open WebAPK if a user reopens
the WebAPK from the launcher.

BUG=702027
==========

to

==========
[Android WebAPKs] Don't navigate WebAPK when launching it from launcher

This CL navigates WebAPK's Web Contents:
- when WebAPK is opened for the first time
- when WebAPK is opened from a deep link and it is already running

In particular, this CL stops navigating an already open WebAPK if a user reopens
the WebAPK from the launcher.

BUG=702027

Review-Url: https://codereview.chromium.org/2758193002
Cr-Commit-Position: refs/heads/master@{#459799}
Committed:
https://chromium.googlesource.com/chromium/src/+/0e44ebc7dcf7c343d45694b97b2e...
==========

[commit-bot: I haz the power, 2017-03-27 16:26:39]

Committed patchset #8 (id:170001) as
https://chromium.googlesource.com/chromium/src/+/0e44ebc7dcf7c343d45694b97b2e...