[webkit-reviews] review granted: [Bug 235259] Introduce dynamicDowncast<>() for convenience : [Attachment 449266] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 235259: Introduce dynamicDowncast<>() for convenience
https://bugs.webkit.org/show_bug.cgi?id=235259

Attachment 449266: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 449266
  --> https://bugs.webkit.org/attachment.cgi?id=449266
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!