Query the network delegate for cookies even if there is no cookie monster

Query the network delegate for cookies even if there is no cookie monster

On iOS, there is a platform specific implementation of the cookie store which is not a CookieMonster.
However, the network delegate is currently only called if the cookie store is a CookieMonster, and thus cookie settings are bypassed.
This CL calls the network delegate even if the cookie store is not a CookieMonster.

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

[droger, 2012-11-23 16:00:12]

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:518:
CheckCookiePolicyAndLoad(CookieList());
I pass an empty list as 'default' argument here, as it's not possible to compute
the actual list using only the CookieStore API.

Note: browsing the code, it seems that no implementation of the NetworkDelegate
actually use the |cookie_list| argument. If it's true, maybe we could remove it?
If we do this, we could even remove the call to
cookie_monster->GetAllCookiesForURLAsync() (line 513 above) altogether!

[erikwright (departed), 2012-11-27 05:17:17]

LGTM.

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:518:
CheckCookiePolicyAndLoad(CookieList());
On 2012/11/23 16:00:12, droger wrote:
> I pass an empty list as 'default' argument here, as it's not possible to
compute
> the actual list using only the CookieStore API.
> 
> Note: browsing the code, it seems that no implementation of the
NetworkDelegate
> actually use the |cookie_list| argument. If it's true, maybe we could remove
it?
> If we do this, we could even remove the call to
> cookie_monster->GetAllCookiesForURLAsync() (line 513 above) altogether!

ChromeNetworkDelegate passes the list to TabSpecificContentSettings, which
stores them for reporting cookies that have been sent/blocked to the user.

[commit-bot: I haz the power, 2012-11-27 10:09:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/11417148/1

[commit-bot: I haz the power, 2012-11-27 10:09:55]

Presubmit check for 11417148-1 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    net

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[droger, 2012-11-27 10:15:14]

+ cbentzel as OWNER for net

[cbentzel, 2012-11-27 19:19:20]

What behavior does this fix? Should there be an associated bug?

[droger, 2012-11-28 09:56:24]

On 2012/11/27 19:19:20, cbentzel wrote:
> What behavior does this fix? Should there be an associated bug?

The problem is that cookie settings are not taken into account by the network
stack on iOS (cookies are always ON).

More details:
The cookie settings are implemented in the network delegate.
The network delegate is called only if the CookieStore is implemented as a
CookieMonster.
On iOS, the CookieStore implementation is not a CookieMonster.

This CL calls the network delegate even if the CookieStore is not a
CookieMonster.

[cbentzel, 2012-11-28 14:33:13]

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:511: net::CookieMonster* cookie_monster
= cookie_store->GetCookieMonster();
Question more for Erik: I find all the GetCookieMonster calls to be a bit gross
(implicit down-cast). Is there a reason we can't just make
GetAllCookiesForURLAsync part of the CookieStore interface and get rid of the if
condition here?

[cbentzel, 2012-11-28 14:36:42]

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:518:
CheckCookiePolicyAndLoad(CookieList());
Out of curiosity: is the Cookie header already supplied at this point for
non-CookieMonster based requests? If so, should this parse the results and pass
it in as a CookieList?

[droger, 2012-11-28 14:47:44]

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:518:
CheckCookiePolicyAndLoad(CookieList());
On 2012/11/28 14:36:42, cbentzel wrote:
> Out of curiosity: is the Cookie header already supplied at this point for
> non-CookieMonster based requests? If so, should this parse the results and
pass
> it in as a CookieList?

Looking at the code, the headers are set a bit after, in OnCookiesLoaded, which
is triggered by DoLoadCookies().

I understand why it is desirable to unify the behavior, but also note that on
iOS I don't currently need the actual list of cookies, because the cookie
setting is "all or nothing".

[droger, 2012-11-30 09:52:28]

cbentzel: ping
erikwright: maybe Chris is waiting for an answer from you, about the
GetCookieMonster() method.

[cbentzel, 2012-11-30 14:08:35]

Yes, waiting for Erik Wright. Otherwise I am happy with this.

[erikwright (departed), 2012-11-30 16:43:52]

Chris, I think you are experiencing here some of what I experience whenever I
look at this.

There is lots of re-design that could be done in the Cookie Store that would
lead to a much more sensible design.

But it's not clear that there are downsides to the current implementation
besides a slight tingling in the back of the throat. Hence I'm not asking you or
Shibl for time (mine or otherwise) to address it.

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:511: net::CookieMonster* cookie_monster
= cookie_store->GetCookieMonster();
On 2012/11/28 14:33:13, cbentzel wrote:
> Question more for Erik: I find all the GetCookieMonster calls to be a bit
gross
> (implicit down-cast). Is there a reason we can't just make
> GetAllCookiesForURLAsync part of the CookieStore interface and get rid of the
if
> condition here?

GetAllCookiesForURLAsync is returning full metadata that is specific to the
cookie monster.

https://codereview.chromium.org/11417148/diff/1/net/url_request/url_request_h...
net/url_request/url_request_http_job.cc:518:
CheckCookiePolicyAndLoad(CookieList());
On 2012/11/28 14:36:42, cbentzel wrote:
> Out of curiosity: is the Cookie header already supplied at this point for
> non-CookieMonster based requests? If so, should this parse the results and
pass
> it in as a CookieList?

The problem is that the CookieList is cookies with all metadata, whereas the
cookie header is just the name-value pairs of cookies that matched the request.
It's pig vs. sausage.

[commit-bot: I haz the power, 2012-12-03 08:48:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/11417148/1

[commit-bot: I haz the power, 2012-12-03 08:48:43]

Presubmit check for 11417148-1 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    net

[cbentzel, 2012-12-04 00:13:30]

LGTM

[commit-bot: I haz the power, 2012-12-04 09:42:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/11417148/1

[commit-bot: I haz the power, 2012-12-04 12:17:43]

Change committed as 170942