Closed
Bug 693258
Opened 13 years ago
Closed 13 years ago
Figure out test failures when turning off new list DOM bindings
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: peterv, Assigned: peterv)
References
Details
Attachments
(1 file, 1 obsolete file)
8.93 KB,
patch
|
bzbarsky
:
review+
Waldo
:
review+
|
Details | Diff | Splinter Review |
I landed the new DOM bindings from bug 648801 initially turned off, but there were a couple of tests that now fail. I had fixed a number of these in the past, but apparently there's more again. We need to figure out what to do, preferably without changing the behaviour with the pref turned off (so that turning off the pref really brings us back to the state without the new DOM bindings).
Assignee | ||
Comment 1•13 years ago
|
||
Known failures:
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/forms/test_datalist_element.html | Should get the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/test_bug595449.html | fieldset.elements should always return the same object - got [object HTMLCollection], expected [object HTMLCollection]
content/html/content/test/test_bug595449.html | fieldset.elements should always return the same object - got [object HTMLCollection], expected [object HTMLCollection]
dom/tests/mochitest/bugs/test_bug633133.html | empty string should be in the div collection
js/src/xpconnect/tests/chrome/test_nodelists.xul | in operator doesn't see phantom element
layout/reftests/css-ui-invalid/select/select-required-multiple-invalid-changed.html | assertion count 1 is more than expected 0 assertions
layout/reftests/css-ui-valid/select/select-required-multiple-valid-changed.html | assertion count 3 is more than expected 0 assertions
Assignee | ||
Comment 2•13 years ago
|
||
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Comment 3•13 years ago
|
||
Comment on attachment 566149 [details] [diff] [review]
v1
>--- a/js/src/xpconnect/src/dombindings.cpp
>+++ b/js/src/xpconnect/src/dombindings.cpp
> JSObject *proto = getPrototype(cx, scope, triedToWrap);
>+ if (!proto && !*triedToWrap)
>+ aWrapperCache->ClearIsProxy();
> if (!proto)
> return NULL;
Personally, I'd write that as
if (!proto) {
if (!*triedToWrap) {
aWrapperCache->ClearIsProxy();
}
return NULL;
}
Assignee | ||
Comment 4•13 years ago
|
||
I don't, because we want to remove those lines when we remove the pref, but not the others.
Comment 5•13 years ago
|
||
Good point
Assignee | ||
Comment 6•13 years ago
|
||
Couple of problems.
I added a wrapper cache to all HTMLCollections, so we need to force the right parent using a PreCreate hook for those (otherwise we end up storing the first wrapper in the cache, and that wrapper can be from whatever scope comes first).
If the bindings are disabled we should unmark the IsProxy bit in the wrapper caches, because we won't be storing a proxy binding in it after all.
Make GetArrayIndexFromId do the |ToString(ToUint32(s)) == s| check from the WebIDL spec, but only for string properties. For properties that are not number and not string I kept the current behaviour. I think this fixes most issues (like empty string not ending up as 0) and we can try to remove the support for those other properties at some point in the future. It does mean that we're changing behaviour slightly (rejecting " 0" for example). Let me know if you think that's a problem, but running a bit out of ideas on how to fix this.
The pref check in test_nodelists.xul is unfortunate, but the alternative seems to be to disable the test.
Asking Luke for review on making js::StringIsArrayIndex public, since it does the ToString(ToUint32(s)) == s| check that's in WebIDL and so seems generally useful.
Attachment #566149 -
Attachment is obsolete: true
Attachment #566304 -
Flags: review?(luke)
Attachment #566304 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•13 years ago
|
||
I still need to file the "remove the dom.new_bindings pref" bug, but I'll mention that we should remove the ClearIsProxy API and the pref check in test_nodelists.xul at that point too.
Comment 8•13 years ago
|
||
> It does mean that we're changing behaviour slightly (rejecting " 0" for example)
Which is what webidl calls for, yes?
But I thought you copied the "get array index from id" code we used to have in classinfo into the new bindings. Why was there a problem here?
Comment 9•13 years ago
|
||
Oh, and the pref check is just working around a bug in the old classinfo-based bindings, right? I think the pref check is OK there.
Comment 10•13 years ago
|
||
Peter pointed out that the named array code in classinfo just went with a string lookup if the id is not JSID_IS_INT. That answers comment 8.
For the rest, the key part is that jsid's int values have a different range than indices. In particular, they can only be in the range:
1536 #define JSID_INT_MIN (-(1 << 30))
1537 #define JSID_INT_MAX ((1 << 30) - 1)
So foo[(1 << 30)] would end up with a string jsid representing a serialization of 1<<30.
In other words we do still need the call to StringIsArrayIndex. In fact, we should strongly consider calling js_IdIsIndex, except it doesn't have the "length" special-case.... Maybe that doesn't matter, though? We could explicitly check for that if we wanted, before the js_IdIsIndex call. That all requires us limiting this to int/string ids, of course, which is probably a followup.
This didn't bite the old code in practice, since the difference between js_CheckForStringIndex and StringIsArrayIndex only shows up for pretty large indices and nodelists don't really ever have a billion nodes in them....
Comment 11•13 years ago
|
||
Comment on attachment 566304 [details] [diff] [review]
v2
r=me but please file a followup on simplifying that id code...
Attachment #566304 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #10)
> In other words we do still need the call to StringIsArrayIndex. In fact, we
> should strongly consider calling js_IdIsIndex, except it doesn't have the
> "length" special-case.... Maybe that doesn't matter, though?
Both the length check and the checking for first char being a-z helped performance, because they are inlined and StringIsArrayIndex wasn't.
> We could
> explicitly check for that if we wanted, before the js_IdIsIndex call. That
> all requires us limiting this to int/string ids, of course, which is
> probably a followup.
I'll file a bug.
Comment 13•13 years ago
|
||
Comment on attachment 566304 [details] [diff] [review]
v2
Sorry to give the runaround, but Waldo is in the middle of rejiggring the whole property string/element/non-element situation so I'd to defer to him on whether it makes sense to export this.
Attachment #566304 -
Flags: review?(luke) → review?(jwalden+bmo)
Comment 14•13 years ago
|
||
Comment on attachment 566304 [details] [diff] [review]
v2
Review of attachment 566304 [details] [diff] [review]:
-----------------------------------------------------------------
Temporary exporting makes sense. Slightly longer-term, I don't think you'll need this when you implement different storage strategies for indexed properties and named properties. So friend-only for now, and going away soon probably, makes sense.
Attachment #566304 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 15•13 years ago
|
||
Target Milestone: --- → mozilla10
Comment 16•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•