Summary: | A URL cannot have a username/password/port if its host is null | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rbuis> | ||||||||||||
Component: | New Bugs | Assignee: | Rob Buis <rbuis> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, cdumez, changseok, cmarcelo, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Safari Technology Preview | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Rob Buis
2020-05-03 13:26:59 PDT
Created attachment 398327 [details]
Patch
Comment on attachment 398327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review Patch looks good. > Source/WTF/wtf/URL.cpp:453 > + if (hostStart() == m_hostEnd || protocolIs("file")) Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug. Comment on attachment 398327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review >> Source/WTF/wtf/URL.cpp:453 >> + if (hostStart() == m_hostEnd || protocolIs("file")) > > Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug. Also optimal solution is to move this check into a private member function marked inline in .cpp instead of duplicating it, especially if the check will have 3 disjunction <-- meaning it's becoming complicated. Created attachment 398355 [details]
Patch
Created attachment 398379 [details]
Patch
Comment on attachment 398327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398327&action=review >>> Source/WTF/wtf/URL.cpp:453 >>> + if (hostStart() == m_hostEnd || protocolIs("file")) >> >> Ok as-is. No change needed. Spec also mentions about checking cannot-be-a-base-URL flag. This is equivalent to checking !isHierarchical() (very quick read + intuition) and the optimal solution would do that. <--- function assets there is :, which I think is covered by m_isValid. If not, then need to check that too to avoid the assert triggering in debug. > > Also optimal solution is to move this check into a private member function marked inline in .cpp instead of duplicating it, especially if the check will have 3 disjunction <-- meaning it's becoming complicated. Thanks for the review! Today I found out cannot-be-a-base-URL is already handled in URLDecomposition, so I moved my code there. Created attachment 398388 [details]
Patch
Comment on attachment 398388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398388&action=review > Source/WTF/wtf/URL.h:169 > + WTF_EXPORT_PRIVATE bool hasEmptyHost() const; This is not significantly more efficient than writing: fullURL.host().isEmpty() That’s one of the benefits of StringView. Not sure we need to add the new function. The name seems to imply some difference between having "no host" and "empty host". > Source/WebCore/html/URLDecomposition.cpp:61 > + if (fullURL.hasEmptyHost() || fullURL.cannotBeABaseURL() || fullURL.protocolIs("file")) Starting to think we need a URL::protocolIsFile function. Also irritated that there’s no concept here, like "cannot be base URL". Created attachment 398510 [details]
Patch
Committed r261173: <https://trac.webkit.org/changeset/261173> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398510 [details]. |