Bug 136839

Summary: Generate Element casting helper functions
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, commit-queue, darin, gyuyoung.kim, kling, koivisto, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 137004, 137007, 137015, 137056    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2014-09-15 14:53:41 PDT
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?
Comment 1 Gyuyoung Kim 2014-09-15 18:45:11 PDT
(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.
Comment 2 Chris Dumez 2014-09-15 18:54:08 PDT
(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.
Comment 3 Benjamin Poulain 2014-09-15 20:09:35 PDT
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.
Comment 4 Chris Dumez 2014-09-15 20:27:05 PDT
(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.
Comment 5 Chris Dumez 2014-09-16 16:46:48 PDT
Created attachment 238220 [details]
WIP Patch
Comment 6 Chris Dumez 2014-09-16 17:05:59 PDT
Created attachment 238223 [details]
WIP Patch

GTK and EFL build fixes
Comment 7 Chris Dumez 2014-09-16 17:24:53 PDT
Created attachment 238226 [details]
WIP Patch

Rebaseline bindings tests and speculative GTK build fix.
Comment 8 Chris Dumez 2014-09-16 17:36:54 PDT
Created attachment 238229 [details]
WIP Patch
Comment 9 Chris Dumez 2014-09-17 14:37:32 PDT
Created attachment 238265 [details]
WIP Patch
Comment 10 Chris Dumez 2014-09-17 14:52:04 PDT
Created attachment 238267 [details]
Patch
Comment 11 Chris Dumez 2014-09-17 14:59:00 PDT
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)
Comment 12 Chris Dumez 2014-09-18 10:43:02 PDT
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.
Comment 13 Chris Dumez 2014-09-18 11:03:03 PDT
(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.
Comment 14 Chris Dumez 2014-09-18 13:18:27 PDT
Created attachment 238321 [details]
Patch
Comment 15 Chris Dumez 2014-09-18 13:19:04 PDT
(In reply to comment #14)
> Created an attachment (id=238321) [details]
> Patch

Hopefully, this one fixes the win-ews build.
Comment 16 Chris Dumez 2014-09-18 16:10:48 PDT
Created attachment 238331 [details]
Patch
Comment 17 Chris Dumez 2014-09-18 17:07:52 PDT
Created attachment 238338 [details]
Patch
Comment 18 Chris Dumez 2014-09-18 18:26:00 PDT
Created attachment 238345 [details]
Patch
Comment 19 Chris Dumez 2014-09-18 19:35:11 PDT
Comment on attachment 238345 [details]
Patch

And the windows bot is finally green. Please take a look.
Comment 20 Darin Adler 2014-09-21 12:53:56 PDT
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.
Comment 21 Chris Dumez 2014-09-21 16:23:52 PDT
Created attachment 238439 [details]
Patch
Comment 22 Chris Dumez 2014-09-21 20:02:40 PDT
Created attachment 238445 [details]
Patch
Comment 23 WebKit Commit Bot 2014-09-21 20:52:02 PDT
Comment on attachment 238445 [details]
Patch

Clearing flags on attachment: 238445

Committed r173804: <http://trac.webkit.org/changeset/173804>
Comment 24 WebKit Commit Bot 2014-09-21 20:52:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Brent Fulgham 2014-09-22 17:55:26 PDT
Note: This broke 64-bit Windows builds. Working on a patch.
Comment 26 Brent Fulgham 2014-09-23 16:27:22 PDT
Build fix checked in for Windows in r173878 <http://trac.webkit.org/changeset/173878>
Comment 27 Chris Dumez 2014-09-23 16:30:59 PDT
(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.
Comment 28 Darin Adler 2014-10-03 09:09:04 PDT
Are we going to do something similar for the render tree?
Comment 29 Chris Dumez 2014-10-03 10:01:48 PDT
(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<>().
Comment 30 Darin Adler 2014-10-03 21:29:29 PDT
(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.