Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889
Review-Url: https://codereview.chromium.org/2898373002
Cr-Original-Commit-Position: refs/heads/master@{#480286}
Committed: https://chromium.googlesource.com/chromium/src/+/4d9776c156c177107053872a437a9639d95a7160
Review-Url: https://codereview.chromium.org/2898373002
Cr-Commit-Position: refs/heads/master@{#480612}
Committed: https://chromium.googlesource.com/chromium/src/+/e1753b5ae0c9972faf0da0b2138fb9b92c3db2e8

[piotrs, 2017-05-24 01:09:58]

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

[commit-bot: I haz the power, 2017-05-24 01:11:09]

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-05-24 07:41:52]

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

[commit-bot: I haz the power, 2017-05-24 07:41:53]

Dry run: 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...)

[piotrs, 2017-05-29 07:39:28]

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

[commit-bot: I haz the power, 2017-05-29 07:39:44]

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-05-29 08:51:20]

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

[commit-bot: I haz the power, 2017-05-29 08:51:21]

Dry run: This issue passed the CQ dry run.

[piotrs, 2017-05-30 01:07:00]

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

[commit-bot: I haz the power, 2017-05-30 01:07:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piotrs, 2017-05-30 03:22:43]

Description was changed from

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.

This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done, this patch brings
the same to _blank and window.open().

BUG=709889
==========

to

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889
==========

[commit-bot: I haz the power, 2017-05-30 03:30:42]

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

[commit-bot: I haz the power, 2017-05-30 03:30:43]

Dry run: 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...)

[piotrs, 2017-05-30 04:37:15]

Patchset #1 (id:1) has been deleted

[piotrs, 2017-05-30 04:37:23]

Patchset #2 (id:40001) has been deleted

[piotrs, 2017-05-30 04:37:31]

Patchset #2 (id:60001) has been deleted

[piotrs, 2017-05-30 04:37:38]

Patchset #1 (id:20001) has been deleted

[piotrs, 2017-05-30 04:37:55]

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

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

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-05-30 06:10:03]

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

[commit-bot: I haz the power, 2017-05-30 06:10:05]

Dry run: This issue passed the CQ dry run.

[piotrs, 2017-05-30 06:32:44]

piotrs@chromium.org changed reviewers:
+ dominickn@chromium.org, yusufo@chromium.org

[piotrs, 2017-05-30 06:32:45]

Hi folks, I've got another PWA -> CCT redirect patch.

@Yusuf: Can you please take a look at CustomTabActivity change? This time I'm
actually messing with your code, please read the end of patch description for
more context.

@Dominick: Could you take a look how this fits into Webapps?

Thank you both,
Piotr

[dominickn, 2017-05-30 06:51:53]

Looks pretty good at first pass!

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

https://codereview.chromium.org/2898373002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:87:
public static final String EXTRA_AFFILIATED_WEBAPP_ID =
It looks like this constant is only used in WebappTabDelegate.java. Can you move
this to be a private constant in that file?

[piotrs, 2017-05-30 07:01:40]

Thanks Dom, done.

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

https://codereview.chromium.org/2898373002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ShortcutHelper.java:87:
public static final String EXTRA_AFFILIATED_WEBAPP_ID =
On 2017/05/30 06:51:52, dominickn wrote:
> It looks like this constant is only used in WebappTabDelegate.java. Can you
move
> this to be a private constant in that file?

Done.

[pkotwicz, 2017-05-30 16:48:53]

This CL is exciting. So I tested it. It looks like if the CCT tries to open a
new window via window.open(), instead of opening a new CCT, the CCT is replaced.
I don't know whether this is the intended behavior. However, it is worth calling
out in the CL description

[piotrs, 2017-05-30 23:50:39]

On 2017/05/30 16:48:53, pkotwicz wrote:
> This CL is exciting. So I tested it. It looks like if the CCT tries to open a
> new window via window.open(), instead of opening a new CCT, the CCT is
replaced.
> I don't know whether this is the intended behavior. However, it is worth
calling
> out in the CL description

Hi Peter,

It is called out at the end of CL description. I actually explicitly implemented
this as it seemed desirable behavior. Having you question this is interesting
and
now I'm wondering whether I should allow PWA to open multiple new task CCTs.
WDYT?

[dominickn, 2017-05-31 01:56:10]

window.open() from a CCT is an interesting question. What is the default
behaviour for, say, GSA or native apps that use a Custom Tab in that case?

[piotrs, 2017-05-31 04:41:41]

On 2017/05/31 01:56:10, dominickn wrote:
> window.open() from a CCT is an interesting question. What is the default
> behaviour for, say, GSA or native apps that use a Custom Tab in that case?

Hangouts, Twitter and Gmail open CCTs in the same task. GSA is the only app I
found that opens CCTs in new tasks. It allows creating multiple Android tasks
if you keep on switching back to GSA and launching new links. Given that,
should we do the same for PWAs? I would be inclined to revert my code closing
existing CCT before opening next one. Opinions?

[piotrs, 2017-05-31 05:50:35]

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

[commit-bot: I haz the power, 2017-05-31 05:51:02]

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-05-31 06:50:13]

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

[commit-bot: I haz the power, 2017-05-31 06:50:14]

Dry run: 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...)

[pkotwicz, 2017-05-31 15:28:46]

yusufo@ do you have a strong opinion w.r.t comment #29?

I personally don't have a strong opinion either way. Can you be a bit more
explicit in the CL description about what occurs when a CCT requests
window.open() ?

[Yusuf, 2017-05-31 18:45:27]

On 2017/05/31 15:28:46, pkotwicz wrote:
> yusufo@ do you have a strong opinion w.r.t comment #29?
> 
> I personally don't have a strong opinion either way. Can you be a bit more
> explicit in the CL description about what occurs when a CCT requests
> window.open() ?

if we think blank_
and window.open() qualifies as starting new flows, it may make sense.

An important thing to consider though:

Although GSA opens new tasks, it is still attributed to a single search query,
if you go to Recents and check the task description, they still actually
launch a bouncer activity to set the title and icon. Opening in the same task
gives us
1)the branding from the launching PWA, 2) A commitment to closing the tab and
avoid cluttering
the recents list.

I think this heavily depends on whether we can pull of the UX for task
management. What will
recents look like, if I end up clicking 3-4 blank_ window.open() links back to
back from a PWA?
If it is going to give me 4 non-branded, grey colored, "Chrome" tasks, then no,
we should open
these in the same task.

[piotrs, 2017-06-01 08:25:08]

Few comments:

@Peter: We might be talking about different things, you wrote "if the CCT tries
to open a new window via window.open()", did you really mean CCT and not PWA?
What happens once user is in CCT is outside of scope of this CL, whatever has
been decided and implemented for CCTs is I assume reasonable. My concerns are
around what should happen when user navigates outside of PWA.

@Everyone:
The thinking behind my patches is that PWA should mimic the browser:
- links with target="_self" and window.top.location put CCT on top of the task,
which mimic reload of content in the browser.
- links with target="_blank" and window.open() create a new task, which mimics
creating a new tab/window in the browser.
- we decided to allow PWA to open multiple CCTs as user action to open _blank,
switch back to PWA with recents and open another _blank is quite involved (order
of magnitude more so than pressing "back" or clicking "x" on CCT). It is
therefore assumed user knows what they're doing and intend to create a new
context (i.e. task).

CCTs are only branded with PWA color, but not with icon. I can investigate what
it would take to pull the PWA for these new task CCTs. WDYT?

[pkotwicz, 2017-06-01 15:21:15]

@piotrs: I meant what occurs when a PWA uses window.open() to open a CCT. The
page in the CCT in turns does window.open()
e.g. if this site were a PWA https://jsfiddle.net/6r886ep8/2/
In particular, is it possible to have multiple CCTs stacked on top of each
other?
I guess you are saying is that the current behavior: running window.open() in a
CCT replaces the current CCT is controlled by the CCT code?

[Yusuf, 2017-06-01 21:16:20]

On 2017/06/01 15:21:15, pkotwicz wrote:
> @piotrs: I meant what occurs when a PWA uses window.open() to open a CCT. The
> page in the CCT in turns does window.open()
> e.g. if this site were a PWA https://jsfiddle.net/6r886ep8/2/
> In particular, is it possible to have multiple CCTs stacked on top of each
> other?

Yes, it should be possible, on different activities and within the same task.
That may actually be a better solution
here if we dont have a nice UX story for multiple unbranded tasks lying around.


> I guess you are saying is that the current behavior: running window.open() in
a
> CCT replaces the current CCT is controlled by the CCT code?

Yes, in that case we create a new tab and assign that as the MainTab of the
customtabactivity and the user
has to go back pressing system back to get to the initial tab again.


I think it would be helpful to see a screenshot of recents with 4 of these tabs
getting launched
from the same PWA.

[piotrs, 2017-06-01 23:45:17]

@Peter: What happens while you're inside CCT is not in my control.

@Yusuf: Indeed in Recents we do just see grey chrome tasks. Screenshot as of
now: https://drive.google.com/file/d/0B8U5hbo87eViLUJYS25PZmx4bmc/view

I will take a look what would it take to style them. It would be nice to style
then as for example Twitter, if it's Twitter Lite that opens them.

[piotrs, 2017-06-02 04:20:15]

On 2017/06/01 23:45:17, piotrs wrote:
> @Peter: What happens while you're inside CCT is not in my control.
> 
> @Yusuf: Indeed in Recents we do just see grey chrome tasks. Screenshot as of
> now: https://drive.google.com/file/d/0B8U5hbo87eViLUJYS25PZmx4bmc/view
> 
> I will take a look what would it take to style them. It would be nice to style
> then as for example Twitter, if it's Twitter Lite that opens them.

I hacked it up, and I don't think it looks good unless I let PWA control icon
and color, while CCT controls the title.

With all properties inherited by PWA:
https://drive.google.com/file/d/0B8U5hbo87eViajFTbHpJTEVTeTg/view

With Color and Icon from PWA, while title set by website:
https://drive.google.com/file/d/0B8U5hbo87eVibVN2b1p2MS1zMjA/view

WDYT?

BTW. code for this is not uploaded here.

[dominickn, 2017-06-02 04:34:50]

Both of those feel a bit confusing to me, but having the title controlled by the
CCT and everything else inherited from the PWA is definitely better I think.

[piotrs, 2017-06-02 05:13:31]

For completeness, screenshots with 3 options (no styling, icon + color, icon +
color + title) are here:
https://goo.gl/photos/qf69x5vzxtvoRwyg9

[piotrs, 2017-06-05 00:15:58]

On 2017/06/02 05:13:31, piotrs wrote:
> For completeness, screenshots with 3 options (no styling, icon + color, icon +
> color + title) are here:
> https://goo.gl/photos/qf69x5vzxtvoRwyg9

Hi Yusuf,

We discussed the CCT branding in Sydney and our consensus is that:
1) Option with CCT inheriting icon and title from PWA is very confusing and
clearly worst option.
2) Option with CCT inheriting icon but not title is problematic because apart
from CCT toolbar color those task have no link back to PWA - once you visited
recents screen, "X" on CCT will take you to homescreen, not PWA. Branding would
make more sense if we allowed PWA more control over CCTs (e.g. adding custom
buttons), but we currently don't. There might also be branding concerns with
showing PWA icon next to controversial website titles.
3) No task branding is most conservative, but it's also what you get in native
apps if you don't do the bouncer activity trick. Our consensus is that this is
what we should go for.

Let me know if you feel differently.

[Yusuf, 2017-06-05 16:42:50]

On 2017/06/05 00:15:58, piotrs wrote:
> On 2017/06/02 05:13:31, piotrs wrote:
> > For completeness, screenshots with 3 options (no styling, icon + color, icon
+
> > color + title) are here:
> > https://goo.gl/photos/qf69x5vzxtvoRwyg9
> 
> Hi Yusuf,
> 
> We discussed the CCT branding in Sydney and our consensus is that:
> 1) Option with CCT inheriting icon and title from PWA is very confusing and
> clearly worst option.
> 2) Option with CCT inheriting icon but not title is problematic because apart
> from CCT toolbar color those task have no link back to PWA - once you visited
> recents screen, "X" on CCT will take you to homescreen, not PWA. Branding
would
> make more sense if we allowed PWA more control over CCTs (e.g. adding custom
> buttons), but we currently don't. There might also be branding concerns with
> showing PWA icon next to controversial website titles.
> 3) No task branding is most conservative, but it's also what you get in native
> apps if you don't do the bouncer activity trick. Our consensus is that this is
> what we should go for.
> 
> Let me know if you feel differently.

To be honest, since this is a UX decision, other than pointing out what can be
done and all others are doing and where they are having trouble, I am not
inclined
to push back on final decisions.

That said, I still have to point out that although the non-branded UI is what
you get
in native apps if they dont use bouncers, nobody does that. We don't have
official
support for custom tabs in their own task (ie, there is no launchInSeparateTask)
in
the public API and other than AGSA and our own Herb experiment, this doesn't
have
any precedence. And for those two, either AGSA or us take extra care to put the
branding in so that we avoid multiple similar looking tasks cluttering the
Recents.
So although it is technically possible, this is a direction off the norm.

If we have UX approval, I would have no arguments against moving forward with it
though.

[piotrs, 2017-06-09 01:45:37]

On 2017/06/05 16:42:50, Yusuf wrote:
> On 2017/06/05 00:15:58, piotrs wrote:
> > On 2017/06/02 05:13:31, piotrs wrote:
> > > For completeness, screenshots with 3 options (no styling, icon + color,
icon
> +
> > > color + title) are here:
> > > https://goo.gl/photos/qf69x5vzxtvoRwyg9
> > 
> > Hi Yusuf,
> > 
> > We discussed the CCT branding in Sydney and our consensus is that:
> > 1) Option with CCT inheriting icon and title from PWA is very confusing and
> > clearly worst option.
> > 2) Option with CCT inheriting icon but not title is problematic because
apart
> > from CCT toolbar color those task have no link back to PWA - once you
visited
> > recents screen, "X" on CCT will take you to homescreen, not PWA. Branding
> would
> > make more sense if we allowed PWA more control over CCTs (e.g. adding custom
> > buttons), but we currently don't. There might also be branding concerns with
> > showing PWA icon next to controversial website titles.
> > 3) No task branding is most conservative, but it's also what you get in
native
> > apps if you don't do the bouncer activity trick. Our consensus is that this
is
> > what we should go for.
> > 
> > Let me know if you feel differently.
> 
> To be honest, since this is a UX decision, other than pointing out what can be
> done and all others are doing and where they are having trouble, I am not
> inclined
> to push back on final decisions.
> 
> That said, I still have to point out that although the non-branded UI is what
> you get
> in native apps if they dont use bouncers, nobody does that. We don't have
> official
> support for custom tabs in their own task (ie, there is no
launchInSeparateTask)
> in
> the public API and other than AGSA and our own Herb experiment, this doesn't
> have
> any precedence. And for those two, either AGSA or us take extra care to put
the
> branding in so that we avoid multiple similar looking tasks cluttering the
> Recents.
> So although it is technically possible, this is a direction off the norm.
> 
> If we have UX approval, I would have no arguments against moving forward with
it
> though.

ainslie@ suggested starting with unbranded CCTs (I CCed you on the thread).
Should
we go for it and continue with code review? BTW. Thanks for pointing out the
options
here!

[piotrs, 2017-06-13 02:41:38]

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

[commit-bot: I haz the power, 2017-06-13 02:41:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piotrs, 2017-06-13 02:46:17]

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

[commit-bot: I haz the power, 2017-06-13 02:46:39]

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-06-13 04:16:55]

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

[commit-bot: I haz the power, 2017-06-13 04:16:56]

Dry run: This issue passed the CQ dry run.

[piotrs, 2017-06-14 11:42:52]

Ping, any chance we could move forward with this patch? Thanks!

[Yusuf, 2017-06-14 18:53:10]

overall it looks good. Lets change the way we handle async web contents a bit
and we should be good to go.

Please let me know if you have any question about createMainTab logic.

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:594:
if (asyncParams != null && asyncParams.getWebContents() != null) {
an overall comment about this segment:

The logic here by itself is extremely complicated and hard to read. We should
strive to at least not regress current state of readability. This block here
makes it look like TRANSFERRED_WEBCONTENTS is the most common case.

It should be safe to push the tab construction below the WebContents related
histogram below. Then all you have is the logic of figuring out what kind of web
contents you want to have.

The order should be:
Async or Prerendered
SpareWebContents
New WebContents

Technically it shouldn't matter which one you are attempting between prerender
and async because they should never match (or something else is wrong).

Would be nice to split all above async related logic to a takeAsyncWebContents,
either here or in AsyncTabParamsManager(similar to takePrerenderedUrl). Maybe
then you can keep the order you have with a similar pattern of:
 tookWebContentsFromX = webContents != null
 if (!tookWebContentsFromX) {
 webContents = attemptToTakeY
 if (webContents != null) set launchtype, histogram value
 
}

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
(right):

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java:42:
customTabIntent.intent.setPackage(mActivity.getPackageName());
let's add IntentHandler.addTrustedIntentExtras instead of this. This way it will
be safe from any purges of non-Chrome intents.

[dominickn, 2017-06-15 00:41:01]

webapps lgtm % yusufo's suggestions.

[piotrs, 2017-06-15 06:24:46]

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

[commit-bot: I haz the power, 2017-06-15 06:24:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piotrs, 2017-06-15 06:25:32]

Addressed comments + removed CCT toolbar styling based on PWA theme color, as
decided with the mocks presented to Ainslie@.

[piotrs, 2017-06-15 06:26:16]

Replies to comments here :)

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:594:
if (asyncParams != null && asyncParams.getWebContents() != null) {
Agreed, it's a bit of a mess. Take a look, I'm quite happy with the new
separation.

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
(right):

https://codereview.chromium.org/2898373002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java:42:
customTabIntent.intent.setPackage(mActivity.getPackageName());
On 2017/06/14 18:53:10, Yusuf wrote:
> let's add IntentHandler.addTrustedIntentExtras instead of this. This way it
will
> be safe from any purges of non-Chrome intents.

Added, but removing setPackage() caused the browser dialog to pop out again, so
I kept it. I don't want to because A) we're staying in the scope of the same
browser and B) it makes testing more difficult. WDYT?

[commit-bot: I haz the power, 2017-06-15 07:44:38]

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

[commit-bot: I haz the power, 2017-06-15 07:44:39]

Dry run: This issue passed the CQ dry run.

[Yusuf, 2017-06-15 16:15:56]

Thanks so much for all the due diligence about this review. I really appreciate
your attention to detail here.

lgtm with style nits.

I added a question about the trusted intent extras. Might be worth inspecting a
bit.

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:612:
} else if ((webContents = takeAsyncWebContents()) != null) {
I know it will make you have a slightly worse nesting here, but lets push these
assignments up to their own lines. I dont think our java styling encourages the
if checks here (which is kind of unfortunate in this situation)

so:

else {
    webContents = takeAsyncWebContents();
    if (webcontents != null) {
        webContentsStateOnLaunch = WEBCONTENTS_STATE_TRANSFERRED_WEBCONTENTS;
    } else {....

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:641:
if (asyncParams == null) {
one line

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
(right):

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java:40:
IntentHandler.addTrustedIntentExtras(customTabIntent.intent);
this is conditioned on
ExternalNavigationDelegateImpl.willChromeHandleIntent(intent, true). and based
on that condition it does set the package name too. So if you are still needing
to set package separately that tells me the condition above fails for some
reason.

If things are working for you, feel free to just remove this line actually (as I
think in the current form it is a no-op based on your findings). But it does
make me pause a bit that willChromeHandleIntent return false here. I leave that
to you to dig deeper or not.

[piotrs, 2017-06-15 23:28:21]

Thanks for comments, done as suggested!

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:612:
} else if ((webContents = takeAsyncWebContents()) != null) {
On 2017/06/15 16:15:56, Yusuf wrote:
> I know it will make you have a slightly worse nesting here, but lets push
these
> assignments up to their own lines. I dont think our java styling encourages
the
> if checks here (which is kind of unfortunate in this situation)
> 
> so:
> 
> else {
>     webContents = takeAsyncWebContents();
>     if (webcontents != null) {
>         webContentsStateOnLaunch = WEBCONTENTS_STATE_TRANSFERRED_WEBCONTENTS;
>     } else {....

Done.

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:641:
if (asyncParams == null) {
On 2017/06/15 16:15:56, Yusuf wrote:
> one line

Done.

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java
(right):

https://codereview.chromium.org/2898373002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java:40:
IntentHandler.addTrustedIntentExtras(customTabIntent.intent);
It doesn't resolve to a single Chrome instance, as (on my test device) there are
multiple options to handle CCT (Chrome, Chromium, Clankium). I think this is
WAI, but I want to pin this intent to particular implementation for the reasons
mentioned earlier.

[piotrs, 2017-06-15 23:30:08]

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

[piotrs, 2017-06-15 23:30:13]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-06-15 23:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/2898373002/#ps200001
(title: "Merge")

[commit-bot: I haz the power, 2017-06-15 23:30:36]

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-06-15 23:36:35]

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

[commit-bot: I haz the power, 2017-06-15 23:36:37]

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...)

[piotrs, 2017-06-16 01:52:26]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-06-16 01:52:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, yusufo@chromium.org
Link to the patchset: https://codereview.chromium.org/2898373002/#ps220001
(title: "Merge")

[commit-bot: I haz the power, 2017-06-16 01:52:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piotrs, 2017-06-16 01:52:50]

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

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

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-06-16 03:26:16]

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

[commit-bot: I haz the power, 2017-06-16 03:26:17]

Dry run: This issue passed the CQ dry run.

[piotrs, 2017-06-16 03:30:46]

The CQ bit was checked by piotrs@chromium.org

[commit-bot: I haz the power, 2017-06-16 03:30:57]

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-06-16 03:40:49]

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

[commit-bot: I haz the power, 2017-06-16 03:40:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[piotrs, 2017-06-16 03:48:39]

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

[piotrs, 2017-06-16 03:48:40]

Hi Dan,

Could you please take a look on TabDelegate change?

Cheers!
Piotr

[gone, 2017-06-16 16:26:45]

TabDelegate lgtm

[piotrs, 2017-06-16 23:24:52]

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

[piotrs, 2017-06-16 23:25:15]

The CQ bit was unchecked by piotrs@chromium.org

[piotrs, 2017-06-16 23:25:16]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-06-16 23:25:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, yusufo@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2898373002/#ps240001
(title: "Merge")

[commit-bot: I haz the power, 2017-06-16 23:25:23]

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-06-17 00:14:08]

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

[commit-bot: I haz the power, 2017-06-17 00:14:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[piotrs, 2017-06-17 05:57:15]

The CQ bit was checked by piotrs@chromium.org

[commit-bot: I haz the power, 2017-06-17 05:57:24]

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-06-17 09:47:33]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497679035405100,
"parent_rev": "10f46a5339ead7c1ffcc83d963f5cc7d6244509a", "commit_rev":
"a73ffaec6287c62e09787f82cbc03fe519e0f55b"}

[commit-bot: I haz the power, 2017-06-17 09:47:49]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497679035405100,
"parent_rev": "c614d37c43ee55267cdf9c758f910cc54ce346b9", "commit_rev":
"e4441b13fabcf4cc265bfc1e453cee4a5b7c595a"}

[commit-bot: I haz the power, 2017-06-17 09:48:04]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1497679035405100,
"parent_rev": "9c5eb06cea5f0916c1859fb899842ecdbe2d657b", "commit_rev":
"4d9776c156c177107053872a437a9639d95a7160"}

[commit-bot: I haz the power, 2017-06-17 09:48:18]

Description was changed from

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889
==========

to

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889

Review-Url: https://codereview.chromium.org/2898373002
Cr-Commit-Position: refs/heads/master@{#480286}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d9776c156c177107053872a437a...
==========

[commit-bot: I haz the power, 2017-06-17 09:48:22]

Committed patchset #9 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/4d9776c156c177107053872a437a...

[johnme, 2017-06-19 10:42:01]

Sorry, reverting this in https://codereview.chromium.org/2950483002 because of
failing tests.

[piotrs, 2017-06-19 20:42:39]

Description was changed from

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889

Review-Url: https://codereview.chromium.org/2898373002
Cr-Commit-Position: refs/heads/master@{#480286}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d9776c156c177107053872a437a...
==========

to

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889

Review-Url: https://codereview.chromium.org/2898373002
Cr-Commit-Position: refs/heads/master@{#480286}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d9776c156c177107053872a437a...
==========

[piotrs, 2017-06-19 20:58:38]

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

[commit-bot: I haz the power, 2017-06-19 20:59:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piotrs, 2017-06-19 21:01:12]

My testing of number of Chrome tasks in recents turned out to not be
deterministic on a few bots. Removing that for now, so tests will not check
whether new Android task is created. Will look for a better method to test this
in a follow up.

[piotrs, 2017-06-19 21:47:49]

The CQ bit was unchecked by piotrs@chromium.org

[piotrs, 2017-06-19 21:47:50]

The CQ bit was checked by piotrs@chromium.org

[piotrs, 2017-06-19 21:47:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, yusufo@chromium.org, dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/2898373002/#ps280001
(title: "Merge")

[commit-bot: I haz the power, 2017-06-19 21:48:31]

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-06-19 22:48:12]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1497908870653590,
"parent_rev": "2b7d556460d750b9d3d8f7968982bc0be2d24cd2", "commit_rev":
"e1753b5ae0c9972faf0da0b2138fb9b92c3db2e8"}

[commit-bot: I haz the power, 2017-06-19 22:48:23]

Description was changed from

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889

Review-Url: https://codereview.chromium.org/2898373002
Cr-Commit-Position: refs/heads/master@{#480286}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d9776c156c177107053872a437a...
==========

to

==========
Redirects _blank and window.open() off-origin navigation from PWA to CCT.

Custom Tab opened from a _blank link or a window.open() call is opened in
a separate Android task to mimic a new window/blank context of a web browser.
This is done in order to improve UX and bring more app-like feel to PWAs.
Transition to CCT for regular navigation is already done
(codereview.chromium.org/2829943002), this patch brings the same to _blank
and window.open().

This requires some changes to Custom Tabs, as WebContents for the new window
is already created when we get to Java layer. I replicated the trick from
ChromeTabCreator with remembering WebContents on AsyncTabParamsManager
during the transition.

Tests asserting that _blank and window.open() are opened in TabbedChrome has
been removed from WebappModeTest. Tests for transition to CCT has been added
to WebappNavigationTest.

On Lollipop and above we maintain at most one CCT task per installed Webapp.
Below Lollipop there is no API I could use to enforce that I think, let me
know if you know the way.

BUG=709889

Review-Url: https://codereview.chromium.org/2898373002
Cr-Original-Commit-Position: refs/heads/master@{#480286}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d9776c156c177107053872a437a...
Review-Url: https://codereview.chromium.org/2898373002
Cr-Commit-Position: refs/heads/master@{#480612}
Committed:
https://chromium.googlesource.com/chromium/src/+/e1753b5ae0c9972faf0da0b2138f...
==========

[commit-bot: I haz the power, 2017-06-19 22:48:26]

Committed patchset #11 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/e1753b5ae0c9972faf0da0b2138f...