Closed
Bug 808991
Opened 12 years ago
Closed 12 years ago
Named getters should be called even for int32_t jsids if there is no indexed getter
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla19
| Tracking | Status | |
|---|---|---|
| firefox18 | --- | unaffected |
| firefox19 | + | fixed |
People
(Reporter: Ms2ger, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
|
25.00 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
http://dev.w3.org/2006/webapi/WebIDL/#getownproperty goes roughly like this: 1. ignore := false 2. If supports indexed properties && JSID_IS_INT(id): ignore := true 3. If supports named properties && id not on prototype && id supported index && !ignore: call the named getter 4. Fall back to expandos while we only call the named getter if JSID_IS_STRING(id).
|
Reporter | |
Comment 1•12 years ago
|
||
This caused a change in behaviour for <http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1895> in bug 803129.
|
Assignee | |
Comment 2•12 years ago
|
||
Er, yes. Good catch.
|
|
Assignee | |
Comment 3•12 years ago
|
||
Attachment #678922 -
Flags: review?(peterv)
|
|
Assignee | |
Updated•12 years ago
|
Whiteboard: [need review]
|
||
Comment 4•12 years ago
|
||
Comment on attachment 678922 [details] [diff] [review] Named getters should get called for indexed properties on objects that don't support indexed properties. Review of attachment 678922 [details] [diff] [review]: ----------------------------------------------------------------- I'm not entirely clear on why we do the JSID_IS_STRING test if we have indexed and named properties, but not if we only have named properties. Probably only an edge case (E4X?), but I'd like to understand. ::: dom/bindings/BindingUtils.h @@ +916,5 @@ > + if (JSID_IS_STRING(id)) { > + *vp = STRING_TO_JSVAL(JSID_TO_STRING(id)); > + return true; > + } > + return JS_IdToValue(cx, id, vp); Would it make sense to use the friend api js::IdToValue (with a debug compartment check)? ::: dom/bindings/test/test_namedNoIndexed.html @@ +12,5 @@ > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=808991">Mozilla Bug 808991</a> > +<p id="display"></p> > +<div id="content" style="display: none" data-1="foo" data-bar="baz"> > + Trailing space.
Attachment #678922 -
Flags: review?(peterv) → review+
|
|
Assignee | |
Comment 5•12 years ago
|
||
> I'm not entirely clear on why we do the JSID_IS_STRING test if we have indexed and named > properties, but not if we only have named properties. Hmm. Really, what we want to test for per spec in the indexed-and-named-properties case is "js id is not an array index". We're using JSID_IS_STRING as a poor proxy for that. I can go through and try to reorganize things so that we're really testing the thing we care about if you prefer; I was just trying to minimize risk and churn, but I guess we have enough time before Aurora that may not be worth it. Thoughts? > Would it make sense to use the friend api js::IdToValue Yes! I hadn't realized we had that. Will do.
|
||
Updated•12 years ago
|
|
|
Assignee | |
Comment 6•12 years ago
|
||
Attachment #679545 -
Flags: review?(peterv)
|
|
Assignee | |
Updated•12 years ago
|
Attachment #678922 -
Attachment is obsolete: true
|
|
||
Comment 7•12 years ago
|
||
Comment on attachment 679545 [details] [diff] [review] This should be better Review of attachment 679545 [details] [diff] [review]: ----------------------------------------------------------------- Great!
Attachment #679545 -
Flags: review?(peterv) → review+
|
|
Assignee | |
Comment 8•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/1c1fa8af5978
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla19
|
||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1c1fa8af5978
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
|
||
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
•