Closed
Bug 808991
Opened 13 years ago
Closed 13 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•13 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•13 years ago
|
||
Er, yes. Good catch.
|
|
Assignee | |
Comment 3•13 years ago
|
||
Attachment #678922 -
Flags: review?(peterv)
|
|
Assignee | |
Updated•13 years ago
|
Whiteboard: [need review]
|
||
Comment 4•13 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•13 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•13 years ago
|
|
|
Assignee | |
Comment 6•13 years ago
|
||
Attachment #679545 -
Flags: review?(peterv)
|
|
Assignee | |
Updated•13 years ago
|
Attachment #678922 -
Attachment is obsolete: true
|
|
||
Comment 7•13 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•13 years ago
|
||
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla19
|
||
Comment 9•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
|
||
Updated•7 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•