A URL cannot have a username/password/port if its host is null [1], so adjust URL.cpp accordingly. Behavior matches Chrome and Firefox. [1] https://url.spec.whatwg.org/#cannot-have-a-username-password-port
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].
<rdar://problem/62889969>