Add external intents for WebAPKs to Launch.HomescreenSource.

Add external intents for WebAPKs to Launch.HomescreenSource.

This CL adds a new EXTERNAL_INTENT source for WebAPK launches that are
created by intenting from other Android apps.

BUG=691739
Review-Url: https://codereview.chromium.org/2790873002
Cr-Commit-Position: refs/heads/master@{#461982}
Committed: https://chromium.googlesource.com/chromium/src/+/a8a006c772c4a44469ae396ff01c2c835a48b670

[dominickn, 2017-03-31 04:59:58]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-03-31 05:00:13]

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

[dominickn, 2017-03-31 05:00:14]

Peter: I *think* this is right. Can you take a look please?

[commit-bot: I haz the power, 2017-03-31 05:00:24]

Dry run: 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-31 05:58:56]

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

[commit-bot: I haz the power, 2017-03-31 05:58:57]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pkotwicz, 2017-03-31 17:19:28]

https://codereview.chromium.org/2790873002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
This is only a subset of the times that a WebAPK is launched via a deep link
- This if() statement will not be called if there are multiple handlers which
can handle the url (For instance, this occurs if you have multiple versions of
Chrome installed)
- This if() statement does not catch the case of the WebAPK getting launched via
GMail (or any other app)

A WebAPK is launched from an external intent whenever
launchHostBrowserInWebApkMode() is called with a non-null |overrideUrl|

[dominickn, 2017-04-03 00:36:15]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 00:36:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-04-03 00:36:57]

Thanks for the explanation Peter. PTAL!

https://codereview.chromium.org/2790873002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
On 2017/03/31 17:19:28, pkotwicz wrote:
> This is only a subset of the times that a WebAPK is launched via a deep link
> - This if() statement will not be called if there are multiple handlers which
> can handle the url (For instance, this occurs if you have multiple versions of
> Chrome installed)
> - This if() statement does not catch the case of the WebAPK getting launched
via
> GMail (or any other app)
> 
> A WebAPK is launched from an external intent whenever
> launchHostBrowserInWebApkMode() is called with a non-null |overrideUrl|

Cool. I've added another source placement in that method, thanks!

[commit-bot: I haz the power, 2017-04-03 03:01:57]

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

[commit-bot: I haz the power, 2017-04-03 03:01:57]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-04-04 01:39:19]

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
Isn't this change redundant now? Isn't your change to MainActivity.java
sufficient?

https://codereview.chromium.org/2790873002/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/2790873002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:98:
: getIntent().getIntExtra(WebApkConstants.EXTRA_SOURCE, 0);
1) You only want to set |source| to
WebApkConstants.SHORTCUT_SOURCE_EXTERNAL_INTENT if

getIntent().getIntExtra(WebApkConstants.EXTRA_SOURCE, 0) != 0

in order to handle the WebAPK being launched from a notification.
ServiceTabLauncher#launchTab()

2) Whenver you modify code in chrome/android/webapk/shell_apk/ you need to
update chrome/android/webapk/shell_apk/shell_apk_version.gni We need to figure
out a process for making these changes because the server needs to be updated as
well. I suspect that the release process will be:
a) Land code changes in chrome/android/webapk/shell_apk/
b) Update server
c) Update version number

[dominickn, 2017-04-04 02:55:59]

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
On 2017/04/04 01:39:19, pkotwicz wrote:
> Isn't this change redundant now? Isn't your change to MainActivity.java
> sufficient?

We know for sure that we're intenting out to the WebAPK here so we may as well
explicitly set that. Particularly since I'm a little confused at your advice for
the other file: setting this explicitly now gels with what I've done there.

https://codereview.chromium.org/2790873002/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/2790873002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:98:
: getIntent().getIntExtra(WebApkConstants.EXTRA_SOURCE, 0);
On 2017/04/04 01:39:19, pkotwicz wrote:
> 1) You only want to set |source| to
> WebApkConstants.SHORTCUT_SOURCE_EXTERNAL_INTENT if
> 
> getIntent().getIntExtra(WebApkConstants.EXTRA_SOURCE, 0) != 0
> 
> in order to handle the WebAPK being launched from a notification.
> ServiceTabLauncher#launchTab()

This doesn't make sense to me. Do you mean override the source if the existing
source *is* = 0? If it's coming in from ServiceTabLauncher the source should be
set to SOURCE_NOTIFICATION, so overriding when = 0 makes sense. Let me know if
I've interpreted this wrong.

> 2) Whenver you modify code in chrome/android/webapk/shell_apk/ you need to
> update chrome/android/webapk/shell_apk/shell_apk_version.gni We need to figure
> out a process for making these changes because the server needs to be updated
as
> well. I suspect that the release process will be:
> a) Land code changes in chrome/android/webapk/shell_apk/
> b) Update server
> c) Update version number

Thanks! BTW you should add a PRESUBMIT.py file in shell_apk that enforces this.

[pkotwicz, 2017-04-04 03:23:11]

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
I still don't think that we should set EXTRA_SOURCE here. It duplicates what we
already do in MainActivity.java

https://codereview.chromium.org/2790873002/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/2790873002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:98:
: getIntent().getIntExtra(WebApkConstants.EXTRA_SOURCE, 0);
- Sorry for the confusion. I meant only override when source *is* == 0

- Can you please file a bug to add a PRESUBMIT ?

[dominickn, 2017-04-04 03:38:25]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 03:38:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominickn, 2017-04-04 03:39:01]

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
On 2017/04/04 03:23:11, pkotwicz wrote:
> I still don't think that we should set EXTRA_SOURCE here. It duplicates what
we
> already do in MainActivity.java

This doesn't actually duplicate what happens in MainActivity.java (that code
isn't run now if the source is already set).

In terms of code health, I think it's better to explicitly set the source here
when we know we're setting it to the right value. We may have a new way of
launching WebAPKs in the future that we want to categorize separately. And it
just feels a bit wrong to have it come out as UNKNOWN from here when we actually
know it's from an intent.

https://codereview.chromium.org/2790873002/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/2790873002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/src/org/chromium/webapk/shell_apk/MainActivity.java:98:
: getIntent().getIntExtra(WebApkConstants.EXTRA_SOURCE, 0);
On 2017/04/04 03:23:11, pkotwicz wrote:
> - Sorry for the confusion. I meant only override when source *is* == 0
> 
> - Can you please file a bug to add a PRESUBMIT ?

Filed crbug.com/708053

[commit-bot: I haz the power, 2017-04-04 05:48:40]

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

[commit-bot: I haz the power, 2017-04-04 05:48:41]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-04-04 14:04:10]

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
It would be set in MainActivity.java if you don't set the source.

The reason that I care is that it is possible for ExternalNavigationHandler to
launch a WebAPK without going through this if() statement. In particular, this
if() statement is not run if ExternalNavigationHandler shows an intent picker
and the user chooses the WebAPK from the intent picker.

[dominickn, 2017-04-05 01:05:03]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[dominickn, 2017-04-05 01:05:49]

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

[dominickn, 2017-04-05 01:05:50]

Thanks!

+isherman for histograms

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/2790873002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:535:
intent.putExtra(ShortcutHelper.EXTRA_SOURCE, ShortcutSource.EXTERNAL_INTENT);
On 2017/04/04 14:04:10, pkotwicz wrote:
> It would be set in MainActivity.java if you don't set the source.
> 
> The reason that I care is that it is possible for ExternalNavigationHandler to
> launch a WebAPK without going through this if() statement. In particular, this
> if() statement is not run if ExternalNavigationHandler shows an intent picker
> and the user chooses the WebAPK from the intent picker.

Fair enough, that reasoning seems sound. Removed the change here.

[commit-bot: I haz the power, 2017-04-05 01:05:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-04-05 01:28:39]

histograms.xml lgtm

[commit-bot: I haz the power, 2017-04-05 02:37:35]

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

[commit-bot: I haz the power, 2017-04-05 02:37:36]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-04-05 03:01:50]

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

https://codereview.chromium.org/2790873002/diff/80001/chrome/android/webapk/l...
chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:28:
public static final int SHORTCUT_SOURCE_UNKNOWN = 9;
You mean 0?

[dominickn, 2017-04-05 03:08:20]

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

https://codereview.chromium.org/2790873002/diff/80001/chrome/android/webapk/l...
chrome/android/webapk/libs/common/src/org/chromium/webapk/lib/common/WebApkConstants.java:28:
public static final int SHORTCUT_SOURCE_UNKNOWN = 9;
On 2017/04/05 03:01:50, pkotwicz wrote:
> You mean 0?

D'oh, of course. Good catch.

[dominickn, 2017-04-05 03:08:22]

The CQ bit was checked by dominickn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 03:08:45]

Dry run: 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-04-05 03:23:08]

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

[commit-bot: I haz the power, 2017-04-05 03:23:08]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[pkotwicz, 2017-04-05 03:46:27]

LGTM

[dominickn, 2017-04-05 04:36:48]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2017-04-05 04:36:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2790873002/#ps100001
(title: "D'oh")

[commit-bot: I haz the power, 2017-04-05 04:36:59]

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-04-05 04:52:05]

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

[commit-bot: I haz the power, 2017-04-05 04:52:07]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[dominickn, 2017-04-05 04:54:33]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2017-04-05 04:54:50]

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-04-05 05:00:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491368073461570,
"parent_rev": "3619dab216f9b1e431f5b271fa668eaee0f95112", "commit_rev":
"a8a006c772c4a44469ae396ff01c2c835a48b670"}

[commit-bot: I haz the power, 2017-04-05 05:01:38]

Description was changed from

==========
Add external intents for WebAPKs to Launch.HomescreenSource.

This CL adds a new EXTERNAL_INTENT source for WebAPK launches that are
created by intenting from other Android apps.

BUG=691739
==========

to

==========
Add external intents for WebAPKs to Launch.HomescreenSource.

This CL adds a new EXTERNAL_INTENT source for WebAPK launches that are
created by intenting from other Android apps.

BUG=691739

Review-Url: https://codereview.chromium.org/2790873002
Cr-Commit-Position: refs/heads/master@{#461982}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8a006c772c4a44469ae396ff01c...
==========

[commit-bot: I haz the power, 2017-04-05 05:01:40]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a8a006c772c4a44469ae396ff01c...