Also register read cookies in the content settings delegate.

Also register read cookies in the content settings delegate.

BUG=63663
TEST=unit tests

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

[jochen (gone - plz use gerrit), 2010-11-24 14:12:32]

please review.

The change of CookieMonster::CurrentTime to StrictlyIncreasingTime is because
some X11 header #defines CurrentTime as 0L

[darin (slow to review), 2010-11-24 18:10:47]

http://codereview.chromium.org/5318002/diff/1/net/base/cookie_monster.h
File net/base/cookie_monster.h (left):

http://codereview.chromium.org/5318002/diff/1/net/base/cookie_monster.h#oldco...
net/base/cookie_monster.h:446: base::Time CurrentTime();
we generally try to avoid including X11 headers instead of morphing our code to
workaround it.  x11_util.h provides insulation from the X11 headers.  perhaps
you can identify the browser code that is including X11 headers directly and
replace it with some indirection through x11_util.h.

http://codereview.chromium.org/5318002/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/5318002/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:447:
request_->context()->cookie_store()->GetCookieMonster()->
this is breaking the abstraction of the CookieStore interface.  there may not be
a CookieMonster behind it.

i'm concerned about the impact this will have on ChromeFrame, which uses a
CookieStore implemented on top of the host browser's network stack.

[jochen (gone - plz use gerrit), 2010-11-24 18:45:37]

http://codereview.chromium.org/5318002/diff/1/net/base/cookie_monster.h
File net/base/cookie_monster.h (left):

http://codereview.chromium.org/5318002/diff/1/net/base/cookie_monster.h#oldco...
net/base/cookie_monster.h:446: base::Time CurrentTime();
On 2010/11/24 18:10:47, darin wrote:
> we generally try to avoid including X11 headers instead of morphing our code
to
> workaround it.  x11_util.h provides insulation from the X11 headers.  perhaps
> you can identify the browser code that is including X11 headers directly and
> replace it with some indirection through x11_util.h.

the headers get included from chrome/browser/gtk/gtk_util.cc via gtk headers. I
could e.g. move the #includes for the gtk headers after the includes of chromium
headers

http://codereview.chromium.org/5318002/diff/1/net/url_request/url_request_htt...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/5318002/diff/1/net/url_request/url_request_htt...
net/url_request/url_request_http_job.cc:447:
request_->context()->cookie_store()->GetCookieMonster()->
On 2010/11/24 18:10:47, darin wrote:
> this is breaking the abstraction of the CookieStore interface.  there may not
be
> a CookieMonster behind it.
> 
> i'm concerned about the impact this will have on ChromeFrame, which uses a
> CookieStore implemented on top of the host browser's network stack.
Since the cookies aren't collected for chrome frame, I could only pass the url
and the request. Then the resource dispatcher host can check whether the context
is not external and grab the cookie monster to get the cookies.

wdyt

[jochen (gone - plz use gerrit), 2010-11-25 09:29:50]

updated patch with different approach

please review

[darin (slow to review), 2010-11-30 18:24:11]

OK, this change makes sense...

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host_delegate.h:394: // Called when
cookies for given URL where read either from within the
nit: where -> were

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1116:
&RenderViewHostDelegate::ContentSettings::OnCookieAccessed,
it seems like OnCookieAccessed should be renamed to better reflect the fact that
it is about "mutation" of a cookie whereas OnCookiesRead is about cookies being
read.  OnCookieChanged is probably better than OnCookieAccessed.

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_message_filter.cc:1798: if
(!context_->IsExternal()) {
Do you care about the case where ResourceMessageFilter::OnGetRawCookies is the
one who constructed the GetCookiesCompletion object?  Should you filter that one
out?  OnGetRawCookies is only used by the inspector IIRC.

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_specific_content_settings.cc (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_specific_content_settings.cc:104:
cookie_monster->GetAllCookiesForURLWithOptions(url, options);
nit: indent by 4 spaces

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_specific_content_settings.cc:104:
cookie_monster->GetAllCookiesForURLWithOptions(url, options);
it seems like the cookies that you fetch here could be different than the ones
that were read.  something else could have happened on the IO thread that could
have mutated the cookie monster.  perhaps you should collect the cookies on the
IO thread to avoid this race condition.

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

http://codereview.chromium.org/5318002/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:447: if (policy ==
net::ERR_ACCESS_DENIED &&
style-nit: it seems like an outer if (request_->context()->cookie_store()) { ...
} wrapper would be better since both branches have the same condition.

http://codereview.chromium.org/5318002/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:475: if (policy ==
net::ERR_ACCESS_DENIED) {
why isn't OnSetCookie similarly disallowed when the cookie_store is null?

[jochen (gone - plz use gerrit), 2010-12-03 16:02:32]

I refactored the CookieMonster::CookieList out of CookieMonster, so I can now
forward declare it and don't get collisions with XLib.h

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_view_host_delegate.h:394: // Called when
cookies for given URL where read either from within the
On 2010/11/30 18:24:11, darin wrote:
> nit: where -> were

Done.

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_dispatcher_host.cc:1116:
&RenderViewHostDelegate::ContentSettings::OnCookieAccessed,
On 2010/11/30 18:24:11, darin wrote:
> it seems like OnCookieAccessed should be renamed to better reflect the fact
that
> it is about "mutation" of a cookie whereas OnCookiesRead is about cookies
being
> read.  OnCookieChanged is probably better than OnCookieAccessed.

Done.

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/renderer_host...
chrome/browser/renderer_host/resource_message_filter.cc:1798: if
(!context_->IsExternal()) {
On 2010/11/30 18:24:11, darin wrote:
> Do you care about the case where ResourceMessageFilter::OnGetRawCookies is the
> one who constructed the GetCookiesCompletion object?  Should you filter that
one
> out?  OnGetRawCookies is only used by the inspector IIRC.

this block is inside an if (!raw_cookies_) (see above)

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/tab_specific_content_settings.cc (right):

http://codereview.chromium.org/5318002/diff/6001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/tab_specific_content_settings.cc:104:
cookie_monster->GetAllCookiesForURLWithOptions(url, options);
On 2010/11/30 18:24:11, darin wrote:
> it seems like the cookies that you fetch here could be different than the ones
> that were read.  something else could have happened on the IO thread that
could
> have mutated the cookie monster.  perhaps you should collect the cookies on
the
> IO thread to avoid this race condition.

Done.

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

http://codereview.chromium.org/5318002/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:447: if (policy ==
net::ERR_ACCESS_DENIED &&
On 2010/11/30 18:24:11, darin wrote:
> style-nit: it seems like an outer if (request_->context()->cookie_store()) {
...
> } wrapper would be better since both branches have the same condition.

Done.

http://codereview.chromium.org/5318002/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:475: if (policy ==
net::ERR_ACCESS_DENIED) {
On 2010/11/30 18:24:11, darin wrote:
> why isn't OnSetCookie similarly disallowed when the cookie_store is null?

Done.

[darin (slow to review), 2010-12-06 22:55:03]

LGTM

http://codereview.chromium.org/5318002/diff/13001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/5318002/diff/13001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host_delegate.h:399: // Called when
cookies for given URL were read either from within the
nit: "for given URL" -> "for the given URL"