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?
(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.
(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.
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.
(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.
Created attachment 238220 [details] WIP Patch
Created attachment 238223 [details] WIP Patch GTK and EFL build fixes
Created attachment 238226 [details] WIP Patch Rebaseline bindings tests and speculative GTK build fix.
Created attachment 238229 [details] WIP Patch
Created attachment 238265 [details] WIP Patch
Created attachment 238267 [details] Patch
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)
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.
(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.
Created attachment 238321 [details] Patch
(In reply to comment #14) > Created an attachment (id=238321) [details] > Patch Hopefully, this one fixes the win-ews build.
Created attachment 238331 [details] Patch
Created attachment 238338 [details] Patch
Created attachment 238345 [details] Patch
Comment on attachment 238345 [details] Patch And the windows bot is finally green. Please take a look.
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.
Created attachment 238439 [details] Patch
Created attachment 238445 [details] Patch
Comment on attachment 238445 [details] Patch Clearing flags on attachment: 238445 Committed r173804: <http://trac.webkit.org/changeset/173804>
All reviewed patches have been landed. Closing bug.
Note: This broke 64-bit Windows builds. Working on a patch.
Build fix checked in for Windows in r173878 <http://trac.webkit.org/changeset/173878>
(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.
Are we going to do something similar for the render tree?
(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<>().
(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.