Get rid of stashing a frame pointer with ResourceRequest and just store the r...

Get rid of stashing a frame pointer with ResourceRequest and just store the routing id directly.  This simplifies the renderer code and also allow this code to be used in worker processes, where we don't have a frame.

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

[jam, 2009-03-13 04:34:41]

Note: I'll send out the WebKit changes upstream, but I wanted to get them
reviewed together here first to make sure the idea is fine.

[darin (slow to review), 2009-03-13 05:19:08]

http://codereview.chromium.org/46026/diff/1029/1043
File third_party/WebKit/WebCore/platform/network/ResourceHandle.h (left):

http://codereview.chromium.org/46026/diff/1029/1043#oldcode103
Line 103: // FIXME: should not need the Frame
it may be a lot of work to remove this.  it would require fixing all of the
other WebCore ports, which you have not done in this CL, so not sure if you are
thinking about that.  i'd probably do a follow-up patch to webkit to eliminate
this Frame pointer.

http://codereview.chromium.org/46026/diff/1029/1042
File third_party/WebKit/WebCore/platform/network/chromium/ResourceRequest.h
(right):

http://codereview.chromium.org/46026/diff/1029/1042#newcode90
Line 90: // Allows the request to be matched up with its frame.
This comment suggests a connection between routingID and frame, but there really
isn't.  A routingID corresponds to a WebView, which is a tree of frames.

I was thinking that it might be nice to hide the concept of routing IDs here... 
but any other name might just be confusing.  I don't have a good suggestion at
this time, so I guess routingID is fine.

http://codereview.chromium.org/46026/diff/1029/1032
File webkit/glue/resource_fetcher.cc (right):

http://codereview.chromium.org/46026/diff/1029/1032#newcode63
Line 63: webview->GetDelegate()->WillSendRequest(webview, 0, &webreq);
hmm... we may instead want to use
frame_loader->client()->dispatchWillSendRequest(...)

in that case you would just create a ResourceRequest instead of a
WebRequestImpl.  i think you could pass NULL for the DocumentLoader parameter.

http://codereview.chromium.org/46026/diff/1029/1032#newcode65
Line 65: WebCore::ResourceRequest req =
webreq.frame_load_request().resourceRequest();
|req| should be a const reference.

http://codereview.chromium.org/46026/diff/1029/1034
File webkit/glue/resource_handle_impl.cc (left):

http://codereview.chromium.org/46026/diff/1029/1034#oldcode397
Line 397: // WinInet dies if blank headers are set.  TODO(darin): Is this still
an
thanks!

http://codereview.chromium.org/46026/diff/1029/1037
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/46026/diff/1029/1037#newcode1274
Line 1274: // Sets the routing id to associate the ResourceRequest with the
RenderView.
ditto about trying to use FrameLoaderClient instead to call WillSendRequest.

[jam, 2009-03-13 18:53:54]

http://codereview.chromium.org/46026/diff/1029/1043
File third_party/WebKit/WebCore/platform/network/ResourceHandle.h (left):

http://codereview.chromium.org/46026/diff/1029/1043#oldcode103
Line 103: // FIXME: should not need the Frame
On 2009/03/13 05:19:08, darin wrote:
> it may be a lot of work to remove this.  it would require fixing all of the
> other WebCore ports, which you have not done in this CL, so not sure if you
are
> thinking about that.  i'd probably do a follow-up patch to webkit to eliminate
> this Frame pointer.

oops!  I wasn't thinking too hard and had just searched our solution for all
mention of this function..  I'll revert this part of the cl, and look into
whether it's possible to remove if no ports depend on it.  building webkit
now...

http://codereview.chromium.org/46026/diff/1029/1042
File third_party/WebKit/WebCore/platform/network/chromium/ResourceRequest.h
(right):

http://codereview.chromium.org/46026/diff/1029/1042#newcode90
Line 90: // Allows the request to be matched up with its frame.
On 2009/03/13 05:19:08, darin wrote:
> This comment suggests a connection between routingID and frame, but there
really
> isn't.  A routingID corresponds to a WebView, which is a tree of frames.

I didn't know if I should mention WebView in the port?  Changed it for now,
unless there are objections.

> 
> I was thinking that it might be nice to hide the concept of routing IDs
here... 
> but any other name might just be confusing.  I don't have a good suggestion at
> this time, so I guess routingID is fine.

http://codereview.chromium.org/46026/diff/1029/1032
File webkit/glue/resource_fetcher.cc (right):

http://codereview.chromium.org/46026/diff/1029/1032#newcode63
Line 63: webview->GetDelegate()->WillSendRequest(webview, 0, &webreq);
On 2009/03/13 05:19:08, darin wrote:
> hmm... we may instead want to use
> frame_loader->client()->dispatchWillSendRequest(...)

good idea, I'm not familiar enough with this code to figure out that there was a
simpler way of not having to bring in other glue classes.  done

> 
> in that case you would just create a ResourceRequest instead of a
> WebRequestImpl.  i think you could pass NULL for the DocumentLoader parameter.

Done.  I originally tried pass in the DocumentLoader from FrameLoader, but then
realized that for the webplugin_impl.cc case, this would mean that the
targetType would get set incorrectly to something else.  so i made
WebFrameLoaderClient::dispatchWillSendRequest handle a null DocumentLoader.

http://codereview.chromium.org/46026/diff/1029/1032#newcode65
Line 65: WebCore::ResourceRequest req =
webreq.frame_load_request().resourceRequest();
On 2009/03/13 05:19:08, darin wrote:
> |req| should be a const reference.

Done.

http://codereview.chromium.org/46026/diff/1029/1037
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/46026/diff/1029/1037#newcode1274
Line 1274: // Sets the routing id to associate the ResourceRequest with the
RenderView.
On 2009/03/13 05:19:08, darin wrote:
> ditto about trying to use FrameLoaderClient instead to call WillSendRequest.

Done.

[jam, 2009-03-13 23:37:05]

renamed to requestorID

[levin, 2009-03-15 06:17:59]

Two issues:
1. The upstream change has landed.  My plan is to put this one in as is in
chromium to allow the webkit merge to land.  Please continue with the review and
John can adjust what I checked in to match the final result.

2. UnittestTestServer::MakeGETRequest has a call to ResourceLoaderBridge::Create
which didn't seem to be fixed in this patch.  I've changed it to be the
following:

      ResourceLoaderBridge::Create("GET",
                                   url,
                                   url,            // policy_url
                                   GURL(),         // no referrer
                                   std::string(),  // no extra headers
                                   net::LOAD_NORMAL,
                                   0,
                                   ResourceType::SUB_RESOURCE,
                                   false,
                                   0));
If this isn't correct, please adjust that call.

[ojan, 2009-03-16 18:46:25]

LGTM