Bug 139557 - Addressing post-review comments in r177035
Summary: Addressing post-review comments in r177035
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-11 12:57 PST by Myles C. Maxfield
Modified: 2014-12-17 14:29 PST (History)
6 users (show)

See Also:


Attachments
Patch (15.74 KB, patch)
2014-12-11 13:11 PST, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-12-11 12:57:16 PST
Addressing post-review comments in r177035
Comment 1 Myles C. Maxfield 2014-12-11 13:11:42 PST
Created attachment 243141 [details]
Patch
Comment 2 Darin Adler 2014-12-12 10:12:03 PST
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.
Comment 3 Myles C. Maxfield 2014-12-15 12:14:19 PST
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.
Comment 4 Myles C. Maxfield 2014-12-15 12:24:20 PST
Committed r177301: <http://trac.webkit.org/changeset/177301>
Comment 5 Darin Adler 2014-12-16 09:30:25 PST
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*.
Comment 6 Myles C. Maxfield 2014-12-16 22:42:50 PST
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.
Comment 7 Darin Adler 2014-12-16 22:44:39 PST
(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.
Comment 8 Myles C. Maxfield 2014-12-17 08:34:57 PST
(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
Comment 9 Darin Adler 2014-12-17 08:46:46 PST
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();
Comment 10 Myles C. Maxfield 2014-12-17 14:29:30 PST
Yeah, the testers realized this mistake and I already fixed it. r177449