Bug 136839 – Generate Element casting helper functions

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

We are currently generating the isHTML*Element() type checking helpers in HTMLElementTypeHelpers.h. However, the corresponding toHTML*Element() casting helpers are being generated by the NODE_TYPE_CASTS() macro. It would be nice if we could generate those helpers automatically, so that we no longer need to use NODE_TYPE_CASTS() for most HTML / SVG / MathML types.

I think those were generated in the past but the change got reverted because the forward declarations for the HTML Elements were forcing the use of reinterpret_cast instead of static_cast, which is unsafe due to multiple inheritance.

My proposal to make it work using static_cast would be to:
- Define the following templated functions in Element.h
***
template<typename T> inline T& toElement(Node& node)
{
    ASSERT_WITH_SECURITY_IMPLICATION(isElementOfType<const T>(node));
    return static_cast<T&>(node);
}
template<typename T> inline T* toElement(Node* node)
{
    ASSERT_WITH_SECURITY_IMPLICATION(!node || isElementOfType<const T>(*node));
    return static_cast<T*>(node);
}
template<typename T> inline const T& toElement(const Node& node)
{
    ASSERT_WITH_SECURITY_IMPLICATION(isElementOfType<const T>(node));
    return static_cast<const T&>(node);
}
template<typename T> inline const T* toElement(const Node* node)
{
    ASSERT_WITH_SECURITY_IMPLICATION(!node || isElementOfType<const T>(*node));
    return static_cast<const T*>(node);
}
***

- Generate toHTML*Element() macros in HTMLElementTypeHelpers.h that call toElement<>() so that we don't need to update existing code. They would look like:
#define toHTMLDivElement(x) WebCore::toElement<WebCore::HTMLDivElement>(x)

Any opinion on this?

[Gyuyoung Kim]

(In reply to comment #0)
> We are currently generating the isHTML*Element() type checking helpers in HTMLElementTypeHelpers.h. However, the corresponding toHTML*Element() casting helpers are being generated by the NODE_TYPE_CASTS() macro. It would be nice if we could generate those helpers automatically, so that we no longer need to use NODE_TYPE_CASTS() for most HTML / SVG / MathML types.
> 
> I think those were generated in the past but the change got reverted because the forward declarations for the HTML Elements were forcing the use of reinterpret_cast instead of static_cast, which is unsafe due to multiple inheritance.
> 
> My proposal to make it work using static_cast would be to:
> - Define the following templated functions in Element.h
> ***
> template<typename T> inline T& toElement(Node& node)
> {
>     ASSERT_WITH_SECURITY_IMPLICATION(isElementOfType<const T>(node));
>     return static_cast<T&>(node);
> }
> template<typename T> inline T* toElement(Node* node)
> {
>     ASSERT_WITH_SECURITY_IMPLICATION(!node || isElementOfType<const T>(*node));
>     return static_cast<T*>(node);
> }
> template<typename T> inline const T& toElement(const Node& node)
> {
>     ASSERT_WITH_SECURITY_IMPLICATION(isElementOfType<const T>(node));
>     return static_cast<const T&>(node);
> }
> template<typename T> inline const T* toElement(const Node* node)
> {
>     ASSERT_WITH_SECURITY_IMPLICATION(!node || isElementOfType<const T>(*node));
>     return static_cast<const T*>(node);
> }
> ***
> 
> - Generate toHTML*Element() macros in HTMLElementTypeHelpers.h that call toElement<>() so that we don't need to update existing code. They would look like:
> #define toHTMLDivElement(x) WebCore::toElement<WebCore::HTMLDivElement>(x)
> 
> Any opinion on this?

Chris, looks good idea ! As you said, I reverted previous toFoo generation patch because of reinterpret_cast's security problem. BTW, I wonder whether this approach can handle multiple tags which are processed by same HTMLFooElement interface.

[Chris Dumez]

(In reply to comment #1)
> (In reply to comment #0)
> > We are currently generating the isHTML*Element() type checking helpers in HTMLElementTypeHelpers.h. However, the corresponding toHTML*Element() casting helpers are being generated by the NODE_TYPE_CASTS() macro. It would be nice if we could generate those helpers automatically, so that we no longer need to use NODE_TYPE_CASTS() for most HTML / SVG / MathML types.
> > 
> > I think those were generated in the past but the change got reverted because the forward declarations for the HTML Elements were forcing the use of reinterpret_cast instead of static_cast, which is unsafe due to multiple inheritance.
> > 
> > My proposal to make it work using static_cast would be to:
> > - Define the following templated functions in Element.h
> > ***
> > template<typename T> inline T& toElement(Node& node)
> > {
> >     ASSERT_WITH_SECURITY_IMPLICATION(isElementOfType<const T>(node));
> >     return static_cast<T&>(node);
> > }
> > template<typename T> inline T* toElement(Node* node)
> > {
> >     ASSERT_WITH_SECURITY_IMPLICATION(!node || isElementOfType<const T>(*node));
> >     return static_cast<T*>(node);
> > }
> > template<typename T> inline const T& toElement(const Node& node)
> > {
> >     ASSERT_WITH_SECURITY_IMPLICATION(isElementOfType<const T>(node));
> >     return static_cast<const T&>(node);
> > }
> > template<typename T> inline const T* toElement(const Node* node)
> > {
> >     ASSERT_WITH_SECURITY_IMPLICATION(!node || isElementOfType<const T>(*node));
> >     return static_cast<const T*>(node);
> > }
> > ***
> > 
> > - Generate toHTML*Element() macros in HTMLElementTypeHelpers.h that call toElement<>() so that we don't need to update existing code. They would look like:
> > #define toHTMLDivElement(x) WebCore::toElement<WebCore::HTMLDivElement>(x)
> > 
> > Any opinion on this?
> 
> Chris, looks good idea ! As you said, I reverted previous toFoo generation patch because of reinterpret_cast's security problem. BTW, I wonder whether this approach can handle multiple tags which are processed by same HTMLFooElement interface.

For HTML / SVG elements that need special handling (such as the case you mentioned), we would still need to use a macro. Those types already need a ElementTypeTraits specialization to be defined for them. The macro would define that specialization as well.

[Benjamin Poulain]

Personally I like the template better than NODE_TYPE_CASTS. 

What we must avoid is mixing both. We can either
1) Make a new styleRule enforcing toXXXElement over toElement<XXX>. Update the style checker accordingly.
2) Get rid of toXXXElement() and update the whole code base.

I don't think (2) is that bad, that's something one can land over a weekend when there is less activity.

[Chris Dumez]

(In reply to comment #3)
> Personally I like the template better than NODE_TYPE_CASTS. 
> 
> What we must avoid is mixing both. We can either
> 1) Make a new styleRule enforcing toXXXElement over toElement<XXX>. Update the style checker accordingly.
> 2) Get rid of toXXXElement() and update the whole code base.
> 
> I don't think (2) is that bad, that's something one can land over a weekend when there is less activity.

I think people are used to the toHTML*Element() helpers so I would prefer to go with 1). I prefer not to have developers learn something new if we can avoid it. Also, it is a lot less work and noise in the code.

[Chris Dumez]

Created attachment 238220 [details]
WIP Patch

[Chris Dumez]

Created attachment 238223 [details]
WIP Patch

GTK and EFL build fixes

[Chris Dumez]

Created attachment 238226 [details]
WIP Patch

Rebaseline bindings tests and speculative GTK build fix.

[Chris Dumez]

Created attachment 238229 [details]
WIP Patch

[Chris Dumez]

Created attachment 238265 [details]
WIP Patch

[Chris Dumez]

Created attachment 238267 [details]
Patch

[Chris Dumez]

Comment on attachment 238267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238267&action=review

> Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.h:42
> +    static TestEventTarget* toImpl(JSC::JSValue);

The reason I did not use a templated to<XXX>() function (and specializations) for these is that:
- Some classes provide a custom implementation in their *Custom.cpp custom binding file
- The prototype actually changes quite a bit from one class to another:
  * SVG types often return different TearOff types
  * Some classes require additional arguments (vm / execState)

[Chris Dumez]

I don't quite get the errors on the win-ews:
    1>..\accessibility\AccessibilityMenuListPopup.cpp(105): error C2819: type 'WebCore::HTMLSelectElement' does not have an overloaded member 'operator ->'
                 C:\cygwin\home\buildbot\WebKit\Source\WebCore\html\HTMLSelectElement.h(39) : see declaration of 'WebCore::HTMLSelectElement'
                 did you intend to use '.' instead?
     1>..\accessibility\AccessibilityMenuListPopup.cpp(105): error C2232: '->WebCore::HTMLSelectElement::listItems' : left operand has 'class' type, use '.'
     1>..\accessibility\AccessibilityMenuListPopup.cpp(106): error C2065: 'listItem' : undeclared identifier
         BarProp.cpp
     1>c:\cygwin\home\buildbot\webkit\source\webcore\accessibility\AXObjectCache.cpp(180): error C2664: 'WebCore::AccessibilityObject *WebCore::AXObjectCache::focusedImageMapUIElement(WebCore::HTMLAreaElement *)' : cannot convert argument 1 from 'WebCore::HTMLAreaElement' to 'WebCore::HTMLAreaElement *' (..\accessibility\AccessibilityAllInOne.cpp)
                 No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
     1>c:\cygwin\home\buildbot\webkit\source\webcore\accessibility\AXObjectCache.cpp(951): error C2819: type 'WebCore::HTMLLabelElement' does not have an overloaded member 'operator ->' (..\accessibility\AccessibilityAllInOne.cpp)

It looks like toHTML*Element(Node*) returns a reference instead of a pointer? I don't get how this is possible yet, especially with msvc only.

[Chris Dumez]

(In reply to comment #12)
> I don't quite get the errors on the win-ews:
>     1>..\accessibility\AccessibilityMenuListPopup.cpp(105): error C2819: type 'WebCore::HTMLSelectElement' does not have an overloaded member 'operator ->'
>                  C:\cygwin\home\buildbot\WebKit\Source\WebCore\html\HTMLSelectElement.h(39) : see declaration of 'WebCore::HTMLSelectElement'
>                  did you intend to use '.' instead?
>      1>..\accessibility\AccessibilityMenuListPopup.cpp(105): error C2232: '->WebCore::HTMLSelectElement::listItems' : left operand has 'class' type, use '.'
>      1>..\accessibility\AccessibilityMenuListPopup.cpp(106): error C2065: 'listItem' : undeclared identifier
>          BarProp.cpp
>      1>c:\cygwin\home\buildbot\webkit\source\webcore\accessibility\AXObjectCache.cpp(180): error C2664: 'WebCore::AccessibilityObject *WebCore::AXObjectCache::focusedImageMapUIElement(WebCore::HTMLAreaElement *)' : cannot convert argument 1 from 'WebCore::HTMLAreaElement' to 'WebCore::HTMLAreaElement *' (..\accessibility\AccessibilityAllInOne.cpp)
>                  No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
>      1>c:\cygwin\home\buildbot\webkit\source\webcore\accessibility\AXObjectCache.cpp(951): error C2819: type 'WebCore::HTMLLabelElement' does not have an overloaded member 'operator ->' (..\accessibility\AccessibilityAllInOne.cpp)
> 
> It looks like toHTML*Element(Node*) returns a reference instead of a pointer? I don't get how this is possible yet, especially with msvc only.

Ah, msvc is using the wrong downcast<>() overload, it is using the "Target& downcast(Source& source)" one with Source=HTMLSelectElement* instead of using the "Target* downcast(Source* source)" one with Source=HTMLSelectElement.

I need to address this.

[Chris Dumez]

Created attachment 238321 [details]
Patch

[Chris Dumez]

(In reply to comment #14)
> Created an attachment (id=238321) [details]
> Patch

Hopefully, this one fixes the win-ews build.

[Chris Dumez]

Created attachment 238331 [details]
Patch

[Chris Dumez]

Created attachment 238338 [details]
Patch

[Chris Dumez]

Created attachment 238345 [details]
Patch

[Chris Dumez]

Comment on attachment 238345 [details]
Patch

And the windows bot is finally green. Please take a look.

[Darin Adler]

Comment on attachment 238345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238345&action=review

Looks like a step in the right direction. But one last thing: For me it’s unpleasant to see so many new call sites using the non-word “impl”. I don’t really think of the thing that’s wrapped by a wrapper as an “impl” even though we make some use of that name today. I think we should consider the function name unwrap() or even toWrapped() instead of toImpl().

> Source/WebCore/bindings/js/JSDOMFormDataCustom.cpp:55
>      HTMLFormElement* form = 0;
>      if (exec->argumentCount() > 0)
> -        form = toHTMLFormElement(exec->argument(0));
> +        form = toHTMLFormElementOrNull(exec->argument(0));

If there are no arguments, exec->argument(0) will return undefined and we would get null from toHTMLFormElementOrNull so we don’t really need the if statement. Also should use nullptr.

[Chris Dumez]

Created attachment 238439 [details]
Patch

[Chris Dumez]

Created attachment 238445 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 238445 [details]
Patch

Clearing flags on attachment: 238445

Committed r173804: <http://trac.webkit.org/changeset/173804>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

Note: This broke 64-bit Windows builds. Working on a patch.

[Brent Fulgham]

Build fix checked in for Windows in r173878 <http://trac.webkit.org/changeset/173878>

[Chris Dumez]

(In reply to comment #26)
> Build fix checked in for Windows in r173878 <http://trac.webkit.org/changeset/173878>

Thanks Brent, and sorry about the breakage. I did fix the win-ews before landing but my symbol exporting wasn't entirely correct apparently.

[Darin Adler]

Are we going to do something similar for the render tree?

[Chris Dumez]

(In reply to comment #28)
> Are we going to do something similar for the render tree?

Render objects are still using the TYPE_CASTS_BASE() macro + a isRendererOfType template specialization. My short-term plan is to port them to the new SPECIALIZE_TYPE_TRAITS_*() macro so that:
- We can use is<>() / downcast<>() on render objects
- We can likely drop the isRendererOfType template specialization as we will likely be able to leverage the TypeCastTraits specialization instead.

I don't know if you were talking about generating the traits specializations without macros (like we do for most HTML / SVG / MathML elements). This part I haven't thought about yet.

Right now I am still in the process of porting CSS objects over to is<>() / downcast<>().

[Darin Adler]

(In reply to comment #29)
> My short-term plan is to port them to the new SPECIALIZE_TYPE_TRAITS_*() macro

Great.

> I don't know if you were talking about generating the traits specializations without macros (like we do for most HTML / SVG / MathML elements).

I wasn’t.