Update the ResourceRequest in DocumentLoader after the embedder modified it

Update the ResourceRequest in DocumentLoader after the embedder modified it

The call on resource will end up calling DocumentLoader::willSendRequest
which stores the request. If we do this before the call on the host
(which will end up calling RenderFrameImpl::willSendRequest),
modifications by the host are not reflected in DocumentLoader::m_request.

BUG=331941
R=japhet@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164696

[Nate Chapin, 2014-01-07 22:30:11]

LGTM with a question:

https://codereview.chromium.org/126753002/diff/1/Source/core/fetch/ResourceLo...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/126753002/diff/1/Source/core/fetch/ResourceLo...
Source/core/fetch/ResourceLoader.cpp:270:
m_host->willSendRequest(m_resource->identifier(), request, redirectResponse,
m_options.initiatorInfo);
If RenderFrameImpl::willSendRequest cancels the load,
m_resource->willSendRequest() might now do something crazy. I can't find any
evidence that it ever does cancel, though. Do you know for sure?

[jochen (gone - plz use gerrit), 2014-01-08 10:32:17]

actually, it doesn't work, because the embedder relies on DocumentLoader to
update the request correctly (e.g. set the first party for cookies).

I tried a new approach, adding a new callback to update the request in the
loader, wdyt?

[Nate Chapin, 2014-01-08 18:05:22]

On 2014/01/08 10:32:17, jochen wrote:
> actually, it doesn't work, because the embedder relies on DocumentLoader to
> update the request correctly (e.g. set the first party for cookies).
> 
> I tried a new approach, adding a new callback to update the request in the
> loader, wdyt?

I don't like it, but I'm not sure there's a better alternative. So....LGTM? :-/

Could we move the setting the first party for cookies and other updates to
FrameFetchContext::willSendRequest? That seems like it might just be a different
flavor of messy code.

If we do it this way, we should at least check whether we can remove any of the
existing cases where we set DocumentLoader::m_request.

[jochen (gone - plz use gerrit), 2014-01-08 18:57:32]

On 2014/01/08 18:05:22, Nate Chapin wrote:
> On 2014/01/08 10:32:17, jochen wrote:
> > actually, it doesn't work, because the embedder relies on DocumentLoader to
> > update the request correctly (e.g. set the first party for cookies).
> > 
> > I tried a new approach, adding a new callback to update the request in the
> > loader, wdyt?
> 
> I don't like it, but I'm not sure there's a better alternative. So....LGTM?
:-/
> 
> Could we move the setting the first party for cookies and other updates to
> FrameFetchContext::willSendRequest? That seems like it might just be a
different
> flavor of messy code.

I think that's actually worse because the FrameFetchContext doesn't do any other
requests mangling

> If we do it this way, we should at least check whether we can remove any of
the
> existing cases where we set DocumentLoader::m_request.

There's basically just one in willSendRequest and that one is required is the
request is updated before we call the embedder's willSendRequest.

[commit-bot: I haz the power, 2014-01-08 18:58:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/126753002/70001

[commit-bot: I haz the power, 2014-01-08 19:05:03]

Change committed as 164696