https://github.com/whatwg/url/issues/539
<rdar://problem/68252256>
Created attachment 408251 [details] Patch
Comment on attachment 408251 [details] Patch r=me
Comment on attachment 408251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408251&action=review > Source/WebCore/html/DOMURL.h:-43 > - static ExceptionOr<Ref<DOMURL>> create(const String& url, const DOMURL& base); Glad we are getting rid of this. I guess the specification changed since we originally wrote the code. > Source/WebCore/html/DOMURL.h:-44 > - static ExceptionOr<Ref<DOMURL>> create(const String& url, const URL& base); Does this "un-optimize" any call sites that were passing a URL before, or did those call sites just not exist?
Comment on attachment 408251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408251&action=review >> Source/WebCore/html/DOMURL.h:-44 >> - static ExceptionOr<Ref<DOMURL>> create(const String& url, const URL& base); > > Does this "un-optimize" any call sites that were passing a URL before, or did those call sites just not exist? Yes, this un-optimizes passing a DOM URL as the second parameter. It used to call the constructor with a const DOMURL& and after my first patch it turns it into a String and reparses it. I'll keep that optimization, which should be undetectable from JavaScript.
Created attachment 408257 [details] Patch
Comment on attachment 408257 [details] Patch I don’t understand how this fix works. If the baseURL is not valid, it seems like we’d get a TypeError exception, both before and after the patch. Where’s the fix?
Comment on attachment 408257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408257&action=review > Source/WebCore/html/DOMURL.cpp:-51 > - if (!base.isValid()) > - return Exception { TypeError }; Wouldn’t a better fix be to add a special case to allow null here. > Source/WebCore/html/DOMURL.cpp:-60 > - return create(url, base.isNull() ? aboutBlankURL() : URL { URL { }, base }); And remove the special case for null here?
Comment on attachment 408257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408257&action=review >> Source/WebCore/html/DOMURL.cpp:-51 >> - return Exception { TypeError }; > > Wouldn’t a better fix be to add a special case to allow null here. This is called from DOMURL::create(const String&, const DOMURL&) where we know the base is non-null and valid. That would be an unneeded check. It would probably be close to equivalent and the difference would probably be undetectable from JS, but I like the logic like this better. >> Source/WebCore/html/DOMURL.cpp:-60 >> - return create(url, base.isNull() ? aboutBlankURL() : URL { URL { }, base }); > > And remove the special case for null here? We need a check for null here because it is ok to not specify the base, in which case baseURL would be null which should not throw a type error.
http://trac.webkit.org/r266748
Comment on attachment 408257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408257&action=review > LayoutTests/inspector/unit-tests/url-utilities.html:576 > + test(new URL("#hash", "about:blank"), "about:blank"); This may cause issues in Web Inspector, specifically usage of `WI.urlWithoutFragment` and `parseURL`. I don't know whether it's actually possible for Web Inspector to be given a Url that's just a fragment/hash, but I can't say for sure that it will never happen.
Ok. One can never be sure that a change won't cause any issues. Let's keep our eyes out, and if we see any we can reconsider this.
*** Bug 216841 has been marked as a duplicate of this bug. ***