Bug 127795 – Implement (most of) URL API

• Component: New Bugs
• Status: RESOLVED FIXED

[Maciej Stachowiak]

Implement (most of) URL API

[Maciej Stachowiak]

Created attachment 222485 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Maciej Stachowiak]

Created attachment 222636 [details]
Patch

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

Created attachment 222656 [details]
Patch

[Maciej Stachowiak]

I think the latest version is ready for review.

[Sam Weinig]

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?

[Anders Carlsson]

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.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

Created attachment 222742 [details]
Patch

[Maciej Stachowiak]

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

[Maciej Stachowiak]

Created attachment 222745 [details]
Patch

[Maciej Stachowiak]

Created attachment 222746 [details]
Patch

[Alexey Proskuryakov]

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

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

Follow-up bug for the setHostname() issue:
https://bugs.webkit.org/show_bug.cgi?id=127970

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

Created attachment 222764 [details]
Patch

[Maciej Stachowiak]

Created attachment 222768 [details]
Addressing the latest review feedback; also fix build

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Maciej Stachowiak]

Created attachment 222788 [details]
Now with fewer test failures.

[Alexey Proskuryakov]

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.

[Maciej Stachowiak]

(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.

[Alexey Proskuryakov]

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.

[Maciej Stachowiak]

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).

[Maciej Stachowiak]

Follow-up bugs are all listed here: https://bugs.webkit.org/show_bug.cgi?id=128023

[Maciej Stachowiak]

Committed r163208: <http://trac.webkit.org/changeset/163208>

[Alexey Proskuryakov]

Updated a test result in <http://trac.webkit.org/changeset/163220>.

[David Kilzer (:ddkilzer)]

<rdar://problem/15184406>