Allow the content browser client to specify a special cookie store to be

Allow the content browser client to specify a special cookie store to be
used for a given render process id. This special cookie store will then
be used for renderer messages pertaining to cookies, url fetches in net,
and websockets. If the special cookie store is NULL, a default cookie store
will be used.

R=erikwright@chromium.org, jam@chromium.org, lambroslambrou@chromium.org, mmenke@chromium.org, tyoshino@chromium.org

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

[tburkard, 2014-03-10 16:59:34]

Hi,

as I have discussed with all of you, here is the first draft preparing a delayed
commit of cookie changes for prerender.

It provides a framework for the content browser client to provide an alternate
cookie store for a given process id. This contains all the necessary changes in
content and net to make this work.

In later stages, I will implement a cookie store that can delay changes, and
make use of this by updating the browser to use it for perenders.

Erik, you are the only one on this CL with whom I haven't discussed a change in
course of the original design doc yet: Rather than having a CookieStoreProvider,
based on suggestion by jam and mmenke, we instead change URLRequest as well as
SocketStream for websockets, and keep the cookie store to be used there. This
will either be the special cookie store for prerender (provided during
construction by content coming from browser), or the default one from the
request context.

The advantage of this new approach is that we do not have all these new
interfaces for cookiestoreprovider in net and in content. This was highly
encouraged by jam, and mmenke from the networking side thinks this is a sound
overall proposal.

Looking forward to everyone's feedback and suggestions.

Thanks.

[mmenke, 2014-03-11 14:45:48]

I think this looks pretty reasonable.  I'll try to get to a full review later
today, but may not have time until tomorrow.

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
net/url_request/url_request.cc:1265: CookieStore* URLRequest::cookie_store()
const {
Definition order should match declaration order.

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
net/url_request/url_request.h:697: CookieStore* cookie_store() const;
I think this should be protected.  URLRequestJob is already a friend, you can
just add another protected function there for URLRequestHttpJob to get at it.

[jam, 2014-03-11 16:47:08]

lgtm with nit

https://codereview.chromium.org/188693003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/188693003/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:889: // prerendes. Since
prerenders will be cancelled on downloads automatically,
don't mention chrome specific features in content, even in comments :)

https://codereview.chromium.org/188693003/diff/140001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/188693003/diff/140001/content/public/browser/...
content/public/browser/content_browser_client.h:627: virtual net::CookieStore*
GetSpecialCookieStoreForRenderProcess(
nit: "special" doesn't mean much, so I'd drop that here and in the other code in
content. the convention we use in the contentclient api is "override", so call
this method OverrideCookieStoreForRenderProcess. also move up before hte ifdef
sections that go last (the last method was added not observing that)

[erikwright (departed), 2014-03-11 19:03:33]

This alternative seems fine. My impression is that the original plan would have
affected far fewer files, but I don't really mind.

LGTM.

[tyoshino (SeeGerritForStatus), 2014-03-12 11:17:10]

lgtm WebSocket

[tburkard, 2014-03-12 14:37:26]

Addressed all pending comments.

Matt, please let me know when you think this is ready to be committed.

Thanks.

https://codereview.chromium.org/188693003/diff/140001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/188693003/diff/140001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:889: // prerendes. Since
prerenders will be cancelled on downloads automatically,
On 2014/03/11 16:47:09, jam wrote:
> don't mention chrome specific features in content, even in comments :)

Done.

https://codereview.chromium.org/188693003/diff/140001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/188693003/diff/140001/content/public/browser/...
content/public/browser/content_browser_client.h:627: virtual net::CookieStore*
GetSpecialCookieStoreForRenderProcess(
On 2014/03/11 16:47:09, jam wrote:
> nit: "special" doesn't mean much, so I'd drop that here and in the other code
in
> content. the convention we use in the contentclient api is "override", so call
> this method OverrideCookieStoreForRenderProcess. also move up before hte ifdef
> sections that go last (the last method was added not observing that)

Done.

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
net/url_request/url_request.cc:1265: CookieStore* URLRequest::cookie_store()
const {
On 2014/03/11 14:45:49, mmenke wrote:
> Definition order should match declaration order.

Done.

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/188693003/diff/140001/net/url_request/url_req...
net/url_request/url_request.h:697: CookieStore* cookie_store() const;
On 2014/03/11 14:45:49, mmenke wrote:
> I think this should be protected.  URLRequestJob is already a friend, you can
> just add another protected function there for URLRequestHttpJob to get at it.

Done.

[mmenke, 2014-03-12 16:42:59]

I'm happy with the net/ changes (Looked at them all) and the browser/net
changes, just wondering about the content/browser/loader changes.

https://codereview.chromium.org/188693003/diff/180001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/188693003/diff/180001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1061:
GetContentClient()->browser()->OverrideCookieStoreForRenderProcess(
I think "OverrideCookieStoreForRenderProcess" isn't a good name for this -
function names should generally be verbs, and read as a verb phrase,
"OverrideCookieStoreForRenderProcess" is a bit misleading.

Maybe add a "Get" in front?

Also, from a prerender perspective, is this enough information to be sure it's a
prerender?  There's nothing to stop other render views from being created in a
process with a prerender in it.

https://codereview.chromium.org/188693003/diff/180001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/188693003/diff/180001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:115: ->CreateRequest(url,
net::DEFAULT_PRIORITY, this, NULL)),
This does seem right, but it's a way that prerenders can still have an effect on
the cookie store.  Have I mentioned how much of a bad idea I think doing all
this cookie stuff for prerender is?  :)

[tburkard, 2014-03-12 16:51:31]

Replied to Matt's last 2 comments without any changes.

Matt, please let me know if there is anything else or if things lgty now.

Thanks.

https://codereview.chromium.org/188693003/diff/180001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/188693003/diff/180001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_impl.cc:1061:
GetContentClient()->browser()->OverrideCookieStoreForRenderProcess(
On 2014/03/12 16:43:00, mmenke wrote:
> I think "OverrideCookieStoreForRenderProcess" isn't a good name for this -
> function names should generally be verbs, and read as a verb phrase,
> "OverrideCookieStoreForRenderProcess" is a bit misleading.
> 
> Maybe add a "Get" in front?
> 
> Also, from a prerender perspective, is this enough information to be sure it's
a
> prerender?  There's nothing to stop other render views from being created in a
> process with a prerender in it.

jam suggested this name.

Also, if you look at the .h file containing the definition, you can see eg
OverrideRequestContextForURL, which is analogous to this and would also need a
"Get" based on your reasoning.

The Get is implied by the return type.

https://codereview.chromium.org/188693003/diff/180001/webkit/browser/appcache...
File webkit/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/188693003/diff/180001/webkit/browser/appcache...
webkit/browser/appcache/appcache_update_job.cc:115: ->CreateRequest(url,
net::DEFAULT_PRIORITY, this, NULL)),
On 2014/03/12 16:43:00, mmenke wrote:
> This does seem right, but it's a way that prerenders can still have an effect
on
> the cookie store.  Have I mentioned how much of a bad idea I think doing all
> this cookie stuff for prerender is?  :)

Ack, nothing to do here.

[mmenke, 2014-03-12 16:54:47]

Ok, I'll defer to jam on the naming.  All directories I own LGTM:  net/,
browser/net, and browser/loader (jam's better than me for the last, anyways).

On 2014/03/12 16:51:31, tburkard wrote:
> Replied to Matt's last 2 comments without any changes.
> 
> Matt, please let me know if there is anything else or if things lgty now.
> 
> Thanks.
> 
>
https://codereview.chromium.org/188693003/diff/180001/content/browser/loader/...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/188693003/diff/180001/content/browser/loader/...
> content/browser/loader/resource_dispatcher_host_impl.cc:1061:
> GetContentClient()->browser()->OverrideCookieStoreForRenderProcess(
> On 2014/03/12 16:43:00, mmenke wrote:
> > I think "OverrideCookieStoreForRenderProcess" isn't a good name for this -
> > function names should generally be verbs, and read as a verb phrase,
> > "OverrideCookieStoreForRenderProcess" is a bit misleading.
> > 
> > Maybe add a "Get" in front?
> > 
> > Also, from a prerender perspective, is this enough information to be sure
it's
> a
> > prerender?  There's nothing to stop other render views from being created in
a
> > process with a prerender in it.
> 
> jam suggested this name.
> 
> Also, if you look at the .h file containing the definition, you can see eg
> OverrideRequestContextForURL, which is analogous to this and would also need a
> "Get" based on your reasoning.
> 
> The Get is implied by the return type.
> 
>
https://codereview.chromium.org/188693003/diff/180001/webkit/browser/appcache...
> File webkit/browser/appcache/appcache_update_job.cc (right):
> 
>
https://codereview.chromium.org/188693003/diff/180001/webkit/browser/appcache...
> webkit/browser/appcache/appcache_update_job.cc:115: ->CreateRequest(url,
> net::DEFAULT_PRIORITY, this, NULL)),
> On 2014/03/12 16:43:00, mmenke wrote:
> > This does seem right, but it's a way that prerenders can still have an
effect
> on
> > the cookie store.  Have I mentioned how much of a bad idea I think doing all
> > this cookie stuff for prerender is?  :)
> 
> Ack, nothing to do here.

[Lambros, 2014-03-12 17:25:32]

remoting/ lgtm

[michaeln, 2014-03-12 17:36:13]

lgtm

[tburkard, 2014-03-12 17:55:20]

Committed patchset #11 manually as r256579 (presubmit successful).

[Peter Beverloo, 2014-03-12 18:30:47]

FYI: I reverted this in https://codereview.chromium.org/197463003.

Example output:
 
http://build.chromium.org/p/chromium.linux/builders/Android%20Clang%20Builder...

The Android android_dbg and android_clang_dbg try-bots on this CL both point out
the compile errors in both their runs. If you choose to not use the commit
queue, please make sure that the try-bots are happy :-).