Upstream ShouldOverrideUrlLoading changes

Rather than using a throttle, we are now using 
HandleNavigation callback for implementing ShouldOverrideUrlLoading for compatibility with old Webview.

BUG=308257
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239646

[mkosiba (inactive), 2013-09-24 14:04:12]

ok, looks like AwContentsClientShouldOverrideUrlLoadingTest#testCanIgnoreLoading
assumes an onPageFinished gets called after we return false from
shouldOverrideUrlLoading. That probably isn't the case in the Classic WebView so
you might want to correct the error.

AwContentsClientShouldInterceptRequestTest is broken because we get two
onPageStarted callbacks (says we expected 1 but got 2). That seems broken.. 

If you can't find an obvious fix maybe the safest approach would be to replace
the InterceptNavigationResourceThrottle with a throttle that doesn't defer the
requests but posts an onPageStarted to the UI thread?

https://codereview.chromium.org/24228003/diff/1/android_webview/renderer/aw_c...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/1/android_webview/renderer/aw_c...
android_webview/renderer/aw_content_renderer_client.cc:78: if (frame->parent())
this is not true - subframes for non-http(s) navigations should be considered as
well.

[sgurun-gerrit only, 2013-09-24 15:14:34]

On 2013/09/24 14:04:12, mkosiba wrote:
> ok, looks like
AwContentsClientShouldOverrideUrlLoadingTest#testCanIgnoreLoading
> assumes an onPageFinished gets called after we return false from
> shouldOverrideUrlLoading. That probably isn't the case in the Classic WebView
so
> you might want to correct the error.
> 
> AwContentsClientShouldInterceptRequestTest is broken because we get two
> onPageStarted callbacks (says we expected 1 but got 2). That seems broken.. 

After some logging I see that we are getting it twice, one for the navigate
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
and for OnDidStartLoading:

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




> 
> If you can't find an obvious fix maybe the safest approach would be to replace
> the InterceptNavigationResourceThrottle with a throttle that doesn't defer the
> requests but posts an onPageStarted to the UI thread?
> 
>
https://codereview.chromium.org/24228003/diff/1/android_webview/renderer/aw_c...
> File android_webview/renderer/aw_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/24228003/diff/1/android_webview/renderer/aw_c...
> android_webview/renderer/aw_content_renderer_client.cc:78: if
(frame->parent())
> this is not true - subframes for non-http(s) navigations should be considered
as
> well.
oh, well. ok will investigate.

[mkosiba (inactive), 2013-09-25 22:23:26]

https://codereview.chromium.org/24228003/diff/23001/android_webview/renderer/...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/23001/android_webview/renderer/...
android_webview/renderer/aw_content_renderer_client.cc:70:
!document_state->navigation_state()->is_content_initiated()
is_content_initiated would normally be false for cross-origin navigations, but
since android_webview doesn't swap out renderers this works fine.
I strongly recommend adding a TODO explaining that if we ever want to swap out
renderers on any
navigations this will stop working.

[sgurun-gerrit only, 2013-10-09 00:26:13]

only one test is failing, but it is a test issue.

https://codereview.chromium.org/24228003/diff/1/android_webview/renderer/aw_c...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/1/android_webview/renderer/aw_c...
android_webview/renderer/aw_content_renderer_client.cc:78: if (frame->parent())
On 2013/09/24 14:04:12, mkosiba wrote:
> this is not true - subframes for non-http(s) navigations should be considered
as
> well.

Done.

https://codereview.chromium.org/24228003/diff/23001/android_webview/renderer/...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/23001/android_webview/renderer/...
android_webview/renderer/aw_content_renderer_client.cc:70:
!document_state->navigation_state()->is_content_initiated()
On 2013/09/25 22:23:26, mkosiba wrote:
> is_content_initiated would normally be false for cross-origin navigations, but
> since android_webview doesn't swap out renderers this works fine.
> I strongly recommend adding a TODO explaining that if we ever want to swap out
> renderers on any
> navigations this will stop working.

Done.

[joth, 2013-10-09 02:22:40]

https://codereview.chromium.org/24228003/diff/54001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/24228003/diff/54001/android_webview/native/aw...
android_webview/native/aw_contents.cc:317: if
(base::subtle::NoBarrier_Load(&g_instance_count) == 0) {
now the delete is delayed, this must be moved to the d'tor, after the decrement
actually happened.

[sgurun-gerrit only, 2013-10-16 21:49:27]

https://codereview.chromium.org/24228003/diff/54001/android_webview/native/aw...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/24228003/diff/54001/android_webview/native/aw...
android_webview/native/aw_contents.cc:317: if
(base::subtle::NoBarrier_Load(&g_instance_count) == 0) {
On 2013/10/09 02:22:40, joth wrote:
> now the delete is delayed, this must be moved to the d'tor, after the
decrement
> actually happened.

Done.

[mkosiba (inactive), 2013-10-17 10:27:44]

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.cc:40: // TODO(sgurun) move
this to its own file.
any reason not to do this now?

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
File android_webview/browser/aw_contents_client_bridge_base.h (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
android_webview/browser/aw_contents_client_bridge_base.h:53: = 0;
uber-nit: please add a newline here.

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/r...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:226:
// We allow intercepting only navigations within main frames. This
This is misleading. We're not really using the throttle for intercepting
anymore. Please rephrase to say "We're only using the
InterceptNavigationDelegate to send an onPageStarted callback, which is only
issued for navigations within main frames. The navigation
interception/shouldOverrideUrlLoading implementation is now handled using
synchronous IPC."

https://codereview.chromium.org/24228003/diff/64001/android_webview/common/re...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/common/re...
android_webview/common/render_view_messages.h:103: // Sent when there is a top
level navigation. Returning true means
The description is a bit vague. Maybe:

"Sent immediately before the main frame (top level) navigation is initiated
within Blink to allow the browser process to cancel that navigation. This is
sent before updating the NavigationController state or creating a URLRequest for
the main frame resource."

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:300: //
The existing contract is that shouldOverrideUrlLoading callbacks are delivered
before
This comment is out of date

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:313:
mContentsClient.getCallbackHelper().postOnPageStarted(url);
per comment above the post is no longer needed. just call the method directly
and remove the postOnPageStarted from the CallbackHelper

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:67:
ThreadUtils.postOnUiThread(new Runnable() {
I believe there is no reason to do the post here. Please call the method
directly.

[mkosiba (inactive), 2013-10-17 10:29:28]

https://codereview.chromium.org/24228003/diff/64001/android_webview/renderer/...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/renderer/...
android_webview/renderer/aw_content_renderer_client.cc:92: // TODO(sgurun) Need
to write a test for allowing about:blank for top
that shouldn't be a lot of work to add to the
AwContentsClientShouldOverrideUrlLoadingTest suite. Please add it as part of the
CL.

[sgurun-gerrit only, 2013-12-06 00:17:47]

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
android_webview/browser/aw_content_browser_client.cc:40: // TODO(sgurun) move
this to its own file.
Given the very temporary nature of these changes, and the fact that you already
have a CL, this will probably be unnecessary.

On 2013/10/17 10:27:45, mkosiba wrote:
> any reason not to do this now?

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
File android_webview/browser/aw_contents_client_bridge_base.h (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/a...
android_webview/browser/aw_contents_client_bridge_base.h:53: = 0;
On 2013/10/17 10:27:45, mkosiba wrote:
> uber-nit: please add a newline here.

Done.

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/r...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/browser/r...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:226:
// We allow intercepting only navigations within main frames. This
On 2013/10/17 10:27:45, mkosiba wrote:
> This is misleading. We're not really using the throttle for intercepting
> anymore. Please rephrase to say "We're only using the
> InterceptNavigationDelegate to send an onPageStarted callback, which is only
> issued for navigations within main frames. The navigation
> interception/shouldOverrideUrlLoading implementation is now handled using
> synchronous IPC."

Done.

https://codereview.chromium.org/24228003/diff/64001/android_webview/common/re...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/common/re...
android_webview/common/render_view_messages.h:103: // Sent when there is a top
level navigation. Returning true means
On 2013/10/17 10:27:45, mkosiba wrote:
> The description is a bit vague. Maybe:
> 
> "Sent immediately before the main frame (top level) navigation is initiated
> within Blink to allow the browser process to cancel that navigation. This is
> sent before updating the NavigationController state or creating a URLRequest
for
> the main frame resource."

Done.

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:300: //
The existing contract is that shouldOverrideUrlLoading callbacks are delivered
before
On 2013/10/17 10:27:45, mkosiba wrote:
> This comment is out of date

Done.

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContents.java:313:
mContentsClient.getCallbackHelper().postOnPageStarted(url);
We now (again) call shouldoverrideurlloading here due to popup case so leaving
the way it is.
On 2013/10/17 10:27:45, mkosiba wrote:
> per comment above the post is no longer needed. just call the method directly
> and remove the postOnPageStarted from the CallbackHelper

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
File android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
(right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/java/src/...
android_webview/java/src/org/chromium/android_webview/AwContentsClient.java:67:
ThreadUtils.postOnUiThread(new Runnable() {
this comment is stale now.
On 2013/10/17 10:27:45, mkosiba wrote:
> I believe there is no reason to do the post here. Please call the method
> directly.

https://codereview.chromium.org/24228003/diff/64001/android_webview/renderer/...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/64001/android_webview/renderer/...
android_webview/renderer/aw_content_renderer_client.cc:92: // TODO(sgurun) Need
to write a test for allowing about:blank for top
On 2013/10/17 10:29:28, mkosiba wrote:
> that shouldn't be a lot of work to add to the
> AwContentsClientShouldOverrideUrlLoadingTest suite. Please add it as part of
the
> CL.

yep a test will be valuable.

[sgurun-gerrit only, 2013-12-06 02:02:19]

PTAL, thanks

[boliu, 2013-12-06 20:28:23]

Only the opener_id question is substantial.

Others are mostly nits, request for adding tests or clean ups. I'm ok with doing
these in follow up patches.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:46:
AwContentsMessageFilter(int process_id);
need explicit keyword for 1-arg constructor

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:48: // BrowserMessageFilter
methods
nit: period

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:91:
AwContentsMessageFilter::AwContentsMessageFilter(int process_id)
nit: why not keep implementation next to the class declaration?

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:119: bool*
ignore_navigation) {
DCHECK this is running on ui thread

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:124:
WillStartRequest(defer);
This can skip the if (io_client && io_client->PendingAssociation()) block in
WillStartRequest.

Also this runs on IO thread right? Can we add a DCHECK? Actually this whole file
seem to be lacking these.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:220:
child_id, route_id, request));
Trying to make sure requests of webviews that already have been destroyed will
go through uninterrupted:

This adds throttle, and in throttle, WillStartRequest should fall into calling
MaybeBlockRequest, which calls ShouldBlockRequest, which does not have an
io_client so just early outs and this whole thing does nothing. Same for
WillRedirectRequest. Correct?

So freaking need a unit test for this.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:222:
// We allow intercepting only navigations within main frames. This
How about this (assuming I understood the code correctly):

We add InterceptNavigationDelegate throttle for main frames only because the
throttle is only used to handle onPageStarted callback. shouldOverrideUrlLoading
is handled separately by a sync IPC.

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
android_webview/common/render_view_messages.h:103: // Sent immediately before
the main frame (top level) navigation is initiated
the main frame comment is not correct anymore

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
android_webview/common/render_view_messages.h:108: int /* view id */,
rouding_id?

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
android_webview/common/render_view_messages.h:109: string16 /* in - url */,
why not GURL?

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:198: //
This flag is to ShouldOverrideUrlNavigation through the resourcethrottle.
The var name is awesome, this comment, not so much...how about something like

Indicates shouldOverrideUrlLoading has not been called on the pending main frame
navigation of a pop up.

Also should really have a test for this path if there isn't already one.

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:293: //
Otherwise, use this delegate only to post onPageStarted messages. The
navigations are
Otherwise, this delegate is used to post onPageStarted messages only.

And I think the rest of the paragraph after this can be deleted.

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:297: // As
for posting the onPageStarted, we could have used WebContentsObserver. However
using
Not using WebContentsObserverAndroid.didStartLoading because...

I think that path can be deleted after this since we were the only ones using
it. Asap so no one else adds an dependency.

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
android_webview/native/aw_contents.cc:320: java_ref_.reset();
Why do we need to release the ref here? Doesn't hurt but seems inconsistent.

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
File android_webview/native/aw_web_contents_delegate.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:164: void
AwWebContentsDelegate::WebContentsCreated(
good one

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:33: using
content::RenderThread;
Either fix all content:: prefix to RenderThread, or just add it to this one new
instance.

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:102: if (opener_id !=
MSG_ROUTING_NONE) {
Doesn't this mean this will *never* send the message for a pop up webview? I
think opener_id_ doesn't change?

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.h (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.h:16: }
This should not be needed since they must be in content_renderer_client.h
already.

[sgurun-gerrit only, 2013-12-07 00:11:07]

thanks for your time and detailed code review! PTAL, thanks,

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
File android_webview/browser/aw_content_browser_client.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:46:
AwContentsMessageFilter(int process_id);
On 2013/12/06 20:28:23, boliu wrote:
> need explicit keyword for 1-arg constructor

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:48: // BrowserMessageFilter
methods
On 2013/12/06 20:28:23, boliu wrote:
> nit: period

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:91:
AwContentsMessageFilter::AwContentsMessageFilter(int process_id)
On 2013/12/06 20:28:23, boliu wrote:
> nit: why not keep implementation next to the class declaration?

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/aw_content_browser_client.cc:119: bool*
ignore_navigation) {
On 2013/12/06 20:28:23, boliu wrote:
> DCHECK this is running on ui thread

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:124:
WillStartRequest(defer);
On 2013/12/06 20:28:23, boliu wrote:
> This can skip the if (io_client && io_client->PendingAssociation()) block in
> WillStartRequest.
> 
Sorry, not sure I understand.
>
> Also this runs on IO thread right? Can we add a DCHECK? Actually this whole
file
> seem to be lacking these.
sure.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:220:
child_id, route_id, request));

On 2013/12/06 20:28:23, boliu wrote:
> Trying to make sure requests of webviews that already have been destroyed will
> go through uninterrupted:
> 
> This adds throttle, and in throttle, WillStartRequest should fall into calling
> MaybeBlockRequest, which calls ShouldBlockRequest, which does not have an
> io_client so just early outs and this whole thing does nothing. Same for
> WillRedirectRequest. Correct?
> 
> So freaking need a unit test for this.
Yes this logic is pretty delicate.
The CTS that I added (testShouldOverrideUrlLoadingOnCreateWindow) partially
tests this logic -- in fact this is how we discovered the raciness issue and
then ordered things this way.
But I agree on a unit test. created crbug.com/326676

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:222:
// We allow intercepting only navigations within main frames. This
On 2013/12/06 20:28:23, boliu wrote:
> How about this (assuming I understood the code correctly):
> 
> We add InterceptNavigationDelegate throttle for main frames only because the
> throttle is only used to handle onPageStarted callback.
shouldOverrideUrlLoading
> is handled separately by a sync IPC.

But it is not only used for onPageStarted anymore. For popup windows, the
shouldoverrideurlloading callback is posted from the throttle (reminder: in
popup case we were sending the shouldoverrideurlloading callback to wrong
AwContents)

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
android_webview/common/render_view_messages.h:103: // Sent immediately before
the main frame (top level) navigation is initiated
On 2013/12/06 20:28:23, boliu wrote:
> the main frame comment is not correct anymore

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
android_webview/common/render_view_messages.h:108: int /* view id */,
On 2013/12/06 20:28:23, boliu wrote:
> rouding_id?

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
android_webview/common/render_view_messages.h:109: string16 /* in - url */,
On 2013/12/06 20:28:23, boliu wrote:
> why not GURL?

Initially the intention was in shouldoverwriteurlloading non-GURL strings would
also work, or at least to have the infrastructure to have them work i.e. the
extreme json string case and others, hence the code. Since we will shortly throw
away the SYNC IPC code in favor of what Darin wants, I don't think we want to
change it to GURL now.

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:198: //
This flag is to ShouldOverrideUrlNavigation through the resourcethrottle.
On 2013/12/06 20:28:23, boliu wrote:
> The var name is awesome, this comment, not so much...how about something like
> 
The longest variable name I have ever written. you are right the comment does
not make sense.

> Indicates shouldOverrideUrlLoading has not been called on the pending main
frame
> navigation of a pop up.


> 
> Also should really have a test for this path if there isn't already one.

Have a CTS test for this path.

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:293: //
Otherwise, use this delegate only to post onPageStarted messages. The
navigations are
On 2013/12/06 20:28:23, boliu wrote:
> Otherwise, this delegate is used to post onPageStarted messages only.
> 
> And I think the rest of the paragraph after this can be deleted.

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:297: // As
for posting the onPageStarted, we could have used WebContentsObserver. However
using
On 2013/12/06 20:28:23, boliu wrote:
> Not using WebContentsObserverAndroid.didStartLoading because...
> 

Done.

> I think that path can be deleted after this since we were the only ones using
> it. Asap so no one else adds an dependency.
Let's take a look at this after this CL.

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
File android_webview/native/aw_web_contents_delegate.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:164: void
AwWebContentsDelegate::WebContentsCreated(
On 2013/12/06 20:28:23, boliu wrote:
> good one

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:33: using
content::RenderThread;
On 2013/12/06 20:28:23, boliu wrote:
> Either fix all content:: prefix to RenderThread, or just add it to this one
new
> instance.

Done.

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.cc:102: if (opener_id !=
MSG_ROUTING_NONE) {
On 2013/12/06 20:28:23, boliu wrote:
> Doesn't this mean this will *never* send the message for a pop up webview? I
> think opener_id_ doesn't change?
exactly. In popup case, it is sent using resourcethrottle.

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
File android_webview/renderer/aw_content_renderer_client.h (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/renderer...
android_webview/renderer/aw_content_renderer_client.h:16: }
On 2013/12/06 20:28:23, boliu wrote:
> This should not be needed since they must be in content_renderer_client.h
> already.

Done.

[boliu, 2013-12-07 01:22:18]

lgtm for upstreaming to match released code

the opener_id might be a bug that we shipped in kitkat, which needs to be fixed
before next release

other things are less urgent clean ups and adding tests

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:124:
WillStartRequest(defer);
On 2013/12/07 00:11:07, sgurun wrote:
> On 2013/12/06 20:28:23, boliu wrote:
> > This can skip the if (io_client && io_client->PendingAssociation()) block in
> > WillStartRequest.
> > 
> Sorry, not sure I understand.

I meant if (io_client && io_client->PendingAssociation()) returns true, then
there is no need to call AwResourceDispatcherHostDelegate::AddPendingThrottle
again, since WillStartRequest must have been called before WillRedirectRequest.
But it doesn't really hurt either.

https://codereview.chromium.org/24228003/diff/215001/android_webview/browser/...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:222:
// We allow intercepting only navigations within main frames. This
On 2013/12/07 00:11:07, sgurun wrote:
> On 2013/12/06 20:28:23, boliu wrote:
> > How about this (assuming I understood the code correctly):
> > 
> > We add InterceptNavigationDelegate throttle for main frames only because the
> > throttle is only used to handle onPageStarted callback.
> shouldOverrideUrlLoading
> > is handled separately by a sync IPC.
> 
> But it is not only used for onPageStarted anymore. For popup windows, the
> shouldoverrideurlloading callback is posted from the throttle (reminder: in
> popup case we were sending the shouldoverrideurlloading callback to wrong
> AwContents)

How about

We add InterceptNavigationDelegate throttle for main frames only because the
throttle is only used to handle onPageStarted callback and the
shouldOverrideUrlLoading callback of the initial main frame navigation of a pop
up.

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/common/r...
android_webview/common/render_view_messages.h:109: string16 /* in - url */,
On 2013/12/07 00:11:07, sgurun wrote:
> On 2013/12/06 20:28:23, boliu wrote:
> > why not GURL?
> 
> Initially the intention was in shouldoverwriteurlloading non-GURL strings
would
> also work, or at least to have the infrastructure to have them work i.e. the
> extreme json string case and others, hence the code.

Oh yeah...that. Now I remember

> Since we will shortly throw
> away the SYNC IPC code in favor of what Darin wants, I don't think we want to
> change it to GURL now.

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
android_webview/native/aw_contents.cc:320: java_ref_.reset();
On 2013/12/06 20:28:23, boliu wrote:
> Why do we need to release the ref here? Doesn't hurt but seems inconsistent.

?

[joth, 2013-12-07 01:52:33]

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
File android_webview/native/aw_contents.cc (right):

https://codereview.chromium.org/24228003/diff/215001/android_webview/native/a...
android_webview/native/aw_contents.cc:320: java_ref_.reset();
On 2013/12/07 01:22:18, boliu wrote:
> On 2013/12/06 20:28:23, boliu wrote:
> > Why do we need to release the ref here? Doesn't hurt but seems inconsistent.
> 
> ?

It ensures that any pending work that runs before teh DeleteSoon() has chance to
run is blocked from calling through to the client. (e.g. after calling
WebView.destroy() I would not expect to get onSizeChanged() etc callbacks).

Although... thinking about it, this doesn't solve AwContentsClients and friends.
Maybe calling SetJavaPeers(NULL, NULL,...) could solve the general case.

[mkosiba (inactive), 2013-12-09 16:18:11]

lgtm

[benm (inactive), 2013-12-09 17:36:10]

lgtm

https://codereview.chromium.org/24228003/diff/235001/android_webview/common/r...
File android_webview/common/render_view_messages.h (right):

https://codereview.chromium.org/24228003/diff/235001/android_webview/common/r...
android_webview/common/render_view_messages.h:111: string16 /* in - url */,
nit: should be base::string16.

[commit-bot: I haz the power, 2013-12-09 18:45:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/24228003/235001

[sgurun-gerrit only, 2013-12-09 18:45:51]

On 2013/12/09 17:36:10, benm wrote:
> lgtm
> 
>
https://codereview.chromium.org/24228003/diff/235001/android_webview/common/r...
> File android_webview/common/render_view_messages.h (right):
> 
>
https://codereview.chromium.org/24228003/diff/235001/android_webview/common/r...
> android_webview/common/render_view_messages.h:111: string16 /* in - url */,
> nit: should be base::string16.

 Thanks guys

[sgurun-gerrit only, 2013-12-09 19:55:44]

@tsepez need lgtm for android_webview/common/render_view_messages.h

[commit-bot: I haz the power, 2013-12-09 20:10:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Tom Sepez, 2013-12-09 20:37:38]

On 2013/12/09 19:55:44, sgurun wrote:
> @tsepez need lgtm for android_webview/common/render_view_messages.h

Should this be passing a GURL instead of a string?  What are the restrictions on
the kinds of urls that can be passed here?

[Tom Sepez, 2013-12-09 20:38:27]

On 2013/12/09 20:37:38, Tom Sepez wrote:
> On 2013/12/09 19:55:44, sgurun wrote:
> > @tsepez need lgtm for android_webview/common/render_view_messages.h
> 
> Should this be passing a GURL instead of a string?  What are the restrictions
on
> the kinds of urls that can be passed here?

And ultimately, what do you do with the URL? That wasn't clear to me from
reading the comment on the messge.  Thanks.

[boliu, 2013-12-09 21:14:49]

On 2013/12/09 20:38:27, Tom Sepez wrote:
> On 2013/12/09 20:37:38, Tom Sepez wrote:
> > On 2013/12/09 19:55:44, sgurun wrote:
> > > @tsepez need lgtm for android_webview/common/render_view_messages.h
> > 
> > Should this be passing a GURL instead of a string?  What are the
restrictions
> on
> > the kinds of urls that can be passed here?
> 
> And ultimately, what do you do with the URL? That wasn't clear to me from
> reading the comment on the messge.  Thanks.

The url is passed to the app embedding the webview via the
WebViewClient.shouldOverrideUrlLoading callback:
http://developer.android.com/reference/android/webkit/WebViewClient.html#shou...,
java.lang.String)

It is intentional to use a string instead of GURL here. The pre-kitkat webview
supported arbitrary strings that clearly will not parse as urls in this
callback. Unfortunately we have found existing apps that have become reliant on
this behavior, so at least for now have to support this behavior.

[sgurun-gerrit only, 2013-12-09 21:46:22]

On 2013/12/09 20:38:27, Tom Sepez wrote:
> On 2013/12/09 20:37:38, Tom Sepez wrote:
> > On 2013/12/09 19:55:44, sgurun wrote:
> > > @tsepez need lgtm for android_webview/common/render_view_messages.h
> > 
> > Should this be passing a GURL instead of a string?  What are the
restrictions
> on
> > the kinds of urls that can be passed here?
> 
> And ultimately, what do you do with the URL? That wasn't clear to me from
> reading the comment on the messge.  Thanks.

This is for compatibility with the old webview (aka webview classic). The url
that we provide is plumbed all the way up to webview's
shouldoverrideurlloading() callback,
http://developer.android.com/reference/android/webkit/WebViewClient.html#shou...,
java.lang.String)
where the application can decide whether or not to cancel url loading by
returning a boolean.

Note that originally this was a string in webview classic, and some apps passed
urls not really conform to valid url specification, so we left it as string for
compatibility purposes.

As for more background: this change is for upstreaming what we have released for
KLP release and temporary. Darin and Jam wants to get rid of the callback that
we rely on (https://codereview.chromium.org/27551003) so we will soon switch to
a new approach.

[Tom Sepez, 2013-12-09 21:49:49]

Thanks for the explanation.  Messages LGTM.

[commit-bot: I haz the power, 2013-12-09 22:09:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/24228003/235001

[commit-bot: I haz the power, 2013-12-09 23:40:14]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-12-09 23:40:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sgurun@chromium.org/24228003/255001

[commit-bot: I haz the power, 2013-12-10 04:33:56]

Change committed as 239646