[webkit-reviews] review denied: [Bug 183028] pushState and replaceState no longer works in local file : [Attachment 335014] WIP

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Danyao Wang
<danyao at chromium.org>'s request for review:
Bug 183028: pushState and replaceState no longer works in local file
https://bugs.webkit.org/show_bug.cgi?id=183028

Attachment 335014: WIP

https://bugs.webkit.org/attachment.cgi?id=335014&action=review




--- Comment #9 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 335014
  --> https://bugs.webkit.org/attachment.cgi?id=335014
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=335014&action=review

I think the overall approach here is good, but the new equality predicate needs
to be added to the URL class so it can be used elsewhere if needed.

I'm also worried that we will inadvertently break this again without some form
of test case. Can you repurpose some of the test cases in
'https://codereview.chromium.org/1632513002/patch/20001/30006'? Perhaps adding
them to 'Tools/TestWebKitAPI/Tests/WebCore/URL.cpp'.

For these reasons, r- for now.

> Source/WebCore/page/History.cpp:52
> +	   for (unsigned i = 0; i < pathEnd; i++) {

WebKit style uses "++i"

>> Source/WebCore/page/History.cpp:58
>> +}
> 
> Why does that need to in the default namespace? Can you make this function
static? If it's only used in one place, maybe it should be moved before
History::stateObjectAdded?

No, this should be added to URL, just like 'equalIgnoringFragmentIdentifier':

WTF/URL.h:
    WEBCORE_EXPORT friend bool equalIgnoringFragmentIdentifier(const URL&,
const URL&);

>> Source/WebCore/page/History.cpp:202
>> +	const SecurityOrigin& documentOrigin =
m_frame->document()->securityOrigin();
> 
> Are you able to use auto here?

I'd suggest using ' const auto& documentSecurityOrigin'

> Source/WebCore/page/History.cpp:203
> +    // We allow sandboxed documents, `data:`/`file:` URLs, etc. to use
'pushState'/'replaceState' to modify the URL query and fragments.

The quotes here are all over the place. Just please use plain-old ASCII
single-quotes: '

>> Source/WebCore/page/History.cpp:208
>> +	    && !allowSandboxException)
> 
> I would put !allowSandboxException first so that lazy evaluation of the
expression is faster when allowSandboxException == true.

+1