Do cookie checks in NetworkDelegate instead of the URLRequest::Delegate.

Do cookie checks in NetworkDelegate instead of the URLRequest::Delegate.

As a side effect, requests going through URLFetcher now also have to pass cookie checks. Requests from the URLFetcher can be optionally annotated with a render view, so the user can be notified about blocked cookies. This plumbing is done for the alternate nav url fetcher, and the template url fetcher.

BUG=116322
TEST=manual/pyauto

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

[jochen (gone - plz use gerrit), 2012-03-01 16:05:10]

darin/chris: fyi

John/Will: please review the changes the URLFetcherImpl

I added content/public/common/content_url_request_user_data.*

Both ResourceDispatcherHost and URLFetcher stash such an object into every
URLRequest. URLFetcherImpl will refuse to work without it.

URLRequest will ask the NetworkDelegate what to do with cookies

ChromeNetworkDelegate retrieves the user data object, and applies content
settings:

- if no profile is present, cookies are allowed (system context)
- if a profile is present, and the ContentURLRequestUserData contains a valid
(process id, routing id), we apply content settings
- otherwise, we currently allow cookies

In a next step, I'll audit all call sites and include the right render view
where appropriate. Then we can block cookies in the NetworkDelegate if no render
view is specified.

The CL looks far bigger than it is, due to all the call sites updating required.

[michaeln, 2012-03-01 17:47:01]

http://codereview.chromium.org/9572001/diff/6001/webkit/appcache/appcache_ser...
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/9572001/diff/6001/webkit/appcache/appcache_ser...
webkit/appcache/appcache_service.h:112: // updates. Should only be accessed on
the UI thread.
please define this method on AppCachePolicy instead of on this class, and
include in it's signature an AppCacheUpdateJob* parameter (otherwise there is no
context with which to determine the values you want associated with the
request).

void AppCachePolicy::AddUserDataForUpdateRequest(job, req);

[jochen (gone - plz use gerrit), 2012-03-01 18:41:00]

http://codereview.chromium.org/9572001/diff/6001/webkit/appcache/appcache_ser...
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/9572001/diff/6001/webkit/appcache/appcache_ser...
webkit/appcache/appcache_service.h:112: // updates. Should only be accessed on
the UI thread.
On 2012/03/01 17:47:01, michaeln wrote:
> please define this method on AppCachePolicy instead of on this class, and
> include in it's signature an AppCacheUpdateJob* parameter (otherwise there is
no
> context with which to determine the values you want associated with the
> request).
> 
> void AppCachePolicy::AddUserDataForUpdateRequest(job, req);

Ideally, I want to attach the process_id/routing_id at this point. Can I derive
these values from the job somehow?

[jam, 2012-03-01 19:30:42]

I started reviewing this but after stepping back a bit I think this can be
simplified a bit by having:

-URLFetcher interface just has a method to set base::SupportsUserData::Data.
this is optional. this can be a struct declared in content\public\browser. if
it's not set, then ChromeNetworkDelegate will automatically disallow cookies.

http://codereview.chromium.org/9572001/diff/6001/content/public/common/conten...
File content/public/common/content_url_request_user_data.cc (right):

http://codereview.chromium.org/9572001/diff/6001/content/public/common/conten...
content/public/common/content_url_request_user_data.cc:5: #include
"content/public/common/content_url_request_user_data.h"
can we avoid this cc file? we try to avoid cc files in public directories unless
we absolutely need them.

http://codereview.chromium.org/9572001/diff/6001/content/public/common/conten...
File content/public/common/content_url_request_user_data.h (right):

http://codereview.chromium.org/9572001/diff/6001/content/public/common/conten...
content/public/common/content_url_request_user_data.h:16: class CONTENT_EXPORT
ContentURLRequestUserData
nit: we dont normally add a "Content" prefix to a class in content, since it's
already in the content namespace (content::ContentFoo is redundant)

http://codereview.chromium.org/9572001/diff/6001/content/public/common/url_fe...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/9572001/diff/6001/content/public/common/url_fe...
content/public/common/url_fetcher.h:163: content::ContentURLRequestUserData*
user_data) = 0;
nit: content:: not needed

why do you have two ways of passing this data (i.e. in
SetContentURLRequestUserData and StartWithRequestContextGetterAndUserData)? it
seems one is enough, and i think the first is the one to pick since not every
request will be associated with a render view


(after looking at this some more)
why even expose ContentURLRequestUserData to embedders? just make this method be
virtual void AssociateWithRenderView(int render_process_id, int render_view_id)
= 0;

and say it's optional

this makes

fetcher_->SetContentURLRequestUserData(
    new content::ContentURLRequestUserData(
        web_contents->GetRenderProcessHost()->GetID(), 
        web_contents->GetRenderViewHost()->routing_id()));

become


fetcher_->AssociateWithRenderView(
    web_contents->GetRenderProcessHost()->GetID(), 
    web_contents->GetRenderViewHost()->routing_id());

which is easier on the eyes :)

http://codereview.chromium.org/9572001/diff/6001/content/public/common/url_fe...
content/public/common/url_fetcher.h:203: virtual void
StartWithRequestContextGetterAndUserData(
nit: it seems redundant to put the parameter names in the function name.. maybe
just keep it the same name?

http://codereview.chromium.org/9572001/diff/6001/content/public/common/url_fe...
content/public/common/url_fetcher.h:205: content::ContentURLRequestUserData*
user_data) = 0;
nit: content:: is not necessary

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request....
net/url_request/url_request.cc:128: return false;
why?

[jochen (gone - plz use gerrit), 2012-03-01 19:39:10]

http://codereview.chromium.org/9572001/diff/6001/content/public/common/url_fe...
File content/public/common/url_fetcher.h (right):

http://codereview.chromium.org/9572001/diff/6001/content/public/common/url_fe...
content/public/common/url_fetcher.h:163: content::ContentURLRequestUserData*
user_data) = 0;
On 2012/03/01 19:30:42, John Abd-El-Malek wrote:
> nit: content:: not needed
> 
> why do you have two ways of passing this data (i.e. in
> SetContentURLRequestUserData and StartWithRequestContextGetterAndUserData)? it
> seems one is enough, and i think the first is the one to pick since not every
> request will be associated with a render view
> 
> 
> (after looking at this some more)
> why even expose ContentURLRequestUserData to embedders? just make this method
be
> virtual void AssociateWithRenderView(int render_process_id, int
render_view_id)
> = 0;
> 
> and say it's optional

I'll do that.

However, I'll still need to expose this to embedders, as the
ChromeNetworkDelegate reads the user data to do the content settings stuff


> 
> this makes
> 
> fetcher_->SetContentURLRequestUserData(
>     new content::ContentURLRequestUserData(
>         web_contents->GetRenderProcessHost()->GetID(), 
>         web_contents->GetRenderViewHost()->routing_id()));
> 
> become
> 
> 
> fetcher_->AssociateWithRenderView(
>     web_contents->GetRenderProcessHost()->GetID(), 
>     web_contents->GetRenderViewHost()->routing_id());
> 
> which is easier on the eyes :)

http://codereview.chromium.org/9572001/diff/6001/content/public/common/url_fe...
content/public/common/url_fetcher.h:203: virtual void
StartWithRequestContextGetterAndUserData(
On 2012/03/01 19:30:42, John Abd-El-Malek wrote:
> nit: it seems redundant to put the parameter names in the function name..
maybe
> just keep it the same name?

I think we could just get rid of this function entirely.

All it does is invoke SetRequestContext() and then Start()

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request....
net/url_request/url_request.cc:128: return false;
On 2012/03/01 19:30:42, John Abd-El-Malek wrote:
> why?

The default of URLRequest::CanSetCookies is false, the Delegate should have the
same default.

Anyway, now that NetworkDelegate does the cookie stuff, I think I can just
remove this method from URLRequest::Delegate

[michaeln, 2012-03-01 19:47:11]

> > void AppCachePolicy::AddUserDataForUpdateRequest(job, req);
> 
> Ideally, I want to attach the process_id/routing_id at this point. Can I
derive
> these values from the job somehow?

Not at the moment, webkit/appcache is far removed from these content layer
concepts.

There's a conceptual disconnect here. These particular request may be on behalf
of multiple <pid,rid> pairs (different tabs using the same cache). And to
implement 'appcache-auto-update', these particular request would not be
associated with any <pid,rid> pair. Nothing like that is implemented yet, but
it's a valid enhancement and something that according to the html5 draft is
permissible by a user-agent.

I'm not sure these details can be reconciled with your model that seems to
require that all requests are associated with exactly one <pid, rid> pair?

[willchan no longer on Chromium, 2012-03-01 20:30:09]

http://codereview.chromium.org/9572001/diff/6001/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/9572001/diff/6001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:20: #include
"net/base/cookie_monster.h"
What's this for?

http://codereview.chromium.org/9572001/diff/6001/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:197: // NULL during tests, or when
we're running in the system context.
I am not too fond of this. Does anyone else have an opinion here? I'd rather
have a SystemNetworkDelegate and a ChromeNetworkDelegate where they both compose
a shared object. It sort of annoys me that the system NetworkDelegate, which
should have no concept of a Profile, uses this class. I think long-term that the
behavioral difference between the system NetworkDelegate and the profile
specific NetworkDelegate will grow, and thus we shouldn't share
ChromeNetworkDelegate.

That said, this is the first major difference (since
ExtensionWebRequestEventRouter likewise assumes Profile*==NULL implies system
events), so I guess I'm fine with the current solution. But later on, I suspect
we'll need to refactor into two separate classes composing a shared
implementation class.

http://codereview.chromium.org/9572001/diff/6001/content/public/common/conten...
File content/public/common/content_url_request_user_data.h (right):

http://codereview.chromium.org/9572001/diff/6001/content/public/common/conten...
content/public/common/content_url_request_user_data.h:24: int process_id() const
{ return process_id_; }
Somewhere there should be documentation about these members being set to -1 if
they don't exist, right?

This sort of begs the question to me, why do we even have the 2nd constructor?
Why don't we make sure this is only ever set when it's valid? Is it just for
transition?

http://codereview.chromium.org/9572001/diff/6001/content/public/common/conten...
content/public/common/content_url_request_user_data.h:27: static const void*
kUserDataKey;
static const void* const

http://codereview.chromium.org/9572001/diff/6001/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/9572001/diff/6001/net/base/network_delegate.h#...
net/base/network_delegate.h:182: // Corresponds to
URLRequestDelegate::CanGetCookies
I think the OnFoo() naming scheme works great for void and int (net::Error)
responses, but not for bool. Please rename to CanGetCookies() and
CanSetCookie().

http://codereview.chromium.org/9572001/diff/6001/net/proxy/network_delegate_e...
File net/proxy/network_delegate_error_observer_unittest.cc (right):

http://codereview.chromium.org/9572001/diff/6001/net/proxy/network_delegate_e...
net/proxy/network_delegate_error_observer_unittest.cc:68: const CookieList&
cookie_list) {
OVERRIDE

http://codereview.chromium.org/9572001/diff/6001/net/proxy/network_delegate_e...
net/proxy/network_delegate_error_observer_unittest.cc:74: CookieOptions*
options) {
OVERRIDE

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request....
net/url_request/url_request.cc:128: return false;
On 2012/03/01 19:39:11, jochen wrote:
> On 2012/03/01 19:30:42, John Abd-El-Malek wrote:
> > why?
> 
> The default of URLRequest::CanSetCookies is false, the Delegate should have
the
> same default.
> 
> Anyway, now that NetworkDelegate does the cookie stuff, I think I can just
> remove this method from URLRequest::Delegate
> 

Please just remove this method. Having it in both places is confusing. The
Delegate shouldn't be in control. If it wanted explicit control to override the
NetworkDelegate, it should be using a net::LoadFlag.

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request_...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/9572001/diff/6001/net/url_request/url_request_...
net/url_request/url_request_unittest.cc:2586:
network_delegate.ResetCookieCounters();
I don't know that I like this as much. Can we just create a new
TestNetworkDelegate within the block scopes?

[willchan no longer on Chromium, 2012-03-01 21:13:55]

On 2012/03/01 19:47:11, michaeln wrote:
> > > void AppCachePolicy::AddUserDataForUpdateRequest(job, req);
> > 
> > Ideally, I want to attach the process_id/routing_id at this point. Can I
> derive
> > these values from the job somehow?
> 
> Not at the moment, webkit/appcache is far removed from these content layer
> concepts.
> 
> There's a conceptual disconnect here. These particular request may be on
behalf
> of multiple <pid,rid> pairs (different tabs using the same cache). And to
> implement 'appcache-auto-update', these particular request would not be
> associated with any <pid,rid> pair. Nothing like that is implemented yet, but
> it's a valid enhancement and something that according to the html5 draft is
> permissible by a user-agent.
> 
> I'm not sure these details can be reconciled with your model that seems to
> require that all requests are associated with exactly one <pid, rid> pair?

Fascinating.

It sounds to me like what jochen is doing for content/ should not exactly apply
to webkit/appcache/. Still, at some point, the information must be available
that ties a URLRequest to a set of tabs. This doesn't have to be stored in the
same way as content::URLRequestUserData <pid,rid> pairs, but the breadcrumbs
must exist somewhere, right? Conceptually, I would imagine that
ChromeNetworkDelegate would get a handle to whatever map or data structure we
can use to do the lookup, and webkit/appcache's URLRequests would stash the
breadcrumb somewhere in URLRequest's UserData.

[michaeln, 2012-03-02 01:51:57]

Jochen pinged me offline and we chatted some about this. I think (hope) he's
going to take another crack at this and separate the inputs needed to perform
the policy check (request url + first party url) from the inputs needed to prop
up some UI that let's the user know of the consequences of those policy
decisions.

Sure, those breadcrumbs definitely do exist, with a little work the appcache
system could produce a (possibly empty) list of <pid,rid> pairs associated with
each of its requests. It amounts to the set of AppCacheHosts (a per-document
thingy) that are currently associated with the cache being updated (that set
changes over time).

> It sounds to me like what jochen is doing for content/ should not exactly
apply
> to webkit/appcache/. Still, at some point, the information must be available
> that ties a URLRequest to a set of tabs. This doesn't have to be stored in the
> same way as content::URLRequestUserData <pid,rid> pairs, but the breadcrumbs
> must exist somewhere, right? Conceptually, I would imagine that
> ChromeNetworkDelegate would get a handle to whatever map or data structure we
> can use to do the lookup, and webkit/appcache's URLRequests would stash the
> breadcrumb somewhere in URLRequest's UserData.

[jam, 2012-03-02 15:53:40]

last iteration is much smaller and easier to review, thanks!

what about my comment #4? I think this can be simplified more if URLFetcher just
has a method to set SupportsUserData::UseData and a key. It can keep these until
the request is started, then it can set them on the URLRequest. The benefit is
that the browser specific url_request_user_data.* can remain in
content\public\browser instead of common.

[jochen (gone - plz use gerrit), 2012-03-02 15:58:37]

On 2012/03/02 15:53:40, John Abd-El-Malek wrote:
> last iteration is much smaller and easier to review, thanks!
> 
> what about my comment #4? I think this can be simplified more if URLFetcher
just
> has a method to set SupportsUserData::UseData and a key. It can keep these
until
> the request is started, then it can set them on the URLRequest. The benefit is
> that the browser specific url_request_user_data.* can remain in
> content\public\browser instead of common.

The URLFetcher creates a series of URLRequests, but the URLRequest takes
ownership of the UserData, so the URLFetcher needs to have a way to clone the
user data.

I also didn't get rid of the url_request_user_data.cc file, because I need to
define the user data key somewhere. (even when URLFetcher would take a
SupportsUserData::UserData pointer, we'd still need a key somewhere in the
content api)

[willchan no longer on Chromium, 2012-03-02 22:17:54]

I took a quick look and this seems more like what I had in mind. I only looked
at net/ and chrome/browser/net/ and chrome/browser/profiles/. I need to
doublecheck later to make sure that the defaults are as I would expect. But
otherwise I don't foresee having any other comments here.

[michaeln, 2012-03-03 18:59:36]

https://chromiumcodereview.appspot.com/9572001/diff/14009/chrome/browser/net/...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/9572001/diff/14009/chrome/browser/net/...
chrome/browser/net/chrome_network_delegate.cc:202: request->url(),
request->first_party_for_cookies());
Do we need to update appcache_update_job.cc to set the first_party_for_cookies
value to the 'manifest_url' of the cache being updated for these policy checks
to function properly?

[jochen (gone - plz use gerrit), 2012-03-05 08:17:36]

https://chromiumcodereview.appspot.com/9572001/diff/14009/chrome/browser/net/...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/9572001/diff/14009/chrome/browser/net/...
chrome/browser/net/chrome_network_delegate.cc:202: request->url(),
request->first_party_for_cookies());
On 2012/03/03 18:59:37, michaeln wrote:
> Do we need to update appcache_update_job.cc to set the first_party_for_cookies
> value to the 'manifest_url' of the cache being updated for these policy checks
> to function properly?

All call-sites need to be updated, but I'd like to do this in a separate (per
callsite) CL

[cbentzel, 2012-03-05 12:56:11]

This seems fine. It opens the door for additional cleanup - removing the cookie
methods from ContentBrowserClient as well as load flag settings near
URLFetcher's.

The only question is what isn't covered by the change to ChromeNetworkDelegate.
It seems like the following won't go through this.

- Requests from the system context.

- Requests in non-Chrome processes (system processes).

- Are URLRequest::Delegate's that are not URLFetcher or ResourceDispatcherHost
covered?

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:1436: return
content::GetContentClient()->browser()->AllowGetCookie(
You should be able to remove AllowGetCookie/AllowSetCookie from
ContentBrowserClient and derived classes now.

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
File content/public/common/url_request_user_data.cc (right):

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
content/public/common/url_request_user_data.cc:16: const void*
URLRequestUserData::kUserDataKey =
Funky - most other cases I've seen use a NULL-terminated-string constant to act
as key, but this will work as well.

Do you actually need the static_cast here? I thought cast-to-void* is allowed
implicitly.

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
File content/public/common/url_request_user_data.h (right):

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
content/public/common/url_request_user_data.h:19: URLRequestUserData(int
process_id, int routing_id);
Nit: this is typically called child_id rather than process_id. The reason is
that process_id doesn't match the OS's pid, so there is concern about confusion.
Or at least that's how John Abd-El-Malek explained it to me.

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request...
net/url_request/url_request.cc:829: if (context_ &&
context_->network_delegate()) {
Would it make sense to DCHECK that LOAD_DO_NOT_SEND_COOKIES is not turned on for
this request?

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request...
net/url_request/url_request.cc:838: if (context_ &&
context_->network_delegate()) {
Similar DCHECK for LOAD_DO_NOT_SAVE_COOKIES

[willchan no longer on Chromium, 2012-03-05 18:31:54]

On Mon, Mar 5, 2012 at 4:56 AM, <cbentzel@chromium.org> wrote:

> This seems fine. It opens the door for additional cleanup - removing the
> cookie
> methods from ContentBrowserClient as well as load flag settings near
> URLFetcher's.
>
> The only question is what isn't covered by the change to
> ChromeNetworkDelegate.
> It seems like the following won't go through this.
>
> - Requests from the system context.
>

The system URLRequestContext uses the ChromeNetworkDelegate, although it
won't have a profile specific HostContentSettings (or whatever that's
called), so whatever we do as the default for ChromeNetworkDelegate will
apply to the system context.


> - Requests in non-Chrome processes (system processes).
>

Just for clarification here, it doesn't apply to the Chrome service process
either.


>
> - Are URLRequest::Delegate's that are not URLFetcher or
> ResourceDispatcherHost
> covered?
>

All URLRequests must use a URLRequestContext (except in tests).
ChromeNetworkDelegate is used by the profile's main request context, app
specific request contexts, the media request context, and the system
request context. It's not used by the PAC script fetching context, nor the
temporary request context used by ConnectionTester (net-internals#test).
Also note that the ServiceURLRequestContext is used by the service process
for Cloud Print and Chromoting. I guess I should probably do a complete
audit since it's not clear to me if the privacy team is aware of all the
possible contexts, although if they are comfortable doing it, I'm happy to
let them handle it.


>
>
> http://codereview.chromium.**org/9572001/diff/14009/**
>
content/browser/renderer_host/**resource_dispatcher_host.cc<http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_host/resource_dispatcher_host.cc>
> File content/browser/renderer_host/**resource_dispatcher_host.cc (left):
>
> http://codereview.chromium.**org/9572001/diff/14009/**
>
content/browser/renderer_host/**resource_dispatcher_host.cc#**oldcode1436<http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_host/resource_dispatcher_host.cc#oldcode1436>
> content/browser/renderer_host/**resource_dispatcher_host.cc:**1436: return
> content::GetContentClient()->**browser()->AllowGetCookie(
> You should be able to remove AllowGetCookie/AllowSetCookie from
> ContentBrowserClient and derived classes now.
>
> http://codereview.chromium.**org/9572001/diff/14009/**
>
content/public/common/url_**request_user_data.cc<http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_request_user_data.cc>
> File content/public/common/url_**request_user_data.cc (right):
>
> http://codereview.chromium.**org/9572001/diff/14009/**
>
content/public/common/url_**request_user_data.cc#newcode16<http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_request_user_data.cc#newcode16>
> content/public/common/url_**request_user_data.cc:16: const void*
> URLRequestUserData::**kUserDataKey =
> Funky - most other cases I've seen use a NULL-terminated-string constant
> to act as key, but this will work as well.
>
> Do you actually need the static_cast here? I thought cast-to-void* is
> allowed implicitly.
>
> http://codereview.chromium.**org/9572001/diff/14009/**
>
content/public/common/url_**request_user_data.h<http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_request_user_data.h>
> File content/public/common/url_**request_user_data.h (right):
>
> http://codereview.chromium.**org/9572001/diff/14009/**
>
content/public/common/url_**request_user_data.h#newcode19<http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_request_user_data.h#newcode19>
> content/public/common/url_**request_user_data.h:19: URLRequestUserData(int
> process_id, int routing_id);
> Nit: this is typically called child_id rather than process_id. The
> reason is that process_id doesn't match the OS's pid, so there is
> concern about confusion. Or at least that's how John Abd-El-Malek
> explained it to me.
>
> http://codereview.chromium.**org/9572001/diff/14009/net/**
>
url_request/url_request.cc<http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request.cc>
> File net/url_request/url_request.cc (right):
>
> http://codereview.chromium.**org/9572001/diff/14009/net/**
>
url_request/url_request.cc#**newcode829<http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request.cc#newcode829>
> net/url_request/url_request.**cc:829: if (context_ &&
> context_->network_delegate()) {
> Would it make sense to DCHECK that LOAD_DO_NOT_SEND_COOKIES is not
> turned on for this request?
>
> http://codereview.chromium.**org/9572001/diff/14009/net/**
>
url_request/url_request.cc#**newcode838<http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request.cc#newcode838>
> net/url_request/url_request.**cc:838: if (context_ &&
> context_->network_delegate()) {
> Similar DCHECK for LOAD_DO_NOT_SAVE_COOKIES
>
>
http://codereview.chromium.**org/9572001/<http://codereview.chromium.org/9572...
>

[jam, 2012-03-05 19:11:02]

On 2012/03/02 15:58:37, jochen wrote:
> On 2012/03/02 15:53:40, John Abd-El-Malek wrote:
> > last iteration is much smaller and easier to review, thanks!
> > 
> > what about my comment #4? I think this can be simplified more if URLFetcher
> just
> > has a method to set SupportsUserData::UseData and a key. It can keep these
> until
> > the request is started, then it can set them on the URLRequest. The benefit
is
> > that the browser specific url_request_user_data.* can remain in
> > content\public\browser instead of common.
> 
> The URLFetcher creates a series of URLRequests, but the URLRequest takes
> ownership of the UserData, so the URLFetcher needs to have a way to clone the
> user data.

ok. since the UserData is set by URLFetcherImpl, it would be nice to not have
embedders know this detail and know the key/struct to get the data out.

can we perhaps put a static method on the URLFetcher interface to return the
associated render_process_id/render_view_id from a URLRequest? then we can hide
url_request_user_data.h from embedders

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:1436: return
content::GetContentClient()->browser()->AllowGetCookie(
On 2012/03/05 12:56:12, cbentzel wrote:
> You should be able to remove AllowGetCookie/AllowSetCookie from
> ContentBrowserClient and derived classes now.

really? it would be great if we can remove them, but they're also called by
other places

http://codereview.chromium.org/9572001/diff/14009/content/common/net/url_fetc...
File content/common/net/url_fetcher_impl.cc (right):

http://codereview.chromium.org/9572001/diff/14009/content/common/net/url_fetc...
content/common/net/url_fetcher_impl.cc:251: int process_id_;                  
// pid/rid of the associated render view.
nit: please don't say "pid" as that's confusing, as process_id is not the pid of
the renderer, but the (always) unique id. i would call these render_process_id
and render_view_id and that'll be self-documenting and consistent with the rest
of the code.

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
File content/public/common/url_request_user_data.cc (right):

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
content/public/common/url_request_user_data.cc:5: #include
"content/public/common/url_request_user_data.h"
is this cc file needed at all? we try to avoid cc files in the public dir unless
necessary.

i know for sure the destructor can move to the header without warnings. i think
the constructor can as well because the members are PODs. we can then move the
static to url_fetcher_impl.cc

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
File content/public/common/url_request_user_data.h (right):

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
content/public/common/url_request_user_data.h:19: URLRequestUserData(int
process_id, int routing_id);
On 2012/03/05 12:56:12, cbentzel wrote:
> Nit: this is typically called child_id rather than process_id. The reason is
> that process_id doesn't match the OS's pid, so there is concern about
confusion.
> Or at least that's how John Abd-El-Malek explained it to me.  

if this is only for renderer processes, then render_process_id/render_view_id is
best.

sometimes code is shared between renderer/workers, in that case child_id is more
correct.

[willchan no longer on Chromium, 2012-03-06 02:59:00]

The parts I own LGTM, I just sent some nits and what not.

https://chromiumcodereview.appspot.com/9572001/diff/6001/content/public/commo...
File content/public/common/content_url_request_user_data.h (right):

https://chromiumcodereview.appspot.com/9572001/diff/6001/content/public/commo...
content/public/common/content_url_request_user_data.h:27: static const void*
kUserDataKey;
On 2012/03/01 20:30:10, willchan wrote:
> static const void* const

?

https://chromiumcodereview.appspot.com/9572001/diff/14009/content/common/net/...
File content/common/net/url_fetcher_impl_unittest.cc (left):

https://chromiumcodereview.appspot.com/9572001/diff/14009/content/common/net/...
content/common/net/url_fetcher_impl_unittest.cc:575:
fetcher_->StartWithRequestContextGetter(
Why did this change? OIC, they're functionally equivalent. I coulda sworn when I
introduced this back in the day, they were different and this was
necessary...but I guess not. OK, seems fine.

https://chromiumcodereview.appspot.com/9572001/diff/14009/content/public/comm...
File content/public/common/url_request_user_data.h (right):

https://chromiumcodereview.appspot.com/9572001/diff/14009/content/public/comm...
content/public/common/url_request_user_data.h:24: int process_id;
How come these are public? Sorry if I'm nitting you on something another
reviewer commented on, but member variables typically should be private in
classes (this is not a struct). I think the other classes that use
SupportsUserData::Data keep member variables private.

https://chromiumcodereview.appspot.com/9572001/diff/14009/net/base/network_de...
File net/base/network_delegate.cc (right):

https://chromiumcodereview.appspot.com/9572001/diff/14009/net/base/network_de...
net/base/network_delegate.cc:96: DCHECK(CalledOnValidThread());
Add DCHECKs like in URLRequest to make sure the appropriate load flag is never
set.

https://chromiumcodereview.appspot.com/9572001/diff/14009/net/url_request/url...
File net/url_request/url_request.cc (right):

https://chromiumcodereview.appspot.com/9572001/diff/14009/net/url_request/url...
net/url_request/url_request.cc:829: if (context_ &&
context_->network_delegate()) {
On 2012/03/05 12:56:12, cbentzel wrote:
> Would it make sense to DCHECK that LOAD_DO_NOT_SEND_COOKIES is not turned on
for
> this request?

Agreed.

https://chromiumcodereview.appspot.com/9572001/diff/14009/net/url_request/url...
net/url_request/url_request.cc:838: if (context_ &&
context_->network_delegate()) {
On 2012/03/05 12:56:12, cbentzel wrote:
> Similar DCHECK for LOAD_DO_NOT_SAVE_COOKIES

Agreed.

https://chromiumcodereview.appspot.com/9572001/diff/14009/net/url_request/url...
File net/url_request/url_request_test_util.cc (right):

https://chromiumcodereview.appspot.com/9572001/diff/14009/net/url_request/url...
net/url_request/url_request_test_util.cc:498: 
1 too many lines

[jochen (gone - plz use gerrit), 2012-03-06 18:39:40]

On 2012/03/05 19:11:02, John Abd-El-Malek wrote:
> On 2012/03/02 15:58:37, jochen wrote:
> > On 2012/03/02 15:53:40, John Abd-El-Malek wrote:
> > > last iteration is much smaller and easier to review, thanks!
> > > 
> > > what about my comment #4? I think this can be simplified more if
URLFetcher
> > just
> > > has a method to set SupportsUserData::UseData and a key. It can keep these
> > until
> > > the request is started, then it can set them on the URLRequest. The
benefit
> is
> > > that the browser specific url_request_user_data.* can remain in
> > > content\public\browser instead of common.
> > 
> > The URLFetcher creates a series of URLRequests, but the URLRequest takes
> > ownership of the UserData, so the URLFetcher needs to have a way to clone
the
> > user data.
> 
> ok. since the UserData is set by URLFetcherImpl, it would be nice to not have
> embedders know this detail and know the key/struct to get the data out.
> 
> can we perhaps put a static method on the URLFetcher interface to return the
> associated render_process_id/render_view_id from a URLRequest? then we can
hide
> url_request_user_data.h from embedders
> 

The data is also set by resource_dispatcher_host, and might need to be set by
the AppCache system in the future, so hiding this in URLFetcher seems wrong.

I can change the interface so that the embedder doesn't need to know the key,
and how to retrieve the data from a URLRequest, something like static
URLRequestUserData* FromURLRequest(net::URLRequest* request); however, this
would still expose a new class to the embedder.

[jochen (gone - plz use gerrit), 2012-03-06 18:39:54]

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:1436: return
content::GetContentClient()->browser()->AllowGetCookie(
On 2012/03/05 19:11:03, John Abd-El-Malek wrote:
> On 2012/03/05 12:56:12, cbentzel wrote:
> > You should be able to remove AllowGetCookie/AllowSetCookie from
> > ContentBrowserClient and derived classes now.
> 
> really? it would be great if we can remove them, but they're also called by
> other places

Yes, the WebSocket implementation still uses it (no, WebSockets don't use the
NetworkDelegate nor URLRequest::Delegate :( )

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
File content/public/common/url_request_user_data.cc (right):

http://codereview.chromium.org/9572001/diff/14009/content/public/common/url_r...
content/public/common/url_request_user_data.cc:5: #include
"content/public/common/url_request_user_data.h"
On 2012/03/05 19:11:03, John Abd-El-Malek wrote:
> is this cc file needed at all? we try to avoid cc files in the public dir
unless
> necessary.
> 
> i know for sure the destructor can move to the header without warnings. i
think
> the constructor can as well because the members are PODs. we can then move the
> static to url_fetcher_impl.cc 

I need the .cc file for the static const, however, it doesn't need to be in
public. What about just moving it to content/common?

[willchan no longer on Chromium, 2012-03-06 18:42:29]

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (left):

http://codereview.chromium.org/9572001/diff/14009/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:1436: return
content::GetContentClient()->browser()->AllowGetCookie(
On 2012/03/06 18:39:54, jochen wrote:
> On 2012/03/05 19:11:03, John Abd-El-Malek wrote:
> > On 2012/03/05 12:56:12, cbentzel wrote:
> > > You should be able to remove AllowGetCookie/AllowSetCookie from
> > > ContentBrowserClient and derived classes now.
> > 
> > really? it would be great if we can remove them, but they're also called by
> > other places
> 
> Yes, the WebSocket implementation still uses it (no, WebSockets don't use the
> NetworkDelegate nor URLRequest::Delegate :( )

FWIW, we're merging the WebSocket implementation with the HTTP stack. It won't
use URLRequest::Delegate, but it's likely to use NetworkDelegate for the shared
portions.

[jam, 2012-03-07 02:01:49]

On 2012/03/06 18:39:40, jochen wrote:
> On 2012/03/05 19:11:02, John Abd-El-Malek wrote:
> > On 2012/03/02 15:58:37, jochen wrote:
> > > On 2012/03/02 15:53:40, John Abd-El-Malek wrote:
> > > > last iteration is much smaller and easier to review, thanks!
> > > > 
> > > > what about my comment #4? I think this can be simplified more if
> URLFetcher
> > > just
> > > > has a method to set SupportsUserData::UseData and a key. It can keep
these
> > > until
> > > > the request is started, then it can set them on the URLRequest. The
> benefit
> > is
> > > > that the browser specific url_request_user_data.* can remain in
> > > > content\public\browser instead of common.
> > > 
> > > The URLFetcher creates a series of URLRequests, but the URLRequest takes
> > > ownership of the UserData, so the URLFetcher needs to have a way to clone
> the
> > > user data.
> > 
> > ok. since the UserData is set by URLFetcherImpl, it would be nice to not
have
> > embedders know this detail and know the key/struct to get the data out.
> > 
> > can we perhaps put a static method on the URLFetcher interface to return the
> > associated render_process_id/render_view_id from a URLRequest? then we can
> hide
> > url_request_user_data.h from embedders
> > 
> 
> The data is also set by resource_dispatcher_host, and might need to be set by
> the AppCache system in the future, so hiding this in URLFetcher seems wrong.

so that's still being set by content, which means we don't need to expose 
URLRequestUserData (i.e. the implementation details of how we're storing this
data) to embedders. exposing it means that embedders can interfere with the
lifetime or change the data, when all we need is to just tell them the
render_proces_id/render_view_id

> 
> I can change the interface so that the embedder doesn't need to know the key,
> and how to retrieve the data from a URLRequest, something like static
> URLRequestUserData* FromURLRequest(net::URLRequest* request); however, this
> would still expose a new class to the embedder.

how about just a static function somewhere in content/public/browser that's
like:
void GetRenderViewForRequest(URLRequest* url_request, int* render_process_id,
int* render_view_id)? 

if you don't want to create a new file, then resource_request_info.h (added in
http://codereview.chromium.org/9580002/) can be a good place. you can say put in
the function comment that this is available for requests started from both RDH
and URLFetcher

[michaeln, 2012-03-07 23:32:29]

Just to clarify... once this is committed will it result in all requests
configured to sent/recv cookes that do not have a first_party_for_cookies()
value set to fail given the new cookie policy tests in ChromeNetworkDelegate.

I want to know because if the answer is "yes"... i'd like to update the appcache
created requests to set that value prior to this being committed so this CL
wouldn't be a breaking change.

> chrome/browser/net/chrome_network_delegate.cc:202: request->url(),
> request->first_party_for_cookies());
> On 2012/03/03 18:59:37, michaeln wrote:
> > Do we need to update appcache_update_job.cc to set the
first_party_for_cookies
> > value to the 'manifest_url' of the cache being updated for these policy
checks
> > to function properly?
> 
> All call-sites need to be updated, but I'd like to do this in a separate (per
> callsite) CL

[jochen (gone - plz use gerrit), 2012-03-08 12:58:23]

On 2012/03/07 02:01:49, John Abd-El-Malek wrote:
> how about just a static function somewhere in content/public/browser that's
> like:
> void GetRenderViewForRequest(URLRequest* url_request, int* render_process_id,
> int* render_view_id)? 
> 
> if you don't want to create a new file, then resource_request_info.h (added in
> http://codereview.chromium.org/9580002/) can be a good place. you can say put
in
> the function comment that this is available for requests started from both RDH
> and URLFetcher

Done

[jochen (gone - plz use gerrit), 2012-03-08 12:59:28]

On 2012/03/07 23:32:29, michaeln wrote:
> Just to clarify... once this is committed will it result in all requests
> configured to sent/recv cookes that do not have a first_party_for_cookies()
> value set to fail given the new cookie policy tests in ChromeNetworkDelegate.
> 
> I want to know because if the answer is "yes"... i'd like to update the
appcache
> created requests to set that value prior to this being committed so this CL
> wouldn't be a breaking change.

To make sure I break nothing, I added the correct first party to app caches, and
I updated the URLFetcher to always set the first party (if it's not specified, I
take the fetched URL as first party)

[jochen (gone - plz use gerrit), 2012-03-08 13:00:02]

http://codereview.chromium.org/9572001/diff/14009/chrome/browser/net/chrome_n...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/9572001/diff/14009/chrome/browser/net/chrome_n...
chrome/browser/net/chrome_network_delegate.cc:202: request->url(),
request->first_party_for_cookies());
On 2012/03/05 08:17:37, jochen wrote:
> On 2012/03/03 18:59:37, michaeln wrote:
> > Do we need to update appcache_update_job.cc to set the
first_party_for_cookies
> > value to the 'manifest_url' of the cache being updated for these policy
checks
> > to function properly?
> 
> All call-sites need to be updated, but I'd like to do this in a separate (per
> callsite) CL

I updated this callsite anyway, because it was straight forward

http://codereview.chromium.org/9572001/diff/14009/content/common/net/url_fetc...
File content/common/net/url_fetcher_impl.cc (right):

http://codereview.chromium.org/9572001/diff/14009/content/common/net/url_fetc...
content/common/net/url_fetcher_impl.cc:251: int process_id_;                  
// pid/rid of the associated render view.
On 2012/03/05 19:11:03, John Abd-El-Malek wrote:
> nit: please don't say "pid" as that's confusing, as process_id is not the pid
of
> the renderer, but the (always) unique id. i would call these render_process_id
> and render_view_id and that'll be self-documenting and consistent with the
rest
> of the code.

Done.

http://codereview.chromium.org/9572001/diff/14009/net/base/network_delegate.cc
File net/base/network_delegate.cc (right):

http://codereview.chromium.org/9572001/diff/14009/net/base/network_delegate.c...
net/base/network_delegate.cc:96: DCHECK(CalledOnValidThread());
On 2012/03/06 02:59:01, willchan wrote:
> Add DCHECKs like in URLRequest to make sure the appropriate load flag is never
> set.

Done.

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request...
net/url_request/url_request.cc:829: if (context_ &&
context_->network_delegate()) {
On 2012/03/06 02:59:01, willchan wrote:
> On 2012/03/05 12:56:12, cbentzel wrote:
> > Would it make sense to DCHECK that LOAD_DO_NOT_SEND_COOKIES is not turned on
> for
> > this request?
> 
> Agreed.

Done.

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request...
net/url_request/url_request.cc:838: if (context_ &&
context_->network_delegate()) {
On 2012/03/06 02:59:01, willchan wrote:
> On 2012/03/05 12:56:12, cbentzel wrote:
> > Similar DCHECK for LOAD_DO_NOT_SAVE_COOKIES
> 
> Agreed.

Done.

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request...
File net/url_request/url_request_test_util.cc (right):

http://codereview.chromium.org/9572001/diff/14009/net/url_request/url_request...
net/url_request/url_request_test_util.cc:498: 
On 2012/03/06 02:59:01, willchan wrote:
> 1 too many lines

Done.

[jam, 2012-03-08 16:30:57]

On 2012/03/08 12:58:23, jochen wrote:
> On 2012/03/07 02:01:49, John Abd-El-Malek wrote:
> > how about just a static function somewhere in content/public/browser that's
> > like:
> > void GetRenderViewForRequest(URLRequest* url_request, int*
render_process_id,
> > int* render_view_id)? 
> > 
> > if you don't want to create a new file, then resource_request_info.h (added
in
> > http://codereview.chromium.org/9580002/) can be a good place. you can say
put
> in
> > the function comment that this is available for requests started from both
RDH
> > and URLFetcher
> 
> Done

lgtm, thanks

[darin (slow to review), 2012-03-08 17:53:37]

LGTM for src/webkit/

[michaeln, 2012-03-08 20:12:57]

http://codereview.chromium.org/9572001/diff/31018/webkit/appcache/appcache_up...
File webkit/appcache/appcache_update_job.cc (right):

http://codereview.chromium.org/9572001/diff/31018/webkit/appcache/appcache_up...
webkit/appcache/appcache_update_job.cc:108:
request_->set_first_party_for_cookies(job_->manifest_url_);
thnx!

[Peter Kasting, 2012-03-08 20:46:27]

chrome/browser/search_engines LGTM

http://codereview.chromium.org/9572001/diff/31018/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_fetcher.h (right):

http://codereview.chromium.org/9572001/diff/31018/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher.h:43: content::WebContents*
web_contents,
Nit: Can you add a comment about what this argument means?  Also your
implementation comments that this can be NULL during tests, which implies that
it shouldn't be NULL in normal use; you might want to comment about that as
well.

http://codereview.chromium.org/9572001/diff/31018/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_fetcher_unittest.cc (right):

http://codereview.chromium.org/9572001/diff/31018/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_fetcher_unittest.cc:150: keyword,
osdd_url, favicon_url, NULL /* web_contents */,
Nit: No need for the /* */ comment since none of the other args have comments.

[commit-bot: I haz the power, 2012-03-09 10:54:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/9572001/41001