1001351 - Allow null-safe conversion from nsINode to nsIDOMNode

Status: RESOLVED FIXED

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

Description

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

I sometimes convert code from using nsIDOMNode to using nsINode.  Often I have to convert from nsINode to nsIDOMNode to call legacy methods.  ->AsDOMNode() is nice, but it crashes if the node is null.  I would like a method

  nsIDOMNode*
  AsDOMNode(nsINode* aNode)
  {
    return aNode ? aNode->AsDOMNode() : nullptr;
  }

to ease conversion.  Or some comparably convenient mechanism, I don't really care what as long as it's not lengthy to type.

Comment 1

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

Attachment: patch

I can use a different technique if you prefer, I don't really care how it's implemented.  The callsite I added that uses it is a case where there was a real crash because I forgot to do a null-check before using ->AsDOMNode(), which fortunately was caught by tests before landing.

Comment 2

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

https://tbpl.mozilla.org/?tree=Try&rev=41d89f527b42

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I would make the method a static method of nsINode. Polluting global scope with AsDOMNode isn't too nice.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

And perhaps it should be GetAsDOMNode(nsINode*) to hint that it may return null.

Comment 5

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

Then instead of

  nsCOMPtr<nsIDOMNode> ret =
    dont_AddRef(AsDOMNode(GetBlockNodeParent(node).take()));

you get

  nsCOMPtr<nsIDOMNode> ret =
    dont_AddRef(nsINode::GetAsDOMNode(GetBlockNodeParent(node).take()));

which I don't like nearly as much.  AsDOMNode is already going to be in mozilla::dom, I think that's enough.

Comment 6

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

Comment on attachment 8412495 [details] [diff] [review]
patch

I would really like to not make this any longer than it is -- the editor cleanup requires a lot of stuff like

+  return InsertNode(AsDOMNode(aContent), AsDOMNode(aParent), aPosition);

which would be a lot more cumbersome if it were longer.  Editor code right now mixes nsINode* and nsIDOMNode* everywhere, and constant conversion is required as I port it to use more nsINode*.

I don't think namespace pollution is an issue here, because "AsDOMNode" is a very specific name, which only makes sense for a function, and nobody is going to make a function with name and signature "AsDOMNode(nsINode*)" that isn't exactly this function.

That said, this blocks ongoing editor cleanup work, so if making the name longer will get it reviewed faster, I guess I will.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8412495 [details] [diff] [review]
patch

Make it GetAsDOMNode so that it is clear that null may be returned.


(And already_AddRefed<nsIDOMNode> GetAsDOMNode(already_AddRefed<nsINode>& aNode);
 might be useful too.)

Comment 8

[(no longer active)]

(Aryeh, if this doesn't suit your needs, please feel free to add something specific to editor/.)

Comment 9

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

https://tbpl.mozilla.org/?tree=Try&rev=af70647529d7

http://hg.mozilla.org/integration/mozilla-inbound/rev/f35027b2931f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f35027b2931f