Bug 38827 – FrameLoader: refactor changeLocation() and urlSelected() to share more code

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Jerdonek]

The following call appears twice:

m_frame->script()->executeIfJavaScriptURL()

Also, when changeLocation() is called, the function unnecessarily gets called twice.

[Chris Jerdonek]

Created attachment 55954 [details]
Proposed patch

[Adam Barth]

Comment on attachment 55954 [details]
Proposed patch

For parameters like shouldReplaceDocumentIfJavascriptURL, we like to use enums with two values so you can read at the call site what the parameter does (instead of just "true" or "false").  For example, we'd add ReplaceDocumentIfJavaScritptURL or DoNotReplaceDocumentIfJavaScritptURL as the argument.

[Chris Jerdonek]

Created attachment 56193 [details]
Proposed patch 2

[Adam Barth]

Comment on attachment 56193 [details]
Proposed patch 2

LGTM.  Please consider these small tweaks below before landing.

WebCore/loader/FrameLoader.cpp:340
 +      bool lockHistory, bool lockBackForwardList, bool userGesture, ReferrerPolicy referrerPolicy)
We usually don't add line breaks for function declarations, even when they're ridiculously long like this one.

WebCore/loader/FrameLoader.cpp:352
 +      if (m_frame->script()->executeIfJavaScriptURL(request.url(), userGesture, shouldReplaceDocumentIfJavaScriptURL == ReplaceDocumentIfJavaScriptURL))
You might consider propagating the enum down into executeIfJavaScriptURL so we don't have to convert here and other callers (if there are any) will look prettier.

[Chris Jerdonek]

Committed r59596: <http://trac.webkit.org/changeset/59596>

[Chris Jerdonek]

(In reply to comment #4)
> (From update of attachment 56193 [details])
> LGTM.  Please consider these small tweaks below before landing.
> 
> WebCore/loader/FrameLoader.cpp:340
>  +      bool lockHistory, bool lockBackForwardList, bool userGesture, ReferrerPolicy referrerPolicy)
> We usually don't add line breaks for function declarations, even when they're ridiculously long like this one.
> 
> WebCore/loader/FrameLoader.cpp:352
>  +      if (m_frame->script()->executeIfJavaScriptURL(request.url(), userGesture, shouldReplaceDocumentIfJavaScriptURL == ReplaceDocumentIfJavaScriptURL))
> You might consider propagating the enum down into executeIfJavaScriptURL so we don't have to convert here and other callers (if there are any) will look prettier.

FYI, I incorporated both of these suggestions in the final commit.  Thanks for your review and helpful comments.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59596 might have broken GTK Linux 32-bit Debug

[Chris Jerdonek]

(In reply to comment #7)
> http://trac.webkit.org/changeset/59596 might have broken GTK Linux 32-bit Debug

The three test cases that failed here also failed in a previous revision (r59591), so it doesn't look like the revision for this report caused the breakage.