Implement (most of) URL API
Created attachment 222485 [details] Patch
Attachment 222485 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 222485 [details] Patch Attachment 222485 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6654629710397440 New failing tests: js/dom/constructor-length.html fast/dom/constructed-objects-prototypes.html fast/dom/DOMURL/check-instanceof-domurl-functions.html
Created attachment 222512 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 222485 [details] Patch Attachment 222485 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6091933696917504 New failing tests: js/dom/constructor-length.html fast/dom/constructed-objects-prototypes.html fast/dom/DOMURL/check-instanceof-domurl-functions.html
Created attachment 222514 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 222636 [details] Patch
Updated patch to fix broken tests, add new tests, and hopefully fix non-Mac builds. Still need to add tests for 'username' and 'password' properties.
Created attachment 222656 [details] Patch
I think the latest version is ready for review.
Comment on attachment 222656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222656&action=review Notes: 1) It doesn't seem like you are doing the [EnforceUTF16] step. > Source/WebCore/html/DOMURL.cpp:96 > + RefPtr<SecurityOrigin> origin = SecurityOrigin::create(href()); > + return origin->toString(); I am not sure if this does what we want. There are many special cases inside both the creation and stringification here which are generally internal. > Source/WebCore/html/DOMURL.cpp:101 > + return href().protocol() + ":"; I think this would be a bit more effiicient if you used ':', rather then ":". > Source/WebCore/html/DOMURL.cpp:151 > + return value.substring(portStart, portEnd - portStart).toUInt(); What happens if the port is greater than UINT_MAX? > Source/WebCore/html/DOMURL.cpp:175 > + // http://dev.w3.org/html5/spec/infrastructure.html#url-decomposition-idl-attributes > + // specifically goes against RFC 3986 (p3.2) and > + // requires setting the port to "0" if it is set to empty string. > + url.setHostAndPort(value.substring(0, separator + 1) + "0"); Should we really do this? Will it ever be right?
Comment on attachment 222656 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222656&action=review > Source/WebCore/html/DOMURL.h:93 > +protected: Why are these protected and not private? > Source/WebCore/html/DOMURL.h:95 > + DOMURL(const String& url, const DOMURL* base, ExceptionCode&); I think DOMURL* should be a reference here since it can't be null.
(In reply to comment #11) > (From update of attachment 222656 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222656&action=review > > Notes: > 1) It doesn't seem like you are doing the [EnforceUTF16] step. I deliberately tried not to make it different from HTMLAnchorElement's existing WebKit behavior, in this patch. Next steps will be to share the code, and then try to match the actual parsing/reassembly requirements of the URL spec. I'll file a follow-up bug. > > > Source/WebCore/html/DOMURL.cpp:96 > > + RefPtr<SecurityOrigin> origin = SecurityOrigin::create(href()); > > + return origin->toString(); > > I am not sure if this does what we want. There are many special cases inside both the creation and stringification here which are generally internal. It's identical to what HTMLAnchorElement's origin() method does, and it's defined to be the same thing by the URL spec. If there's specific bugs with this approach that you're aware of I can file follow-up bugs. > > > Source/WebCore/html/DOMURL.cpp:101 > > + return href().protocol() + ":"; > > I think this would be a bit more effiicient if you used ':', rather then ":". OK, will change (though I'm surprised since all info to make it equally performant should be available at compile-time). > > > Source/WebCore/html/DOMURL.cpp:151 > > + return value.substring(portStart, portEnd - portStart).toUInt(); > > What happens if the port is greater than UINT_MAX? This matches what HTMLAnchorElement does. I'm not sure what is actually correct, but I'll make both DTRT once I unify the code. Will file follow-up bug. > > > Source/WebCore/html/DOMURL.cpp:175 > > + // http://dev.w3.org/html5/spec/infrastructure.html#url-decomposition-idl-attributes > > + // specifically goes against RFC 3986 (p3.2) and > > + // requires setting the port to "0" if it is set to empty string. > > + url.setHostAndPort(value.substring(0, separator + 1) + "0"); > > Should we really do this? Will it ever be right? It matches HTMLAnchorElement. I'm pretty sure it's wrong per the latest spec and doesn't match latest browsers. Will file follow-up bug.
(In reply to comment #12) > (From update of attachment 222656 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222656&action=review > > > Source/WebCore/html/DOMURL.h:93 > > +protected: > > Why are these protected and not private? Blind copying. I'll make them private. > > > Source/WebCore/html/DOMURL.h:95 > > + DOMURL(const String& url, const DOMURL* base, ExceptionCode&); > > I think DOMURL* should be a reference here since it can't be null. OK. Although I don't think I can do that at the ::create() level since the bindings call it and don't know to expect a reference. So for correctness the ::create() call would need an ASSERT for the case of possible callers other than the bindings.
Created attachment 222742 [details] Patch
Follow-up bugs: https://bugs.webkit.org/show_bug.cgi?id=127953 https://bugs.webkit.org/show_bug.cgi?id=127954 https://bugs.webkit.org/show_bug.cgi?id=127955 https://bugs.webkit.org/show_bug.cgi?id=127956 https://bugs.webkit.org/show_bug.cgi?id=127957 https://bugs.webkit.org/show_bug.cgi?id=127958
Created attachment 222745 [details] Patch
Created attachment 222746 [details] Patch
Comment on attachment 222746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222746&action=review This mostly makes sense to me, except for one biggie that makes further review hard - I don't understand base URL handling. > Source/WebCore/html/DOMURL.cpp:46 > + return adoptRef(new DOMURL(url, base, ec)); It might be slightly better to return nullptr with an exception, not sure. > Source/WebCore/html/DOMURL.cpp:51 > +PassRefPtr<DOMURL> DOMURL::create(const String& url, const DOMURL* base, ExceptionCode& ec) > +{ > + ASSERT(base); Can it be passed as reference? > Source/WebCore/html/DOMURL.cpp:62 > + : m_baseURL(URL(), base) > + , m_url(m_baseURL, url) Hopefully we have a lot of tests for invalid base URLs to go with this. URL::init code is a bit unconfident about what it's doing: // Allow resolutions with a null or empty base URL, but not with any other invalid one. // FIXME: Is this a good rule? > Source/WebCore/html/DOMURL.cpp:98 > + if (!m_url.isValid()) { > + m_url = URL(); I'm not sure if this is what the spec calls for. Shouldn't "input" be made equal to the invalid URL? > Source/WebCore/html/DOMURL.cpp:209 > + // Before setting new value: > + // Remove all leading U+002F SOLIDUS ("/") characters. I don't see any text requiring this in the URL spec. Why is this needed here, but not everywhere we use url.setHost()? > Source/WebCore/html/DOMURL.cpp:293 > + return AtomicString(String("#" + fragmentIdentifier)); Like elsewhere, adding a character is faster than adding a single character string. Apparently you can't have both const char* and const char[N] specializations for a template, which is why we lose the size at compile time, and use strlen. > Source/WebCore/html/DOMURL.h:98 > + URL m_baseURL; I'm having huge difficulty understanding what URL standard tries to say about base URL. Step 4 of constructor algorithm says "Let result's get the base return base". But "get the base" is an algorithm that must be defined by an external spec. How can we change what some unknown algorithm returns? > Source/WebCore/html/URLUtils.idl:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY APPLE COMPUTER
(In reply to comment #19) > (From update of attachment 222746 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222746&action=review > > This mostly makes sense to me, except for one biggie that makes further review hard - I don't understand base URL handling. What's the specific question about it? The basic concept is that if you construct a relative URL, or assign href to a relative value, you resolve against the base. Unlike HTMLAnchorElement, which resolves against the document base, DOMURL defaults to "about:blank" and lets you specify an explicit base. > > > Source/WebCore/html/DOMURL.cpp:46 > > + return adoptRef(new DOMURL(url, base, ec)); > > It might be slightly better to return nullptr with an exception, not sure. The exception is only really important for the JS binding. Not sure it is worth the extra effort to return nullptr but it can be done. > > > Source/WebCore/html/DOMURL.cpp:51 > > +PassRefPtr<DOMURL> DOMURL::create(const String& url, const DOMURL* base, ExceptionCode& ec) > > +{ > > + ASSERT(base); > > Can it be passed as reference? I don't know of a way to get the bindings generator to pass a reference instead of a pointer (this ::create() function gets called by the generated constructor). > > > Source/WebCore/html/DOMURL.cpp:62 > > + : m_baseURL(URL(), base) > > + , m_url(m_baseURL, url) > > Hopefully we have a lot of tests for invalid base URLs to go with this. URL::init code is a bit unconfident about what it's doing: > > // Allow resolutions with a null or empty base URL, but not with any other invalid one. > // FIXME: Is this a good rule? > > > Source/WebCore/html/DOMURL.cpp:98 > > + if (!m_url.isValid()) { > > + m_url = URL(); > > I'm not sure if this is what the spec calls for. Shouldn't "input" be made equal to the invalid URL? It says "url" should be made null. I was not able to figure out what other effect "input" would have but maybe if I read closer, I'll figure out the intended observable effect. > > > Source/WebCore/html/DOMURL.cpp:209 > > + // Before setting new value: > > + // Remove all leading U+002F SOLIDUS ("/") characters. > > I don't see any text requiring this in the URL spec. Why is this needed here, but not everywhere we use url.setHost()? > This is copied from HTMLAnchorElement. I don't think it's right per the URL spec, but I'd like to fix it after I make the code shared. Will fix follow-up bug. I think what the spec calls for is %-escaping the "/" (effectively). > > Source/WebCore/html/DOMURL.cpp:293 > > + return AtomicString(String("#" + fragmentIdentifier)); > > Like elsewhere, adding a character is faster than adding a single character string. Will fix. > > Apparently you can't have both const char* and const char[N] specializations for a template, which is why we lose the size at compile time, and use strlen. > > > Source/WebCore/html/DOMURL.h:98 > > + URL m_baseURL; > > I'm having huge difficulty understanding what URL standard tries to say about base URL. > > Step 4 of constructor algorithm says "Let result's get the base return base". But "get the base" is an algorithm that must be defined by an external spec. How can we change what some unknown algorithm returns? URLUtils is a generic interface that is also implemented by other things (e.g. HTMLAnchorElement and Location) which may have their own definition of "get the base". For URL itself, "get the base" is defined to return the base that was specified to the constructor. > > > Source/WebCore/html/URLUtils.idl:13 > > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > > APPLE COMPUTER Will fix.
Follow-up bug for the setHostname() issue: https://bugs.webkit.org/show_bug.cgi?id=127970
Comment on attachment 222746 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222746&action=review >>> Source/WebCore/html/DOMURL.cpp:293 >>> + return AtomicString(String("#" + fragmentIdentifier)); >> >> Like elsewhere, adding a character is faster than adding a single character string. >> >> Apparently you can't have both const char* and const char[N] specializations for a template, which is why we lose the size at compile time, and use strlen. > > Will fix. Turns out the char, String specialization of operator+ doesn't exist, so I can't make this change at the moment.
Created attachment 222764 [details] Patch
Created attachment 222768 [details] Addressing the latest review feedback; also fix build
Comment on attachment 222768 [details] Addressing the latest review feedback; also fix build Attachment 222768 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5321528434491392 New failing tests: fast/dom/DOMURL/set-href-attribute-host.html fast/dom/DOMURL/set-href-attribute-pathname.html fast/dom/DOMURL/set-href-attribute-hostname.html fast/dom/DOMURL/set-href-attribute-hash.html
Created attachment 222779 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 222768 [details] Addressing the latest review feedback; also fix build Attachment 222768 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5256801935163392 New failing tests: fast/dom/DOMURL/set-href-attribute-host.html fast/dom/DOMURL/set-href-attribute-pathname.html fast/dom/DOMURL/set-href-attribute-hostname.html fast/dom/DOMURL/set-href-attribute-hash.html
Created attachment 222780 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 222788 [details] Now with fewer test failures.
Comment on attachment 222788 [details] Now with fewer test failures. View in context: https://bugs.webkit.org/attachment.cgi?id=222788&action=review r=me assuming that you would prefer to fix invalid URL handling in a separate patch. Between the comments below, my only strong request to address before landing is to fix regression tests, please don't split anything into script-tests/*.js files. > Source/WebCore/html/DOMURL.cpp:77 > + : m_baseURL(blankURL()) Did you verify that URL("foobar") doesn't result in "about:foobar"? I couldn't easily find that within regression tests. > Source/WebCore/html/DOMURL.cpp:108 > + RefPtr<SecurityOrigin> origin = SecurityOrigin::create(href()); This function, and all other functions that call href(), should first verify that it is a valid URL. The URL standard says: "The origin attribute must run these steps: 1. If url is null, return the empty string." Besides, not a lot of WebCore code is prepared to handle invalid URLs, certainly not SecurityOrigin. Please add regression tests for these fixes. Finally, all calls to href() should just use m_url. > Source/WebCore/html/DOMURL.cpp:120 > + URL url = href(); > + url.setProtocol(value); Setters are particularly tricky - the URL Standard says that they must be no-op when url is invalid, but WebCore URL class can turn an invalid URL into a valid one. Which may really be a bug in URL, not here. > Source/WebCore/html/DOMURL.cpp:164 > + return value.substring(portStart, portEnd - portStart).toUInt(); It may be helpful to have FIXMEs for bugs that you filed, depending on how soon you intend to get to these. > LayoutTests/fast/dom/DOMURL/get-href-attribute-port.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> <!DOCTYPE html> You can use our make-new-script-test script that creates boilerplate for script tests. > LayoutTests/fast/dom/DOMURL/get-href-attribute-port.html:7 > +<script src="script-tests/get-href-attribute-port.js"></script> Please don't add any new tests whose meaningful content is outside the main HTML file, with the only exception being pure js tests meant to be run without even building WebCore. It is extremely annoying and wasteful to be several steps away from seeing what a given test does. > LayoutTests/fast/dom/DOMURL/script-tests/TEMPLATE.html:10 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > +<html> > +<head> > +<script src="../../../resources/js-test-pre.js"></script> > +</head> > +<body> > +<script src="YOUR_JS_FILE_HERE"></script> > +<script src="../../../resources/js-test-post.js"></script> > +</body> > +</html> Please don't add this file.
(In reply to comment #30) > (From update of attachment 222788 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222788&action=review > > r=me assuming that you would prefer to fix invalid URL handling in a separate patch. > > Between the comments below, my only strong request to address before landing is to fix regression tests, please don't split anything into script-tests/*.js files. All right, I'll recombine the tests. > > > Source/WebCore/html/DOMURL.cpp:77 > > + : m_baseURL(blankURL()) > > Did you verify that URL("foobar") doesn't result in "about:foobar"? I couldn't easily find that within regression tests. new URL("foobar") throws. I have other tests for invalid attempts to call the constructor throwing but not specifically something that could look like a relative URL with a relative-supporting base. I will add one. > > > Source/WebCore/html/DOMURL.cpp:108 > > + RefPtr<SecurityOrigin> origin = SecurityOrigin::create(href()); > > This function, and all other functions that call href(), should first verify that it is a valid URL. The URL standard says: "The origin attribute must run these steps: 1. If url is null, return the empty string." HTMLAnchorElement code doesn't do that. I'll file a bug but I'm not entirely sure this is necessary in cases other than origin (I do think the origin property will do the wrong thing as written). > > Besides, not a lot of WebCore code is prepared to handle invalid URLs, certainly not SecurityOrigin. Really it should be - nothing protects against invalid URLs entering the system, possibly with the exception of a URL that you know you successfully loaded from. > > Please add regression tests for these fixes. > > Finally, all calls to href() should just use m_url. I'd rather not do that. My plan for sharing code with HTMLAnchorElement is to make a template that takes a class parameter which implements href() and setHref() methods. I think that will be sufficient for implementing all the other methods. At least, it seems to work so far. > > > Source/WebCore/html/DOMURL.cpp:120 > > + URL url = href(); > > + url.setProtocol(value); > > Setters are particularly tricky - the URL Standard says that they must be no-op when url is invalid, but WebCore URL class can turn an invalid URL into a valid one. Which may really be a bug in URL, not here. I'll file a follow-up bug for this. > > > Source/WebCore/html/DOMURL.cpp:164 > > + return value.substring(portStart, portEnd - portStart).toUInt(); > > It may be helpful to have FIXMEs for bugs that you filed, depending on how soon you intend to get to these. Within the next few days or weeks most likely. Would rather not add FIXMEs since bugzilla already has a record of the problems. > > > LayoutTests/fast/dom/DOMURL/get-href-attribute-port.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > <!DOCTYPE html> > > You can use our make-new-script-test script that creates boilerplate for script tests. Most of the tests are copied from HTMLAnchorElement tests, but I'll gladly convert them to the more modern style. > > > LayoutTests/fast/dom/DOMURL/get-href-attribute-port.html:7 > > +<script src="script-tests/get-href-attribute-port.js"></script> > > Please don't add any new tests whose meaningful content is outside the main HTML file, with the only exception being pure js tests meant to be run without even building WebCore. > > It is extremely annoying and wasteful to be several steps away from seeing what a given test does. > > > LayoutTests/fast/dom/DOMURL/script-tests/TEMPLATE.html:10 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > +<html> > > +<head> > > +<script src="../../../resources/js-test-pre.js"></script> > > +</head> > > +<body> > > +<script src="YOUR_JS_FILE_HERE"></script> > > +<script src="../../../resources/js-test-post.js"></script> > > +</body> > > +</html> > > Please don't add this file. Also copied from HTMLAnchorElement tests. Will remove.
Comment on attachment 222788 [details] Now with fewer test failures. View in context: https://bugs.webkit.org/attachment.cgi?id=222788&action=review >>> Source/WebCore/html/DOMURL.cpp:108 >>> + RefPtr<SecurityOrigin> origin = SecurityOrigin::create(href()); >> >> This function, and all other functions that call href(), should first verify that it is a valid URL. The URL standard says: "The origin attribute must run these steps: 1. If url is null, return the empty string." >> >> Besides, not a lot of WebCore code is prepared to handle invalid URLs, certainly not SecurityOrigin. >> >> Please add regression tests for these fixes. >> >> Finally, all calls to href() should just use m_url. > > HTMLAnchorElement code doesn't do that. I'll file a bug but I'm not entirely sure this is necessary in cases other than origin (I do think the origin property will do the wrong thing as written). Fair enough, other getters do look like they should be OK indeed. Tests for each one would be useful anyway.
I filed the following follow-up bugs: https://bugs.webkit.org/show_bug.cgi?id=128020 https://bugs.webkit.org/show_bug.cgi?id=128021 I also made all of the requested changes (additional tests, flattening and modernizing test files, etc).
Follow-up bugs are all listed here: https://bugs.webkit.org/show_bug.cgi?id=128023
Committed r163208: <http://trac.webkit.org/changeset/163208>
Updated a test result in <http://trac.webkit.org/changeset/163220>.
<rdar://problem/15184406>