Defer loads on WebView popup windows until webcontents delegate attached

Defer loads on WebView popup windows until webcontents delegate attached

In WebView we initalize popup windows asynchronously so loading a url
synchronously can result in it being loaded before the WebView
sets its webcontentsdelegate.

BUG=548316, 542548
Committed: https://crrev.com/f9205b6fc8434ff6e6ee1a66fa682668d01f9a64
Cr-Commit-Position: refs/heads/master@{#357127}

[gsennton, 2015-10-30 15:02:43]

gsennton@chromium.org changed reviewers:
+ boliu@chromium.org

[gsennton, 2015-10-30 15:02:43]

https://codereview.chromium.org/1405333005/diff/20001/android_webview/native/...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/1405333005/diff/20001/android_webview/native/...
android_webview/native/aw_contents.cc:251:
web_contents_->ForEachFrame(base::Bind(&OnIoThreadClientReady));
Not sure whether ResumeLoadingCreatedWebContents should be called before or
after OnIoThreadClientReady...

[boliu, 2015-10-30 15:27:15]

oh nice, didn't know it was this easy, but let's sort through the failures first

[gsennton, 2015-10-30 16:06:38]

Yippie, ptal :)

[boliu, 2015-10-30 16:17:13]

Wait.. we don't set the WebContentsDelegate until SetJavaPeers. So how does the
delegate prevent the load on the pop up in that case?

https://codereview.chromium.org/1405333005/diff/40001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1405333005/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1040:
nativeResumeLoadingCreatedPopupWebContents(popupNativeAwContents);
nit: you can use mNativeAwContents here now

[gsennton, 2015-10-30 16:27:22]

If I understand this correctly it is the delegate of the original webcontents
that determines whether we should defer the request for the popup window:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

https://codereview.chromium.org/1405333005/diff/40001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1405333005/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:1040:
nativeResumeLoadingCreatedPopupWebContents(popupNativeAwContents);
On 2015/10/30 16:17:13, boliu wrote:
> nit: you can use mNativeAwContents here now

Done.

[boliu, 2015-10-30 16:29:25]

On 2015/10/30 16:27:22, gsennton wrote:
> If I understand this correctly it is the delegate of the original webcontents
> that determines whether we should defer the request for the popup window:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

Ahh, good point. lgtm

Also add 542548 to bugs list, since this fixes that one as well, right?

[gsennton, 2015-10-30 16:31:26]

Description was changed from

==========
Defer loads on WebView popup windows until webcontents delegate attached

In WebView we initalize popup windows asynchronously so loading a url
synchronously can result in it being loaded before the WebView
sets its webcontentsdelegate.

BUG=548316
==========

to

==========
Defer loads on WebView popup windows until webcontents delegate attached

In WebView we initalize popup windows asynchronously so loading a url
synchronously can result in it being loaded before the WebView
sets its webcontentsdelegate.

BUG=548316, 542548
==========

[gsennton, 2015-10-30 16:32:53]

On 2015/10/30 16:29:25, boliu wrote:
> On 2015/10/30 16:27:22, gsennton wrote:
> > If I understand this correctly it is the delegate of the original
webcontents
> > that determines whether we should defer the request for the popup window:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
> 
> Ahh, good point. lgtm
> 
> Also add 542548 to bugs list, since this fixes that one as well, right?

Yup

[gsennton, 2015-10-30 16:32:57]

The CQ bit was checked by gsennton@chromium.org

[commit-bot: I haz the power, 2015-10-30 16:34:47]

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

[commit-bot: I haz the power, 2015-10-30 16:57:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-30 16:58:20]

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

[sgurun-gerrit only, 2015-10-30 17:52:30]

On 2015/10/30 16:58:20, commit-bot: I haz the power wrote:
> Patchset 4 (id:??) landed as
> https://crrev.com/f9205b6fc8434ff6e6ee1a66fa682668d01f9a64
> Cr-Commit-Position: refs/heads/master@{#357127}

two bugs fixed with one simple change! bravo!