The following call appears twice: m_frame->script()->executeIfJavaScriptURL() Also, when changeLocation() is called, the function unnecessarily gets called twice.
Created attachment 55954 [details] Proposed patch
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.
Created attachment 56193 [details] Proposed patch 2
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.
Committed r59596: <http://trac.webkit.org/changeset/59596>
(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.
http://trac.webkit.org/changeset/59596 might have broken GTK Linux 32-bit Debug
(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.