Closed Bug 409885 Opened 13 years ago Closed 4 years ago

SetDomain shouldn't do string manipulation of URIs

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: dragana)

References

Details

(Whiteboard: [missed 1.9 checkin])

Attachments

(1 file, 2 obsolete files)

Attached patch Fix (obsolete) — Splinter Review
Whenever I see this sort of thing I get a little weirded out.  And one of the results of the way it works now is that on non-hierarchical URIs we do very very weird things (instead of just throwing, which is what we _should_ be doing, imo).

I left the SetUserPass() in there because that's what we do now, but I'm not really sure we should.
Attachment #294587 - Flags: superreview?(jst)
Attachment #294587 - Flags: review?(jst)
Attachment #294587 - Flags: superreview?(jst)
Attachment #294587 - Flags: superreview+
Attachment #294587 - Flags: review?(jst)
Attachment #294587 - Flags: review+
Attachment #294587 - Flags: approval1.9+
Checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reopening.  Looks like this method doesn't work.  :(  Serves me right for assuming things...

Filed bug 410293 on making it work.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: bzbarsky → nobody
Status: REOPENED → NEW
Depends on: 410293
Summary: [FIX]SetDomain shouldn't do string manipulation of URIs → SetDomain shouldn't do string manipulation of URIs
Target Milestone: mozilla1.9 M11 → ---
Whiteboard: [missed 1.9 checkin]
Comment on attachment 294587 [details] [diff] [review]
Fix

Clearing approval since this patch was backed out and we have closed for 1.9.
Attachment #294587 - Flags: approval1.9+
HTML5 currently says document.domain represents only the hostname portion of the origin, not the port, and you can't set it to a hostport combination.  If that sticks (and as a complexity-reducing measure I'm in favor), we could reapply the patch with SetHostPort changed to SetHost and probably be good to go.
If that spec survives real-world use cases, sure.
Does the spec survive real-world use cases?  Or should I fix bug 410293 so this one can get fixed?
> Does the spec survive real-world use cases?

No idea.  Do other browsers implement it?
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
I was cleaning up some necko bugs and this one was referred by one of them. It is a easy fix.
Attached patch bug_409885.patch (obsolete) — Splinter Review
Attachment #294587 - Attachment is obsolete: true
Attachment #8733991 - Flags: review?(bzbarsky)
Comment on attachment 8733991 [details] [diff] [review]
bug_409885.patch

r=me.  Thanks for following up on this!
Attachment #8733991 - Flags: review?(bzbarsky) → review+
Comment on attachment 8733991 [details] [diff] [review]
bug_409885.patch

Er, actually just realized a problem in the new code.  This:

>+  if (NS_FAILED(rv2)) {
>+    rv.Throw(rv2);
>   }

should be returning after the Throw() call, no?  Same in the other two places that do this.  Please fix before pushing.
Flags: needinfo?(dd.mozilla)
(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8733991 [details] [diff] [review]
> bug_409885.patch
> 
> Er, actually just realized a problem in the new code.  This:
> 
> >+  if (NS_FAILED(rv2)) {
> >+    rv.Throw(rv2);
> >   }
> 
> should be returning after the Throw() call, no?  Same in the other two
> places that do this.  Please fix before pushing.

yes, I will fix it.
Flags: needinfo?(dd.mozilla)
Attached patch bug_409885.patchSplinter Review
Attachment #8733991 - Attachment is obsolete: true
Attachment #8734293 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d81ec460382e
Status: ASSIGNED → RESOLVED
Closed: 13 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1292522
Depends on: 1299165
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.