Move NodeIterator to Paris bindings

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: Ms2ger, Assigned: bzbarsky)

Tracking

(Blocks 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Posted patch WIPSplinter Review
I've got a patch, but it fails <http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/mochitest/test_bug502959.html?force=1>. IIRC, doublewrapped.acceptNode was undefined.
> IIRC, doublewrapped.acceptNode was undefined.

That's... quite odd.  That should be taking the normal xpconnect codepaths....
I need this and TreeWalker to fix bug 838686.  Stealing.
Assignee: nobody → bzbarsky
Blocks: 838686
On the other hand, there's a good chance that without bug 838686 fixing this bug will just make things fail too, because the WebIDL call won't prevent XPConnect from reporting the exception.

I need to think about patch ordering here a bit.
Depends on: 839088
No longer blocks: 838686
Depends on: 838686
WebIDL object, so calling createNodeIterator or createTreeWalker via
an Xray will call the XPCOM versions of those methods.  That means
that I can't just disable XPCOM-based wrapping for TreeWalker and
NodeIterator altogether, unfortunately, which means a web page could
try stashing a TreeWalker in something like userdata and then getting
it back and end up wrapping it as an XPCOM object the second time.

I could "fix" that by adding a wrapper cache and whatnot, I guess, if
desired...  But the problem will go away once we convert Document in
any case.
Attachment #712210 - Flags: review?(peterv)
Whiteboard: [need review]
Try run at https://tbpl.mozilla.org/?tree=Try&rev=84f47dbdf6ce

The M2 bits are all unexpected pass.  I'll adjust the failure annotations to deal with that.

The M4 bit I'm looking into.
So the failures in js/xpconnect/tests/mochitest/test_bug502959.html are because it expects this:

26 var iter = document.createNodeIterator(document, NodeFilter.SHOW_ELEMENT, foo);
27 var doublewrapped = iter.filter;

to make doublewrapped an XPCWrappedNative around an XPCWrappedJS. But per WebIDL, doublewrapped === foo in this case.

So we should probably adjust the test... or something.  Blake, any idea how?
Flags: needinfo?(mrbkap)
Comment on attachment 712209 [details] [diff] [review]
part 3.  Add a WebIDL API to NodeIterator and TreeWalker.

Review of attachment 712209 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/NodeIterator.cpp
@@ +183,5 @@
>  
>  /* readonly attribute nsIDOMNode root; */
>  NS_IMETHODIMP NodeIterator::GetRoot(nsIDOMNode * *aRoot)
>  {
> +    return CallQueryInterface(Root(), aRoot);

Maybe

NS_ADDREF(*aRoot = Root().AsDOMNode());
Comment on attachment 712210 [details] [diff] [review]
part 4.  Turn on WebIDL bindings for NodeIterator and TreeWalker.   behavior here is a bit weird because Document is still not a

Review of attachment 712210 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +5378,5 @@
>                                               rv).get();
>    return rv.ErrorCode();
>  }
>  
> +already_AddRefed<mozilla::dom::NodeIterator>

Nit: I don't think you need the namespace qualifications in the .cpp.
> NS_ADDREF(*aRoot = Root().AsDOMNode());

Done, with Root()->AsDOMNode(); and a "return NS_OK;"

> Nit: I don't think you need the namespace qualifications in the .cpp.

Indeed, done.
(In reply to Boris Zbarsky (:bz) from comment #9)
> So we should probably adjust the test... or something.  Blake, any idea how?

Reading the test, I wonder if we can start ripping some of the code out as well as the tests. As I understand it, WebIDL mandates that in cases where we currently hand out double wrapped objects we should instead hand out the original object. Does that mean that we won't have to worry about double wrapped objects in content once we switch entirely over to the new bindings?
Flags: needinfo?(mrbkap)
For WebIDL objects, there is no double-wrapping.  So if we completely stop exposing all JS objects not exposed via WebIDL to web pages, then you're probably right...

But that's a pretty long-term thing.  In the short term, what do I do with this test?  ;)
Flags: needinfo?(mrbkap)
I'd say nuke it unless you can think of a non-converted callback. They're getting harder and harder to find (and therefore less and less important).
Flags: needinfo?(mrbkap)
> I'd say nuke it unless you can think of a non-converted callback.

I nuked it as part of part 4.
Attachment #712207 - Flags: review?(peterv) → review+
Attachment #712208 - Flags: review?(peterv) → review+
Attachment #712209 - Flags: review?(peterv) → review+
Attachment #712210 - Flags: review?(peterv) → review+
Unfortunately, I had to backout because something in this push caused a spike in mochitest-a11y assertions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb572a342efa

https://tbpl.mozilla.org/php/getParsedLog.php?id=20189591&tree=Mozilla-Inbound

A quick skim of the log shows them to be of the type:
###!!! ASSERTION: bad size recorded: 'aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize', file ../../../xpcom/base/nsTraceRefcntImpl.cpp, line 441
Apparently refcount logging requires that all class names passed to it be unique.  Since there's a "TreeWalker" in a11y code already, have to pass dom::TreeWalker to the addref/release macros.  Did that, and:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf95d0e6941
   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0a9285a7aa
   https://hg.mozilla.org/integration/mozilla-inbound/rev/2ff820824d40
   https://hg.mozilla.org/integration/mozilla-inbound/rev/54d365f29ee5
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.