Bug 269199 – AX: Replace is<T> downcast<t> with dynamicDowncast<T> uncheckedDowncast<T> when appropriate.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

.

[Radar WebKit Bug Importer]

<rdar://problem/122804785>

[Andres Gonzalez]

Created attachment 469826 [details]
Patch

[Tyler Wilcock]

Comment on attachment 469826 [details]
Patch

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

> COMMIT_MESSAGE:9
> +is<T> followed by downcast<t> has a performance impact since it is performing the type check twice. In those cases the code can be re-wtitten using dynamicDonwcast<T>. In other cases where we are certain that the downcast is returning the correct type, we should use uncheckedDowncast. This patch makes these changes in some of the accessibility core code. More similar changes in subsequent patches. this patch also include some code cleanup and more adoption of smart mpointer to replace raw pointers.

typos:

re-wtitten
dynamicDonwcast
mpointer

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:183
> +        continuation = uncheckedDowncast<RenderInline>(*continuation).continuation();

I get this downcast existed before this patch...but I can't understand why it's safe not to type-check here. Furthermore, isn't the downcast unnecessary? continuation() exists on RenderBoxModelObject, so by my reading we shouldn't need it?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:203
> +        firstChild = firstChildInContinuation(uncheckedDowncast<RenderInline>(renderer));

Rather than doing an unchecked downcast, can we make firstChildInContinuation take a RenderBoxModelObject?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:451
> +    WeakPtr renderInline = dynamicDowncast<RenderInline>(renderer);
> +    if (renderInline && !renderer.isReplacedOrInlineBlock())
> +        return renderInline->continuation();

The work we perform to cast here is not used if !renderer.isReplacedOrInlineBlock().

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:718
> +                auto& altText = renderTextFragment->altText();

const auto&?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:721
> +                return renderTextFragment ? renderTextFragment->contentString() : String();

Do we need a null-check here? Entering this block is predicated on renderTextFragment being non-null, and I don't think acquiring a reference to a string via altText() will ever invalidate the WeakPtr.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:724
> +            return renderText ? renderText->text() : String();

Do we need a null-check here? Entering this block is predicated on renderText being non-null.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1375
> -    if (!m_renderer || !is<RenderView>(*m_renderer))
> +    if (!m_renderer)
>          return 0;
> -    return downcast<RenderView>(*m_renderer).frameView().layoutContext().layoutCount();
> +
> +    auto* view = dynamicDowncast<RenderView>(*m_renderer);
> +    return view ? view->frameView().layoutContext().layoutCount() : 0;

Can write this more succinctly:

auto* view = dynamicDowncast<RenderView>(m_renderer.get());
return view ? view->frameView().layoutContext().layoutCount() : 0;

[Andres Gonzalez]

Created attachment 469835 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 469826 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469826&action=review
> 
> > COMMIT_MESSAGE:9
> > +is<T> followed by downcast<t> has a performance impact since it is performing the type check twice. In those cases the code can be re-wtitten using dynamicDonwcast<T>. In other cases where we are certain that the downcast is returning the correct type, we should use uncheckedDowncast. This patch makes these changes in some of the accessibility core code. More similar changes in subsequent patches. this patch also include some code cleanup and more adoption of smart mpointer to replace raw pointers.
> 
> typos:
> 
> re-wtitten
> dynamicDonwcast
> mpointer

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:183
> > +        continuation = uncheckedDowncast<RenderInline>(*continuation).continuation();
> 
> I get this downcast existed before this patch...but I can't understand why
> it's safe not to type-check here. Furthermore, isn't the downcast
> unnecessary? continuation() exists on RenderBoxModelObject, so by my reading
> we shouldn't need it?

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:203
> > +        firstChild = firstChildInContinuation(uncheckedDowncast<RenderInline>(renderer));
> 
> Rather than doing an unchecked downcast, can we make
> firstChildInContinuation take a RenderBoxModelObject?

Changed the type of the param to firstChildInContinuation but we still need a cast here since renderer is a RenderObject.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:451
> > +    WeakPtr renderInline = dynamicDowncast<RenderInline>(renderer);
> > +    if (renderInline && !renderer.isReplacedOrInlineBlock())
> > +        return renderInline->continuation();
> 
> The work we perform to cast here is not used if
> !renderer.isReplacedOrInlineBlock().

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:718
> > +                auto& altText = renderTextFragment->altText();
> 
> const auto&?

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:721
> > +                return renderTextFragment ? renderTextFragment->contentString() : String();
> 
> Do we need a null-check here? Entering this block is predicated on
> renderTextFragment being non-null, and I don't think acquiring a reference
> to a string via altText() will ever invalidate the WeakPtr.

I'd rather keep the null-check here since altText() may not be a trivial method.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:724
> > +            return renderText ? renderText->text() : String();
> 
> Do we need a null-check here? Entering this block is predicated on
> renderText being non-null.

Likewise here.
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1375
> > -    if (!m_renderer || !is<RenderView>(*m_renderer))
> > +    if (!m_renderer)
> >          return 0;
> > -    return downcast<RenderView>(*m_renderer).frameView().layoutContext().layoutCount();
> > +
> > +    auto* view = dynamicDowncast<RenderView>(*m_renderer);
> > +    return view ? view->frameView().layoutContext().layoutCount() : 0;
> 
> Can write this more succinctly:
> 
> auto* view = dynamicDowncast<RenderView>(m_renderer.get());
> return view ? view->frameView().layoutContext().layoutCount() : 0;

Fixed.

[Andres Gonzalez]

Created attachment 469838 [details]
Patch

[Andres Gonzalez]

Created attachment 469839 [details]
Patch

[EWS]

Committed 274522@main (cfef85306eb0): <https://commits.webkit.org/274522@main>

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