Closed Bug 1455559 Opened 6 years ago Closed 6 years ago

FromNode should work for references.

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

Right now you need to use:

  Foo::FromNode(&aRef)

Which is quite ugly.
Component: DOM: Animation → DOM
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.
Attachment #8969585 - Flags: review?(nika) → review+
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.
Attachment #8969585 - Flags: review?(bzbarsky) → review+
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 :)
Attachment #8969585 - Flags: review+ → review?(bzbarsky)
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 on attachment 8969585 [details]
Bug 1455559: Make FromNode work for references.

https://reviewboard.mozilla.org/r/238348/#review245466
Attachment #8969585 - Flags: review?(bzbarsky) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ad5095a897e5
Make FromNode work for references. r=bz,Nika
https://hg.mozilla.org/mozilla-central/rev/ad5095a897e5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.