Bug 235259 – Introduce dynamicDowncast<>() for convenience

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Introduce dynamicDowncast<>() for convenience. It can result in more concise code for the common pattern where we call `is<T>(x)` and then `downcast<T>(x)`.

[Chris Dumez]

Created attachment 449237 [details]
Patch

[Chris Dumez]

Created attachment 449266 [details]
Patch

[Darin Adler]

Comment on attachment 449266 [details]
Patch

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

Looks good.

I kind of liked how the other pattern resulted in a reference and fewer lines of code later in the function that were using a pointer where you had to understand why it’s not null checked. But on the other hand the two separate function template calls were not good. This is better. Too bad this is not like Swift where we can null check and end up with something you can use like a reference.

> Source/WebCore/bindings/js/JSLazyEventListener.cpp:93
>      auto& node = downcast<Node>(eventTarget);
> -    if (!is<SVGElement>(node))
> +    auto* element = dynamicDowncast<SVGElement>(node);
> +    if (!element || !element->correspondingElement())
>          return false;

Node is never used here. I think this should just be:

    auto* element = dynamicDowncast<SVGElement>(eventTarget);

And if that doesn’t compile, then we should fix the underlying mechanism so it does.

> Source/WebCore/bindings/js/JSLazyEventListener.cpp:131
> -    Element* element = m_originalNode && !m_originalNode->isDocumentNode() && is<Element>(*m_originalNode) ? downcast<Element>(m_originalNode.get()) : nullptr;
> +    Element* element = m_originalNode && !m_originalNode->isDocumentNode() ? dynamicDowncast<Element>(m_originalNode.get()) : nullptr;

Since the cast is to Element, and a document node is not an element, there is no need to call isDocumentNode:

    auto* element = dynamicDowncast<Element>(m_originalNode.get());

> Source/WebCore/dom/TypedElementDescendantIterator.h:211
> +    if (auto elementDescendant = dynamicDowncast<ElementType>(descendant))
> +        return ElementDescendantIterator<ElementType>(m_root, &elementDescendant);

The &elementDescendant here looks wrong, pointer to a pointer. Surprised it compiles.

Also, above we use the name descendantElement for the same thing. I like that name better.

> Source/WebCore/page/DragController.cpp:349
>      if (inputElement->isTextButton() && is<ShadowRoot>(inputElement->treeScope().rootNode())) {
>          auto& host = *downcast<ShadowRoot>(inputElement->treeScope().rootNode()).host();
> -        inputElement = is<HTMLInputElement>(host) ? &downcast<HTMLInputElement>(host) : nullptr;
> +        inputElement = dynamicDowncast<HTMLInputElement>(host);
>      }

Might just want to do one longer expression here instead of two lines. Wouldn’t we want to use dynamicDowncast<ShadowRoot> here too? Even better if we had a function that returned the host, and null if the root node is not a ShadowRoot. Maybe such a function does exist?

> Source/WebCore/page/DragController.cpp:511
>      if (is<PluginDocument>(document)) {
> -        const Widget* widget = downcast<PluginDocument>(*document).pluginWidget();
> -        const PluginViewBase* pluginView = is<PluginViewBase>(widget) ? downcast<PluginViewBase>(widget) : nullptr;
> +        auto* widget = downcast<PluginDocument>(*document).pluginWidget();
> +        auto* pluginView = dynamicDowncast<PluginViewBase>(widget);
>  
>          if (pluginView)
>              pluginDocumentAcceptsDrags = pluginView->shouldAllowNavigationFromDrags();

Local variable doesn’t help here. I would write:

    if (auto* pluginDocument = dynamicDowncast<PluginDocument>(document)) {
        if (auto* view = dynamicDowncast<PluginViewBase>(pluginDocument->pluginWidget())
            pluginDocumentAcceptsDrags = view->shouldAllowNavigationFromDrags();
    }

> Source/WebCore/platform/DragImage.cpp:83
> -        : element(is<Element>(node) ? &downcast<Element>(node) : nullptr)
> +        : element(dynamicDowncast<Element>(node))

Makes me wonder if we really want the parentElement here when the node is not an element. Presumably not, but so strange that we just don’t tell any element if a non-element node is being dragged. Maybe in practice the node is always an element and the checking here is a dead code path.

> Source/WebKitLegacy/mac/DOM/DOMUIKitExtensions.mm:275
>              if (is<RenderBox>(*renderer)) {
> -                RenderBox& asBox = renderer->enclosingBox();
> -                RenderObject* parent = asBox.parent();
> -                RenderBox* parentRenderBox = is<RenderBox>(parent) ? downcast<RenderBox>(parent) : nullptr;
> +                auto& asBox = renderer->enclosingBox();
> +                auto* parentRenderBox = dynamicDowncast<RenderBox>(asBox.parent());
>                  if (parentRenderBox && asBox.width() == parentRenderBox->width()) {
>                      noCost = YES;
>                  }

Noticed I am really not happy with the asBox argument name, because it’s not renderer casted to RenderBox, even though the code above does check that renderer is a RenderBox. Not a new concern, but messy. The variable name is basically an incorrect comment!

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 449266 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449266&action=review
> 
> Looks good.
> 
> I kind of liked how the other pattern resulted in a reference and fewer
> lines of code later in the function that were using a pointer where you had
> to understand why it’s not null checked. But on the other hand the two
> separate function template calls were not good. This is better. Too bad this
> is not like Swift where we can null check and end up with something you can
> use like a reference.
> 
> > Source/WebCore/bindings/js/JSLazyEventListener.cpp:93
> >      auto& node = downcast<Node>(eventTarget);
> > -    if (!is<SVGElement>(node))
> > +    auto* element = dynamicDowncast<SVGElement>(node);
> > +    if (!element || !element->correspondingElement())
> >          return false;
> 
> Node is never used here. I think this should just be:
> 
>     auto* element = dynamicDowncast<SVGElement>(eventTarget);
> 
> And if that doesn’t compile, then we should fix the underlying mechanism so
> it does.
> 
> > Source/WebCore/bindings/js/JSLazyEventListener.cpp:131
> > -    Element* element = m_originalNode && !m_originalNode->isDocumentNode() && is<Element>(*m_originalNode) ? downcast<Element>(m_originalNode.get()) : nullptr;
> > +    Element* element = m_originalNode && !m_originalNode->isDocumentNode() ? dynamicDowncast<Element>(m_originalNode.get()) : nullptr;
> 
> Since the cast is to Element, and a document node is not an element, there
> is no need to call isDocumentNode:
> 
>     auto* element = dynamicDowncast<Element>(m_originalNode.get());
> 
> > Source/WebCore/dom/TypedElementDescendantIterator.h:211
> > +    if (auto elementDescendant = dynamicDowncast<ElementType>(descendant))
> > +        return ElementDescendantIterator<ElementType>(m_root, &elementDescendant);
> 
> The &elementDescendant here looks wrong, pointer to a pointer. Surprised it
> compiles.
> 
> Also, above we use the name descendantElement for the same thing. I like
> that name better.
> 
> > Source/WebCore/page/DragController.cpp:349
> >      if (inputElement->isTextButton() && is<ShadowRoot>(inputElement->treeScope().rootNode())) {
> >          auto& host = *downcast<ShadowRoot>(inputElement->treeScope().rootNode()).host();
> > -        inputElement = is<HTMLInputElement>(host) ? &downcast<HTMLInputElement>(host) : nullptr;
> > +        inputElement = dynamicDowncast<HTMLInputElement>(host);
> >      }
> 
> Might just want to do one longer expression here instead of two lines.
> Wouldn’t we want to use dynamicDowncast<ShadowRoot> here too? Even better if
> we had a function that returned the host, and null if the root node is not a
> ShadowRoot. Maybe such a function does exist?
> 
> > Source/WebCore/page/DragController.cpp:511
> >      if (is<PluginDocument>(document)) {
> > -        const Widget* widget = downcast<PluginDocument>(*document).pluginWidget();
> > -        const PluginViewBase* pluginView = is<PluginViewBase>(widget) ? downcast<PluginViewBase>(widget) : nullptr;
> > +        auto* widget = downcast<PluginDocument>(*document).pluginWidget();
> > +        auto* pluginView = dynamicDowncast<PluginViewBase>(widget);
> >  
> >          if (pluginView)
> >              pluginDocumentAcceptsDrags = pluginView->shouldAllowNavigationFromDrags();
> 
> Local variable doesn’t help here. I would write:
> 
>     if (auto* pluginDocument = dynamicDowncast<PluginDocument>(document)) {
>         if (auto* view =
> dynamicDowncast<PluginViewBase>(pluginDocument->pluginWidget())
>             pluginDocumentAcceptsDrags =
> view->shouldAllowNavigationFromDrags();
>     }
> 
> > Source/WebCore/platform/DragImage.cpp:83
> > -        : element(is<Element>(node) ? &downcast<Element>(node) : nullptr)
> > +        : element(dynamicDowncast<Element>(node))
> 
> Makes me wonder if we really want the parentElement here when the node is
> not an element. Presumably not, but so strange that we just don’t tell any
> element if a non-element node is being dragged. Maybe in practice the node
> is always an element and the checking here is a dead code path.
> 
> > Source/WebKitLegacy/mac/DOM/DOMUIKitExtensions.mm:275
> >              if (is<RenderBox>(*renderer)) {
> > -                RenderBox& asBox = renderer->enclosingBox();
> > -                RenderObject* parent = asBox.parent();
> > -                RenderBox* parentRenderBox = is<RenderBox>(parent) ? downcast<RenderBox>(parent) : nullptr;
> > +                auto& asBox = renderer->enclosingBox();
> > +                auto* parentRenderBox = dynamicDowncast<RenderBox>(asBox.parent());
> >                  if (parentRenderBox && asBox.width() == parentRenderBox->width()) {
> >                      noCost = YES;
> >                  }
> 
> Noticed I am really not happy with the asBox argument name, because it’s not
> renderer casted to RenderBox, even though the code above does check that
> renderer is a RenderBox. Not a new concern, but messy. The variable name is
> basically an incorrect comment!

Wow, I just looked deeper at the code and this essentially casts the renderer to a RenderBox is a very weird way.

enclosingBox() is defined as:
```
RenderBox& RenderObject::enclosingBox() const
{
    return *lineageOfType<RenderBox>(const_cast<RenderObject&>(*this)).first();
}
```

Since we previously checked that renderer was a RenderBox, lineageOfType<RenderBox>(renderer).first() WILL actually return the renderer casted to a RenderBox.

So weird, I'll look into using a simple cast.

[Chris Dumez]

Created attachment 449271 [details]
Patch

[Darin Adler]

Comment on attachment 449266 [details]
Patch

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

>> Source/WebCore/page/DragController.cpp:349
>>      }
> 
> Might just want to do one longer expression here instead of two lines. Wouldn’t we want to use dynamicDowncast<ShadowRoot> here too? Even better if we had a function that returned the host, and null if the root node is not a ShadowRoot. Maybe such a function does exist?

And, yes Node::shadowHost() is that function. And given the rest of the context, the new code should just be this:

    if (inputElement->isTextButton())
        inputElement = dynamicDowncast<HTMLInputElement>(inputElement->shadowHost());

I studied the function and we don’t need other checks.

[Chris Dumez]

Created attachment 449272 [details]
Patch

[EWS]

Committed r288069 (246089@main): <https://commits.webkit.org/246089@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449272 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87643782>