Bug 76816 - Implement the URL decomposition IDL attributes
: Implement the URL decomposition IDL attributes
Status: ASSIGNED
: WebKit
HTML DOM
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
: http://dvcs.w3.org/hg/url/raw-file/ti...
:
: 78478
: 71968 74385 86223
  Show dependency treegraph
 
Reported: 2012-01-23 01:26 PST by
Modified: 2014-02-05 11:14 PST (History)


Attachments
Patch (37.17 KB, patch)
2012-01-23 02:53 PST, Kaustubh Atrawalkar
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Updated Patch (37.16 KB, patch)
2012-01-24 23:30 PST, Kaustubh Atrawalkar
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Updated Patch (37.02 KB, patch)
2012-02-15 02:35 PST, Kaustubh Atrawalkar
haraken: review-
Review Patch | Details | Formatted Diff | Diff
Updated Patch (37.26 KB, patch)
2012-02-23 04:31 PST, Kaustubh Atrawalkar
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Updated Patch (36.25 KB, patch)
2012-02-24 06:06 PST, Kaustubh Atrawalkar
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Updated Patch (36.36 KB, patch)
2012-03-07 10:09 PST, Kaustubh Atrawalkar
arv: review-
arv: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated Patch (45.67 KB, patch)
2012-03-12 05:06 PST, Kaustubh Atrawalkar
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated Patch (52.23 KB, patch)
2012-03-14 04:13 PST, Kaustubh Atrawalkar
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.31 KB, patch)
2012-05-09 05:07 PST, Kaustubh Atrawalkar
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.26 KB, patch)
2012-07-06 03:34 PST, Kaustubh Atrawalkar
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (342.43 KB, application/zip)
2012-07-06 04:19 PST, WebKit Review Bot
no flags Details
Fix for failing test case (52.20 KB, patch)
2012-07-25 06:10 PST, Kaustubh Atrawalkar
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-01-23 01:26:18 PST
The URL decomposition IDL attributes are the protocol, username, password, host, hostname, port, pathname, search, hash, and href attributes. Implement the IDL attributes and their setter/getter functions.
------- Comment #1 From 2012-01-23 02:53:09 PST -------
Created an attachment (id=123534) [details]
Patch
------- Comment #2 From 2012-01-23 12:03:13 PST -------
(From update of attachment 123534 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=123534&action=review

> Source/WebCore/html/DOMURL.cpp:180
> +    m_href = KURL(KURL(), url);
> +    // Resolve the URL relative to baseURL if is provide.
> +    if (!baseURL.isEmpty())
> +        m_href = KURL(m_href, baseURL);

Why do we parse the URL twice?  That doesn't seem right.

> Source/WebCore/html/DOMURL.cpp:193
> +    if (m_href.hostEnd() == m_href.pathStart())
> +        return m_href.host();
> +    if (isDefaultPortForProtocol(m_href.port(), m_href.protocol()))
> +        return m_href.host();
> +    return m_href.host() + ":" + String::number(m_href.port());

Should this code be shared with Location and HTMLAnchorElement?
------- Comment #3 From 2012-01-24 03:35:58 PST -------
(In reply to comment #2)
> 
> Why do we parse the URL twice?  That doesn't seem right.
> 

To resolve the URL if base URL is given. I think the double parsing of the url given can be avoided still as u said. Will try to fix that.

> 
> Should this code be shared with Location and HTMLAnchorElement?

I am not sure how we can share the code.
------- Comment #4 From 2012-01-24 05:26:39 PST -------
DOMURL::DOMURL(const String& url, const String& baseURL)
{
    KURL parsedBaseURL = (baseURL.isEmpty()) ? KURL() : KURL(KURL(), baseURL);

    // Resolve the URL relative to baseURL if is provide.
    m_href = KURL(parsedBaseURL, url);
}


Does this change looks good for not parsing URL twice?
------- Comment #5 From 2012-01-24 11:20:38 PST -------
Why not just:

m_href = KURL(KURL(KURL(), baseURL), url);

> > Should this code be shared with Location and HTMLAnchorElement?
> 
> I am not sure how we can share the code.

This code looks very similar:

http://trac.webkit.org/browser/trunk/Source/WebCore/page/Location.cpp#L74

Perhaps we need a common base class?  Maybe a small object to composite into these classes?  The thing we're trying to avoid is to have the same logic repeated in multiple places ("Don't Repeat Yourself").
------- Comment #6 From 2012-01-24 23:03:32 PST -------
(In reply to comment #5)
> Why not just:
> 
> m_href = KURL(KURL(KURL(), baseURL), url);
> 
OK. Fixed this.

> > > Should this code be shared with Location and HTMLAnchorElement?
> > 
> > I am not sure how we can share the code.
> 
> This code looks very similar:
> 
> http://trac.webkit.org/browser/trunk/Source/WebCore/page/Location.cpp#L74
> 
> Perhaps we need a common base class?  Maybe a small object to composite into these classes?  The thing we're trying to avoid is to have the same logic repeated in multiple places ("Don't Repeat Yourself").

Yes I agree there are many functions which can be shared across DOMURL, HTMLAnchorElement and Location. But, IMHO, if you allow, I can address this creation of base class or may be even a interface to create shared functions for all these 3 classes.
------- Comment #7 From 2012-01-24 23:26:58 PST -------
> I can address this creation of base class or may be even a interface to create shared functions for all these 3 classes.

That sounds like a good plan.  We should do that before duplicating this code again though.
------- Comment #8 From 2012-01-24 23:30:17 PST -------
Created an attachment (id=123889) [details]
Updated Patch

Will be addressing the creating of base class issue in another fix as this will change many things here.
------- Comment #9 From 2012-01-24 23:42:31 PST -------
(From update of attachment 123889 [details])
As I wrote above, we shouldn't be introducing all this copy/paste code.  Making this change will require some refactoring to share code between the now three places it appears.
------- Comment #10 From 2012-02-15 02:35:50 PST -------
Created an attachment (id=127144) [details]
Updated Patch

As per comments from Alexey https://bugs.webkit.org/show_bug.cgi?id=78478#c9 as base class is not required, updated the patch with latest changes.
------- Comment #11 From 2012-02-15 04:11:55 PST -------
Attachment 127144 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
Index mismatch: 0dfd183742a71cb5de5dadc3ae177fc72b63a194 != 9cdcda984def14b8bf8a32b6da6784c8a6ef7b3a
rereading 8567f8d3c2539a28a496edaf1048483e973975c2
    M    LayoutTests/fast/forms/radio-nested-labels.html
    M    LayoutTests/ChangeLog
107798 = 3671b2d23de7ade4cb1d1e78a3f6f7673db6a6c9 already exists! Why are we refetching it?
 at /usr/lib/git-core/git-svn line 5210

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #12 From 2012-02-16 04:01:30 PST -------
Can you review the patch? Thanks.
------- Comment #13 From 2012-02-21 23:14:33 PST -------
(From update of attachment 127144 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=127144&action=review

> Source/WebCore/ChangeLog:7
> +

Please describe what your patch is doing.
Isn't there any spec to support this change?

> Source/WebCore/html/DOMURL.idl:33
> +        Replaceable

[Replaceable] cannot be specified on an interface. What did you intend by [Replaceable]? Please see the WebKit IDL document (https://trac.webkit.org/wiki/WebKitIDL).

> Source/WebCore/html/DOMURL.idl:45
> +        readonly attribute DOMString origin;

Maybe "readonly attribute [Reflect, URL] DOMString origin"? Please confirm the spec and the WebKit IDL document.

> Source/WebCore/html/DOMURL.idl:46
> +        stringifier attribute DOMString href;

stringifier is not implemented by any code generators. Maybe "attribute [Reflect, URL] DOMString href"?

> Source/WebCore/html/DOMURL.idl:47
> +        [DontEnum] DOMString toString();

[DontEnum] is renamed to [NotEnumerable]
------- Comment #14 From 2012-02-21 23:33:48 PST -------
Thanks Haraken for the review. Here is the spec which i m implementing - http://dvcs.w3.org/hg/url/raw-file/tip/Overview.html#interface-url. I will update the patch with your review comments.
------- Comment #15 From 2012-02-21 23:43:51 PST -------
(In reply to comment #13)

> > Source/WebCore/html/DOMURL.idl:33
> > +        Replaceable
> 
> [Replaceable] cannot be specified on an interface. What did you intend by [Replaceable]? Please see the WebKit IDL document (https://trac.webkit.org/wiki/WebKitIDL).
Yes, with replacable, the original URL value should be retained.

> 
> > Source/WebCore/html/DOMURL.idl:45
> > +        readonly attribute DOMString origin;
> 
> Maybe "readonly attribute [Reflect, URL] DOMString origin"? Please confirm the spec and the WebKit IDL document.
> 
As per spec it seems right.

> > Source/WebCore/html/DOMURL.idl:46
> > +        stringifier attribute DOMString href;
> 
> stringifier is not implemented by any code generators. Maybe "attribute [Reflect, URL] DOMString href"?
>
Yes, will change that.

> > Source/WebCore/html/DOMURL.idl:47
> > +        [DontEnum] DOMString toString();
> 
> [DontEnum] is renamed to [NotEnumerable]

Sorry missed that. will update in new patch.
------- Comment #16 From 2012-02-21 23:45:11 PST -------
(In reply to comment #14)
> Thanks Haraken for the review. Here is the spec which i m implementing - http://dvcs.w3.org/hg/url/raw-file/tip/Overview.html#interface-url. I will update the patch with your review comments.

Ah, the spec is really in draft...:-)

At least, I would guess that "href" and "origin" should be [Reflect, URL]. Please refer to the spec of HTMLAreaElement for reference.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-map-element.html#the-area-element
------- Comment #17 From 2012-02-21 23:59:31 PST -------
(In reply to comment #15)
> (In reply to comment #13)
> > [Replaceable] cannot be specified on an interface. What did you intend by [Replaceable]? Please see the WebKit IDL document (https://trac.webkit.org/wiki/WebKitIDL).
> Yes, with replacable, the original URL value should be retained.

Ah... the spec of URL seems wrong.
http://dvcs.w3.org/hg/url/raw-file/tip/Overview.html#interface-url

According to the Web IDL spec, the [Replaceable] extended attribute should appear on a read only attribute.
http://dev.w3.org/2006/webapi/WebIDL/#Replaceable

Maybe we need to first clarify what the spec is intended to do. Would you please file a bug of the spec?
------- Comment #18 From 2012-02-22 00:03:16 PST -------
(In reply to comment #17)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > [Replaceable] cannot be specified on an interface. What did you intend by [Replaceable]? Please see the WebKit IDL document (https://trac.webkit.org/wiki/WebKitIDL).
> > Yes, with replacable, the original URL value should be retained.
> 
> Maybe we need to first clarify what the spec is intended to do. Would you please file a bug of the spec?

Oh, the spec editor is Adam!

abarth: Would you please clarify which attribute needs [Reflect], [URL] or [Replaceable]?
------- Comment #19 From 2012-02-22 00:44:14 PST -------
> Oh, the spec editor is Adam!

I'm not surprised the spec has bugs.

> abarth: Would you please clarify which attribute needs [Reflect], [URL] or [Replaceable]?

I don't think it needs any of those attributes.  [Reflect] is related to DOM attributes, but, in this case, there is no DOM node.
------- Comment #20 From 2012-02-23 04:31:25 PST -------
Created an attachment (id=128459) [details]
Updated Patch

Haraken, I have updated the patch to remove [Replaceable] and strigifier as suggested by Adam. Also updated the ChangeLog.
------- Comment #21 From 2012-02-23 06:09:46 PST -------
(From update of attachment 128459 [details])
r+ for all IDL changes and test cases. But I'd like to delegate the review to Adam, since I am not familiar with how DOMURL should behave. Thanks for the patch!
------- Comment #22 From 2012-02-23 11:42:55 PST -------
(From update of attachment 128459 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=128459&action=review

> Source/WebCore/html/DOMURL.cpp:166
> +void DOMURL::setHost(const String& value)
> +{
> +    if (value.isEmpty())
> +        return;
> +
> +    if (!m_href.canSetHostOrPort())
> +        return;
> +
> +    size_t colon = value.find(':');
> +    if (!colon)
> +        return;
> +
> +    if (colon == notFound)
> +        m_href.setHostAndPort(value);
> +    else {
> +        unsigned portStart = colon + 1;
> +        unsigned portEnd = portStart;
> +
> +        while (isASCIIDigit(value[portEnd]))
> +            ++portEnd;
> +
> +        unsigned port = value.substring(portStart, portEnd - portStart).toUInt();
> +        if (!port)
> +            m_href.setHostAndPort(value.substring(0, colon + 1) + "0");
> +        else {
> +            if (isDefaultPortForProtocol(port, m_href.protocol()))
> +                m_href.setHostAndPort(value.substring(0, colon));
> +            else
> +                m_href.setHostAndPort(value.substring(0, portEnd));
> +        }
> +    }
> +}

Why is this function so different from http://trac.webkit.org/browser/trunk/Source/WebCore/page/Location.cpp#L155 ?

> Source/WebCore/html/DOMURL.cpp:202
> +void DOMURL::setPort(const String& value)
> +{
> +    if (!m_href.canSetHostOrPort())
> +        return;
> +
> +    unsigned port = value.toUInt();
> +    if (isDefaultPortForProtocol(port, m_href.protocol()))
> +        m_href.removePort();
> +    else
> +        m_href.setPort(value.toUInt());
> +}

This function also seems different from http://trac.webkit.org/browser/trunk/Source/WebCore/page/Location.cpp#L173 .  For example, why does one check against 0xFFFF but the other doesn't?

> Source/WebCore/html/DOMURL.cpp:222
> +    newSearch.replace('#', "%23");

Why does this function make this change bug http://trac.webkit.org/browser/trunk/Source/WebCore/page/Location.cpp#L195 doesn't.
------- Comment #23 From 2012-02-24 01:38:43 PST -------
I was refereeing to HTMLAnchorElement rather than Location for implementing the functions. I will try to fix these function as per Location. Thanks.
------- Comment #24 From 2012-02-24 06:06:27 PST -------
Created an attachment (id=128723) [details]
Updated Patch

Updated patch with suggested function migrated to match Location APIs.
------- Comment #25 From 2012-02-24 10:08:40 PST -------
Why does HTMLAnchorElement and Location have such different code?  Should they be the same, or is there a reason that they're different?
------- Comment #26 From 2012-02-26 03:29:51 PST -------
(In reply to comment #25)
> Why does HTMLAnchorElement and Location have such different code?  Should they be the same, or is there a reason that they're different?

I am sure they both should be same. Both of the DOM interfaces implements same URL decomposition attributes. IMHO due to different requirement of test cases there is little bit different implementation. I will try to fix those as well.
------- Comment #27 From 2012-02-26 09:33:23 PST -------
(From update of attachment 128723 [details])
IMHO, we should dig through the history to understand why they're that way and we should test other browsers to see if they have the same differences.  This stuff requires careful study before being changed.
------- Comment #28 From 2012-02-28 00:50:04 PST -------
(In reply to comment #27)
> (From update of attachment 128723 [details] [details])
> IMHO, we should dig through the history to understand why they're that way and we should test other browsers to see if they have the same differences.  This stuff requires careful study before being changed.

Hi Adam.

I have little study about all URL decomposition attributes used in Location & HTML Anchor Element. Here is the brief summary about the same -

1) Hash - getter function is same. Setter function has modified for Location by you in https://bugs.webkit.org/attachment.cgi?id=77735&action=prettypatch. This was changed for handling the case when fragment identifiers are invalid.
2) Protocol  - Both setter & getter functions are same.
3) Port  - getter functions is same. setter function for Location can have any number as port. But in case of HTMLAnchorElement it should be from DefaultPortsMap if protcol of the url is defined.
4) Hostname - Getter function is same. Setter function of HTMLAnchorElement has added checks for extra "/" but this is no more needed as KURL takes care of the same.
5) Host - Getter function is same. Setter function of HTMLAnchoreElement has added checks for ":" and port parsing. Again the same is taken care by KURL. not needed.
6) Pathname - Getter function is same. Setter functions of both classes have added check of "/". But no more needed as KURL has already taking care of the same.
7) Search - Getter function os same. Setter function for HTMLAnchorElement has extra check added for # but its already been added as FIXME for KURL.

I am trying to remove extrac checks from HTMLAnchorElement and test the debug build. So that both should be exactly same. Just in case of Port and Hash they both are different.
------- Comment #29 From 2012-02-28 02:59:53 PST -------
In case Host/Port/Search attributes -

Location class has very limited set of attribute setter/getter test cases (as far as I could find). Due to which there is not much checking over the setHost, setPort & setSearch functions. The implementation in HTMLAnchorElement for these functions seems reasonable as there is strict checking added as per the specs in Layout tests (fast/dom/HTMLAnchorElement). setSearch also should replace # with %23 as per the specs for URL manipulation (http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#url-manipulation-and-creation) which Location does not implements. So the # in query may leak to hash.
I think we should be able to add these checks and extra code in KURL itself.

I test other attributes with removing extra checks added for HTMLAnchorElement. If you want I can file a bug and provide the patch for same.
------- Comment #30 From 2012-02-28 08:59:00 PST -------
If we can make the code the same for these two, that would be good, assuming we want their behaviors to be the same.  Looking at the existing test cases is valuable, but we also need to consider what effects these changes will have (if any) on web compatibility.
------- Comment #31 From 2012-02-28 10:36:39 PST -------
(In reply to comment #30)
> If we can make the code the same for these two, that would be good, assuming we want their behaviors to be the same.  Looking at the existing test cases is valuable, but we also need to consider what effects these changes will have (if any) on web compatibility.

Surely. I have some optimizations for HTMLAnchorElement. I can file a different bug for the same. But in case of DOMURL, I have implemented considering the specs as mentioned. Also the layout test cases takes care of almost all use cases. I can then land the same for
------- Comment #32 From 2012-02-28 10:37:24 PST -------
(In reply to comment #31)

> Surely. I have some optimizations for HTMLAnchorElement. I can file a different bug for the same. But in case of DOMURL, I have implemented considering the specs as mentioned. Also the layout test cases takes care of almost all use cases. I can then land the same for HTMLAnchorElement as well.
------- Comment #33 From 2012-03-07 10:09:08 PST -------
Created an attachment (id=130641) [details]
Updated Patch
------- Comment #34 From 2012-03-07 10:39:10 PST -------
(From update of attachment 130641 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=130641&action=review

Thanks for expanding the tests. It is very helpful.

We need to test non ascii values in all of theses tests too.

I found some incorrect results. Please fix implementation.

Did we resolve how to reduce the code duplication?

> Source/WebCore/html/DOMURL.cpp:119
> +    m_href = KURL(KURL(KURL(), baseURL), url);

This cannot be right? Do we really need to call KURL three times here?

> Source/WebCore/html/DOMURL.h:79
> +    void setHref(const String& value) { m_href = KURL(KURL(), value); }

Same here. Can't this be done with a single call to KURL?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:23
> +

Do we want some mailto: tests? How about other common things like file system urls?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:29
> +shouldBe("domurl.href", "'https://www.otherdomain.com:0/path/'");

As 0 a valid port?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-hostname.html:23
> +

Please include mailto: too

> LayoutTests/fast/dom/DOMURL/domurl-attribute-hostname.html:30
> +shouldBe("domurl.href", "'https://www.otherdomain.com:8080/path/'");

You can use shouldBeEqualToString to simplify these

> LayoutTests/fast/dom/DOMURL/domurl-attribute-hostname.html:43
> +shouldBe("domurl.href", "'https://:8080/path/'");

Is this really what we want?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-pathname.html:22
> +debug("pathname to null");
> +var domurl = new webkitURL("https://www.mydomain.com/?key=value");
> +shouldBeEqualToString("domurl.pathname", "/");

Try this one too

"https://www.mydomain.com?key=value"

> LayoutTests/fast/dom/DOMURL/domurl-attribute-pathname.html:23
> +

Do we want to try some ".." paths too? I'm not sure it matters

> LayoutTests/fast/dom/DOMURL/domurl-attribute-port.html:42
> +shouldBe("domurl.href", "'http://webkit.org:4/%00'");

This looks wrong

> LayoutTests/fast/dom/DOMURL/domurl-attribute-protocol.html:28
> +shouldBe("domurl.href", "'http://www.mydomain.com/path/'");

Is this the right behavior?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-protocol.html:32
> +shouldBe("domurl.href", "'http://www.mydomain.com/path/'");

Same here?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-search.html:19
> +

Please test these too:

"https://www.mydomain.com/path/?a?b?c"
"https://www.mydomain.com/path/#?key=value"
"https://www.mydomain.com/path/?"

> LayoutTests/fast/dom/DOMURL/domurl-attribute-search.html:51
> +shouldBe("domurl.href", "'https://www.mydomain.com/path/?key=value'");

This is wrong. It needs to clear the search
------- Comment #35 From 2012-03-07 11:02:39 PST -------
Thanks Eric for the review. I will fix the implementation and add more tests to make the code more concrete. Regarding code duplication, as per AP's comments in the 78478, as all these 3 classes are using KURL implementation already, it is better to make KURL implementation rigid enough so that all these APIs will be just direct calls to the KURL apis. As of now out of 7 host, search& port attributes are having slightly different implementation depending upon the requirements. I have mentioned analysis of all these attributes in my previous comments. Please suggest your opinion.
------- Comment #36 From 2012-03-12 05:06:33 PST -------
Created an attachment (id=131316) [details]
Updated Patch

Update patch.
------- Comment #37 From 2012-03-12 06:03:16 PST -------
(From update of attachment 131316 [details])
Attachment 131316 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11940847

New failing tests:
fast/dom/DOMURL/domurl-attribute-hash.html
fast/dom/DOMURL/domurl-attribute-host.html
fast/dom/DOMURL/domurl-attribute-hostname.html
fast/dom/DOMURL/domurl-attribute-port.html
------- Comment #38 From 2012-03-12 06:52:34 PST -------
(In reply to comment #37)
> (From update of attachment 131316 [details] [details])
> Attachment 131316 [details] [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11940847
> 
> New failing tests:
> fast/dom/DOMURL/domurl-attribute-hash.html
> fast/dom/DOMURL/domurl-attribute-host.html
> fast/dom/DOMURL/domurl-attribute-hostname.html
> fast/dom/DOMURL/domurl-attribute-port.html

Ahh is it because of KURL behaves differently issue -
https://bugs.webkit.org/show_bug.cgi?id=80172
------- Comment #39 From 2012-03-12 11:23:16 PST -------
(In reply to comment #38)
> (In reply to comment #37)
> > (From update of attachment 131316 [details] [details] [details])
> > Attachment 131316 [details] [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11940847
> > 
> > New failing tests:
> > fast/dom/DOMURL/domurl-attribute-hash.html
> > fast/dom/DOMURL/domurl-attribute-host.html
> > fast/dom/DOMURL/domurl-attribute-hostname.html
> > fast/dom/DOMURL/domurl-attribute-port.html
> 
> Ahh is it because of KURL behaves differently issue -
> https://bugs.webkit.org/show_bug.cgi?id=80172

You will have to accept some failures here since KURL has bugs
------- Comment #40 From 2012-03-12 11:56:40 PST -------
(From update of attachment 131316 [details])
In general GURL is correct and KURL is wrong. You are probably better of building and testing this with --chromium and checking in different test expectations for KURL based platforms.

View in context: https://bugs.webkit.org/attachment.cgi?id=131316&action=review

> LayoutTests/fast/dom/DOMURL/domurl-attribute-hash.html:52
> +domurl.hash = null;
> +shouldBeEqualToString("domurl.href", "https://www.mydomain.com/testurl.html#");

This doesn't match Location

window.location.hash = null
null
window.location.hash
"#null"

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:70
> +domurl.host = "www.other\dom/ain.com";

What is '\d'? '\d' has no special meaning in JS so it is treated as 'd'. If you want some escape sequence use something of the form \u1234. Maybe you just wanted a backslash? Then use '\\'

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:71
> +shouldBeEqualToString("domurl.href", "https://www.otherdom/ain.com/path/");

This is wrong. It should be

'https://www.otherdom%2Fain.com/path/'

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:74
> +domurl.href = "https:/\rww.my@domain.com:8080/path/";

Did you really mean \r as in carriage return?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:76
> +shouldBeEqualToString("domurl.href", "https:/\rww.my@domain.com:8080/path/");

This is wrong. It should be

'https://ww.my@www.other%21domain.com:15/path/'

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:85
> +domurl.host = "www.other!domain.com:25";

'!' is not allowed in the domain name

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:86
> +shouldBeEqualToString("domurl.href", "https://rwwmy@www.other!domain.com:25/pa..th/");

The expected URL here is

'https://rwwmy@www.other%21domain.com:25/pa..th/'
------- Comment #41 From 2012-03-14 04:13:35 PST -------
Created an attachment (id=131822) [details]
Updated Patch

Hi Arv, I have fixed the test cases as per expected and your comments. The KURL fails in setting hostname with ASCII characters (there is FIXME in KURL.cpp for the same). I have added its platform specific expected file. Also, chromium port fails in setting hashname, port & hash to mailto & foo protocols. I have added them as platform specific expected for chromium.
------- Comment #42 From 2012-04-10 04:25:41 PST -------
(From update of attachment 131822 [details])
I am really wishing to take this implementation forward. Can someone review the patch? I will update the patch for latest rebaseline incorporate any review comments.
------- Comment #43 From 2012-05-03 14:04:10 PST -------
Adam, I'm not a reviewer but I believe we should r+ this so we can move forward with this.
------- Comment #44 From 2012-05-03 14:35:42 PST -------
(From update of attachment 131822 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=131822&action=review

> Source/WebCore/html/DOMURL.cpp:119
> +    if (url.isNull())
> +        m_href = KURL();

This is overwritten below. What is the intention?

Also, how do we get a null string into this? Maybe all we need is an assert?

Can this be tested?

> Source/WebCore/html/DOMURL.cpp:249
> +    // The below replyace change is to make sure that '#' in the query does not leak to the hash.

Typo

> LayoutTests/fast/dom/DOMURL/domurl-attribute-port.html:8
> +description('Tests for the port attribute of the DOMURL.');

We should probably add tests for numbers that are outside the range of int.

I looked at the URI spec and it seems like I need to update it a bit since the port handling does not seem to match any browser. But before I do that I need to better understand how the RFC for URI treats ports.

I'm fine handling all these issues in new bugs so that this is not blocked by all these edge cases.
------- Comment #45 From 2012-05-08 02:15:03 PST -------
Thanks Erik for the review.  I got the overwritten issue. I guess i need to return the KURL() directly when url is null.  I will add an assert as well.
------- Comment #46 From 2012-05-09 05:07:29 PST -------
Created an attachment (id=140920) [details]
Patch
------- Comment #47 From 2012-05-09 10:20:44 PST -------
(From update of attachment 140920 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140920&action=review

If I was a reviewer I would r+

> Source/WebCore/html/DOMURL.cpp:150
> +    String query = href().query();

In some places you use href() and some other you use m_href. I feel like m_href would be better here since it is clearer that it is the internal KURL object we care about.

> Source/WebCore/html/DOMURL.cpp:231
> +    int port = value.toInt();

We need to revisit this to match the spec but for now I'm OK that we do the wrong thing for numbers that are too large to be represented as ints.

> Source/WebCore/html/DOMURL.cpp:232
> +    if (port < 0 || port > 0xFFFF || value.isEmpty())

If value is empty there is no reason to do toInt.

if (value.isEmpty()) {
    m_href.removePort();
} else {
  int port = ...

}
------- Comment #48 From 2012-07-06 03:34:01 PST -------
Created an attachment (id=151047) [details]
Patch

Updated final patch with Arv's comment. Please review.
------- Comment #49 From 2012-07-06 04:19:06 PST -------
(From update of attachment 151047 [details])
Attachment 151047 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13154264

New failing tests:
fast/files/url-null.html
------- Comment #50 From 2012-07-06 04:19:11 PST -------
Created an attachment (id=151051) [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
------- Comment #51 From 2012-07-25 06:10:01 PST -------
Created an attachment (id=154334) [details]
Fix for failing test case
------- Comment #52 From 2014-02-05 10:58:36 PST -------
(From update of attachment 154334 [details])
Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.