Bug 76816 – Implement the URL decomposition IDL attributes

• Component: DOM
• Status: RESOLVED FIXED

[Kaustubh Atrawalkar]

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.

[Kaustubh Atrawalkar]

Created attachment 123534 [details]
Patch

[Adam Barth]

Comment on attachment 123534 [details]
Patch

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?

[Kaustubh Atrawalkar]

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

[Kaustubh Atrawalkar]

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?

[Adam Barth]

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

[Kaustubh Atrawalkar]

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

[Adam Barth]

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

[Kaustubh Atrawalkar]

Created attachment 123889 [details]
Updated Patch

Will be addressing the creating of base class issue in another fix as this will change many things here.

[Adam Barth]

Comment on attachment 123889 [details]
Updated Patch

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.

[Kaustubh Atrawalkar]

Created attachment 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.

[WebKit Review Bot]

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.

[Kaustubh Atrawalkar]

Can you review the patch? Thanks.

[Kentaro Hara]

Comment on attachment 127144 [details]
Updated Patch

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]

[Kaustubh Atrawalkar]

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.

[Kaustubh Atrawalkar]

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

[Kentaro Hara]

(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

[Kentaro Hara]

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

[Kentaro Hara]

(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]?

[Adam Barth]

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

[Kaustubh Atrawalkar]

Created attachment 128459 [details]
Updated Patch

Haraken, I have updated the patch to remove [Replaceable] and strigifier as suggested by Adam. Also updated the ChangeLog.

[Kentaro Hara]

Comment on attachment 128459 [details]
Updated Patch

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!

[Adam Barth]

Comment on attachment 128459 [details]
Updated Patch

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.

[Kaustubh Atrawalkar]

I was refereeing to HTMLAnchorElement rather than Location for implementing the functions. I will try to fix these function as per Location. Thanks.

[Kaustubh Atrawalkar]

Created attachment 128723 [details]
Updated Patch

Updated patch with suggested function migrated to match Location APIs.

[Adam Barth]

Why does HTMLAnchorElement and Location have such different code?  Should they be the same, or is there a reason that they're different?

[Kaustubh Atrawalkar]

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

[Adam Barth]

Comment on attachment 128723 [details]
Updated Patch

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.

[Kaustubh Atrawalkar]

(In reply to comment #27)
> (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.

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.

[Kaustubh Atrawalkar]

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.

[Adam Barth]

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.

[Kaustubh Atrawalkar]

(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

[Kaustubh Atrawalkar]

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

[Kaustubh Atrawalkar]

Created attachment 130641 [details]
Updated Patch

[Erik Arvidsson]

Comment on attachment 130641 [details]
Updated Patch

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

[Kaustubh Atrawalkar]

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.

[Kaustubh Atrawalkar]

Created attachment 131316 [details]
Updated Patch

Update patch.

[WebKit Review Bot]

Comment on attachment 131316 [details]
Updated Patch

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

[Kaustubh Atrawalkar]

(In reply to comment #37)
> (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

Ahh is it because of KURL behaves differently issue -
https://bugs.webkit.org/show_bug.cgi?id=80172

[Erik Arvidsson]

(In reply to comment #38)
> (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

You will have to accept some failures here since KURL has bugs

[Erik Arvidsson]

Comment on attachment 131316 [details]
Updated Patch

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:/\[e-mail]:8080/path/";

Did you really mean \r as in carriage return?

> LayoutTests/fast/dom/DOMURL/domurl-attribute-host.html:76
> +shouldBeEqualToString("domurl.href", "https:/\[e-mail]:8080/path/");

This is wrong. It should be

'https://[e-mail]%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://[e-mail]!domain.com:25/pa..th/");

The expected URL here is

'https://[e-mail]%21domain.com:25/pa..th/'

[Kaustubh Atrawalkar]

Created attachment 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.

[Kaustubh Atrawalkar]

Comment on attachment 131822 [details]
Updated Patch

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.

[Erik Arvidsson]

Adam, I'm not a reviewer but I believe we should r+ this so we can move forward with this.

[Erik Arvidsson]

Comment on attachment 131822 [details]
Updated Patch

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.

[Kaustubh Atrawalkar]

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.

[Kaustubh Atrawalkar]

Created attachment 140920 [details]
Patch

[Erik Arvidsson]

Comment on attachment 140920 [details]
Patch

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

}

[Kaustubh Atrawalkar]

Created attachment 151047 [details]
Patch

Updated final patch with Arv's comment. Please review.

[WebKit Review Bot]

Comment on attachment 151047 [details]
Patch

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

[WebKit Review Bot]

Created attachment 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

[Kaustubh Atrawalkar]

Created attachment 154334 [details]
Fix for failing test case

[Anders Carlsson]

Comment on attachment 154334 [details]
Fix for failing test case

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Rob Buis]

I think this can be closed.

[Darin Adler]

Yes, someone did take care this a while back. Someone could pinpoint when and identify the revision and bug we used for the work.

[Radar WebKit Bug Importer]

<rdar://problem/65678678>