Closed
Bug 409885
Opened 17 years ago
Closed 8 years ago
SetDomain shouldn't do string manipulation of URIs
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
2.01 KB,
patch
|
dragana
:
review+
|
Details | Diff | 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)
Updated•17 years ago
|
Attachment #294587 -
Flags: superreview?(jst)
Attachment #294587 -
Flags: superreview+
Attachment #294587 -
Flags: review?(jst)
Attachment #294587 -
Flags: review+
Attachment #294587 -
Flags: approval1.9+
Reporter | ||
Comment 1•16 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 2•16 years ago
|
||
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 → ---
Reporter | ||
Updated•16 years ago
|
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 → ---
Updated•16 years ago
|
Whiteboard: [missed 1.9 checkin]
Comment 3•16 years ago
|
||
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+
Comment 4•16 years ago
|
||
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.
Reporter | ||
Comment 5•16 years ago
|
||
If that spec survives real-world use cases, sure.
Comment 6•13 years ago
|
||
Does the spec survive real-world use cases? Or should I fix bug 410293 so this one can get fixed?
Reporter | ||
Comment 7•13 years ago
|
||
> Does the spec survive real-world use cases?
No idea. Do other browsers implement it?
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b93a72f7ef7f
Assignee | ||
Comment 9•8 years ago
|
||
I was cleaning up some necko bugs and this one was referred by one of them. It is a easy fix.
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #294587 -
Attachment is obsolete: true
Attachment #8733991 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 11•8 years ago
|
||
Comment on attachment 8733991 [details] [diff] [review] bug_409885.patch r=me. Thanks for following up on this!
Attachment #8733991 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
(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)
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8733991 -
Attachment is obsolete: true
Attachment #8734293 -
Flags: review+
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db72e508f12d
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d81ec460382e
Keywords: checkin-needed
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d81ec460382e
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•