More progress towards removing content settings code from the content layer. We can't use Cookie...

More progress towards removing content settings code from the content layer.  We can't use CookiePolicy anymore since, being in the network layer, we can't give it render_process_id/render_view_id, which are needed to put up the content settings UI.  Instead of the networking code calling CookiePolicy then calling the delegate (ResourceDispatcherHost) to inform it if something is blocked, we directly ask the delegate if something is allowed.  ResourceDispatcherHost then calls the embedder, and passes along the IDs to identify the tab.  In the next change, I'll use this mechansim in RenderMessageFilter and remove the CookiePolicy class.

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

[willchan no longer on Chromium, 2011-05-10 18:36:56]

I'm just about done with the review but have to run off.

The thing I need to mull over after lunch is whether or not the new methods
belong in URLRequest::Delegate as opposed to NetworkDelegate. I'm not sure if
it's a big deal. But I should think it over.

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
chrome/browser/chrome_content_browser_client.cc:236: net::CookieOptions*
options,
Nit: maybe this should be last? It looks like everything else is an input param,
whereas this is the only one that's modified. Would be more consistent with
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param....

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
File chrome/browser/chrome_content_browser_client.h (right):

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
chrome/browser/chrome_content_browser_client.h:36: int render_view_id);
Nit: I leave it up to you, but I think the recommended practice nowadays may be
to add a OVERRIDE at the end of these declarations, so that no one subclasses
this and re-overrides the member function. OVERRIDE is defined in
base/compiler_specific.h.

http://codereview.chromium.org/6995013/diff/66/chrome/browser/extensions/user...
File chrome/browser/extensions/user_script_listener_unittest.cc (right):

http://codereview.chromium.org/6995013/diff/66/chrome/browser/extensions/user...
chrome/browser/extensions/user_script_listener_unittest.cc:93: request_id,
ResourceType::MAIN_FRAME, 0, false, false, false, *context);
Haha, this looks weird :P Maybe you should use the MockResourceContext? It's a
singleton. Up to you.

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
File content/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/render_message_filter.h:249: const
content::ResourceContext& resource_context_;
Nit: eroman doesn't like storing references as member vars. I didn't care, but
he does, so we should probably be consistent and not use them.

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
File content/browser/renderer_host/resource_dispatcher_host_request_info.h
(right):

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/resource_dispatcher_host_request_info.h:158: const
content::ResourceContext& context() { return context_; }
Nit: make this const? up to you.

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/resource_dispatcher_host_request_info.h:213: const
content::ResourceContext& context_;
Again on the reference member vars.

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
File content/browser/renderer_host/resource_queue_unittest.cc (right):

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/resource_queue_unittest.cc:76: const
content::ResourceContext* context = NULL;
I recommend MockResourceContext again, but up to you.

[jam, 2011-05-10 19:27:22]

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
chrome/browser/chrome_content_browser_client.cc:236: net::CookieOptions*
options,
On 2011/05/10 18:36:56, willchan wrote:
> Nit: maybe this should be last? It looks like everything else is an input
param,
> whereas this is the only one that's modified. Would be more consistent with
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Param....

Done.

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
File chrome/browser/chrome_content_browser_client.h (right):

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
chrome/browser/chrome_content_browser_client.h:36: int render_view_id);
On 2011/05/10 18:36:56, willchan wrote:
> Nit: I leave it up to you, but I think the recommended practice nowadays may
be
> to add a OVERRIDE at the end of these declarations, so that no one subclasses
> this and re-overrides the member function.

I thought the point is to guard against the base class' function changes, and
people forgetting to update derived classes.  I've grown to see the benefit of
it recently, but I find in these embedding APIs, where there's only one
implementation of the interface, it's not necessary since there's only one
place.  If someone changes ContentBrowserClient, they surely are doing it for
ChroemContentBrowserClient.

> OVERRIDE is defined in base/compiler_specific.h.

http://codereview.chromium.org/6995013/diff/66/chrome/browser/extensions/user...
File chrome/browser/extensions/user_script_listener_unittest.cc (right):

http://codereview.chromium.org/6995013/diff/66/chrome/browser/extensions/user...
chrome/browser/extensions/user_script_listener_unittest.cc:93: request_id,
ResourceType::MAIN_FRAME, 0, false, false, false, *context);
On 2011/05/10 18:36:56, willchan wrote:
> Haha, this looks weird :P Maybe you should use the MockResourceContext? It's a
> singleton. Up to you.

Done (didn't know about it!)

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
File content/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/render_message_filter.h:249: const
content::ResourceContext& resource_context_;
On 2011/05/10 18:36:56, willchan wrote:
> Nit: eroman doesn't like storing references as member vars. I didn't care, but
> he does, so we should probably be consistent and not use them.

Done.  I don't feel strongly either way; I didn't know there was a convention.

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
File content/browser/renderer_host/resource_dispatcher_host_request_info.h
(right):

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/resource_dispatcher_host_request_info.h:158: const
content::ResourceContext& context() { return context_; }
On 2011/05/10 18:36:56, willchan wrote:
> Nit: make this const? up to you.

Done.

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/resource_dispatcher_host_request_info.h:213: const
content::ResourceContext& context_;
On 2011/05/10 18:36:56, willchan wrote:
> Again on the reference member vars.

Done.

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
File content/browser/renderer_host/resource_queue_unittest.cc (right):

http://codereview.chromium.org/6995013/diff/66/content/browser/renderer_host/...
content/browser/renderer_host/resource_queue_unittest.cc:76: const
content::ResourceContext* context = NULL;
On 2011/05/10 18:36:56, willchan wrote:
> I recommend MockResourceContext again, but up to you.

Done.

[willchan no longer on Chromium, 2011-05-10 22:39:22]

After thinking it over some more, I think I'm fine with the methods being in
URLRequest::Delegate.

LGTM.

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
File chrome/browser/chrome_content_browser_client.h (right):

http://codereview.chromium.org/6995013/diff/66/chrome/browser/chrome_content_...
chrome/browser/chrome_content_browser_client.h:36: int render_view_id);
That's a perfectly valid opinion and I think it makes sense.

On 2011/05/10 19:27:22, John Abd-El-Malek wrote:
> On 2011/05/10 18:36:56, willchan wrote:
> > Nit: I leave it up to you, but I think the recommended practice nowadays may
> be
> > to add a OVERRIDE at the end of these declarations, so that no one
subclasses
> > this and re-overrides the member function.
> 
> I thought the point is to guard against the base class' function changes, and
> people forgetting to update derived classes.  I've grown to see the benefit of
> it recently, but I find in these embedding APIs, where there's only one
> implementation of the interface, it's not necessary since there's only one
> place.  If someone changes ContentBrowserClient, they surely are doing it for
> ChroemContentBrowserClient.
> 
> > OVERRIDE is defined in base/compiler_specific.h.
>