Allow asynchronous tab creation when opener_suppressed is true.

Allow asynchronous tab creation when opener_suppressed is true.

On Android, when merge tabs and apps is enabled, we need to ensure a new
activity has been started before loading URL in web contents since the
web contents delegate is not ready until then.

We already have a code path to handle this for window.open() scenario.
This completes the code path for when a link with noreferrer is clicked
by saving OpenURLParams of the pending load and resuming the load when
the activity is ready.

BUG=508366
Committed: https://crrev.com/a4971c13e156194f1b51d3b019806bbf64cfa8f6
Cr-Commit-Position: refs/heads/master@{#339711}

[Maria, 2015-07-10 00:35:29]

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

[Maria, 2015-07-10 00:43:10]

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

[commit-bot: I haz the power, 2015-07-10 00:43:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233453003/1

[Maria, 2015-07-10 01:45:59]

mariakhomenko@chromium.org changed reviewers:
+ jam@chromium.org

[commit-bot: I haz the power, 2015-07-10 02:28:58]

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

[commit-bot: I haz the power, 2015-07-10 02:28:59]

Dry run: This issue passed the CQ dry run.

[nasko, 2015-07-10 07:42:20]

nasko@chromium.org changed reviewers:
+ nasko@chromium.org

[nasko, 2015-07-10 07:42:20]

https://codereview.chromium.org/1233453003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1233453003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2683: return;
Why is there a return here? Skipping the initialization of RVH and RFH doesn't
seem like a good idea. It will very likely break window.open() cases if this
delayed path is hit.

[Maria, 2015-07-10 16:26:26]

https://codereview.chromium.org/1233453003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1233453003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2683: return;
On 2015/07/10 07:42:20, nasko (paris) wrote:
> Why is there a return here? Skipping the initialization of RVH and RFH doesn't
> seem like a good idea. It will very likely break window.open() cases if this
> delayed path is hit.

This function is not called in the initialization path for new window load with
opener_suppressed usually. Usually we synchronously start OpenURL on web
contents in that path. However, on Android we have to finish setting up our
tab/web contents delegate asynchronously before we start the page load. 

We already have code that calls into ResumeLoadingCreatedWebContents() from Java
when such setup is completed (for window.open() cases). I added this here
because we would use the same signal to complete the load in opener_suppressed
case, but returning early because I believe we wouldn't have usually initialized
RVH/RFH through here.

[jam, 2015-07-10 21:45:34]

lgtm

[Maria, 2015-07-10 22:17:57]

On 2015/07/10 21:45:34, jam wrote:
> lgtm

I just wanted to provide a bit more context on why I think this fix works. There
are two code paths with a window launched from another window, one happens if
opener_suppressed is set to true and the other one if opener_suppressed set to
false (the former is generally link clicks with rel=noreferrer and the latter
are regular links and window.open() calls). 

In the opener_suppressed=false code path we get CreateNewWindow() call, we add
the newly created web_contents to pending_contents_ map. Then when
ShowCreatedWindow() is called, we find the web_contents stashed in
pending_contents_ map and call delegate->AddNewContents(). There are two
possible cases in that code, one is synchronous path which desktop uses, where
ResumeLoadingCreatedWebContents() gets called inline in that method and the
other one is asynchronous path that android MAY use which keeps the contents
paused until we finished java-side activity initialization, in which case
ResumeLoadingCreatedWebContents() will get called later via java code.

Now, turns out opener_suppressed=true code path wasn't really working on Android
with asynchronous activity creation because in that code path CreateNewWindow
doesn't add anything to pending_contents_ map, but rather immediately calls
delegate->AddNewContents() and then OpenURL() on the created web contents. This
means that code in ShowCreatedWindow() is not executed (because that code only
runs when pending_contents_ map contains a web contents) and hence
ResumeLoadingCreatedWebContents() code is never called by any C++ path. Now,
with my fix, we will delay calling OpenURL() in the CreateNewWindow function and
instead wait for a java signal that asynchronous initialization is done on the
java side (ResumeLoadingCreatedWebContents() call). Since we are trying to match
the C++ behaviour overall, I believe all we need to do is call OpenURL and the
RVH initialization is not required.

[chromium-reviews, 2015-07-15 05:46:58]

Nasko, does this sound ok to you?

On Fri, Jul 10, 2015 at 3:17 PM <mariakhomenko@chromium.org> wrote:

> On 2015/07/10 21:45:34, jam wrote:
> > lgtm
>
> I just wanted to provide a bit more context on why I think this fix works.
> There
> are two code paths with a window launched from another window, one happens
> if
> opener_suppressed is set to true and the other one if opener_suppressed set
> to
> false (the former is generally link clicks with rel=noreferrer and the
> latter
> are regular links and window.open() calls).
>
> In the opener_suppressed=false code path we get CreateNewWindow() call, we
> add
> the newly created web_contents to pending_contents_ map. Then when
> ShowCreatedWindow() is called, we find the web_contents stashed in
> pending_contents_ map and call delegate->AddNewContents(). There are two
> possible cases in that code, one is synchronous path which desktop uses,
> where
> ResumeLoadingCreatedWebContents() gets called inline in that method and the
> other one is asynchronous path that android MAY use which keeps the
> contents
> paused until we finished java-side activity initialization, in which case
> ResumeLoadingCreatedWebContents() will get called later via java code.
>
> Now, turns out opener_suppressed=true code path wasn't really working on
> Android
> with asynchronous activity creation because in that code path
> CreateNewWindow
> doesn't add anything to pending_contents_ map, but rather immediately calls
> delegate->AddNewContents() and then OpenURL() on the created web contents.
> This
> means that code in ShowCreatedWindow() is not executed (because that code
> only
> runs when pending_contents_ map contains a web contents) and hence
> ResumeLoadingCreatedWebContents() code is never called by any C++ path.
> Now,
> with my fix, we will delay calling OpenURL() in the CreateNewWindow
> function and
> instead wait for a java signal that asynchronous initialization is done on
> the
> java side (ResumeLoadingCreatedWebContents() call). Since we are trying to
> match
> the C++ behaviour overall, I believe all we need to do is call OpenURL and
> the
> RVH initialization is not required.
>
> https://codereview.chromium.org/1233453003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2015-07-16 08:11:58]

On 2015/07/15 05:46:58, chromium-reviews wrote:
> Nasko, does this sound ok to you?
> 
> On Fri, Jul 10, 2015 at 3:17 PM <mailto:mariakhomenko@chromium.org> wrote:
> 
> > On 2015/07/10 21:45:34, jam wrote:
> > > lgtm
> >
> > I just wanted to provide a bit more context on why I think this fix works.
> > There
> > are two code paths with a window launched from another window, one happens
> > if
> > opener_suppressed is set to true and the other one if opener_suppressed set
> > to
> > false (the former is generally link clicks with rel=noreferrer and the
> > latter
> > are regular links and window.open() calls).
> >
> > In the opener_suppressed=false code path we get CreateNewWindow() call, we
> > add
> > the newly created web_contents to pending_contents_ map. Then when
> > ShowCreatedWindow() is called, we find the web_contents stashed in
> > pending_contents_ map and call delegate->AddNewContents(). There are two
> > possible cases in that code, one is synchronous path which desktop uses,
> > where
> > ResumeLoadingCreatedWebContents() gets called inline in that method and the
> > other one is asynchronous path that android MAY use which keeps the
> > contents
> > paused until we finished java-side activity initialization, in which case
> > ResumeLoadingCreatedWebContents() will get called later via java code.
> >
> > Now, turns out opener_suppressed=true code path wasn't really working on
> > Android
> > with asynchronous activity creation because in that code path
> > CreateNewWindow
> > doesn't add anything to pending_contents_ map, but rather immediately calls
> > delegate->AddNewContents() and then OpenURL() on the created web contents.
> > This
> > means that code in ShowCreatedWindow() is not executed (because that code
> > only
> > runs when pending_contents_ map contains a web contents) and hence
> > ResumeLoadingCreatedWebContents() code is never called by any C++ path.
> > Now,
> > with my fix, we will delay calling OpenURL() in the CreateNewWindow
> > function and
> > instead wait for a java signal that asynchronous initialization is done on
> > the
> > java side (ResumeLoadingCreatedWebContents() call). Since we are trying to
> > match
> > the C++ behaviour overall, I believe all we need to do is call OpenURL and
> > the
> > RVH initialization is not required.
> >
> > https://codereview.chromium.org/1233453003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sorry for the delay, I was out of office for a few days. LGTM

[Maria, 2015-07-21 18:12:03]

The CQ bit was checked by mariakhomenko@chromium.org

[commit-bot: I haz the power, 2015-07-21 18:13:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233453003/1

[commit-bot: I haz the power, 2015-07-21 19:04:50]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-07-21 19:05:39]

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