Bug 39395 – FrameLoader: eliminate some unnecessary logic from urlSelected()

• Component: WebCore Misc.
• Status: NEW

[Chris Jerdonek]

Some minor simplifications can be made to some of the referrer/origin code in FrameLoader -- particularly in the urlSelected() and loadFrameRequest() calls.

[Chris Jerdonek]

I have a question.  In this code--

http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=59829#L343

Does the call to addHTTPOriginIfNeeded() in line 376 seem superfluous to you?

> 372	    if (referrerPolicy == NoReferrer)
> 373	        m_suppressOpenerInNewFrame = true;
> 374	    else if (frameRequest.resourceRequest().httpReferrer().isEmpty())
> 375	        frameRequest.resourceRequest().setHTTPReferrer(m_outgoingReferrer);
> 376	    addHTTPOriginIfNeeded(frameRequest.resourceRequest(), outgoingOrigin());

The reason is that the private urlSelected(changeLocation(const KURL& url,const ResourceRequest& request, ...) overload only gets called by changeLocation(const KURL& url, ...) and urlSelected(const KURL& url, ...) above it.  You can see in both of those methods that the ResourceRequest is getting constructed for the first time.  Since ResourceRequest initially has httpMethod "GET" when it's constructed, the call to addHTTPOriginIfNeeded() won't do anything (it only does something if the method is not "GET" or "HEAD").

If this analysis is right, it seems odd that the outgoingOrigin() expression was in there in the first place.

FYI, this recent simple refactoring is what made this much easier to see:

http://trac.webkit.org/changeset/59829

(tightening the FrameLoader API by making urlSelected() publicly accept only KURL instead of ResourceRequest)

[Chris Jerdonek]

(In reply to comment #1)
> The reason is that the private urlSelected(changeLocation(const KURL& url,const ResourceRequest& request, ...) overload only gets called by changeLocation(const KURL& url, ...) and urlSelected(const KURL& url, ...) above it.

Paste-miss.  That should have been--

The reason is that the private urlSelected(const ResourceRequest& request, ...) overload only gets called by changeLocation(const KURL& url, ...) and urlSelected(const KURL& url, ...) above it.

[Adam Barth]

It's the job of addHTTPOriginIfNeeded to know that Origin doesn't get attached to GET requests.  In principle, the callers of that method aren't supposed to know that.

[Chris Jerdonek]

Created attachment 56722 [details]
Proposed patch

[Adam Barth]

Comment on attachment 56722 [details]
Proposed patch

Mostly questions.  Thanks for digging into this class.

WebCore/loader/FrameLoader.cpp:1781
 +  void FrameLoader::loadFrameRequest(const ResourceRequest& request, const String& frameName, bool lockHistory, bool lockBackForwardList, PassRefPtr<Event> event, PassRefPtr<FormState> formState, ReferrerPolicy referrerPolicy, String origin)
Strange that we use the name |request| for both FrameLoadRequest and ResourceRequest.  Is that what we do elsewhere?

WebCore/loader/FrameLoader.cpp:1778
 +      loadFrameRequest(request.resourceRequest(), request.frameName(), lockHistory, lockBackForwardList, event, formState, referrerPolicy, String());
The last arg here is somewhat unfortunate.

WebCore/loader/FrameLoader.cpp:3211
 +  // The inRequest's httpReferrer() is not examined by this method.  The newReferrer
Crazy

WebCore/loader/FrameLoader.h:385
 +      void addExtraFieldsToRequest(ResourceRequest&, FrameLoadType loadType, bool isMainResource, bool cookiePolicyURLFromRequest, String httpOrigin);
Why httpOrigin instead of origin?

[Adam Barth]

Comment on attachment 56722 [details]
Proposed patch

Sadly cjerdonek appears to no longer be contributing to the project.  Hopefully he'll return and finish this patch.