Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

The response cookies are currently copied for every request and made available to consumers. The cookies are then passed through different parts of the code needlessly as they are only used in one place. This CL removes most of that logic. The sole consumer of the response cookies now retrieves them from the response headers.

Some tests that were relying on the old (cookies are already parsed) logic are updated as well.

BUG=598452
Committed: https://crrev.com/e74f617c9ca6471411b874fdf43db60a3ef59ccf
Cr-Commit-Position: refs/heads/master@{#399939}

[martijnc, 2016-06-05 21:05:29]

Patchset #1 (id:1) has been deleted

[martijnc, 2016-06-07 18:12:01]

Patchset #1 (id:20001) has been deleted

[martijnc, 2016-06-07 18:12:25]

Patchset #1 (id:40001) has been deleted

[martijnc, 2016-06-07 19:27:29]

Description was changed from

==========
Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

BUG=598452
==========

to

==========
Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

The response cookies are currently copied for every request and made available
to consumers. The cookies are then passed through different parts of the code
needlessly as they are only used in one place. This CL removes most of that
logic. The sole consumer of the response cookies now retrieves them from the
response headers.

Some tests that were relying on the old—cookies are already parsed—logic are
updated as well.

BUG=598452
==========

[martijnc, 2016-06-07 19:27:29]

martijn@martijnc.be changed reviewers:
+ mmenke@chromium.org

[martijnc, 2016-06-07 19:27:55]

Description was changed from

==========
Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

The response cookies are currently copied for every request and made available
to consumers. The cookies are then passed through different parts of the code
needlessly as they are only used in one place. This CL removes most of that
logic. The sole consumer of the response cookies now retrieves them from the
response headers.

Some tests that were relying on the old—cookies are already parsed—logic are
updated as well.

BUG=598452
==========

to

==========
Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

The response cookies are currently copied for every request and made available
to consumers. The cookies are then passed through different parts of the code
needlessly as they are only used in one place. This CL removes most of that
logic. The sole consumer of the response cookies now retrieves them from the
response headers.

Some tests that were relying on the old (cookies are already parsed) logic are
updated as well.

BUG=598452
==========

[martijnc, 2016-06-07 21:17:12]

Can you review this patch? Thanks!

https://codereview.chromium.org/2035293002/diff/60001/net/url_request/url_fet...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/2035293002/diff/60001/net/url_request/url_fet...
net/url_request/url_fetcher_impl_unittest.cc:1524:
EXPECT_FALSE(delegate.fetcher()->GetResponseHeaders());
Because the request is expected to fail it has no response headers so there
can't be response cookies either. This change makes the test more strict though.
Would it be better to just remove this line?

[mmenke, 2016-06-08 14:46:44]

LGTM, thanks for taking care of this!

https://codereview.chromium.org/2035293002/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2035293002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:67:
const char kOAuthCodeCookie[] = "Set-Cookie: oauth_code=1234; Secure; HttpOnly";
Maybe rename this kSetOAuthCodeCookie?

https://codereview.chromium.org/2035293002/diff/60001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher_unittest.cc (right):

https://codereview.chromium.org/2035293002/diff/60001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher_unittest.cc:40: }
nit:  // namespace

https://codereview.chromium.org/2035293002/diff/60001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher_unittest.cc:40: }
Also, while you're here, this should probably be expanded to include all the
const character arrays below.

[martijnc, 2016-06-08 16:08:09]

martijn@martijnc.be changed reviewers:
+ nparker@chromium.org, rogerta@chromium.org, stanisc@chromium.org,
thakis@chromium.org, tnagel@chromium.org

[martijnc, 2016-06-08 16:08:10]

+tnagel@chromium.org:
    components/policy/core/common/cloud/device_management_service.cc
   
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc

+nparker@chromium.org:
    chrome/browser/safe_browsing/*

+thakis@chromium.org:
    chrome/service/cloud_print/*

+rogerta@chromium.org:
    chrome/browser/ui/webui/signin/inline_login_ui_browsertest.cc
    google_apis/gaia/*

+stanisc@chromium.org:
    sync/internal_api/http_bridge_unittest.cc

Can you take a look?

https://codereview.chromium.org/2035293002/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2035293002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:67:
const char kOAuthCodeCookie[] = "Set-Cookie: oauth_code=1234; Secure; HttpOnly";
On 2016/06/08 at 14:46:44, mmenke wrote:
> Maybe rename this kSetOAuthCodeCookie?

Done.

https://codereview.chromium.org/2035293002/diff/60001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher_unittest.cc (right):

https://codereview.chromium.org/2035293002/diff/60001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher_unittest.cc:40: }
On 2016/06/08 at 14:46:44, mmenke wrote:
> nit:  // namespace

Both done.

[Nico, 2016-06-08 16:22:31]

chrome/service/cloud_print lgtm

[Nathan Parker, 2016-06-08 16:33:57]

lgtm

safe_browsing

[stanisc, 2016-06-08 17:27:51]

sync/internal_api/http_bridge_unittest.cc lgtm

[Thiemo Nagel, 2016-06-14 14:31:24]

On 2016/06/08 16:08:10, martijnc wrote:
> components/policy/core/common/cloud/device_management_service.cc   
> chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc

*/policy/* LGTM

[Roger Tawa OOO till Jul 10th, 2016-06-14 15:57:40]

lgtm for inline_login_ui_browsertest.cc and google_apis with couple of nits
below.

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
File google_apis/gaia/gaia_auth_fetcher.cc (right):

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
google_apis/gaia/gaia_auth_fetcher.cc:75: void
GetCookiesFromResponse(net::HttpResponseHeaders* headers,
This should be a const ptr.

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
File google_apis/gaia/gaia_auth_fetcher_unittest.cc (right):

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
google_apis/gaia/gaia_auth_fetcher_unittest.cc:40: }
I think this should be part of the HttpResponseHeaders API.  Something like:

void HttpResponseHeaders::AddCookie(std::string& cookie_string) {
  AddHeader("Set-Cookie: " + cookie_string);
}

I mention this because I saw this pattern repeated in this CL.  And
HttpResponseHeaders already has special handling (for example
UpdateWithNewRange()) for common headers line Content-Length.

[martijnc, 2016-06-14 20:10:50]

Patchset #4 (id:120001) has been deleted

[martijnc, 2016-06-14 22:18:08]

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
File google_apis/gaia/gaia_auth_fetcher.cc (right):

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
google_apis/gaia/gaia_auth_fetcher.cc:75: void
GetCookiesFromResponse(net::HttpResponseHeaders* headers,
On 2016/06/14 at 15:57:40, Roger Tawa wrote:
> This should be a const ptr.

Done.

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
File google_apis/gaia/gaia_auth_fetcher_unittest.cc (right):

https://codereview.chromium.org/2035293002/diff/100001/google_apis/gaia/gaia_...
google_apis/gaia/gaia_auth_fetcher_unittest.cc:40: }
On 2016/06/14 at 15:57:40, Roger Tawa wrote:
> I think this should be part of the HttpResponseHeaders API.  Something like:
> 
> void HttpResponseHeaders::AddCookie(std::string& cookie_string) {
>   AddHeader("Set-Cookie: " + cookie_string);
> }
> 
> I mention this because I saw this pattern repeated in this CL.  And
HttpResponseHeaders already has special handling (for example
UpdateWithNewRange()) for common headers line Content-Length.

Done.

[martijnc, 2016-06-15 16:05:19]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2016-06-15 16:05:20]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
stanisc@chromium.org, thakis@chromium.org, nparker@chromium.org,
rogerta@chromium.org, tnagel@chromium.org
Link to the patchset: https://codereview.chromium.org/2035293002/#ps140001
(title: "comments rogerta")

[commit-bot: I haz the power, 2016-06-15 16:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035293002/140001

[commit-bot: I haz the power, 2016-06-15 17:19:40]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-06-15 17:22:34]

Description was changed from

==========
Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

The response cookies are currently copied for every request and made available
to consumers. The cookies are then passed through different parts of the code
needlessly as they are only used in one place. This CL removes most of that
logic. The sole consumer of the response cookies now retrieves them from the
response headers.

Some tests that were relying on the old (cookies are already parsed) logic are
updated as well.

BUG=598452
==========

to

==========
Remove URLRequest::GetResponseCookies and URLRequestJob::GetResponseCookies.

The response cookies are currently copied for every request and made available
to consumers. The cookies are then passed through different parts of the code
needlessly as they are only used in one place. This CL removes most of that
logic. The sole consumer of the response cookies now retrieves them from the
response headers.

Some tests that were relying on the old (cookies are already parsed) logic are
updated as well.

BUG=598452

Committed: https://crrev.com/e74f617c9ca6471411b874fdf43db60a3ef59ccf
Cr-Commit-Position: refs/heads/master@{#399939}
==========

[commit-bot: I haz the power, 2016-06-15 17:22:36]

Patchset 4 (id:??) landed as
https://crrev.com/e74f617c9ca6471411b874fdf43db60a3ef59ccf
Cr-Commit-Position: refs/heads/master@{#399939}