Bug 55570 – Remove dependency of dom/InputElement.cpp on html/ and wml/

• Component: DOM
• Status: RESOLVED FIXED

[Roland Steiner]

Currently WebCore/dom/InputElement.cpp is dependent on html/ and wml/, only because of the way toInputElement() is implemented. This can be simplified (and implemented more elegantly, IMHO) by having a pure virtual asInputElement() function on Element that has overrides in HTMLInputElement and WMLInputElement, which are derived from InputElement.

[Roland Steiner]

Created attachment 84397 [details]
Patch

as described (read 'pure' as 'having a default implementation' :p)

[Ryosuke Niwa]

Comment on attachment 84397 [details]
Patch

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

This seems like a net win but I'm not sure if asInputElement is a good name.  Also, it doesn't seem so elegant to have a global toInputElement that just calls element->asInputElement.  It might make sense to just replace call sites of toInputElement(element) by element->toInputElement().

My only concern is that this will increase the size of vtables for all elements, and that doesn't seem like a good tradeoff if the motivation  is purely aesthetics.

> Source/WebCore/dom/Element.h:307
> +    virtual const InputElement* asInputElement() const { return 0; }

Why do we need this version?

> Source/WebCore/dom/InputElement.h:167
> -InputElement* toInputElement(Element*);
> +inline InputElement* toInputElement(Element* element)
> +{
> +    return element->asInputElement();
> +}

It doesn't seem so elegant to have a global toInputElement that calls element->asInputElement().  It might make more sense to replace all calls of toInputElement(element) by element->toInputElement().

[Kent Tamura]

Comment on attachment 84397 [details]
Patch

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

>> Source/WebCore/dom/InputElement.h:167
>> +}
> 
> It doesn't seem so elegant to have a global toInputElement that calls element->asInputElement().  It might make more sense to replace all calls of toInputElement(element) by element->toInputElement().

I agree with Niwa-san.

[Ryosuke Niwa]

Comment on attachment 84397 [details]
Patch

I'd say r- for now.

[Roland Steiner]

(In reply to comment #2)
> (From update of attachment 84397 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84397&action=review
> 
> This seems like a net win but I'm not sure if asInputElement is a good name.  Also, it doesn't seem so elegant to have a global toInputElement that just calls element->asInputElement.  It might make sense to just replace call sites of toInputElement(element) by element->toInputElement().

I chose 'asInputElement', because "to<xyz>(Node*)" as a stand-alone function is a common idiom in WebCore that I didn't want to break/confuse with a member function of the same name. 

Note that toInputElement would be now inline and should thus be replaced by element->asInputElement() by the compiler. 

Since there shouldn't be a performance cost, I didn't want to change the existing call places, since "to<xyz>(nodePtr)" is generally known and used, "elementPtr->as<xyz>()" is not. (That, and I'm lazy... ^_-).


> My only concern is that this will increase the size of vtables for all elements, and that doesn't seem like a good tradeoff if the motivation  is purely aesthetics.

Not sure I follow: there aren't that many instances of a given vtable that this should be a serious concern (?).


> > Source/WebCore/dom/Element.h:307
> > +    virtual const InputElement* asInputElement() const { return 0; }
> 
> Why do we need this version?

It's not necessary for toInputElement (as this doesn't have a const version), but I added it for completeness.


> > Source/WebCore/dom/InputElement.h:167
> > -InputElement* toInputElement(Element*);
> > +inline InputElement* toInputElement(Element* element)
> > +{
> > +    return element->asInputElement();
> > +}
> 
> It doesn't seem so elegant to have a global toInputElement that calls element->asInputElement().  It might make more sense to replace all calls of toInputElement(element) by element->toInputElement().

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 84397 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84397&action=review
> > 
> > This seems like a net win but I'm not sure if asInputElement is a good name.  Also, it doesn't seem so elegant to have a global toInputElement that just calls element->asInputElement.  It might make sense to just replace call sites of toInputElement(element) by element->toInputElement().
> 
> I chose 'asInputElement', because "to<xyz>(Node*)" as a stand-alone function is a common idiom in WebCore that I didn't want to break/confuse with a member function of the same name.
> 
> Note that toInputElement would be now inline and should thus be replaced by element->asInputElement() by the compiler. 
> 
> Since there shouldn't be a performance cost, I didn't want to change the existing call places, since "to<xyz>(nodePtr)" is generally known and used, "elementPtr->as<xyz>()" is not. (That, and I'm lazy... ^_-).

Okay.  But now an Element have two ways of giving the corresponding InputElement: toInputElement(element) and element->asInputElement() since there's nothing to prevent to do the latter.  And it's not great to have two ways of doing the same thing.  We should either make asInputElement() a private member of Element and then make toInputElement a friend of Element, or rename asInputElement to toInputElement and get rid of the global function.  I prefer the latter as there is a precedent set by EventTarget and Node.  There, EventTarget has a virtual toNode, which is implemented by Node.

> > My only concern is that this will increase the size of vtables for all elements, and that doesn't seem like a good tradeoff if the motivation  is purely aesthetics.
> 
> Not sure I follow: there aren't that many instances of a given vtable that this should be a serious concern (?).

There are quite few classes that inherit from Element but you're right that it won't have a serious memory impact.

> > > Source/WebCore/dom/Element.h:307
> > > +    virtual const InputElement* asInputElement() const { return 0; }
> > 
> > Why do we need this version?
> 
> It's not necessary for toInputElement (as this doesn't have a const version), but I added it for completeness.

I don't think we should.  Since we don't use a const Element pointer, I don't think this function is ever used.

[Roland Steiner]

(In reply to comment #6)
> Okay.  But now an Element have two ways of giving the corresponding InputElement: toInputElement(element) and element->asInputElement() since there's nothing to prevent to do the latter.  And it's not great to have two ways of doing the same thing.  We should either make asInputElement() a private member of Element and then make toInputElement a friend of Element, or rename asInputElement to toInputElement and get rid of the global function.  I prefer the latter as there is a precedent set by EventTarget and Node.  There, EventTarget has a virtual toNode, which is implemented by Node.

In that case I wonder if toInputElement shouldn't be moved to Node then instead of Element.

OTOH, looking more at InputElement, I'm starting to wonder if the whole thing is even worth it. realizing that it pulls everything and the kitchen sink anyway (e.g., rendering stuff, that in turn will again pull html stuff), rendering the whole thing a bit moot. Although I _do_ like my patch implementation better... :)

One last detail: the original version is non-virtual for HTMLInputElement, which might make a difference if called in a constructor/destructor (it's virtual for WMLInputElement), FWIW.

[Ryosuke Niwa]

(In reply to comment #7)
> One last detail: the original version is non-virtual for HTMLInputElement, which might make a difference if called in a constructor/destructor (it's virtual for WMLInputElement), FWIW.

I don't get what you mean.

[Roland Steiner]

Base_of_HTMLInputElement::Base_of_HTMLInputElement() : Base_of_Base_of_HTMLInputElement()
{
  InputElement* inputelement = toInputElement();
  ...

in the original version, inputelement would be 'this' (because Node::m_nodeFlags and Element::m_tagName are already set), while with my patch it would return 0.

For WMLInputElement it would return 0 in either case.

[Ryosuke Niwa]

(In reply to comment #9)
> Base_of_HTMLInputElement::Base_of_HTMLInputElement() : Base_of_Base_of_HTMLInputElement()
> {
>   InputElement* inputelement = toInputElement();
>   ...
> 
> in the original version, inputelement would be 'this' (because Node::m_nodeFlags and Element::m_tagName are already set), while with my patch it would return 0.
> 
> For WMLInputElement it would return 0 in either case.

Ah, that's a good point.  Is there any code that depend on such a behavior?

[Roland Steiner]

(In reply to comment #10)
> Ah, that's a good point.  Is there any code that depend on such a behavior?

I don't think so, but I'm still wondering why it was implemented the way it was implemented. Also, the only way you could use such a pointer safely would be to store it and only use it after construction finished (but then you could just wait until construction finished in the first place). In such a case, dereferencing 0 is probably a better crash than dereferening a not-fully-de/constructed object pointer.

I could add an ASSERT(!hasTagName(htmlInputTag)) in the default method that returns 0.

Provided we proceed with the patch in the first place (see the rest of my comments in #7).

[Ryosuke Niwa]

(In reply to comment #11)
> I don't think so, but I'm still wondering why it was implemented the way it was implemented. Also, the only way you could use such a pointer safely would be to store it and only use it after construction finished (but then you could just wait until construction finished in the first place). In such a case, dereferencing 0 is probably a better crash than dereferening a not-fully-de/constructed object pointer.
> 
> I could add an ASSERT(!hasTagName(htmlInputTag)) in the default method that returns 0.

That sounds like a reasonable compromise.

[Roland Steiner]

Created attachment 84713 [details]
Patch

New patch with the discussed changes. Moved toInputElement to Node. This also saves some casts to Element.

[WebKit Review Bot]

Attachment 84713 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8086538

[Ryosuke Niwa]

Comment on attachment 84713 [details]
Patch

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

r- due to compilation failure on chromium but the change looks like in the right direction.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:591
>      if (!m_renderer->node() || !m_renderer->node()->isElementNode())
>          return false;
>  
> -    InputElement* inputElement = toInputElement(static_cast<Element*>(m_renderer->node()));
> +    InputElement* inputElement = m_renderer->node()->toInputElement();

It  seems like we don't need " || !m_renderer->node()->isElementNode()" anymore.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:602
>      if (elementNode && elementNode->isElementNode()) {
> -        InputElement* input = toInputElement(static_cast<Element*>(elementNode));
> +        InputElement* input = elementNode->toInputElement();

Ditto.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:614
>          return false;

Ditto on the line above.

> Source/WebCore/rendering/RenderTheme.cpp:778
>      if (!o->node() || !o->node()->isElementNode())

Ditto.

> Source/WebCore/rendering/RenderTheme.cpp:790
>      if (!o->node() || !o->node()->isElementNode())

Ditto.

[WebKit Review Bot]

Attachment 84713 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8094073

[Roland Steiner]

Created attachment 85300 [details]
Patch

[Collabora GTK+ EWS bot]

Attachment 85300 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8125010

[Roland Steiner]

Committed r80811: <http://trac.webkit.org/changeset/80811>

[Ryosuke Niwa]

Comment on attachment 85300 [details]
Patch

Clearing cq?.