[webkit-reviews] review granted: [Bug 127795] Implement (most of) URL API : [Attachment 222788] Now with fewer test failures.

Thu Jan 30 22:36:05 PST 2014

Alexey Proskuryakov <ap at webkit.org> has granted Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 127795: Implement (most of) URL API
https://bugs.webkit.org/show_bug.cgi?id=127795

Attachment 222788: Now with fewer test failures.
https://bugs.webkit.org/attachment.cgi?id=222788&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.