[Webkit-unassigned] [Bug 183028] pushState and replaceState no longer works in local file

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=183028

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #335014|review?                     |review-
              Flags|                            |

--- 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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180307/bbd90390/attachment.html>