[Android WebView] AwContentsClient.shouldCreateWindow callback part 1.

[Android WebView] AwContentsClient.shouldCreateWindow callback part 1.

Provide the plumbing from WebContentsDelegate out to AwContentsClient
to allow the embedder to respond to requests to create pop up windows.
For now we only support the embedder blocking the pop up window; code
to support showing the window to follow in a later patch.

Note that the AwResourceDispatcherHostDelegate is refactored to
support lazily checking the AwContentsIoThreadClient, as we may
receive resource requests for the popup window before the Java
side has been entirely hooked up.

Android bots happy.
NOTRY=true

Note: part 2 is at https://chromiumcodereview.appspot.com/11348075/

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167933

[benm (inactive), 2012-11-13 16:59:46]

d'oh, forgot to add you on this on Friday joth. PTAL!

[mkosiba (inactive), 2012-11-13 17:25:10]

lgtm

https://codereview.chromium.org/11362183/diff/2012/android_webview/browser/re...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/11362183/diff/2012/android_webview/browser/re...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:62:
if (!GetIoThreadClient() || ShouldCancel()) {
I'd prefer to assert that an IoThreadClient exists at this point (otherwise
you're masking potential race conditions) especially that the subsequent change
will make us block the first request until the client is set.

[benm (inactive), 2012-11-13 17:34:10]

http://codereview.chromium.org/11362183/diff/2012/android_webview/browser/ren...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/11362183/diff/2012/android_webview/browser/ren...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:62:
if (!GetIoThreadClient() || ShouldCancel()) {
I changed it to this as even with the "false path", we will request the initial
resource for the popup. Leaving it as a dcheck means I'd be checking in crashing
code. I suppose you could argue that adding the throttle to defer the load until
the Client is ready could come as part of this patch rather than the next one to
avoid the crash...

[joth, 2012-11-14 18:34:52]

https://codereview.chromium.org/11362183/diff/13/android_webview/browser/aw_c...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11362183/diff/13/android_webview/browser/aw_c...
android_webview/browser/aw_content_browser_client.cc:243: //
AwContentsClient.shouldCreateWindow callback).
I think Bo's TODO still stands: we want to set this based on
WebSettings.SupportMultipleWindow

https://codereview.chromium.org/11362183/diff/13/android_webview/browser/rend...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/11362183/diff/13/android_webview/browser/rend...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:154:
child_id, route_id, request));
I think it could be even simpler to just have one throttle that we pass the URL
into on creation, and then move all the logic above into its WillStartRequest()
method. (i.e. look up the scheme and make the Java-side call all there in one
place).

also need a comment here that we can't yet call through to the IO client as in
some cases (e.g. popup window creation) it wouldn't yet have been set.

https://codereview.chromium.org/11362183/diff/13/android_webview/native/aw_we...
File android_webview/native/aw_web_contents_delegate.cc (right):

https://codereview.chromium.org/11362183/diff/13/android_webview/native/aw_we...
android_webview/native/aw_web_contents_delegate.cc:69: void
AwWebContentsDelegate::AddNewContents(content::WebContents* source,
this method is spurious: WebContentsDelegateAndroid::AddNewContents already does
this, so we just need to override the method java side.

https://codereview.chromium.org/11362183/diff/13/android_webview/native/aw_we...
android_webview/native/aw_web_contents_delegate.cc:70: content::WebContents*
new_contents,
wonky indent

https://codereview.chromium.org/11362183/diff/13/android_webview/native/aw_we...
android_webview/native/aw_web_contents_delegate.cc:80: bool is_dialog = false;
I'd just say is_dialog = (disposition == NEW_POPUP)

This definition seems near enough:-

WebNavigationPolicy ChromeClientImpl::getNavigationPolicy()
{
    // If our default configuration was modified by a script or wasn't
    // created by a user gesture, then show as a popup. Else, let this
    // new window be opened as a toplevel window.
    bool asPopup = !m_toolbarsVisible
        || !m_statusbarVisible
        || !m_scrollbarsVisible
        || !m_menubarVisible
        || !m_resizable;

[joth, 2012-11-14 18:40:03]

https://codereview.chromium.org/11362183/diff/8001/android_webview/browser/aw...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/11362183/diff/8001/android_webview/browser/aw...
android_webview/browser/aw_content_browser_client.cc:243: //
AwContentsClient.onCreateWindow callback).
(maybe comment that WebKit already handles the WebSetting multiple windows bool)

[benm (inactive), 2012-11-14 18:42:34]

thanks joth!

http://codereview.chromium.org/11362183/diff/13/android_webview/browser/aw_co...
File android_webview/browser/aw_content_browser_client.cc (right):

http://codereview.chromium.org/11362183/diff/13/android_webview/browser/aw_co...
android_webview/browser/aw_content_browser_client.cc:243: //
AwContentsClient.shouldCreateWindow callback).
Will update the TODO with a comment, we won't actually get to this point if the
setting is such that we block the popup (it's checked in webkit).

http://codereview.chromium.org/11362183/diff/13/android_webview/browser/rende...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/11362183/diff/13/android_webview/browser/rende...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:154:
child_id, route_id, request));
makes sense. will do.

http://codereview.chromium.org/11362183/diff/13/android_webview/native/aw_web...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/11362183/diff/13/android_webview/native/aw_web...
android_webview/native/aw_web_contents_delegate.cc:70: content::WebContents*
new_contents,
On 2012/11/14 18:34:52, joth wrote:
> wonky indent

Done.

http://codereview.chromium.org/11362183/diff/13/android_webview/native/aw_web...
android_webview/native/aw_web_contents_delegate.cc:80: bool is_dialog = false;
On 2012/11/14 18:34:52, joth wrote:
> I'd just say is_dialog = (disposition == NEW_POPUP)
> 
> This definition seems near enough:-
> 
> WebNavigationPolicy ChromeClientImpl::getNavigationPolicy()
> {
>     // If our default configuration was modified by a script or wasn't
>     // created by a user gesture, then show as a popup. Else, let this
>     // new window be opened as a toplevel window.
>     bool asPopup = !m_toolbarsVisible
>         || !m_statusbarVisible
>         || !m_scrollbarsVisible
>         || !m_menubarVisible
>         || !m_resizable;

Done.

[joth, 2012-11-15 14:54:31]

lgtm

[commit-bot: I haz the power, 2012-11-15 16:39:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/11362183/14001

[commit-bot: I haz the power, 2012-11-15 16:39:50]

Change committed as 167933