Implement a ResourceThrottle for URL overriding in Chrome on Android.

Implement a ResourceThrottle for URL overriding in Chrome on Android.

This gives the embedder the opportunity to cancel top level navigations. This
is required in Chrome on Android if we want to handle a url (such as
m.youtube.com) in an external application.

BUG=130006
TEST=unit_tests

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

[joth, 2012-05-11 15:34:13]

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:175:
throttles->push_back(new ShouldIgnoreNavigationResourceThrottle(request));
this looks like a good point to also make it conditional on a runtime flag, so
we can avoid this check if we're running in browser mode.

(or we invent a new #define to use here for the embedded API mode, but I think
the runtime maybe better as eventually we could even use this optimization in
API mode too if the client hasn't been set / hasn't overridden the callback)

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
File chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc
(right):

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:41:
render_view_host, url, referrer, is_content_initiated);
Enclose in {} when it covers more than one physical line.

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:49: }
// anonymous namespace
nit: // namespace

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:52:
net::URLRequest* request) : request_(request) {
nit: initializer on its own line when the c'tor does not entirely fit on one
line.

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:67:
ShouldDeferRequest(new_url, defer);
nit: ShouldDeferRequest is a slightly open name (sounds like it should return
bool to me).

Is this extra function call really needed at all?

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:87:
// TODO: Promote has_user_gesture to ResourceRequestInfo?
TODO(mkosiba). Or word as a 'in future we may want to...' comment if it's not
strictly something needing doing.

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:88:
const bool is_content_initiated =
nit: chromium style generally shuns const here. I think you can then squish this
down by one line.

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:103:
AsWeakPtr())));
currytastic :)

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:112:
else
nit: use braces when there's an else

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
File chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.h
(right):

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.h:18:
class ShouldIgnoreNavigationResourceThrottle
class comment

https://chromiumcodereview.appspot.com/10310124/diff/1/chrome/browser/rendere...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.h:20:
public base::SupportsWeakPtr<ShouldIgnoreNavigationResourceThrottle> {
nit: as WeakPtr is only used as an internal impl detail I think it's nicer to
aggregate a WeakPtrFactory rather than use naughty multiple inheritance

https://chromiumcodereview.appspot.com/10310124/diff/1/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10310124/diff/1/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:2440: this, validated_url,
referrer, is_content_initiated);
feels like it should be possible to write a test case for this... guess the main
snag is it would be ifdef ANDROID only (unless we make a way to include this
path on all platforms)

https://chromiumcodereview.appspot.com/10310124/diff/1/content/public/browser...
File content/public/browser/render_view_host_delegate.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/1/content/public/browser...
content/public/browser/render_view_host_delegate.h:335: bool
is_content_initiated);
I wonder if it would be preferable to have this under an ifdef.

[mkosiba (inactive), 2012-05-15 14:20:23]

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

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/c...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:175:
throttles->push_back(new ShouldIgnoreNavigationResourceThrottle(request));
On 2012/05/11 15:34:13, joth wrote:
> this looks like a good point to also make it conditional on a runtime flag, so
> we can avoid this check if we're running in browser mode.
> 
> (or we invent a new #define to use here for the embedded API mode, but I think
> the runtime maybe better as eventually we could even use this optimization in
> API mode too if the client hasn't been set / hasn't overridden the callback)

But we need this in browser mode!

It _might_ make sense to use this instead of
browser_handles_top_level_navigations in  Chrome Frame (in which case we'd
probably need to make the addition of the throttle depend on some runtime flag)
but that's a separate change.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
File chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:41:
render_view_host, url, referrer, is_content_initiated);
On 2012/05/11 15:34:13, joth wrote:
> Enclose in {} when it covers more than one physical line.

Done.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:49: }
// anonymous namespace
On 2012/05/11 15:34:13, joth wrote:
> nit: // namespace

Done.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:52:
net::URLRequest* request) : request_(request) {
On 2012/05/11 15:34:13, joth wrote:
> nit: initializer on its own line when the c'tor does not entirely fit on one
> line.

Done.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:52:
net::URLRequest* request) : request_(request) {
On 2012/05/11 15:34:13, joth wrote:
> nit: initializer on its own line when the c'tor does not entirely fit on one
> line.

Done.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:67:
ShouldDeferRequest(new_url, defer);
On 2012/05/11 15:34:13, joth wrote:
> nit: ShouldDeferRequest is a slightly open name (sounds like it should return
> bool to me).
Done

> 
> Is this extra function call really needed at all?

I think the extra function makes it a bit cleaner. The alternative is to call
WillRedirectRequest from WillStartRequest which (to me) is like saying
"redirecting is the same as starting a request". With the extra function it
feels more like "on starting and redirecting a request do the same thing".

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:88:
const bool is_content_initiated =
On 2012/05/11 15:34:13, joth wrote:
> nit: chromium style generally shuns const here. I think you can then squish
this
> down by one line.

Done.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.cc:112:
else
On 2012/05/11 15:34:13, joth wrote:
> nit: use braces when there's an else

Done.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
File chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.h:18:
class ShouldIgnoreNavigationResourceThrottle
On 2012/05/11 15:34:13, joth wrote:
> class comment

Done.

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/s...
chrome/browser/renderer_host/should_ignore_navigation_resource_throttle.h:20:
public base::SupportsWeakPtr<ShouldIgnoreNavigationResourceThrottle> {
On 2012/05/11 15:34:13, joth wrote:
> nit: as WeakPtr is only used as an internal impl detail I think it's nicer to
> aggregate a WeakPtrFactory rather than use naughty multiple inheritance

Done.

http://codereview.chromium.org/10310124/diff/1/content/browser/web_contents/w...
File content/browser/web_contents/web_contents_impl.cc (right):

http://codereview.chromium.org/10310124/diff/1/content/browser/web_contents/w...
content/browser/web_contents/web_contents_impl.cc:2440: this, validated_url,
referrer, is_content_initiated);
On 2012/05/11 15:34:13, joth wrote:
> feels like it should be possible to write a test case for this... guess the
main
> snag is it would be ifdef ANDROID only (unless we make a way to include this
> path on all platforms)

Should be possible to inject the throttle into requests in a unit test...

http://codereview.chromium.org/10310124/diff/1/content/public/browser/render_...
File content/public/browser/render_view_host_delegate.h (right):

http://codereview.chromium.org/10310124/diff/1/content/public/browser/render_...
content/public/browser/render_view_host_delegate.h:335: bool
is_content_initiated);
On 2012/05/11 15:34:13, joth wrote:
> I wonder if it would be preferable to have this under an ifdef.

As the change is now the new throttle is built (and available) on all platforms.
The only way in which Android is special is that the throttle is actually added
to top level requests.

[joth, 2012-05-15 14:30:43]

This LG from my side. I suggest OWNERS review for the content/public changes
next, then other owners reviews.

Thanks!

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

http://codereview.chromium.org/10310124/diff/1/chrome/browser/renderer_host/c...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:175:
throttles->push_back(new ShouldIgnoreNavigationResourceThrottle(request));
On 2012/05/15 14:20:23, Martin Kosiba wrote:
> On 2012/05/11 15:34:13, joth wrote:
> > this looks like a good point to also make it conditional on a runtime flag,
so
> > we can avoid this check if we're running in browser mode.
> > 
> > (or we invent a new #define to use here for the embedded API mode, but I
think
> > the runtime maybe better as eventually we could even use this optimization
in
> > API mode too if the client hasn't been set / hasn't overridden the callback)
> 
> But we need this in browser mode!
> 
> It _might_ make sense to use this instead of
> browser_handles_top_level_navigations in  Chrome Frame (in which case we'd
> probably need to make the addition of the throttle depend on some runtime
flag)
> but that's a separate change.

Yep makes sense. As discussed I was getting mixed up with the resource request
intercept case (again).

http://codereview.chromium.org/10310124/diff/6002/content/browser/renderer_ho...
File content/browser/renderer_host/resource_request_info_impl.cc (right):

http://codereview.chromium.org/10310124/diff/6002/content/browser/renderer_ho...
content/browser/renderer_host/resource_request_info_impl.cc:185: bool
ResourceRequestInfoImpl::GetHasUserGesture() const {
suggest: GetHasUserGesture => HasUserGesture
(in style of other bool-returning methods above, they tend not to have the
GetXxx prefix)

[mkosiba (inactive), 2012-05-16 15:46:34]

Hi Charlie,
    I'm adding you to the review since this is a ResourceThrottle-based
reimplementation of http://codereview.chromium.org/9978015/. Please take a look.

Thanks!
    Martin

[Charlie Reis, 2012-05-17 22:18:56]

This looks like a much better approach.  Rubber stamp LGTM with the nits below,
but please get someone familiar with ResourceThrottles (e.g., mpcomplete?) to
review as well.

http://codereview.chromium.org/10310124/diff/13002/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10310124/diff/13002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:191:
throttles->push_back(new ShouldIgnoreNavigationResourceThrottle(request));
nit: IgnoreNavigationResourceThrottle seems more consistent with the other
throttle classes.

http://codereview.chromium.org/10310124/diff/13002/content/public/browser/web...
File content/public/browser/web_contents_delegate.cc (right):

http://codereview.chromium.org/10310124/diff/13002/content/public/browser/web...
content/public/browser/web_contents_delegate.cc:11: #include
"content/public/browser/navigation_entry.h"
You don't need these.

[mkosiba (inactive), 2012-05-18 12:33:01]

http://codereview.chromium.org/10310124/diff/13002/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10310124/diff/13002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:191:
throttles->push_back(new ShouldIgnoreNavigationResourceThrottle(request));
On 2012/05/17 22:18:57, creis wrote:
> nit: IgnoreNavigationResourceThrottle seems more consistent with the other
> throttle classes.

Done.

http://codereview.chromium.org/10310124/diff/13002/content/public/browser/web...
File content/public/browser/web_contents_delegate.cc (right):

http://codereview.chromium.org/10310124/diff/13002/content/public/browser/web...
content/public/browser/web_contents_delegate.cc:11: #include
"content/public/browser/navigation_entry.h"
On 2012/05/17 22:18:57, creis wrote:
> You don't need these.

Done.

[Matt Perry, 2012-05-18 20:38:08]

I don't know much about ResourceThrottles, actually.

BTW, waiting for the UI thread does add a surprising amount of latency to
network requests, last time I tried this. Though, I tested it for every request
rather than just toplevel ones, so maybe it won't be that significant. Just
something to watch out for.

[mkosiba (inactive), 2012-05-21 10:19:02]

On 2012/05/18 20:38:08, Matt Perry wrote:
> I don't know much about ResourceThrottles, actually.
> 
Any idea who would?

> BTW, waiting for the UI thread does add a surprising amount of latency to
> network requests, last time I tried this. Though, I tested it for every
request
> rather than just toplevel ones, so maybe it won't be that significant. Just
> something to watch out for.

Thanks for the warning! I don't think this is high enough on our priority list
for now, but I've opened crbug.com/128962

[mkosiba (inactive), 2012-05-23 09:38:14]

Hi Darin,
    Please take a look.

Thanks!
    Martin

[darin (slow to review), 2012-05-23 18:03:49]

This sounds like something that should piggyback on WebIntents, which also wants
to enable "external apps" to handle URL loads.  Have you spoken with jhawkins
and gbillock about this?

[darin (slow to review), 2012-05-23 18:03:58]

This sounds like something that should piggyback on WebIntents, which also wants
to enable "external apps" to handle URL loads.  Have you spoken with jhawkins
and gbillock about this?

[darin (slow to review), 2012-05-23 18:18:28]

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
File chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc
(right):

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:24: void
CheckShouldIgnoreNavigationOnUiThread(
nit: UiThread -> UIThread

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:63: *defer =
ShouldDeferRequestForUrl(request_->url());
nit: ForUrl -> ForURL

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:82: const
ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request_);
you should just call ResourceRequestInfo::GetAssociatedRenderView() here instead
of using GetRenderViewForRequest.

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:83: // The
ShouldIngoreNavigationResourceThrottle should only be created for top
nit: what is a "ShouldIngoreNavigationResourceThrottle"?

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:107:
controller()->Cancel();
this may not be the ideal way to cancel the request in this case.  take a look
at how mailto: URLs get handled.  there we cancel the URLRequest so that its
status is URLRequestStatus::HANDLED_EXTERNALLY.  I think you are trying to
emulate that behavior, and so you need to make sure the URLRequest gets its
status set properly.  we may need to tweak the ResourceThrottleController
interface for this purpose.

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
File chrome/browser/renderer_host/ignore_navigation_resource_throttle.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.h:20: class
IgnoreNavigationResourceThrottle
nit: this class name suggests unconditional ignoring of navigations.  but in
reality you are applying some conditions.

maybe InterceptNavigationResourceThrottle would be a better name?

https://chromiumcodereview.appspot.com/10310124/diff/4003/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/4003/content/public/brow...
content/public/browser/web_contents_delegate.h:36: struct Referrer;
nit: "class" sorts before "struct"

[mkosiba (inactive), 2012-05-28 16:06:52]

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
File chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc
(right):

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:24: void
CheckShouldIgnoreNavigationOnUiThread(
On 2012/05/23 18:18:29, darin wrote:
> nit: UiThread -> UIThread

Done.

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:63: *defer =
ShouldDeferRequestForUrl(request_->url());
On 2012/05/23 18:18:29, darin wrote:
> nit: ForUrl -> ForURL

Done.

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:82: const
ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request_);
On 2012/05/23 18:18:29, darin wrote:
> you should just call ResourceRequestInfo::GetAssociatedRenderView() here
instead
> of using GetRenderViewForRequest.

Done.

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:83: // The
ShouldIngoreNavigationResourceThrottle should only be created for top
On 2012/05/23 18:18:29, darin wrote:
> nit: what is a "ShouldIngoreNavigationResourceThrottle"?

Done.

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.cc:107:
controller()->Cancel();
On 2012/05/23 18:18:29, darin wrote:
> this may not be the ideal way to cancel the request in this case.  take a look
> at how mailto: URLs get handled.  there we cancel the URLRequest so that its
> status is URLRequestStatus::HANDLED_EXTERNALLY.  I think you are trying to
> emulate that behavior, and so you need to make sure the URLRequest gets its
> status set properly.  we may need to tweak the ResourceThrottleController
> interface for this purpose.

Done.

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
File chrome/browser/renderer_host/ignore_navigation_resource_throttle.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/4003/chrome/browser/rend...
chrome/browser/renderer_host/ignore_navigation_resource_throttle.h:20: class
IgnoreNavigationResourceThrottle
On 2012/05/23 18:18:29, darin wrote:
> nit: this class name suggests unconditional ignoring of navigations.  but in
> reality you are applying some conditions.
> 
> maybe InterceptNavigationResourceThrottle would be a better name?

Done.

https://chromiumcodereview.appspot.com/10310124/diff/4003/content/public/brow...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/4003/content/public/brow...
content/public/browser/web_contents_delegate.h:36: struct Referrer;
On 2012/05/23 18:18:29, darin wrote:
> nit: "class" sorts before "struct"

Done.

[mkosiba (inactive), 2012-05-29 09:14:07]

Updated version is uploaded, please take a look.

[darin (slow to review), 2012-05-29 20:54:43]

On 2012/05/29 09:14:07, Martin Kosiba wrote:
> Updated version is uploaded, please take a look.

What about my comment about using WebIntents?  Why are we building a redundant
mechanism for this functionality?  Has there been a discussion with jhawkins@
and gbillock@ about this?

[mkosiba (inactive), 2012-05-29 22:03:51]

On 2012/05/29 20:54:43, darin wrote:
> On 2012/05/29 09:14:07, Martin Kosiba wrote:
> > Updated version is uploaded, please take a look.
> 
> What about my comment about using WebIntents?  Why are we building a redundant
> mechanism for this functionality?  Has there been a discussion with jhawkins@
> and gbillock@ about this?

I sent an email to both of them. Greg replied saying that the navigation
interactions with WebIntents are yet to be designed but the code I'm introducing
looks like something they could use.

[Greg Billock, 2012-05-29 22:10:45]

If we had an existing mechanism for mapping web intents to Android
intents, including navigational integration, then this ought to fit
into that mechanism. Darin, are you suggesting working on that rather
than this more special-purpose mapping?

This CL looks like the place we'll plug in navigation->intent
functionality generally, and so once we have our great Android
integration story ready, a lot of this code will be helpful to that. I
don't know the urgency of this work, though. It may be a while before
we are ready to tackle the above. Marcin, if you're keen to work on
it, it'd be awesome, though! :-)


On Tue, May 29, 2012 at 1:54 PM,  <darin@chromium.org> wrote:
> On 2012/05/29 09:14:07, Martin Kosiba wrote:
>>
>> Updated version is uploaded, please take a look.
>
>
> What about my comment about using WebIntents?  Why are we building a
> redundant
> mechanism for this functionality?  Has there been a discussion with
> jhawkins@
> and gbillock@ about this?
>
> https://chromiumcodereview.appspot.com/10310124/

[James Hawkins, 2012-05-29 23:07:08]

I had a chat with Greg off-line about this, and we're in agreement with each
other (and I think, Darin, if I'm not reading too much into your quote). 
Martin, we should VC ASAP and talk about the right long-term solution for this
integration point.  The correct long-term solution for URL 'overriding' in
Chrome is through Intents, and Android should hook into Chrome's implementation
at the right points.

[James Hawkins, 2012-05-29 23:08:01]

The parenthetical should read:

and I think with Darin as well, if I'm not reading too much into your quote

[darin (slow to review), 2012-05-29 23:19:57]

Right.  If this current CL is the correct stepping stone toward that longer
term plan, then I'm super happy.
-Darin

On Tue, May 29, 2012 at 4:07 PM, <jhawkins@chromium.org> wrote:

> I had a chat with Greg off-line about this, and we're in agreement with
> each
> other (and I think, Darin, if I'm not reading too much into your quote).
> Martin, we should VC ASAP and talk about the right long-term solution for
> this
> integration point.  The correct long-term solution for URL 'overriding' in
> Chrome is through Intents, and Android should hook into Chrome's
> implementation
> at the right points.
>
>
https://chromiumcodereview.**appspot.com/10310124/<https://chromiumcodereview...
>

[darin (slow to review), 2012-05-29 23:39:32]

https://chromiumcodereview.appspot.com/10310124/diff/25002/net/url_request/ur...
File net/url_request/url_request.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/25002/net/url_request/ur...
net/url_request/url_request.h:541: void CancelWithHandledExternallyStatus();
It looks like this may actually be moving in the wrong direction code-wise.  The
network stack really has no need to know about the HANDLED_EXTERNALLY bit.  It
is only the WebURLLoaderImpl that needs to see this bit so that the navigation
can be suppressed on the WebKit side.

I think that instead of teaching src/net/ about HANDLED_EXTERNALLY more, we
should just rip that out entirely.  We should probably remove HANDLED_EXTERNALLY
from URLRequestStatus, and we should invent a new data structure for conveying
network response status information to the WebURLLoaderImpl.

Sorry to add more work, but I think it was always a mistake to have this
HANDLED_EXTERNALLY flag defined in the network stack.  It doesn't need to know
about this flag.

[mkosiba (inactive), 2012-05-30 14:03:11]

On 2012/05/29 23:39:32, darin wrote:
>
https://chromiumcodereview.appspot.com/10310124/diff/25002/net/url_request/ur...
> File net/url_request/url_request.h (right):
> 
>
https://chromiumcodereview.appspot.com/10310124/diff/25002/net/url_request/ur...
> net/url_request/url_request.h:541: void CancelWithHandledExternallyStatus();
> It looks like this may actually be moving in the wrong direction code-wise. 
The
> network stack really has no need to know about the HANDLED_EXTERNALLY bit.  It
> is only the WebURLLoaderImpl that needs to see this bit so that the navigation
> can be suppressed on the WebKit side.
> 
> I think that instead of teaching src/net/ about HANDLED_EXTERNALLY more, we
> should just rip that out entirely.  We should probably remove
HANDLED_EXTERNALLY
> from URLRequestStatus, and we should invent a new data structure for conveying
> network response status information to the WebURLLoaderImpl.
> 
> Sorry to add more work, but I think it was always a mistake to have this
> HANDLED_EXTERNALLY flag defined in the network stack.  It doesn't need to know
> about this flag.

I removed the CancelWithHandledExternallyStatus stuff and I'd like to submit
this change as it is now. I'd prefer do the changes you describe in a followup
change.

[darin (slow to review), 2012-05-31 21:13:39]

https://chromiumcodereview.appspot.com/10310124/diff/33001/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10310124/diff/33001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2734: bool
WebContentsImpl::ShouldIgnoreNavigation(
I'm trying to understand why the embedder of a WebContents would call a function
on WebContents that is just going to turn around and call a function back on the
embedder via WebContentsDelegate.  It seems like the embedder could just talk
directly to himself instead of going this round-about way.  The only difference
is that in this path FilterURL is called.  Perhaps you should just expose
FilterURL as a content API?

https://chromiumcodereview.appspot.com/10310124/diff/33001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2745:
render_view_host_impl->FilterURL(
nit: FilterURL is a static function so you don't need a RenderViewHostImpl
instance to call it!!

[mkosiba (inactive), 2012-06-01 20:35:11]

Uploaded new version.

https://chromiumcodereview.appspot.com/10310124/diff/33001/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10310124/diff/33001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2734: bool
WebContentsImpl::ShouldIgnoreNavigation(
On 2012/05/31 21:13:39, darin wrote:
> I'm trying to understand why the embedder of a WebContents would call a
function
> on WebContents that is just going to turn around and call a function back on
the
> embedder via WebContentsDelegate.  It seems like the embedder could just talk
> directly to himself instead of going this round-about way.  The only
difference
> is that in this path FilterURL is called.  Perhaps you should just expose
> FilterURL as a content API?

Done. The main reason for this code is the FilterURL call. In the previous
version of my patch the 'entry point' for this code was on the
RenderViewHostDelegate, but that got recently moved out of public/browser so I
made the simplest fix to have it work again.

https://chromiumcodereview.appspot.com/10310124/diff/33001/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:2745:
render_view_host_impl->FilterURL(
On 2012/05/31 21:13:39, darin wrote:
> nit: FilterURL is a static function so you don't need a RenderViewHostImpl
> instance to call it!!

Done.

[darin (slow to review), 2012-06-01 20:57:41]

https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
File content/public/browser/render_view_host.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
content/public/browser/render_view_host.h:67: static void
FilterURL(ChildProcessSecurityPolicy* policy,
it seems like you shouldn't bother with the policy argument here.  FilterURL can
just call ChildProcessSecurityPolicyImpl::GetInstance().

then you would not need to expose the CanRequestURL method on the public
ChildProcessSecurityPolicy interface.

https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
content/public/browser/web_contents_delegate.h:84: WebContents* source,
It seems like this function is no longer necessary as src/content/ doesn't call
it.

[Greg Billock, 2012-06-04 17:20:14]

On 2012/06/01 20:57:41, darin wrote:
>
https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
> File content/public/browser/render_view_host.h (right):
> 
>
https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
> content/public/browser/render_view_host.h:67: static void
> FilterURL(ChildProcessSecurityPolicy* policy,
> it seems like you shouldn't bother with the policy argument here.  FilterURL
can
> just call ChildProcessSecurityPolicyImpl::GetInstance().
> 
> then you would not need to expose the CanRequestURL method on the public
> ChildProcessSecurityPolicy interface.
> 
>
https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
> File content/public/browser/web_contents_delegate.h (right):
> 
>
https://chromiumcodereview.appspot.com/10310124/diff/35001/content/public/bro...
> content/public/browser/web_contents_delegate.h:84: WebContents* source,
> It seems like this function is no longer necessary as src/content/ doesn't
call
> it.

This change looks good as an access point for future web intents integration. No
objections here.

[mkosiba (inactive), 2012-06-06 16:11:19]

New version ready for review. The big difference is that the throttle is now
under src/content/. 
I'm surprised by the link errors on the chromeos try bots - could that have
anything to do with CONTENT_EXPORT?

http://codereview.chromium.org/10310124/diff/35001/content/public/browser/ren...
File content/public/browser/render_view_host.h (right):

http://codereview.chromium.org/10310124/diff/35001/content/public/browser/ren...
content/public/browser/render_view_host.h:67: static void
FilterURL(ChildProcessSecurityPolicy* policy,
On 2012/06/01 20:57:41, darin wrote:
> it seems like you shouldn't bother with the policy argument here.  FilterURL
can
> just call ChildProcessSecurityPolicyImpl::GetInstance().
> 
> then you would not need to expose the CanRequestURL method on the public
> ChildProcessSecurityPolicy interface.

Done.

http://codereview.chromium.org/10310124/diff/35001/content/public/browser/web...
File content/public/browser/web_contents_delegate.h (right):

http://codereview.chromium.org/10310124/diff/35001/content/public/browser/web...
content/public/browser/web_contents_delegate.h:84: WebContents* source,
On 2012/06/01 20:57:41, darin wrote:
> It seems like this function is no longer necessary as src/content/ doesn't
call
> it.

For Chrome on Android it makes sense for the callback to be on the
WebContentsDelegate. Let me move the throttle to src/content/ ...

[joth, 2012-06-13 22:32:49]

LGTM

It looks like the overall structure is now in line with the discussion.
Who is this waiting on for final approval?

(I have a couple small nits/suggestions but mostly want to get this unblocked so
can address those in a follow up if we can move ahead on this sooner)

http://codereview.chromium.org/10310124/diff/57003/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10310124/diff/57003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:46: //
TODO(oshima): Enable this for other platforms.
nit: either move this TODO inside the #if, or split out the two sections to be
independent #if blocks.

http://codereview.chromium.org/10310124/diff/57003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:143:
#elif defined(OS_ANDROID)
again would be tempted to do these as unconnected #if , unless there's a logical
reason we could never have both throttles together?

http://codereview.chromium.org/10310124/diff/57003/content/browser/renderer_h...
File
content/browser/renderer_host/intercept_navigation_resource_throttle_impl.cc
(right):

http://codereview.chromium.org/10310124/diff/57003/content/browser/renderer_h...
content/browser/renderer_host/intercept_navigation_resource_throttle_impl.cc:124:
return true;
maybe:

// Defer request, while we wait for the UI thread to determine final answer.

http://codereview.chromium.org/10310124/diff/57003/content/browser/renderer_h...
content/browser/renderer_host/intercept_navigation_resource_throttle_impl.cc:132:
// TODO: Cancel() results in a CANCELLED status but what we really want is
TODO(mkosiba)

http://codereview.chromium.org/10310124/diff/57003/content/public/browser/ren...
File content/public/browser/render_view_host.h (right):

http://codereview.chromium.org/10310124/diff/57003/content/public/browser/ren...
content/public/browser/render_view_host.h:66: // empty_allowed must be set to
false for navigations for security reasons.
nit: |empty_allowed|

[darin (slow to review), 2012-06-13 23:26:44]

On 2012/06/06 16:11:19, Martin Kosiba wrote:
> New version ready for review. The big difference is that the throttle is now
> under src/content/. 

<snip>

http://codereview.chromium.org/10310124/diff/35001/content/public/browser/web...
> content/public/browser/web_contents_delegate.h:84: WebContents* source,
> On 2012/06/01 20:57:41, darin wrote:
> > It seems like this function is no longer necessary as src/content/ doesn't
> call
> > it.
> 
> For Chrome on Android it makes sense for the callback to be on the
> WebContentsDelegate. Let me move the throttle to src/content/ ...

I see.  I don't think that is the right solution at all.  You should
not be putting the throttle in src/content/.

From the chrome side you should be able to get to the implementation
of the WebContentsDelegate and just communicate directly with it from
the throttle.  You shouldn't need to proxy through the WebContentDelegate.

[mkosiba (inactive), 2012-06-15 11:05:09]

On 2012/06/13 23:26:44, darin wrote:
> On 2012/06/06 16:11:19, Martin Kosiba wrote:
> > New version ready for review. The big difference is that the throttle is now
> > under src/content/. 
> 
> <snip>
> 
>
http://codereview.chromium.org/10310124/diff/35001/content/public/browser/web...
> > content/public/browser/web_contents_delegate.h:84: WebContents* source,
> > On 2012/06/01 20:57:41, darin wrote:
> > > It seems like this function is no longer necessary as src/content/ doesn't
> > call
> > > it.
> > 
> > For Chrome on Android it makes sense for the callback to be on the
> > WebContentsDelegate. Let me move the throttle to src/content/ ...
> 
> I see.  I don't think that is the right solution at all.  You should
> not be putting the throttle in src/content/.
>

Ok... moved it back. That's sort of unfortunate since the long-term goal of the
WebView implementation is to sit on top of the content API. I guess we'll tackle
moving that back at a later time.

> From the chrome side you should be able to get to the implementation
> of the WebContentsDelegate and just communicate directly with it from
> the throttle.  You shouldn't need to proxy through the WebContentDelegate.

so the Android WebContentsDelegate impl isn't upstreamed yet, which makes
communicating with it directly somewhat of a pain. Anyway, the throttle now
takes a callback so it should be fine, right?

[mkosiba (inactive), 2012-06-15 11:05:36]

new version ready

http://codereview.chromium.org/10310124/diff/57003/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
(right):

http://codereview.chromium.org/10310124/diff/57003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:46: //
TODO(oshima): Enable this for other platforms.
On 2012/06/13 22:32:49, joth wrote:
> nit: either move this TODO inside the #if, or split out the two sections to be
> independent #if blocks.

Done.

http://codereview.chromium.org/10310124/diff/57003/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:143:
#elif defined(OS_ANDROID)
On 2012/06/13 22:32:49, joth wrote:
> again would be tempted to do these as unconnected #if , unless there's a
logical
> reason we could never have both throttles together?

No logical reason to not have the 2 throttles together. It does however make
sense to have an elif since OS_CHROMEOS and OS_ANDROID won't be declared at the
same time.

http://codereview.chromium.org/10310124/diff/57003/content/browser/renderer_h...
File
content/browser/renderer_host/intercept_navigation_resource_throttle_impl.cc
(right):

http://codereview.chromium.org/10310124/diff/57003/content/browser/renderer_h...
content/browser/renderer_host/intercept_navigation_resource_throttle_impl.cc:124:
return true;
On 2012/06/13 22:32:49, joth wrote:
> maybe:
> 
> // Defer request, while we wait for the UI thread to determine final answer.

Done.

http://codereview.chromium.org/10310124/diff/57003/content/browser/renderer_h...
content/browser/renderer_host/intercept_navigation_resource_throttle_impl.cc:132:
// TODO: Cancel() results in a CANCELLED status but what we really want is
On 2012/06/13 22:32:49, joth wrote:
> TODO(mkosiba)

Done.

http://codereview.chromium.org/10310124/diff/57003/content/public/browser/ren...
File content/public/browser/render_view_host.h (right):

http://codereview.chromium.org/10310124/diff/57003/content/public/browser/ren...
content/public/browser/render_view_host.h:66: // empty_allowed must be set to
false for navigations for security reasons.
On 2012/06/13 22:32:49, joth wrote:
> nit: |empty_allowed|

Done.

[darin (slow to review), 2012-06-15 19:52:55]

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:28: void
CheckShouldIgnoreNavigationOnUIThread(
nit: CheckIfShouldIgnoreNavigationOnUIThread

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:52:
web_contents, validated_url, referrer, is_content_initiated);
nit: indentation

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:68:
weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
nit: maybe just extend from SupportsWeakPtr so you can avoid this line?

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:85: bool
InterceptNavigationResourceThrottle::ShouldDeferRequestForURL(
it seems like this function is not named that well.  it does a lot more than
just determine if the request should be deferred.  in fact, it is almost always
going to defer the request.  it'll only defer in really broken cases, where the
request has unknown info or no associated renderview.  that doesn't happen
unless the system is broken.

maybe CheckIfShouldIgnoreNavigation?

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:124: //
TODO(mkosiba): Cancel() results in a CANCELLED status but what we really
I've been thinking of providing an alternate Cancel(int error_code) that would
help here.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:18: class
WebContents;
usually code in renderer_host does not see WebContents.  at this level we
usually pass around RenderViewHost instead.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:26: //
This class allows the embedder (via WebContentsDelegate) to selectively
this comment seems wrong now.

[mkosiba (inactive), 2012-06-18 10:47:06]

Hi Darin,
  Thanks for the feedback! New version ready for review.

Cheers!

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:28: void
CheckShouldIgnoreNavigationOnUIThread(
On 2012/06/15 19:52:55, darin wrote:
> nit: CheckIfShouldIgnoreNavigationOnUIThread

Done.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:52:
web_contents, validated_url, referrer, is_content_initiated);
On 2012/06/15 19:52:55, darin wrote:
> nit: indentation

Done.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:68:
weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
On 2012/06/15 19:52:55, darin wrote:
> nit: maybe just extend from SupportsWeakPtr so you can avoid this line?

Extending from SupportsWeakPtr makes the WeakPtr support public. Since the
WeakPtrFactory is an implementation detail I'd like to keep it hidden.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:85: bool
InterceptNavigationResourceThrottle::ShouldDeferRequestForURL(
On 2012/06/15 19:52:55, darin wrote:
> it seems like this function is not named that well.  it does a lot more than
> just determine if the request should be deferred.  in fact, it is almost
always
> going to defer the request.  it'll only defer in really broken cases, where
the
> request has unknown info or no associated renderview.  that doesn't happen
> unless the system is broken.
> 
> maybe CheckIfShouldIgnoreNavigation?

Done.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:124: //
TODO(mkosiba): Cancel() results in a CANCELLED status but what we really
On 2012/06/15 19:52:55, darin wrote:
> I've been thinking of providing an alternate Cancel(int error_code) that would
> help here.

I don't think that'd help unless you also include a status code. I need to set a
status of (HANDLED_EXTERNALLY, OK), rather than (CANCELED, error_code).

I'd really like to work on getting the right way of communicating the
HANDLED_EXTERNALLY status in a followup CL if that's OK with you.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:18: class
WebContents;
On 2012/06/15 19:52:55, darin wrote:
> usually code in renderer_host does not see WebContents.  at this level we
> usually pass around RenderViewHost instead.

Done.

http://codereview.chromium.org/10310124/diff/59001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:26: //
This class allows the embedder (via WebContentsDelegate) to selectively
On 2012/06/15 19:52:55, darin wrote:
> this comment seems wrong now.

Done.

[darin (slow to review), 2012-06-19 17:28:08]

On 2012/06/18 10:47:06, Martin Kosiba wrote:
...
> chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:68:
> weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
> On 2012/06/15 19:52:55, darin wrote:
> > nit: maybe just extend from SupportsWeakPtr so you can avoid this line?
> 
> Extending from SupportsWeakPtr makes the WeakPtr support public. Since the
> WeakPtrFactory is an implementation detail I'd like to keep it hidden.

That's correct, but no one uses an InterceptNavigationResourceThrottle
except to create it.  You really don't have to be that worried about
hiding the weak pointer details.  Anyways, no need to make the change.


> chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:124: //
> TODO(mkosiba): Cancel() results in a CANCELLED status but what we really
> On 2012/06/15 19:52:55, darin wrote:
> > I've been thinking of providing an alternate Cancel(int error_code) that
would
> > help here.
> 
> I don't think that'd help unless you also include a status code. I need to set
a
> status of (HANDLED_EXTERNALLY, OK), rather than (CANCELED, error_code).
> 
> I'd really like to work on getting the right way of communicating the
> HANDLED_EXTERNALLY status in a followup CL if that's OK with you.

Yes, doesn't have to be part of this CL.  Note:  I think we want to
nuke HANDLED_EXTERNALLY, and instead invent an error code that means
something like this.  Notice that HandleExternalProtocol, which is
very similar to URL interception, works by sending down an error code
of net::ERR_UNKNOWN_URL_SCHEME.  That error code is not really
semantically correct, but you could generate the same error code in
your code, and things would work properly.

[darin (slow to review), 2012-06-19 17:33:02]

LGTM

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:90: if
(!info->GetAssociatedRenderView(&render_process_id, &render_view_id)) {
nit: be consistent about whether or not to put brackets around single line
statements.  above you do not, yet here you do.

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:20: } //
namespace content
nit: two spaces before inline comment

nit: don't both commenting on the closing line of a namespace when the contents
only span a couple lines like this.

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:34:
CheckOnUIThreadCallback;
nit: add a new line below here to visibly separate the typedef from the
constructor

[mkosiba (inactive), 2012-06-20 10:16:08]

Nits fixed. Thanks for the review everyone!

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc
(right):

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.cc:90: if
(!info->GetAssociatedRenderView(&render_process_id, &render_view_id)) {
On 2012/06/19 17:33:02, darin wrote:
> nit: be consistent about whether or not to put brackets around single line
> statements.  above you do not, yet here you do.

Done.

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/intercept_navigation_resource_throttle.h
(right):

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:20: } //
namespace content
On 2012/06/19 17:33:02, darin wrote:
> nit: two spaces before inline comment
> 
> nit: don't both commenting on the closing line of a namespace when the
contents
> only span a couple lines like this.

Done.

http://codereview.chromium.org/10310124/diff/65002/chrome/browser/renderer_ho...
chrome/browser/renderer_host/intercept_navigation_resource_throttle.h:34:
CheckOnUIThreadCallback;
On 2012/06/19 17:33:02, darin wrote:
> nit: add a new line below here to visibly separate the typedef from the
> constructor

Done.

[commit-bot: I haz the power, 2012-06-20 10:16:37]

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

[commit-bot: I haz the power, 2012-06-20 10:42:52]

Try job failure for 10310124-56007 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-20 10:54:18]

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

[commit-bot: I haz the power, 2012-06-20 14:03:56]

Change committed as 143167