773780 - New DOM binding APIs for node

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[(no longer active)]

Boris, is it safe to return null from nsINode::GetParentObject() for now?

Comment 2

[(no longer active)]

Another question: Node.webidl accesses a bunch of interfaces such as Element.  Forward declaring Element leads to an error like this:

Traceback (most recent call last):
  File "/Users/ehsanakhgari/moz/mozilla-central/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/Users/ehsanakhgari/moz/mozilla-central/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/BindingGen.py", line 73, in <module>
    main()
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/BindingGen.py", line 66, in main
    generate_binding_header(config, outputPrefix, webIDLFile);
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/BindingGen.py", line 20, in generate_binding_header
    root = CGBindingRoot(config, outputprefix, webidlfile)
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/Codegen.py", line 4417, in __init__
    curr)
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/Codegen.py", line 376, in __init__
    typeDesc = d.getDescriptor(t.unroll().inner.identifier.name)
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/Configuration.py", line 119, in getDescriptor
    return self.config.getDescriptor(interfaceName, self.workers)
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/Configuration.py", line 83, in getDescriptor
    iface = self.getInterface(interfaceName)
  File "/Users/ehsanakhgari/moz/mozilla-central/dom/bindings/Configuration.py", line 51, in getInterface
    return self.interfaces[ifname]
KeyError: 'Element'
make[2]: *** [NodeBinding.h] Error 1

Which makes me think that I also need to have a valid Element.webidl.  Is that correct?  Does Element.webidl need to contain anything useful?

Comment 3

[(no longer active)]

Yet another question: once I get to a patch which compiles and passes the basic tests, what tests do I need to perform to make sure that my patch is correct?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Boris, is it safe to return null from nsINode::GetParentObject() for now?

No, but you should be able to move the parent-object code from nsNodeSH::PreCreate (in nsDOMClassInfo.cpp) into nsINode::GetParentObject(), and just call GetParentObject() from PreCreate there?

I guess there's the problem of what should happen for the document, where right now we actually just use a global directly in some cases.  I guess _those_ cases could return null.  We'd need to double-check that the WrapNative call in the "GetScopeObject() is not null" case does the same thing as our new-binding code or WrapNativeParent too, I guess.

Comment 5

[(no longer active)]

(In reply to comment #4)
> > Boris, is it safe to return null from nsINode::GetParentObject() for now?
> 
> No, but you should be able to move the parent-object code from
> nsNodeSH::PreCreate (in nsDOMClassInfo.cpp) into nsINode::GetParentObject(),
> and just call GetParentObject() from PreCreate there?

Makes sense.

> I guess there's the problem of what should happen for the document, where right
> now we actually just use a global directly in some cases.  I guess _those_
> cases could return null.  We'd need to double-check that the WrapNative call in
> the "GetScopeObject() is not null" case does the same thing as our new-binding
> code or WrapNativeParent too, I guess.

I'm not sure if I understand this part...

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Which makes me think that I also need to have a valid Element.webidl.  Is that correct?

What you actually need there is an entry for Element in Bindings.conf.  For now, just

  addExternalIface('Element', nativeType='mozilla::dom::Element')

should do the trick, I think.  I'll add that to the docs once I can edit devmo again.  :(

> what tests do I need to perform to make sure that my patch is correct?

That's a good question.

I think just doing Node doesn't give you enough to go on, because I don't think there are any direct Node instances around (so no instance objects whose proto is Node.prototype itself).  On the premise that we'll need it at some point, and if our tests don't test it already, you could check that once you're done

  Node.prototype.appendChild.call(someElement, someOtherElement)

still does the right thing.  Which it won't without codegen changes, of course.  ;) I assume you're not really planning on doing those yourself, right?

The other alternative is also converting something that inherits from Node to the point where you can work with a real object.  The path of least resistance there might be to do nsIDOMCharacterData and then nsIDOMComment.  You wouldn't be able to test appendChild on it (well, except to make sure it throws), but can test nextSibling/previousSibling and perhaps compareDocumentPosition?

In the long run, simply passing our normal test suites with the new bindings for everything will theoretically exercise it all.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I'm not sure if I understand this part...

The way GetParentObject works is that you return some C++ object and then the caller actually JS-wraps it to get the JSObject* it wants.

But the code in nsNodeSH::PreCreate for documents does one of two things, neither of which quite matches the above model.  It either uses the given global JSObject* directly (which corresponds to GetParentObject returning null, I believe), or it gets the scope object from the document and then JS-wraps it... but it doesn't use the same exact function for this as the code that uses the return value of GetParentObject, at first glance.  So we should double-check to make sure that the two functions end up doing the same thing in the end and that just returning the scope object from GetParentObject() for documents makes sense.

Comment 8

[(no longer active)]

(In reply to comment #6)
> > Which makes me think that I also need to have a valid Element.webidl.  Is that correct?
> 
> What you actually need there is an entry for Element in Bindings.conf.  For
> now, just
> 
>   addExternalIface('Element', nativeType='mozilla::dom::Element')
> 
> should do the trick, I think.  I'll add that to the docs once I can edit devmo
> again.  :(

OK, I'll try that.

> > what tests do I need to perform to make sure that my patch is correct?
> 
> That's a good question.
> 
> I think just doing Node doesn't give you enough to go on, because I don't think
> there are any direct Node instances around (so no instance objects whose proto
> is Node.prototype itself).  On the premise that we'll need it at some point,
> and if our tests don't test it already, you could check that once you're done
> 
>   Node.prototype.appendChild.call(someElement, someOtherElement)

OK, I'll do that.

> still does the right thing.  Which it won't without codegen changes, of course.
>  ;) I assume you're not really planning on doing those yourself, right?

Nope!  I thought you volunteered to do that?  ;-)

(Is there a bug on file for that?  I'd file one, but I don't quite know what to put in it.)

> The other alternative is also converting something that inherits from Node to
> the point where you can work with a real object.  The path of least resistance
> there might be to do nsIDOMCharacterData and then nsIDOMComment.  You wouldn't
> be able to test appendChild on it (well, except to make sure it throws), but
> can test nextSibling/previousSibling and perhaps compareDocumentPosition?

Hmm, does that make any sense though?  (Unless you meant that I should do that in my local repo, test and make sure that things work, then throw away the patch?)

> In the long run, simply passing our normal test suites with the new bindings
> for everything will theoretically exercise it all.

Hmm yeah I guess testing this really requires a bunch of other stuff to be moved over as well.  Maybe I'll try that when I'm done with this bug.

Comment 9

[(no longer active)]

(In reply to comment #7)
> > I'm not sure if I understand this part...
> 
> The way GetParentObject works is that you return some C++ object and then the
> caller actually JS-wraps it to get the JSObject* it wants.
> 
> But the code in nsNodeSH::PreCreate for documents does one of two things,
> neither of which quite matches the above model.  It either uses the given
> global JSObject* directly (which corresponds to GetParentObject returning null,
> I believe), or it gets the scope object from the document and then JS-wraps
> it... but it doesn't use the same exact function for this as the code that uses
> the return value of GetParentObject, at first glance.  So we should
> double-check to make sure that the two functions end up doing the same thing in
> the end and that just returning the scope object from GetParentObject() for
> documents makes sense.

OK, where does this other code live?  And when you say "make sure", do you mean just testing in a few test cases that I construct myself?  Because I'm not sure if I have enough knowledge to ensure this by just comparing the two pieces of code... :/

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Is there a bug on file for that?

I filed bug 774970.

> Hmm, does that make any sense though? 

Yes, it does.  It's work we'll have to do anyway, so if you do it, might as well land it.  Again, it's just a path of least resistance to having an actual concrete DOM node switched to new bindings.  You could do Node and then Element instead, and test on some createElementNS("random-namespace", "something"), but Element has a lot more members than CharacterData and Comment do....

> OK, where does this other code live?

One lives in BindingUtils.h; see the WrapNativeParent template there.

The other is whatever code it is the document case in nsNodeSH::PreCreate ends up calling right now.

I just looked at them.  The nsDOMClassInfo code ends up calling nsXPConnect::WrapNativeToJSVal(cx, global, GetScopeObject(), NULL, NULL, vp, false), which calls NativeInterface2JSObject with basically those arguments in the order (cx, vp, helper, NULL, NULL, false) where "helper" is constructed from the nsISupports and one of the nulls.

The BindingUtils code calls NativeInterface2JSObject(cx, vp, helper, NULL, NULL, false).  So looks like they do the same thing and we win.  Peter should probably double-check that, though.

Comment 11

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> > Hmm, does that make any sense though? 
> 
> Yes, it does.  It's work we'll have to do anyway, so if you do it, might as
> well land it.  Again, it's just a path of least resistance to having an
> actual concrete DOM node switched to new bindings.  You could do Node and
> then Element instead, and test on some createElementNS("random-namespace",
> "something"), but Element has a lot more members than CharacterData and
> Comment do....

OK, sold!  :-)

> > OK, where does this other code live?
> 
> One lives in BindingUtils.h; see the WrapNativeParent template there.
> 
> The other is whatever code it is the document case in nsNodeSH::PreCreate
> ends up calling right now.
> 
> I just looked at them.  The nsDOMClassInfo code ends up calling
> nsXPConnect::WrapNativeToJSVal(cx, global, GetScopeObject(), NULL, NULL, vp,
> false), which calls NativeInterface2JSObject with basically those arguments
> in the order (cx, vp, helper, NULL, NULL, false) where "helper" is
> constructed from the nsISupports and one of the nulls.
> 
> The BindingUtils code calls NativeInterface2JSObject(cx, vp, helper, NULL,
> NULL, false).  So looks like they do the same thing and we win.  Peter
> should probably double-check that, though.

Great!  CCing Peter.

(On a related note, I didn't actually get to do any work on this tonight.  This isn't blocking any other work, right?  If it does, I need to know!)

Comment 12

[Boris Zbarsky [:bzbarsky]]

No, this isn't blocking anything.

Comment 13

[(no longer active)]

Attachment: WIP

This is my WIP patch, posting to that Peter can potentially finish it up.  What's left is to finish up GetParentObject(), and then implement the accessors on nsINode where needed, I think.  I have obviously not tried this yet as it doesn't build until those accessors are all in place.

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 669503 [details] [diff] [review]
v1

>+++ b/accessible/src/xforms/nsXFormsAccessible.cpp
>+  nsINodeList* nodes = mContent->ChildNodesList();

As long as we're renaming it anyway, why not name it ChildNodes() instead of adding a ChildNodes() alias for ChildNodesList()?

>   NS_ENSURE_STATE(nodes);

Can't be null.

>+++ b/content/base/public/FragmentOrElement.h
>-  NS_IMETHOD GetTextContent(nsAString &aTextContent);
>-  NS_IMETHOD SetTextContent(const nsAString& aTextContent);
>+  virtual void GetTextContentInternal(nsAString& aTextContent);
>+  virtual void SetTextContentInternal(const nsAString& aTextContent,

Again, why not just call the virtual methods GetTextContent and SetTextContent instead of having this forwarding setup?  At the very least, please document the reason.

>-  static nsresult InternalIsSupported(nsISupports* aObject,

Watch out for merge issues with bug 776515.  Maybe that should hold off for now?

>+++ b/content/base/public/nsINode.h
>+  virtual JSObject* WrapNode(JSContext *aCx, JSObject *aScope,

Pleaes document what this is for.

>+    // globalObj as the nodes parent since that would give the node the

s/nodes/node's/

>+  uint16_t GetNodeType() const

Shouldn't be needed; bindings will call NodeType() now.

>+  nsINode* GetParentNode()
>+  {
>+    return GetNodeParent();

I would vote for just renaming GetNodeParent to GetParentNode.  Followup bug OK for this, since there are a bunch of consumers, I bet.

>+  uint16_t CompareDocumentPosition(nsINode& aOther) const
>+    return CompareDocPosition(&aOther);

Again, followup on just getting rid of CompareDocPosition and making consumers use CompareDocumentPosition and changing it to take an nsINode&.

>+  void GetNodeValue(nsAString& aNodeValue)
>+    GetNodeValueInternal(aNodeValue);

Again, document why we need this "internal" thing.  Assuming we actually do.  I still don't see why we need it.

>+  void SetNodeValue(const nsAString& aNodeValue,
>+    SetNodeValueInternal(aNodeValue, aError);

And here.

>+  nsINode* InsertBefore(nsINode& aNode, nsINode* aChild,
>+    return ReplaceOrInsertBefore(false, &aNode, aChild, aError);

Maybe another followup on ReplaceOrInsertBefore to assume that its args are sane so it doesn't duplicate checks we already do elsewhere?

>+  bool IsEqualNode(nsINode* aNode)
>+    return IsEqualTo(aNode);

We only have one caller of IsEqualTo, looks like.  Why not rename IsEqualTo to IsEqualNode?

>+  void GetNamespaceURI(nsAString& aNamespaceURI, mozilla::ErrorResult& aError) const

I think mNodeInfo->GetNamespaceURI() should always do the right thing here, even in the non-attribute-or-element cases.  So we don't really need to add that branch, I think.  Might be worth asserting in nsNodeInfo that it has no useful namespace in the non-attribute-element cases, though.

>+  void GetPrefix(nsAString& aPrefix)

Again, this can just call mNodeInfo->GetPrefix(), I think.

And then we don't need IsAttributeOrElement() at all.  ;)

>+  nsresult RemoveFromParent()

This is not part of WebIDL, right?  Might be worth documenting that.

>+    nsINode* parent = GetNodeParent();
>+    parent->RemoveChild(*this, rv);

Don't we need to null-check parent?  Some of the callers used to.

>+++ b/content/base/src/nsDOMAttribute.cpp

Losing all those warnings is a bit unfortunate.  I'm not sure how much we care about keeping them, though...  And in the new bindings we'd lose them anyway because we don't want to virtualize all that stuff.  :(  Unless we plan to stop attrs being nodes before we migrate them to new bindings?

If we do lose the warnings, can you remove those from the warning enum and remove the corresponding localized string stuff?

>+++ b/content/base/src/nsINode.cpp
>+nsINode::ReplaceOrInsertBefore(bool aReplace, nsIDOMNode *aNewChild,
>+  nsCOMPtr<nsINode> refChild = do_QueryInterface(aRefChild);

The old code for this bailed out of aRefChild && !refChild.  Which I think we need to check for because nsIDOMNode is not [builtinclass].  :(  So we need to keep doing that.

>+nsINode::WrapObject(JSContext *aCx, JSObject *aScope, bool *aTriedToWrap)
>+  // (1) it has a script handling object,

s/it/Our owner document/

>+  // (2) has had one, or has been marked to have had one,

"Our owner document has had a script handling object, or has been marked to have had one"?

>+nsINode::GetAttributes(nsIDOMNamedNodeMap** aAttributes)
>+  if (!IsElement()) {
>+    return NS_OK;

Seems safer to set *aAttributes to null explicitly here....

>+++ b/dom/webidl/Node.webidl
>+  // Mozilla-specific stuff
>+  // These have been moved to Element in the spec.

Followup bug to consider making this change?  Compat fun, I bet.  :(

>+  boolean isSupported(DOMString feature, DOMString version);

This one is actually completely gone in the spec, not moved to Element, right?

I'm amazed at how much stuff the spec removes.  I wonder what the compat impact will be... :(

>+  [ChromeOnly]
>+  readonly attribute Principal nodePrincipal;
>+  [ChromeOnly]
>+  readonly attribute URI? baseURIObject;

This part is so lovely.  ;)

r=me with the above nits.  This is great stuff!

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

FWIW, I'm fidgeting with Node interface stuff right now.  If you're actively working on this stuff and plan to land soonish, I'm happy to put off landing until you're done, since this work is a lot more important and my patches will be very easy to rebase on top of yours (mostly just removals).  But if you don't plan to land this soon, let me know and I'll land my patches as soon as they get review.

Comment 17

[Peter Van der Beken [:peterv]]

(In reply to :Aryeh Gregor from comment #16)
> But if
> you don't plan to land this soon, let me know and I'll land my patches as
> soon as they get review.

I was planning to land yesterday, but ran into unexpected problems due to wrapping node parents failing as a combination of this patch and bug 799465.

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Additional patch for review comments

> As long as we're renaming it anyway, why not name it ChildNodes() instead of
> adding a ChildNodes() alias for ChildNodesList()?

Done.

> Can't be null.

Done.

> Again, why not just call the virtual methods GetTextContent and
> SetTextContent instead of having this forwarding setup?  At the very least,
> please document the reason.

That gives a conflict with GetTextContent/SetTextContent on nsIDOMNode, I'd need to add |using ...| all over. The alternative is to use binaryname on nsIDOMNode::Get/SetTextContent.

> Please document what this is for.

   * WrapNode is called from WrapObject to actually wrap this node, WrapObject
   * does some additional checks and fix-up that's common to all nodes. WrapNode
   * should just call the DOM binding's Wrap function.

> >+    // globalObj as the nodes parent since that would give the node the
> 
> s/nodes/node's/

Done.

> Shouldn't be needed; bindings will call NodeType() now.

Done.

> I would vote for just renaming GetNodeParent to GetParentNode.  Followup bug
> OK for this, since there are a bunch of consumers, I bet.

Done.

> >+  uint16_t CompareDocumentPosition(nsINode& aOther) const
> >+    return CompareDocPosition(&aOther);
> 
> Again, followup on just getting rid of CompareDocPosition and making
> consumers use CompareDocumentPosition and changing it to take an nsINode&.

> Again, document why we need this "internal" thing.  Assuming we actually do.
> I still don't see why we need it.

> And here.

See above.

> Maybe another followup on ReplaceOrInsertBefore to assume that its args are
> sane so it doesn't duplicate checks we already do elsewhere?

I moved the check for "!aReplace || aRefChild" to the XPCOM method, I don't think I can do anything else here?

> We only have one caller of IsEqualTo, looks like.  Why not rename IsEqualTo
> to IsEqualNode?

Done.

> I think mNodeInfo->GetNamespaceURI() should always do the right thing here,
> even in the non-attribute-or-element cases.  So we don't really need to add
> that branch, I think.  Might be worth asserting in nsNodeInfo that it has no
> useful namespace in the non-attribute-element cases, though.

Done.

> Again, this can just call mNodeInfo->GetPrefix(), I think.
> And then we don't need IsAttributeOrElement() at all.  ;)

Done.

> This is not part of WebIDL, right?  Might be worth documenting that.

Done.

> >+    nsINode* parent = GetNodeParent();
> >+    parent->RemoveChild(*this, rv);
> 
> Don't we need to null-check parent?  Some of the callers used to.

One did, I changed it to not use this and documented that the node needs to have a parent.

> Losing all those warnings is a bit unfortunate.  I'm not sure how much we
> care about keeping them, though...  And in the new bindings we'd lose them
> anyway because we don't want to virtualize all that stuff.  :(  Unless we
> plan to stop attrs being nodes before we migrate them to new bindings?
> 
> If we do lose the warnings, can you remove those from the warning enum and
> remove the corresponding localized string stuff?

Yeah, I've been thinking how to deal with it but most solutions involve slowing down these methods and properties on all other node types. I do think we should try to unhook Attr from Node sooner rather than later, maybe even by duplicating all the Node stuff on Attr for now.

> >+nsINode::ReplaceOrInsertBefore(bool aReplace, nsIDOMNode *aNewChild,
> >+  nsCOMPtr<nsINode> refChild = do_QueryInterface(aRefChild);
> 
> The old code for this bailed out of aRefChild && !refChild.  Which I think
> we need to check for because nsIDOMNode is not [builtinclass].  :(  So we
> need to keep doing that.

Done.

> "Our owner document has had a script handling object, or has been marked to
> have had one"?

Done.

> Seems safer to set *aAttributes to null explicitly here....

Done.

Comment 19

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f68b28becd0

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f68b28becd0