WebAPKs: Open links which are outside of WebAPK scope in Chrome not in WebAPK

WebAPKs: Open links which are outside of WebAPK scope in Chrome not in WebAPK

BUG=609122
TEST=ExternalNavigationHandlerTest.*

Committed: https://crrev.com/a673c630f315de4b132fa512b70b63525833e44c
Cr-Commit-Position: refs/heads/master@{#397600}

[pkotwicz, 2016-05-23 22:33:10]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-05-23 22:34:13]

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

[pkotwicz, 2016-05-23 22:34:14]

Xi, can you please take a look before I send this off to dfalcantara@?

[pkotwicz, 2016-05-27 00:31:18]

Xi, I will rebase this CL onto your CL which just landed super soon :)

[Xi Han, 2016-05-27 13:40:21]

On 2016/05/27 00:31:18, pkotwicz wrote:
> Xi, I will rebase this CL onto your CL which just landed super soon :)

Cool, it is not blocked anymore!

[pkotwicz, 2016-05-27 19:24:10]

Xi can you please take a look? This CL now depends on
https://codereview.chromium.org/2018113002/

[Xi Han, 2016-05-30 20:16:08]

lgtm

https://codereview.chromium.org/2005053002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java
(right):

https://codereview.chromium.org/2005053002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java:38:
* @return Package name of the browser associated with the currently running
activity.
@return -> Returns?

https://codereview.chromium.org/2005053002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegate.java:43:
* @return Package name of the WebAPK associated with the currently running
activity. Null if
@return -> Returns?

[pkotwicz, 2016-05-31 17:24:07]

Patchset #3 (id:60001) has been deleted

[pkotwicz, 2016-05-31 17:26:16]

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

[pkotwicz, 2016-05-31 17:26:17]

dfalcantara@ can you please take a look?

[gone, 2016-05-31 20:42:53]

dfalcantara@chromium.org changed reviewers:
+ mariakhomenko@google.com

[gone, 2016-05-31 20:42:55]

+Maria as expert on ExternalNavigationBlahBlahFactory

[gone, 2016-05-31 20:43:14]

dfalcantara@chromium.org changed reviewers:
+ mariakhomenko@chromium.org
- mariakhomenko@google.com

[gone, 2016-05-31 20:43:15]

+Maria for realz.

[Maria, 2016-05-31 23:31:21]

This is not correctly structured. ExternalNavigationDelegateImpl exists to add
seams for touching the context and webApkName shouldn't be passed into its
constructor as a flag for behaviour change.

What I would propose instead is:

1. Add a boolean to ExternalNavigationParams to indicate if request isFromWebApk

2. Add a seam in InterceptNavigationDelegateImpl for creating (or adding)
parameters
3. Make your own implementation that overrides this to add isFromWebApk(true) in
WebApkActivity
4. Use that parameter in ExternalNavigationHandler logic

https://chromiumcodereview.appspot.com/2005053002/diff/80001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://chromiumcodereview.appspot.com/2005053002/diff/80001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:346:
mDelegate.startChromeActivity(params.getUrl());
We shouldn't be re-creating the logic for generating an intent inside
startChromeActivity(). Pass in the intent we've generated so far -- it should be
correct. The only thing you might need to do in this method is set the package
to ensure it goes directly to Chrome.

[pkotwicz, 2016-06-01 03:37:39]

Patchset #4 (id:100001) has been deleted

[pkotwicz, 2016-06-01 03:38:06]

Maria, can you please take another look? I implemented your suggestions. The
code looks cleaner now.

[Maria, 2016-06-01 16:50:09]

lgtm

[gone, 2016-06-02 17:15:27]

lgtm % nits

https://codereview.chromium.org/2005053002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationParams.java
(right):

https://codereview.chromium.org/2005053002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationParams.java:47:
private boolean mIsWebApk;
private final boolean mIsWebApk;

[pkotwicz, 2016-06-03 00:48:30]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-06-03 00:48:31]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org,
dfalcantara@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2005053002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-06-03 00:49:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005053002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2005053002/140001

[commit-bot: I haz the power, 2016-06-03 02:47:07]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-06-03 02:50:04]

Description was changed from

==========
WebAPKs: Open links which are outside of WebAPK scope in Chrome not in WebAPK

BUG=609122
TEST=ExternalNavigationHandlerTest.*
==========

to

==========
WebAPKs: Open links which are outside of WebAPK scope in Chrome not in WebAPK

BUG=609122
TEST=ExternalNavigationHandlerTest.*

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

[commit-bot: I haz the power, 2016-06-03 02:50:06]

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