Closed Bug 403480 Opened 14 years ago Closed 14 years ago

setting nsIURI::port to its default value shouldn't be possible

Categories

(Core :: Networking, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: Gavin, Assigned: Gavin)

Details

Attachments

(1 file)

The existing code seems to make it impossible to set port to it's default value except in one specific case: set it to a different port and then set it back to the default. This seems inconsistent.
Attached patch patchSplinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #288311 - Flags: superreview?(cbiesinger)
Attachment #288311 - Flags: review?(cbiesinger)
The reason I looked into this was the patch for bug 396316, which has:

+  // If the URI explicitly specified a port, only include it when
+  // it's not the default. (We never want "http://foo.com:80")
+  port = uri.port;
+  if (port != -1) {
+      var handler = this._ioService.getProtocolHandler(scheme);
+      if (port != handler.defaultPort)
+          realm += ":" + port;
+  }

I'm assuming this change means that the getProtocolHandler/defaultPort code wouldn't be necessary, and it could just check for -1. Is that a safe assumption?
Comment on attachment 288311 [details] [diff] [review]
patch

(I suppose the comment isn't quite right in the case of mDefaultPort == -1, suggestions for a better one are welcome.)
Hmm, while we're here perhaps an input value of "-1" (or any negative number, really) should also be illegal? "http://google.com:-1" doesn't seem like it should work, but it does.
Attachment #288311 - Flags: superreview?(cbiesinger)
Attachment #288311 - Flags: superreview+
Attachment #288311 - Flags: review?(cbiesinger)
Attachment #288311 - Flags: review+
Comment on attachment 288311 [details] [diff] [review]
patch

Simple correctness fix that'll help simplify the patch for blocker bug 396316.
Attachment #288311 - Flags: approval1.9?
Attachment #288311 - Flags: approval1.9? → approval1.9+
mozilla/netwerk/base/src/nsStandardURL.cpp 	1.102
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
You need to log in before you can comment on or make changes to this bug.