Bug 150341 – Drop unnecessary Node::toInputElement() virtual function

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

[Chris Dumez]

Drop unnecessary Node::toInputElement() virtual function and use the usual is<HTMLInputElement>() / downcast< HTMLInputElement >() instead.

[Chris Dumez]

Created attachment 263522 [details]
Patch

[Chris Dumez]

Created attachment 263541 [details]
Patch

[Darin Adler]

Comment on attachment 263541 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:675
>      Node* node = this->node();
> -    if (!node)
> +    if (!is<HTMLInputElement>(node))
>          return false;
>  
> -    HTMLInputElement* inputElement = node->toInputElement();
> -    if (!inputElement)
> -        return false;
> -
> -    return inputElement->shouldAppearIndeterminate();
> +    return downcast<HTMLInputElement>(*node).shouldAppearIndeterminate();

I might have written it:

    return is<HTMLInputElement>(node) && downcast<HTMLInputElement>(*node).shouldAppearIndeterminate();

I noticed you doing something similar in a function below.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:575
>      // Only HTMLInputElement creates SliderThumbElement instances as its shadow nodes.
>      // So, shadowHost() must be an HTMLInputElement.
> -    Element* host = shadowHost();
> -    return host ? host->toInputElement() : 0;
> +    auto* host = shadowHost();
> +    return is<HTMLInputElement>(host) ? downcast<HTMLInputElement>(host) : nullptr;

It’s peculiar that this code checks is<HTMLInputElement> yet has a comment stating that the check is not needed. I’m thinking that the comment is likely correct, and we could probably write:

    return downcast<HTMLInputElement>(shadowHost());

But obviously it’s less risky to leave the behavior unchanged.

[Chris Dumez]

Created attachment 263545 [details]
Patch

[Chris Dumez]

Comment on attachment 263541 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:675
>> +    return downcast<HTMLInputElement>(*node).shouldAppearIndeterminate();
> 
> I might have written it:
> 
>     return is<HTMLInputElement>(node) && downcast<HTMLInputElement>(*node).shouldAppearIndeterminate();
> 
> I noticed you doing something similar in a function below.

Done before landing.

>> Source/WebCore/html/shadow/SliderThumbElement.cpp:575
>> +    return is<HTMLInputElement>(host) ? downcast<HTMLInputElement>(host) : nullptr;
> 
> It’s peculiar that this code checks is<HTMLInputElement> yet has a comment stating that the check is not needed. I’m thinking that the comment is likely correct, and we could probably write:
> 
>     return downcast<HTMLInputElement>(shadowHost());
> 
> But obviously it’s less risky to leave the behavior unchanged.

I checked that this is only used by RangeInputType so I made the change.

[WebKit Commit Bot]

Comment on attachment 263545 [details]
Patch

Clearing flags on attachment: 263545

Committed r191327: <http://trac.webkit.org/changeset/191327>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.