Add FLAG_ACTIVITY_NEW_TASK to trusted Intents

Add FLAG_ACTIVITY_NEW_TASK to trusted Intents

Because we are starting trusted intents from the application context, we
need FLAG_ACTIVITY_NEW_TASK on pre-N versions of Android.  On N+ we can
get away with specifying a task ID or not specifying an options bundle.
This fixes a crash with the Physical Web that is currently failing to
set this flag in its Intent.

BUG=663842, 681441
Review-Url: https://codereview.chromium.org/2638153002
Cr-Commit-Position: refs/heads/master@{#444180}
Committed: https://chromium.googlesource.com/chromium/src/+/12a49ca76a658d24d34f6dbe9b6976adf65537a9

[cco3, 2017-01-17 20:05:24]

cco3@chromium.org changed reviewers:
+ tedchoc@chromium.org

[cco3, 2017-01-17 20:05:25]

Hi Ted, this is a follow up to https://codereview.chromium.org/2601273003

[Ted C, 2017-01-17 21:00:38]

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

https://codereview.chromium.org/2638153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:580:
copiedIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
Right now, all callers have added this before calling this method, so I would
suggest doing that for physical web as well (at least at the moment).

In particular, it seems nuanced with the changes in N+, which I wasn't aware of,
and you could also specify ACTIVITY_NEW_DOCUMENT L+.  I am all for adutting the
codebase and unifying this, but if we add this, then it will be weird why we add
it in most cases except for physical web.

[cco3, 2017-01-17 21:08:27]

On 2017/01/17 21:00:38, Ted C wrote:
>
https://codereview.chromium.org/2638153002/diff/1/chrome/android/java/src/org...
> File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
> (right):
> 
>
https://codereview.chromium.org/2638153002/diff/1/chrome/android/java/src/org...
> chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:580:
> copiedIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
> Right now, all callers have added this before calling this method, so I would
> suggest doing that for physical web as well (at least at the moment).
> 
> In particular, it seems nuanced with the changes in N+, which I wasn't aware
of,
> and you could also specify ACTIVITY_NEW_DOCUMENT L+.  I am all for adutting
the
> codebase and unifying this, but if we add this, then it will be weird why we
add
> it in most cases except for physical web.

The other two uses of this method also use the intent for other things (so I
don't know if it'd be safe to just remove the flag in those instances).  I'll go
ahead and add this for PW.  I think we should make sure it's added in
IntentHandler though, just in case someone else comes along and makes the same
mistake I did (getting their code running on N, but not other versions of
Android).

[Ted C, 2017-01-17 21:40:02]

lgtm ... w/ switch to assert in IntentHandler

https://codereview.chromium.org/2638153002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
(right):

https://codereview.chromium.org/2638153002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:580:
copiedIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
For now, I would just make this an assert

assert (copiedIntent.getFlags() & Intent.FLAG_ACTIVITY_NEW_TASK) != 0;

Then we could update as needed (to support NEW_DOCUMENT).  Or we go through all
call sites and remove NEW_TASK from each and make this the only place that adds
it.

[cco3, 2017-01-17 21:58:46]

https://codereview.chromium.org/2638153002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java
(right):

https://codereview.chromium.org/2638153002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java:580:
copiedIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
On 2017/01/17 21:40:02, Ted C wrote:
> For now, I would just make this an assert
> 
> assert (copiedIntent.getFlags() & Intent.FLAG_ACTIVITY_NEW_TASK) != 0;
> 
> Then we could update as needed (to support NEW_DOCUMENT).  Or we go through
all
> call sites and remove NEW_TASK from each and make this the only place that
adds
> it.

Done.  Will need to rebase on other change.

[cco3, 2017-01-17 22:05:44]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-01-17 22:05:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2638153002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-17 22:06:16]

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-01-17 22:28:26]

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

[commit-bot: I haz the power, 2017-01-17 22:28:28]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[cco3, 2017-01-17 22:40:15]

The CQ bit was checked by cco3@chromium.org

[cco3, 2017-01-17 22:40:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2638153002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-17 22:40:48]

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-01-17 23:17:47]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484692815512460,
"parent_rev": "db478183363dad5b460070122b991a8302a1f98a", "commit_rev":
"12a49ca76a658d24d34f6dbe9b6976adf65537a9"}

[commit-bot: I haz the power, 2017-01-17 23:18:23]

Description was changed from

==========
Add FLAG_ACTIVITY_NEW_TASK to trusted Intents

Because we are starting trusted intents from the application context, we
need FLAG_ACTIVITY_NEW_TASK on pre-N versions of Android.  On N+ we can
get away with specifying a task ID or not specifying an options bundle.
This fixes a crash with the Physical Web that is currently failing to
set this flag in its Intent.

BUG=663842,681441
==========

to

==========
Add FLAG_ACTIVITY_NEW_TASK to trusted Intents

Because we are starting trusted intents from the application context, we
need FLAG_ACTIVITY_NEW_TASK on pre-N versions of Android.  On N+ we can
get away with specifying a task ID or not specifying an options bundle.
This fixes a crash with the Physical Web that is currently failing to
set this flag in its Intent.

BUG=663842,681441

Review-Url: https://codereview.chromium.org/2638153002
Cr-Commit-Position: refs/heads/master@{#444180}
Committed:
https://chromium.googlesource.com/chromium/src/+/12a49ca76a658d24d34f6dbe9b69...
==========

[commit-bot: I haz the power, 2017-01-17 23:18:25]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/12a49ca76a658d24d34f6dbe9b69...