| Summary: | URLParser: Handle windows drive letters after two slashes in relative URLs according to spec | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | ||||||
| Priority: | P2 | ||||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Alex Christensen
2016-09-27 17:06:08 PDT
Created attachment 290028 [details]
Patch
Comment on attachment 290028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290028&action=review r=me > Source/WebCore/platform/URLParser.cpp:1457 > + bool windowsQuirk = c.codeUnitsSince(authorityOrHostBegin) == 2 && isWindowsDriveLetter(authorityOrHostBegin); Isn't the ==2 check redundant here since isWindowsDriveLetter only returns true if you've moved 2 points along? Or is the goal to quickly rule out the isWindowsDriveLetter(...) check? > Tools/TestWebKitAPI/Tests/WebCore/URLParser.cpp:643 > + checkRelativeURLDifferences("//C|/foo/bar", "file:///tmp/mock/path", I never knew "|" is OK there. Comment on attachment 290028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290028&action=review >> Source/WebCore/platform/URLParser.cpp:1457 >> + bool windowsQuirk = c.codeUnitsSince(authorityOrHostBegin) == 2 && isWindowsDriveLetter(authorityOrHostBegin); > > Isn't the ==2 check redundant here since isWindowsDriveLetter only returns true if you've moved 2 points along? > Or is the goal to quickly rule out the isWindowsDriveLetter(...) check? It's needed for correctness. isWindowsDriveLetter(iterator) returns true if the String the iterator is pointing to starts with C: but I need it to fail if it contains more than just C: This is covered by the test with //C:asdf/foo/bar >> Tools/TestWebKitAPI/Tests/WebCore/URLParser.cpp:643 >> + checkRelativeURLDifferences("//C|/foo/bar", "file:///tmp/mock/path", > > I never knew "|" is OK there. It's equivalent to a : in this case, but when canonicalized it becomes a :, so it's a syntax violation. |