Make clear that SetHostPort will not reset the port if the host parameter does not have a port number and add new function that does the opposite

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 3 obsolete attachments)

If uri had a port number and the parameter of SetHostPort does not have a port number it should set port number to default. At least the function name suggest that.
Assignee

Updated

3 years ago
Assignee: nobody → dd.mozilla
Whiteboard: [necko-active]
Assignee

Comment 1

3 years ago
Posted patch bug_1295636.patch (obsolete) — Splinter Review
Attachment #8781623 - Flags: review?(valentin.gosu)
Attachment #8781623 - Flags: review?(bugs)
Baku might recall why SetHostPort behaves as it behaves.
See also https://bugzilla.mozilla.org/show_bug.cgi?id=1292522#c9
Flags: needinfo?(amarchesini)
Comment on attachment 8781623 [details] [diff] [review]
bug_1295636.patch

But this changes the behavior of most of the users of SetHostPort.
Would it make sense to have enum param for SetHostPort telling whether missing port means reseting to default, or whether the old port should be reused?
Attachment #8781623 - Flags: review?(bugs) → review-
We introduced SetHostPort for URL API. In this API if you do:

var a = new URL("http://example.com:443");
a.host = "example.net";
alert(a);

It will should show: "http://example.net:443".

The spec says:

The host attribute’s setter must run these steps:
    [...]
If the given value for the host attribute’s setter lacks a port, context object’s url’s port will not change. This can be unexpected as host attribute’s getter does return a port so one might have assumed the setter to always "reset" both.
Flags: needinfo?(amarchesini)
Yeah, that is about URL spec, but SetHostPort is about our internal URL, and based on bugs its current meaning isn't clear.
But ok, URL API needs SetHostPorts current behavior, but ::SetDomain needs different behavior.
And nothing in the name SetHostPort hints that it keeps the old port.
Either we need to rename the method or add some enum param or something.
Assignee

Comment 6

3 years ago
Posted patch bug_1295636.patch (obsolete) — Splinter Review
I have added a new function and some comments so that we do not get another bug because of this confusing function name.
Attachment #8781623 - Attachment is obsolete: true
Attachment #8781623 - Flags: review?(valentin.gosu)
Attachment #8781890 - Flags: review?(valentin.gosu)
Attachment #8781890 - Flags: review?(bugs)
Assignee

Comment 7

3 years ago
Posted patch bug_1295636.patch (obsolete) — Splinter Review
Sorry, I have only change some comments.
Attachment #8781890 - Attachment is obsolete: true
Attachment #8781890 - Flags: review?(valentin.gosu)
Attachment #8781890 - Flags: review?(bugs)
Attachment #8781899 - Flags: review?(valentin.gosu)
Attachment #8781899 - Flags: review?(bugs)
Assignee

Updated

3 years ago
Summary: SetHostPort should reset the port if the host parameter does not have a port number → Make clear that SetHostPort will not reset the port if the host parameter does not have a port number and add new function that does the opposite
Comment on attachment 8781899 [details] [diff] [review]
bug_1295636.patch

I guess fine. Still error prone, but I see that hostPort is attribute after all, so passing enum isn't possible.
Attachment #8781899 - Flags: review?(bugs) → review+
Assignee

Comment 9

3 years ago
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8781899 [details] [diff] [review]
> bug_1295636.patch
> 
> I guess fine. Still error prone, but I see that hostPort is attribute after
> all, so passing enum isn't possible.

That is why I did it this way.

At least now there is a comment :(
Comment on attachment 8781899 [details] [diff] [review]
bug_1295636.patch

Review of attachment 8781899 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsStandardURL.cpp
@@ +1847,5 @@
>      return NS_OK;
>  }
>  
> +// This function is different than SetHostPort in that the port number will be
> +// reset as well if aValue parameter does not contain a port port number. 

Trailing whitespace.

@@ +1857,5 @@
> +  nsresult rv = SetPort(-1);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = SetHostPort(aValue);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +}

Wouldn't you get a "not all control paths return a value" warning here?
This is why I don't like NS_ENSURE_SUCCESS very much.
Just return rv here.
Attachment #8781899 - Flags: review?(valentin.gosu) → review+
Assignee

Comment 11

3 years ago
Attachment #8781899 - Attachment is obsolete: true
Attachment #8782295 - Flags: review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 12

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1627e53676b
SetHostPort should reset the port if the host parameter does not have a port number. r=valentin, r=smaug
Keywords: checkin-needed

Comment 13

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b1627e53676b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1296664
You need to log in before you can comment on or make changes to this bug.