Componentize IgnoreNavigationResourceThrottle and add chrome and webview specific implementations.

Componentize IgnoreNavigationResourceThrottle and add chrome and webview specific implementations.

This change moves the IgnoreNavigationResourceThrottle to a separate component
as it will be shared between chrome/android and android_webview.
This change also adds the chrome/ and android_webview/ specializations of the
throttle (or more precisely the callback that is used to find the right method
on the right delegate).

BUG=130006
TBR=ben@chromium.org

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

[mkosiba (inactive), 2012-09-18 13:10:08]

Joi,
    Could you please check the component-related stuff?

Joth,
    Please take a look at the rest. Some questions:

I didn't want a common shouldIgnoreNavigation method on
WebContentsDelegateAndroid since I think that these will have to change in the
future to accommodate for different requirements (see crbugs: 143118, 138605).
Do you agree?

To fetch the WebContentsDelegate I added a getter to AwContents and TabAndroid,
is adding a getter method the way to go, or should I associate the delegate with
the WebContents using SupportsUserData?

[Jói, 2012-09-18 14:20:24]

LGTM for component bits.

On Tue, Sep 18, 2012 at 1:10 PM,  <mkosiba@chromium.org> wrote:
> Reviewers: Jói, joth,
>
> Message:
> Joi,
>     Could you please check the component-related stuff?
>
> Joth,
>     Please take a look at the rest. Some questions:
>
> I didn't want a common shouldIgnoreNavigation method on
> WebContentsDelegateAndroid since I think that these will have to change in
> the
> future to accommodate for different requirements (see crbugs: 143118,
> 138605).
> Do you agree?
>
> To fetch the WebContentsDelegate I added a getter to AwContents and
> TabAndroid,
> is adding a getter method the way to go, or should I associate the delegate
> with
> the WebContents using SupportsUserData?
>
>
>
> Description:
> Componentize IgnoreNavigationResourceThrottle and add chrome and webview
> specific implementations.
>
> This change moves the IgnoreNavigationResourceThrottle to a separate
> component
> as it will be shared between chrome/android and android_webview.
> This change also adds the chrome/ and android_webview/ specializations of
> the
> throttle (or more precisely the callback that is used to find the right
> method
> on the right delegate).
>
> BUG=130006
>
>
> Please review this at https://codereview.chromium.org/10946008/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M android_webview/android_webview.gyp
>   A android_webview/browser/aw_intercept_navigation_resource_throttle.h
>   M
> android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
>   M
> android_webview/java/src/org/chromium/android_webview/AwContentsClient.java
>   M
>
android_webview/java/src/org/chromium/android_webview/AwWebContentsDelegate.java
>   M
>
android_webview/javatests/src/org/chromium/android_webview/test/NullContentsClient.java
>   M android_webview/native/android_webview_jni_registrar.cc
>   M android_webview/native/aw_contents.h
>   M android_webview/native/aw_contents.cc
>   A android_webview/native/aw_intercept_navigation_resource_throttle.cc
>   M android_webview/native/aw_web_contents_delegate.h
>   M android_webview/native/aw_web_contents_delegate.cc
>   M android_webview/native/webview_native.gyp
>   M
>
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java
>   A chrome/browser/android/chrome_intercept_navigation_resource_throttle.h
>   A chrome/browser/android/chrome_intercept_navigation_resource_throttle.cc
>   M chrome/browser/android/chrome_web_contents_delegate_android.h
>   M chrome/browser/android/chrome_web_contents_delegate_android.cc
>   M chrome/browser/android/tab_android.h
>   M chrome/browser/component/components.gyp
>   A chrome/browser/component/navigation_interception/DEPS
>   A +
>
chrome/browser/component/navigation_interception/intercept_navigation_resource_throttle.h
>   A +
>
chrome/browser/component/navigation_interception/intercept_navigation_resource_throttle.cc
>   A +
>
chrome/browser/component/navigation_interception/intercept_navigation_resource_throttle_unittest.cc
>   A
> chrome/browser/component/navigation_interception/navigation_interception.gypi
>   M
>
chrome/browser/component/web_contents_delegate_android/java/src/org/chromium/chrome/browser/component/web_contents_delegate_android/WebContentsDelegateAndroid.java
>   M
>
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.h
>   M
>
chrome/browser/component/web_contents_delegate_android/web_contents_delegate_android.cc
>   M chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
>   D chrome/browser/renderer_host/intercept_navigation_resource_throttle.h
>   D chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc
>   D
>
chrome/browser/renderer_host/intercept_navigation_resource_throttle_unittest.cc
>   M chrome/chrome_browser.gypi
>   M chrome/chrome_tests.gypi
>
>

[benm (inactive), 2012-09-19 11:04:04]

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/aw_int...
File android_webview/browser/aw_intercept_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/aw_int...
android_webview/browser/aw_intercept_navigation_resource_throttle.h:1: #ifndef
ANDROID_WEBVIEW_BROWSER_AW_INTERCEPT_NAVIGATION_RESOURCE_THROTTLE_H_
copyright missing?

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/render...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/render...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:8:
#include "android_webview/browser/aw_intercept_navigation_resource_throttle.h"
alpha ordering

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_inte...
File android_webview/native/aw_intercept_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_inte...
android_webview/native/aw_intercept_navigation_resource_throttle.cc:1: #include
"android_webview/browser/aw_intercept_navigation_resource_throttle.h"
copyright

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_web_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_web_...
android_webview/native/aw_web_contents_delegate.cc:28: if (!url.is_valid())
braces?

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_web_...
android_webview/native/aw_web_contents_delegate.cc:38: bool ret =
Java_AwWebContentsDelegate_shouldIgnoreNavigation(
nit: no need for local ret?

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/compo...
File chrome/browser/component/components.gyp (right):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/compo...
chrome/browser/component/components.gyp:9:
'navigation_interception/navigation_interception.gypi',
alpha ordering

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/c...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/c...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:49:
#elif defined(OS_ANDROID)
should this be it's own #if? Doesn't seem like an alternative to the chromeos
#if

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/i...
File
chrome/browser/renderer_host/intercept_navigation_resource_throttle_unittest.cc
(left):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/i...
chrome/browser/renderer_host/intercept_navigation_resource_throttle_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
did we lose this test?

[Jói, 2012-09-24 12:27:13]

LGTM with a nit.

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/navig...
File chrome/browser/component/navigation_interception/DEPS (right):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/navig...
chrome/browser/component/navigation_interception/DEPS:4: # Temporary until test
case moves into a separate test runner.
Perhaps these two could be specific include rules?

specific_include_rules = {
  '.*_(a-z)+test\.cc': [
    "!chrome/test/base",
    "+testing",
  ],
}

[mkosiba (inactive), 2012-09-25 18:01:47]

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/aw_int...
File android_webview/browser/aw_intercept_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/aw_int...
android_webview/browser/aw_intercept_navigation_resource_throttle.h:1: #ifndef
ANDROID_WEBVIEW_BROWSER_AW_INTERCEPT_NAVIGATION_RESOURCE_THROTTLE_H_
On 2012/09/19 11:04:04, benm wrote:
> copyright missing?

Done.

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/render...
File
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10946008/diff/1/android_webview/browser/render...
android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc:8:
#include "android_webview/browser/aw_intercept_navigation_resource_throttle.h"
On 2012/09/19 11:04:04, benm wrote:
> alpha ordering

Done.

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_inte...
File android_webview/native/aw_intercept_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_inte...
android_webview/native/aw_intercept_navigation_resource_throttle.cc:1: #include
"android_webview/browser/aw_intercept_navigation_resource_throttle.h"
On 2012/09/19 11:04:04, benm wrote:
> copyright

Done.

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_web_...
File android_webview/native/aw_web_contents_delegate.cc (right):

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_web_...
android_webview/native/aw_web_contents_delegate.cc:28: if (!url.is_valid())
On 2012/09/19 11:04:04, benm wrote:
> braces?

optional according to the coding style.

http://codereview.chromium.org/10946008/diff/1/android_webview/native/aw_web_...
android_webview/native/aw_web_contents_delegate.cc:38: bool ret =
Java_AwWebContentsDelegate_shouldIgnoreNavigation(
On 2012/09/19 11:04:04, benm wrote:
> nit: no need for local ret?

Done.

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/compo...
File chrome/browser/component/components.gyp (right):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/compo...
chrome/browser/component/components.gyp:9:
'navigation_interception/navigation_interception.gypi',
On 2012/09/19 11:04:04, benm wrote:
> alpha ordering

Done.

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/navig...
File chrome/browser/component/navigation_interception/DEPS (right):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/component/navig...
chrome/browser/component/navigation_interception/DEPS:4: # Temporary until test
case moves into a separate test runner.
On 2012/09/24 12:27:13, Jói wrote:
> Perhaps these two could be specific include rules?
> 
> specific_include_rules = {
>   '.*_(a-z)+test\.cc': [
>     "!chrome/test/base",
>     "+testing",
>   ],
> }

sure! Didn't know you could do that.

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/c...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/c...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:49:
#elif defined(OS_ANDROID)
On 2012/09/19 11:04:04, benm wrote:
> should this be it's own #if? Doesn't seem like an alternative to the chromeos
> #if

Done.

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/i...
File
chrome/browser/renderer_host/intercept_navigation_resource_throttle_unittest.cc
(left):

http://codereview.chromium.org/10946008/diff/1/chrome/browser/renderer_host/i...
chrome/browser/renderer_host/intercept_navigation_resource_throttle_unittest.cc:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2012/09/19 11:04:04, benm wrote:
> did we lose this test?

it should moved under chrome/browser/component/, not sure why the CR tool isn't
indicating it's a move.

[joth, 2012-09-25 19:02:04]

On 18 September 2012 06:10, <mkosiba@chromium.org> wrote:

> Reviewers: Jói, joth,
>
> Message:
> Joi,
>     Could you please check the component-related stuff?
>
> Joth,
>     Please take a look at the rest. Some questions:
>
> I didn't want a common shouldIgnoreNavigation method on
> WebContentsDelegateAndroid since I think that these will have to change in
> the
> future to accommodate for different requirements (see crbugs: 143118,
> 138605).
> Do you agree?
>
> Agree we don't want to force a shared *implementation*
of shouldIgnoreNavigation().
But of we can we'd like to  share the native -> JNI -> java plumbing
between the two, winding through to an empty impl, and then have the
different WCDA sub-classes provide the appropriate handling.
The fact we have an exact duplicate
of CheckIfShouldIgnoreNavigationOnUIThread() across both users of the
throttle is also annoying, and would fall away if we had a way to share the
JNI plumbing.


A separate question is whether this is an abuse of WCDA to be adding on
other methods that are not defined in content::WebContentDelegate -- it's
turning it back into bit of a mega-class that will implicitly have
knowledge of every other browser component, and how to plumb input/output
params for each of those components across JNI. SO we don't want to push it
all back into there.



Pondering it more, could the navigation_interception component include a
(optional?) android-only part that knows how to do its own JNI? This would
need us to define a new Java abstract interface inside that component, a
helper to allow the embedder to associate their [java]
NavInterceptionDelegate instance with a WebContents (registered as a weak
java ref inside UserData struct) and a static function suitable for use
with the throttle that does the UI thread bounce, looks up
said NavInterceptionDelegate instance, and calls it via JNI.
Then the AW and browser changes would be mostly java-only, to subclass this
interface (i.e. in AwContentsClient and TabAndroid?) and wire it up where
the do their other WebContents creation set up.

WDYT?


If it's too much patch-churn to go down that approach now, I can live with
the current approach. But I think reducing the duplication would have long
term benefit.











> To fetch the WebContentsDelegate I added a getter to AwContents and
> TabAndroid,
> is adding a getter method the way to go, or should I associate the
> delegate with
> the WebContents using SupportsUserData?
>
>
>
> Description:
> Componentize IgnoreNavigationResourceThrott**le and add chrome and webview
> specific implementations.
>
> This change moves the IgnoreNavigationResourceThrott**le to a separate
> component
> as it will be shared between chrome/android and android_webview.
> This change also adds the chrome/ and android_webview/ specializations of
> the
> throttle (or more precisely the callback that is used to find the right
> method
> on the right delegate).
>
> BUG=130006
>
>
> Please review this at
https://codereview.chromium.**org/10946008/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M android_webview/android_**webview.gyp
>   A android_webview/browser/aw_**intercept_navigation_resource_**
> throttle.h
>   M android_webview/browser/**renderer_host/aw_resource_**
> dispatcher_host_delegate.cc
>   M android_webview/java/src/org/**chromium/android_webview/**
> AwContentsClient.java
>   M android_webview/java/src/org/**chromium/android_webview/**
> AwWebContentsDelegate.java
>   M android_webview/javatests/src/**org/chromium/android_webview/**
> test/NullContentsClient.java
>   M android_webview/native/**android_webview_jni_registrar.**cc
>   M android_webview/native/aw_**contents.h
>   M android_webview/native/aw_**contents.cc
>   A android_webview/native/aw_**intercept_navigation_resource_**
> throttle.cc
>   M android_webview/native/aw_web_**contents_delegate.h
>   M android_webview/native/aw_web_**contents_delegate.cc
>   M android_webview/native/**webview_native.gyp
>   M chrome/android/java/src/org/**chromium/chrome/browser/**
> ChromeWebContentsDelegateAndro**id.java
>   A chrome/browser/android/chrome_**intercept_navigation_resource_**
> throttle.h
>   A chrome/browser/android/chrome_**intercept_navigation_resource_**
> throttle.cc
>   M chrome/browser/android/chrome_**web_contents_delegate_android.**h
>   M chrome/browser/android/chrome_**web_contents_delegate_android.**cc
>   M chrome/browser/android/tab_**android.h
>   M chrome/browser/component/**components.gyp
>   A chrome/browser/component/**navigation_interception/DEPS
>   A + chrome/browser/component/**navigation_interception/**
> intercept_navigation_resource_**throttle.h
>   A + chrome/browser/component/**navigation_interception/**
> intercept_navigation_resource_**throttle.cc
>   A + chrome/browser/component/**navigation_interception/**
> intercept_navigation_resource_**throttle_unittest.cc
>   A chrome/browser/component/**navigation_interception/**
> navigation_interception.gypi
>   M chrome/browser/component/web_**contents_delegate_android/**
> java/src/org/chromium/chrome/**browser/component/web_**
> contents_delegate_android/**WebContentsDelegateAndroid.**java
>   M chrome/browser/component/web_**contents_delegate_android/web_**
> contents_delegate_android.h
>   M chrome/browser/component/web_**contents_delegate_android/web_**
> contents_delegate_android.cc
>   M chrome/browser/renderer_host/**chrome_resource_dispatcher_**
> host_delegate.cc
>   D chrome/browser/renderer_host/**intercept_navigation_resource_**
> throttle.h
>   D chrome/browser/renderer_host/**intercept_navigation_resource_**
> throttle.cc
>   D chrome/browser/renderer_host/**intercept_navigation_resource_**
> throttle_unittest.cc
>   M chrome/chrome_browser.gypi
>   M chrome/chrome_tests.gypi
>
>
>

[mkosiba (inactive), 2012-10-01 15:47:36]

On 2012/09/25 19:02:04, joth wrote:

> Agree we don't want to force a shared *implementation*
> of shouldIgnoreNavigation().
> But of we can we'd like to  share the native -> JNI -> java plumbing
> between the two, winding through to an empty impl, and then have the
> different WCDA sub-classes provide the appropriate handling.
> The fact we have an exact duplicate
> of CheckIfShouldIgnoreNavigationOnUIThread() across both users of the
> throttle is also annoying, and would fall away if we had a way to share the
> JNI plumbing.

The reason I did this is that I know the chrome/ layer implementation to diverge
in the near future. 

> 
> 
> A separate question is whether this is an abuse of WCDA to be adding on
> other methods that are not defined in content::WebContentDelegate -- it's
> turning it back into bit of a mega-class that will implicitly have
> knowledge of every other browser component, and how to plumb input/output
> params for each of those components across JNI. SO we don't want to push it
> all back into there.
> 
> 
> 
> Pondering it more, could the navigation_interception component include a
> (optional?) android-only part that knows how to do its own JNI? This would
> need us to define a new Java abstract interface inside that component, a
> helper to allow the embedder to associate their [java]
> NavInterceptionDelegate instance with a WebContents (registered as a weak
> java ref inside UserData struct) and a static function suitable for use
> with the throttle that does the UI thread bounce, looks up
> said NavInterceptionDelegate instance, and calls it via JNI.
> Then the AW and browser changes would be mostly java-only, to subclass this
> interface (i.e. in AwContentsClient and TabAndroid?) and wire it up where
> the do their other WebContents creation set up.
> 
> WDYT?
> 
> 
> If it's too much patch-churn to go down that approach now, I can live with
> the current approach. But I think reducing the duplication would have long
> term benefit.

So I was thinking about it a bit more, and it does make more sense to converge
initially and fork the chrome/ layer implementation when it gets sufficiently
different.

I initially wanted to do the UserData thing but with a separate chrome/ and
android_webview/ impl, but that'd be creating even more duplication.

[mkosiba (inactive), 2012-10-03 17:14:48]

Joth, Ben,
   New version is up for another round of feedback. Everything except for the
Java test case is fair game.

https://codereview.chromium.org/10946008/diff/15001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java
(right):

https://codereview.chromium.org/10946008/diff/15001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java:5:
package org.chromium.android_webview.test;
Please ignore the file for now - I need to move the test to use the
TestWebServer.

[joth, 2012-10-03 22:01:32]

I like this, basically all lg2me except some nits and yr test TODO.

https://codereview.chromium.org/10946008/diff/15001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/10946008/diff/15001/android_webview/native/aw...
android_webview/native/aw_contents.h:56: AwWebContentsDelegate*
GetAwWebContentsDelegate() const;
I missed where this is used. If we can avoid this (e.g. pass the delegate into
the thing that needs it) it avoids build up of spaghetti lines of ask-A-for-B
just to ask it for a C...

https://codereview.chromium.org/10946008/diff/15001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java
(left):

https://codereview.chromium.org/10946008/diff/15001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java:17:

nit: revert this file.

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File chrome/browser/component/navigation_interception/DEPS (right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/DEPS:7: '.*_(a-z)+test\.cc': [
wow, not seen this before! we could go mad with this couldn't we.

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc
(right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc:39:
if (!web_contents) return false;
nit: in c++ we put the non-braced statement on its own line. (and below)

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc:51:
void InterceptNavigationDelegate::Associate(
note in header that it transfers ownership. |delegate| will get deleted when the
|web_contents| is deleted

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File
chrome/browser/component/navigation_interception/intercept_navigation_delegate.h
(right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/intercept_navigation_delegate.h:25:
class InterceptNavigationDelegate : public base::SupportsUserData::Data {
class comment? (mainly need to point out how to use one of these to wire the
throttle to the java counterpart delegate.)

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File
chrome/browser/component/navigation_interception/java/src/org/chromium/chrome/browser/component/navigation_interception/InterceptNavigationDelegate.java
(right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/java/src/org/chromium/chrome/browser/component/navigation_interception/InterceptNavigationDelegate.java:16:
* @param isContentInitiated true if the navigation was initiated by the user.
nit: isContentInitiated -> isUserGesture 

or is this consistent with existing java-land terminology?

[benm (inactive), 2012-10-05 13:42:15]

looks good, tiny nit!

http://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/n...
File
chrome/browser/component/navigation_interception/intercept_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/n...
chrome/browser/component/navigation_interception/intercept_navigation_resource_throttle.h:34:
bool /*has_user_gesture*/)>
s/has/is/ ?

[mkosiba (inactive), 2012-10-08 15:09:11]

New patch out, PTAL

https://codereview.chromium.org/10946008/diff/15001/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java
(right):

https://codereview.chromium.org/10946008/diff/15001/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java:5:
package org.chromium.android_webview.test;
On 2012/10/03 17:14:48, Martin Kosiba wrote:
> Please ignore the file for now - I need to move the test to use the
> TestWebServer.

new iteration of test is good to go

https://codereview.chromium.org/10946008/diff/15001/android_webview/native/aw...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/10946008/diff/15001/android_webview/native/aw...
android_webview/native/aw_contents.h:56: AwWebContentsDelegate*
GetAwWebContentsDelegate() const;
On 2012/10/03 22:01:32, joth wrote:
> I missed where this is used. If we can avoid this (e.g. pass the delegate into
> the thing that needs it) it avoids build up of spaghetti lines of ask-A-for-B
> just to ask it for a C...

this was left over from the previous approach. removed

https://codereview.chromium.org/10946008/diff/15001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java
(left):

https://codereview.chromium.org/10946008/diff/15001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/ChromeWebContentsDelegateAndroid.java:17:

On 2012/10/03 22:01:32, joth wrote:
> nit: revert this file. 

Done.

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File chrome/browser/component/navigation_interception/DEPS (right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/DEPS:7: '.*_(a-z)+test\.cc': [
On 2012/10/03 22:01:32, joth wrote:
> wow, not seen this before! we could go mad with this couldn't we.

yes! Joi was kind enough to point this out :)

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc
(right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc:39:
if (!web_contents) return false;
On 2012/10/03 22:01:32, joth wrote:
> nit: in c++ we put the non-braced statement on its own line. (and below)

Done.

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc:51:
void InterceptNavigationDelegate::Associate(
On 2012/10/03 22:01:32, joth wrote:
> note in header that it transfers ownership. |delegate| will get deleted when
the
> |web_contents| is deleted

well.. the use of scoped_ptr should imply that..

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File
chrome/browser/component/navigation_interception/intercept_navigation_delegate.h
(right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/intercept_navigation_delegate.h:25:
class InterceptNavigationDelegate : public base::SupportsUserData::Data {
On 2012/10/03 22:01:32, joth wrote:
> class comment? (mainly need to point out how to use one of these to wire the
> throttle to the java counterpart delegate.)

Done.

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File
chrome/browser/component/navigation_interception/intercept_navigation_resource_throttle.h
(right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/intercept_navigation_resource_throttle.h:34:
bool /*has_user_gesture*/)>
On 2012/10/05 13:42:15, benm wrote:
> s/has/is/ ?

This is just a copy of the ResourceRequestInfo::GetHasUserGesture() value, so
I'd like to keep the name to indicate that they're the same.

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
File
chrome/browser/component/navigation_interception/java/src/org/chromium/chrome/browser/component/navigation_interception/InterceptNavigationDelegate.java
(right):

https://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/...
chrome/browser/component/navigation_interception/java/src/org/chromium/chrome/browser/component/navigation_interception/InterceptNavigationDelegate.java:16:
* @param isContentInitiated true if the navigation was initiated by the user.
On 2012/10/03 22:01:32, joth wrote:
> nit: isContentInitiated -> isUserGesture 
> 
> or is this consistent with existing java-land terminology?

good find! thanks!

[joth, 2012-10-09 02:13:52]

+boliu@chromium.org fyi as he's been hacking the test servera bit recently.

lgtm.

http://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/n...
File
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc
(right):

http://codereview.chromium.org/10946008/diff/15001/chrome/browser/component/n...
chrome/browser/component/navigation_interception/intercept_navigation_delegate.cc:51:
void InterceptNavigationDelegate::Associate(
On 2012/10/08 15:09:12, Martin Kosiba wrote:
> On 2012/10/03 22:01:32, joth wrote:
> > note in header that it transfers ownership. |delegate| will get deleted when
> the
> > |web_contents| is deleted
> 
> well.. the use of scoped_ptr should imply that..

Oh Yes. My eyes are still blind to the idea of pass-by-value scoped_ptr. I'll
get there eventually :)

http://codereview.chromium.org/10946008/diff/30030/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java
(right):

http://codereview.chromium.org/10946008/diff/30030/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java:31:
* Tests for the WebViewClient.shouldOverrideUrlLoading() method.
are a bunch of these tests also checked in somewhere for exercising similar
subset of use-cases on the browser? If so, and if meaningful to do so, could
mention this.

[mkosiba (inactive), 2012-10-09 09:56:09]

https://codereview.chromium.org/10946008/diff/30030/android_webview/javatests...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java
(right):

https://codereview.chromium.org/10946008/diff/30030/android_webview/javatests...
android_webview/javatests/src/org/chromium/android_webview/test/AwShouldIgnoreNavigationTest.java:31:
* Tests for the WebViewClient.shouldOverrideUrlLoading() method.
On 2012/10/09 02:13:53, joth wrote:
> are a bunch of these tests also checked in somewhere for exercising similar
> subset of use-cases on the browser? If so, and if meaningful to do so, could
> mention this.

So when jknotten and I get around to implementing the chrome/ side of the
Throttle we'll add some chrome-specific tests. For now I think the native test
covers most of the shared implementation anyway.

[commit-bot: I haz the power, 2012-10-09 16:58:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10946008/30032

[commit-bot: I haz the power, 2012-10-09 16:58:33]

Presubmit check for 10946008-30032 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  base/android/jni_generator/jni_generator.py, line 183, 99 chars

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.2s to calculate.

[mkosiba (inactive), 2012-10-09 17:08:18]

Ben@,
    I've TBR'd you for chrome/*.gyp changes.

[commit-bot: I haz the power, 2012-10-09 17:08:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10946008/30032

[commit-bot: I haz the power, 2012-10-09 17:08:52]

Presubmit check for 10946008-30032 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  base/android/jni_generator/jni_generator.py, line 183, 99 chars

Presubmit checks took 1.2s to calculate.

[commit-bot: I haz the power, 2012-10-09 17:14:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10946008/34031

[boliu, 2012-10-09 18:27:39]

http://codereview.chromium.org/10946008/diff/34031/android_webview/javatests/...
File
android_webview/javatests/src/org/chromium/android_webview/test/util/TestWebServer.java
(right):

http://codereview.chromium.org/10946008/diff/34031/android_webview/javatests/...
android_webview/javatests/src/org/chromium/android_webview/test/util/TestWebServer.java:327:
}
Sorry for not comment earlier, and it's not really a blocker, but should update
mResponseCountMap and mLastRequestMap here. Also the response == null check is
below, so should move this below that if block. I can send out a cl later for
this.

[commit-bot: I haz the power, 2012-10-09 18:32:06]

Retried try job too often for step(s) interactive_ui_tests

[Ben Goodger (Google), 2012-10-09 21:13:49]

lgtm

[commit-bot: I haz the power, 2012-10-09 21:16:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/10946008/34031

[commit-bot: I haz the power, 2012-10-09 22:10:49]

Change committed as 160945