[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

--- Comment #12 from Danyao Wang <danyao at chromium.org> ---
Thanks for the review. PTAL.

(In reply to Brent Fulgham from comment #9)
> Comment on attachment 335014 [details]
> 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'.

Done.

I added LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.php, which essentially tests the behavior in replace_state.html but for unique origin. This covers the IsUnique() part of |allowSandboxException|.

Is there a way to create a LayoutTest that is always as file:// scheme in run-webkit-tests? Then we can test the IsLocal() part of |allowSandboxException|.

I tried to create a pushstate-at-local-origin-denied.html test. But for some reason the file:// scheme is not detected inside testharness.js test when running using run-webkit-tests. It insists that document.URL is simply "pushstate-at-local-origin-denied.html", when I expected "file://<WebKitPath>/LayoutTests/security/pushstate-at-local-origin-denied.html"...

> 
> For these reasons, r- for now.
> 
> > Source/WebCore/page/History.cpp:52
> > +        for (unsigned i = 0; i < pathEnd; i++) {
> 
> WebKit style uses "++i"

Done.

> 
> >> 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&);

Done. I'm not sure what the 'friend' modifier is for. So omitted it for now.

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

Done.

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

Done.

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

Done.

-- 
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/20180310/881541fc/attachment.html>