Bug 139557 – Addressing post-review comments in r177035

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Addressing post-review comments in r177035

[Myles C. Maxfield]

Created attachment 243141 [details]
Patch

[Darin Adler]

Comment on attachment 243141 [details]
Patch

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

I like the direction here, but we need a little more attention to various details, I think, to get the full benefit here. Some call sites are using an empty rectangle when there is no rectangle where instead they should be doing nothing at all in that case. Other call sites are not updating layout but it’s often wrong to get a renderer without doing that first. And all the call sites are using RenderObject* but I think that’s often not a specific-enough type.

> Source/WebCore/accessibility/AccessibilitySlider.cpp:162
> +    if (RenderObject* thumbRenderer = downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement()->renderer())

I don’t think an HTMLInputElement’s renderer is a RenderObject. I think it’s a more specific class, probably RenderElement. I suggest using auto* or RenderElement* instead of explicitly widening the type to RenderObject. Same might apply to the m_parent->renderer result above, or maybe not. But also, it’s kind of weak to check that the renderer is a RenderSlider and then not cast to that type before getting the element. Generally once we do a type check we want to use the narrower type because we might get more efficient code, even if not now, in the future.

> Source/WebCore/html/ColorInputType.cpp:209
> +    if (RenderObject* renderer = element().renderer())
> +        element().document().view()->contentsToRootView(renderer->absoluteBoundingBoxRect());
> +    return IntRect();

Same comment. Not sure RenderObject is the correct type here.

I’m also not sure why it’s right for this to call renderer() without updating layout first. Finally, I think early return reads better even though we lose the opportunity to narrowly scope the local variable by defining it in the if condition.

> Source/WebCore/html/HTMLInputElement.cpp:1880
> +    if (RenderObject* renderer = this->renderer())

Again.

> Source/WebCore/html/ValidationMessage.cpp:182
> +    if (RenderObject* renderer = m_element->renderer())

Again.

> Source/WebCore/html/ValidationMessage.cpp:184
> +    adjustBubblePosition(boundingBox, m_bubble.get());

I think it’s strange to pass an empty rectangle in when there’s no renderer. We should think about whether that’s actually helpful. Maybe we should be skipping more code rather than just using an empty rectangle. Probably applies to many more of the call sites, not just this one.

[Myles C. Maxfield]

Comment on attachment 243141 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilitySlider.cpp:162
>> +    if (RenderObject* thumbRenderer = downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement()->renderer())
> 
> I don’t think an HTMLInputElement’s renderer is a RenderObject. I think it’s a more specific class, probably RenderElement. I suggest using auto* or RenderElement* instead of explicitly widening the type to RenderObject. Same might apply to the m_parent->renderer result above, or maybe not. But also, it’s kind of weak to check that the renderer is a RenderSlider and then not cast to that type before getting the element. Generally once we do a type check we want to use the narrower type because we might get more efficient code, even if not now, in the future.

Done.

m_parent is an AccessibilityObject, which means that renderer() returns a RenderObject.

I think you're right about downcasting the RenderSlider, but in this case, I'm not sure it actually makes sense. Here, we are only getting the renderer so that we can get the node from it, and RenderSlider::node() doesn't have any overrides of RenderObject::node(). If we downcasted to a RenderSlider, it would look like downcast<HTMLInputElement>(downcast<RenderSlider>(sliderRenderer).node()) which seems too verbose for not really any gain.

>> Source/WebCore/html/ColorInputType.cpp:209
>> +    return IntRect();
> 
> Same comment. Not sure RenderObject is the correct type here.
> 
> I’m also not sure why it’s right for this to call renderer() without updating layout first. Finally, I think early return reads better even though we lose the opportunity to narrowly scope the local variable by defining it in the if condition.

Done.

I'm not sure either. I've updated https://bugs.webkit.org/show_bug.cgi?id=139512 to include this bit of research.

>> Source/WebCore/html/HTMLInputElement.cpp:1880
>> +    if (RenderObject* renderer = this->renderer())
> 
> Again.

This call actually returns a RenderObject.

>> Source/WebCore/html/ValidationMessage.cpp:182
>> +    if (RenderObject* renderer = m_element->renderer())
> 
> Again.

Done.

>> Source/WebCore/html/ValidationMessage.cpp:184
>> +    adjustBubblePosition(boundingBox, m_bubble.get());
> 
> I think it’s strange to pass an empty rectangle in when there’s no renderer. We should think about whether that’s actually helpful. Maybe we should be skipping more code rather than just using an empty rectangle. Probably applies to many more of the call sites, not just this one.

Yeah, you're right. Done.

[Myles C. Maxfield]

Committed r177301: <http://trac.webkit.org/changeset/177301>

[Darin Adler]

Comment on attachment 243141 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilitySlider.cpp:162
>>> +    if (RenderObject* thumbRenderer = downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement()->renderer())
>> 
>> I don’t think an HTMLInputElement’s renderer is a RenderObject. I think it’s a more specific class, probably RenderElement. I suggest using auto* or RenderElement* instead of explicitly widening the type to RenderObject. Same might apply to the m_parent->renderer result above, or maybe not. But also, it’s kind of weak to check that the renderer is a RenderSlider and then not cast to that type before getting the element. Generally once we do a type check we want to use the narrower type because we might get more efficient code, even if not now, in the future.
> 
> Done.
> 
> m_parent is an AccessibilityObject, which means that renderer() returns a RenderObject.
> 
> I think you're right about downcasting the RenderSlider, but in this case, I'm not sure it actually makes sense. Here, we are only getting the renderer so that we can get the node from it, and RenderSlider::node() doesn't have any overrides of RenderObject::node(). If we downcasted to a RenderSlider, it would look like downcast<HTMLInputElement>(downcast<RenderSlider>(sliderRenderer).node()) which seems too verbose for not really any gain.

No, you got this wrong. It’s *wrong* for this code to hardcode the knowledge that the node() of a RenderSlider is an HTMLInputElement. That assumption is simply incorrect to include in the accessibility code. It needs to be in RenderSlider’s own code. And contrary to what you say, everything needed is in place.

There is a function named RenderSlider::element, which returns an HTMLInputElement&. That is definitely what you should be using instead of calling RenderObject::node() and then casting to HTMLInputElement*. You say that RenderSlider::node doesn’t have overrides of RenderObject::node, but you’re missing the fact that RenderElement, a class in the RenderSlider hierarchy, completely deletes the RenderObject::node() function so you can’t call it without casting to RenderObject. The code should look like this:

    if (auto* thumbRenderer = downcast<RenderSlider>(*sliderRenderer).element().renderer())

Or if you prefer to use RenderElement* instead of auto, that would be OK.

>>> Source/WebCore/html/HTMLInputElement.cpp:1880
>>> +    if (RenderObject* renderer = this->renderer())
>> 
>> Again.
> 
> This call actually returns a RenderObject.

No.

HTMLInputElement has ContainerNode in its inheritance hierarchy. ContainerNode::renderer returns a RenderElement*.

[Myles C. Maxfield]

Comment on attachment 243141 [details]
Patch

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

Because these changes are trivial, I'll manually commit them tomorrow (It's too late tonight).

>>>> Source/WebCore/accessibility/AccessibilitySlider.cpp:162
>>>> +    if (RenderObject* thumbRenderer = downcast<HTMLInputElement>(sliderRenderer->node())->sliderThumbElement()->renderer())
>>> 
>>> I don’t think an HTMLInputElement’s renderer is a RenderObject. I think it’s a more specific class, probably RenderElement. I suggest using auto* or RenderElement* instead of explicitly widening the type to RenderObject. Same might apply to the m_parent->renderer result above, or maybe not. But also, it’s kind of weak to check that the renderer is a RenderSlider and then not cast to that type before getting the element. Generally once we do a type check we want to use the narrower type because we might get more efficient code, even if not now, in the future.
>> 
>> Done.
>> 
>> m_parent is an AccessibilityObject, which means that renderer() returns a RenderObject.
>> 
>> I think you're right about downcasting the RenderSlider, but in this case, I'm not sure it actually makes sense. Here, we are only getting the renderer so that we can get the node from it, and RenderSlider::node() doesn't have any overrides of RenderObject::node(). If we downcasted to a RenderSlider, it would look like downcast<HTMLInputElement>(downcast<RenderSlider>(sliderRenderer).node()) which seems too verbose for not really any gain.
> 
> No, you got this wrong. It’s *wrong* for this code to hardcode the knowledge that the node() of a RenderSlider is an HTMLInputElement. That assumption is simply incorrect to include in the accessibility code. It needs to be in RenderSlider’s own code. And contrary to what you say, everything needed is in place.
> 
> There is a function named RenderSlider::element, which returns an HTMLInputElement&. That is definitely what you should be using instead of calling RenderObject::node() and then casting to HTMLInputElement*. You say that RenderSlider::node doesn’t have overrides of RenderObject::node, but you’re missing the fact that RenderElement, a class in the RenderSlider hierarchy, completely deletes the RenderObject::node() function so you can’t call it without casting to RenderObject. The code should look like this:
> 
>     if (auto* thumbRenderer = downcast<RenderSlider>(*sliderRenderer).element().renderer())
> 
> Or if you prefer to use RenderElement* instead of auto, that would be OK.

I see, that makes sense. Thanks for the explanation.

Done.

>>>> Source/WebCore/html/HTMLInputElement.cpp:1880
>>>> +    if (RenderObject* renderer = this->renderer())
>>> 
>>> Again.
>> 
>> This call actually returns a RenderObject.
> 
> No.
> 
> HTMLInputElement has ContainerNode in its inheritance hierarchy. ContainerNode::renderer returns a RenderElement*.

You're right; this is a case of trusting Xcode's index over actually checking. I command-clicked on renderer() and it took me to AccessibilityRenderObject::renderer() and I only looked at its return type. :-|

Done.

[Darin Adler]

(In reply to comment #6)
> Because these changes are trivial, I'll manually commit them tomorrow (It's
> too late tonight).

No rush. Just wanted to get this as clean as possible while we are modifying the code.

[Myles C. Maxfield]

(In reply to comment #7)
> (In reply to comment #6)
> > Because these changes are trivial, I'll manually commit them tomorrow (It's
> > too late tonight).
> 
> No rush. Just wanted to get this as clean as possible while we are modifying
> the code.

Committed in r177444

[Darin Adler]

Oops, you checked in my bad code!

I wrote this:

    if (auto* thumbRenderer = downcast<RenderSlider>(*sliderRenderer).element().renderer()) 
        return thumbRenderer->absoluteBoundingBoxRect(); 

But it needs to be this:

    if (auto* thumbRenderer = downcast<RenderSlider>(*sliderRenderer).element().sliderThumbElement()->renderer()) 
        return thumbRenderer->absoluteBoundingBoxRect();

[Myles C. Maxfield]

Yeah, the testers realized this mistake and I already fixed it. r177449