Remove prerender cookie store, part 4.

Remove prerender cookie store, part 4.

This removes the ability to pass in a custom CookieStore to a URLRequest. It's
a small change to net/url_request which then balloons to everything in the
project that ever makes a URLRequest.

This reverts the rest of https://codereview.chromium.org/188693003 (and then
does a whole lot more because URLRequest constructors were unified recently to
always require passing in the fourth argument).

BUG=457344
Committed: https://crrev.com/151423e1851b45000257ef2d32ee0984a24133c7
Cr-Commit-Position: refs/heads/master@{#321820}

[davidben, 2015-03-14 00:37:24]

davidben@chromium.org changed reviewers:
+ mmenke@chromium.org

[davidben, 2015-03-14 00:37:25]

This depends on https://codereview.chromium.org/1002363002/.

You'll want to look at net/url_request first.

Probably the way to do this is for you to review everything I'd have OWNERS
coverage of myself (net, chrome/browser/prerender, and content/) and then I'll
ask the relevant OWNERS to review their bits. Or, if you really want, you can
page through the whole thing.

Also, sorry. :-(

(Although it's somewhat your fault for getting rid of the URLRequest
constructor...)

[mmenke, 2015-03-14 01:11:13]

On 2015/03/14 00:37:25, David Benjamin wrote:
> This depends on https://codereview.chromium.org/1002363002/.
> 
> You'll want to look at net/url_request first.
> 
> Probably the way to do this is for you to review everything I'd have OWNERS
> coverage of myself (net, chrome/browser/prerender, and content/) and then I'll
> ask the relevant OWNERS to review their bits. Or, if you really want, you can
> page through the whole thing.
> 
> Also, sorry. :-(
> 
> (Although it's somewhat your fault for getting rid of the URLRequest
> constructor...)

Yea, it's completely my fault (I even knew this might happen when I did it).  I
decided at the time if it were ever removed, I just wouldn't bother until/unless
something else needed to be changed.

I didn't just TBR the original CL that unified the constructors, but I think
this is a TBR-worthy change, once I review it.

[mmenke, 2015-03-20 14:55:53]

https://codereview.chromium.org/1003953008/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (left):

https://codereview.chromium.org/1003953008/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:594: int render_process_id);
Shouldn't this be part of the other CL?

[davidben, 2015-03-20 17:35:27]

https://codereview.chromium.org/1003953008/diff/1/content/public/browser/cont...
File content/public/browser/content_browser_client.h (left):

https://codereview.chromium.org/1003953008/diff/1/content/public/browser/cont...
content/public/browser/content_browser_client.h:594: int render_process_id);
On 2015/03/20 14:55:53, mmenke wrote:
> Shouldn't this be part of the other CL?

Oops! Moved it over to the other CL. (I'll upload a new version of this after
the parent CL lands and I can actually run try jobs. It needs constant rebasing
anyway.)

[davidben, 2015-03-20 21:33:11]

Prerequisite CL has cleared and this has been rebased. Hopefully try jobs are
happy and I haven't missed a spot. (I'm sure I'll need to rebase it again
several times.)

[mmenke, 2015-03-20 21:35:35]

On 2015/03/20 21:33:11, David Benjamin wrote:
> Prerequisite CL has cleared and this has been rebased. Hopefully try jobs are
> happy and I haven't missed a spot. (I'm sure I'll need to rebase it again
> several times.)

LGTM, assuming all tryjobs passed.

[davidben, 2015-03-20 23:52:29]

davidben@chromium.org changed reviewers:
+ jam@chromium.org

[davidben, 2015-03-20 23:52:30]

+jam for a somewhat broad OWNERS check. I'm not entirely sure when you're
supposed to pull in the top-level OWNERS. I can split this up into a bunch of
reviewers if that would be more appropriate.

It's an extremely mechanical review... this is removing a now useless NULL
parameter at the end of net::URLRequestContext::CreateRequest. We create
requests all over the codebase. :-/ The entirety of this CL, outside of
net/url_request (which Matt and I have coverage for), is in removing that
useless parameter.

(content/browser/service_worker/service_worker_write_to_cache_job.cc contains
the one remaining non-NULL instance of this parameter, but it's also equivalent
to passing NULL.)

You can skip content/ and net/ since I already have coverage there.

[jam, 2015-03-23 16:10:40]

This doesn't have enough background to review. The linked bug just says to
remove this functionality, which presumably was added for a good reason (i.e.
don't want prerendered pages to change cookie store if they're not used).
Someone asked in comment 2 about that, but there are no replies to that
question.

[davidben, 2015-03-23 16:26:13]

On 2015/03/23 16:10:40, jam wrote:
> This doesn't have enough background to review. The linked bug just says to
> remove this functionality, which presumably was added for a good reason (i.e.
> don't want prerendered pages to change cookie store if they're not used).

The functionality has already been removed, this is just removing the final
piece of support code.

It's being removed because it's a ton of complexity (see also: four part CL to
get rid of), and wasn't doing any good. It was causing site compatibility
issues, making prerender difficult to deploy on the same site, and doesn't
actually make prerenders stateless when unused. They've never been stateless and
didn't become stateless after this landed. For instance, changes to localStorage
within a prerender show up immediately.

It was landed originally as a possible solution to address some issues GWS
prerender came across, but it never addressed that and we ended up going with a
GWS-side solution. I pinged the various potential stakeholders in this mechanism
before embarking on this, and no one needs it. I think it landing at all was
somewhat of a miscommunication. (There was a lot of "oh, it because abc needed
to solve xyz, remember? // abc: oh, we never needed this. we solved xyz
differently...".)

> Someone asked in comment 2 about that, but there are no replies to that
> question.

Oh, I somehow missed those. Responding now.

[jam, 2015-03-23 17:03:56]

lgtm

note that per OWNERS policy on the wiki, for mechanical changes like this you
only need lgtm from owner of the API you're changing. you can tbr all the owners
that are using this API

[davidben, 2015-03-23 17:46:38]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-03-23 17:46:39]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1003953008/#ps60001
(title: "rebase (just in case since this is so huge)")

[commit-bot: I haz the power, 2015-03-23 17:46:58]

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

[commit-bot: I haz the power, 2015-03-23 18:48:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-23 18:49:34]

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