Bug 39112 – FrameLoader: refactor loadFrameRequest() not to use FrameLoadRequest

• Component: WebCore Misc.
• Status: ASSIGNED

[Chris Jerdonek]

Refactor FrameLoader::loadFrameRequest() to take a ResourceRequest and String frame name instead of a FrameLoadRequest.  This will make the signature of loadFrameRequest() more similar to the signatures used predominantly by the other load methods.  This should aid future refactoring of the FrameLoader class by making opportunities for code sharing more evident.  This is also a step towards eliminating the FrameLoadRequest class (bug 39111).

[Chris Jerdonek]

Created attachment 56137 [details]
Proposed patch

[Chris Jerdonek]

Created attachment 56138 [details]
Proposed patch 2

I forgot to re-diff before attaching patch 1.

[Adam Barth]

Comment on attachment 56138 [details]
Proposed patch 2

Hum...  It seems strange that "loadFrameRequest" wouldn't load a FrameLoadRequest.

I think we probably want to go in the other direction: make a bigger "request" object that we can pass around that includes things like lock history, etc.  Is there some reason you think breaking this down into a smaller object is better?

[Chris Jerdonek]

(In reply to comment #3)
> (From update of attachment 56138 [details])
> Hum...  It seems strange that "loadFrameRequest" wouldn't load a FrameLoadRequest.

Yes, I noticed this, though wouldn't the correct name be loadFrameLoadRequest()?

> I think we probably want to go in the other direction: make a bigger "request" object that we can pass around that includes things like lock history, etc.

It's funny you mention this.  When I had studied the history of FrameLoadRequest, the last change was to remove lock history:

http://trac.webkit.org/changeset/40373/trunk/WebCore/page/FrameLoadRequest.h

> Is there some reason you think breaking this down into a smaller object is better?

For now, yes.  I think there is a good chance we may eventually want some abstraction like this, but I don't know if it's the right one.  In the short-term, given that the 4-year life of FrameLoadRequest hasn't really bought us anything here, I thought it would be better to make the various load methods look more similar.  This will make it somewhat easier to see opportunities for code sharing across the various load methods, etc.  If we prematurely clump things, it may be harder to do this.

After this change, FrameLoader::createWindow() will be the only place in FrameLoader.h using FrameLoadRequest.  And that function is getting moved out of the FrameLoader class in bug 39156.

Given all that, the dependency doesn't seem needed.

[Chris Jerdonek]

Comment on attachment 56138 [details]
Proposed patch 2

Marking r- to remove from pending review while considering Adam's comments.