1455559 - FromNode should work for references.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

Right now you need to use:

  Foo::FromNode(&aRef)

Which is quite ugly.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1455559: Make FromNode work for references.

Review commit: https://reviewboard.mozilla.org/r/238348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238348/

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8969585 [details]
Bug 1455559: Make FromNode work for references.

https://reviewboard.mozilla.org/r/238348/#review244288

This is fine. I'm not convinced as to how useful having a reference overload is, but I don't personally mind it.

::: dom/base/nsINode.h:2169
(Diff revision 1)
> -  static const _class* FromNode(const ArgType* aNode)                   \
> -  {                                                                     \
> -    return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \
> -  }                                                                     \
> -  template<typename ArgType>                                            \
> -  static const _class* FromNodeOrNull(const ArgType* aNode)             \
> -  {                                                                     \
> -    return aNode ? FromNode(aNode) : nullptr;                           \
> +  template<typename T, template<typename> typename SmartPtr>             \
> +  static _const _class* FromNode(_const SmartPtr<T>& aNode)              \
> +  {                                                                      \
> +    return FromNode(aNode.get());                                        \
> +  }                                                                      \
> +  template<typename T, template<typename> typename SmartPtr>             \
> +  static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode)        \
> +  {                                                                      \
> +    return FromNodeOrNull(aNode.get());                                  \
>    }

I _think_ you can get rid of these two overloads if you change the `_const T&` version to instead be:

    static auto FromNode(_const T& aNode) -> decltype(static_cast<_const _class*>(&aNode))

which uses SFINAE to check if the cast would succeed before selecting the method during overload resolution.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969585 [details]
Bug 1455559: Make FromNode work for references.

https://reviewboard.mozilla.org/r/238348/#review244380

::: dom/base/nsINode.h:2175
(Diff revision 1)
> -  {                                                                     \
> -    return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \
> -  }                                                                     \
> -  template<typename ArgType>                                            \
> -  static const _class* FromNodeOrNull(const ArgType* aNode)             \
> -  {                                                                     \
> +  static _const _class* FromNode(_const SmartPtr<T>& aNode)              \
> +  {                                                                      \
> +    return FromNode(aNode.get());                                        \
> +  }                                                                      \
> +  template<typename T, template<typename> typename SmartPtr>             \
> +  static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode)        \

This overload doesn't quite makes sense to me.  A const smartptr returns a pointer to a non-const thing from get(), so there's no real reason to have `_const` in the args for the smartptr overloads.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8969585 [details]
Bug 1455559: Make FromNode work for references.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238348/diff/1-2/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8969585 [details]
Bug 1455559: Make FromNode work for references.

I ended up doing something in between because our compilers on automation didn't like template template parameters, could do a once-over :)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8969585 [details]
Bug 1455559: Make FromNode work for references.

https://reviewboard.mozilla.org/r/238348/#review244436

::: dom/base/nsINode.h:2169
(Diff revision 1)
> -  static const _class* FromNode(const ArgType* aNode)                   \
> -  {                                                                     \
> -    return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \
> -  }                                                                     \
> -  template<typename ArgType>                                            \
> -  static const _class* FromNodeOrNull(const ArgType* aNode)             \
> -  {                                                                     \
> -    return aNode ? FromNode(aNode) : nullptr;                           \
> +  template<typename T, template<typename> typename SmartPtr>             \
> +  static _const _class* FromNode(_const SmartPtr<T>& aNode)              \
> +  {                                                                      \
> +    return FromNode(aNode.get());                                        \
> +  }                                                                      \
> +  template<typename T, template<typename> typename SmartPtr>             \
> +  static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode)        \
> +  {                                                                      \
> +    return FromNodeOrNull(aNode.get());                                  \
>    }

Nice trick! I ended up using it :)

::: dom/base/nsINode.h:2175
(Diff revision 1)
> -  {                                                                     \
> -    return aNode->_check ? static_cast<const _class*>(aNode) : nullptr; \
> -  }                                                                     \
> -  template<typename ArgType>                                            \
> -  static const _class* FromNodeOrNull(const ArgType* aNode)             \
> -  {                                                                     \
> +  static _const _class* FromNode(_const SmartPtr<T>& aNode)              \
> +  {                                                                      \
> +    return FromNode(aNode.get());                                        \
> +  }                                                                      \
> +  template<typename T, template<typename> typename SmartPtr>             \
> +  static _const _class* FromNodeOrNull(_const SmartPtr<T>& aNode)        \

You're right, it should read `const SmartPtr<_const T>&` I guess. Though it's gone in the next version. I guess we don't use those much in DOM Code anyway.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8969585 [details]
Bug 1455559: Make FromNode work for references.

https://reviewboard.mozilla.org/r/238348/#review245466

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad5095a897e5
Make FromNode work for references. r=bz,Nika

Comment 9

[Dorel Luca [:dluca]]

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