Bug 137728 – Have offsetFromContainer() / offsetFromAncestorContainer() take a RenderElement&

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Dumez]

Have offsetFromContainer() / offsetFromAncestorContainer() take a RenderElement& instead of a RenderObject*. The argument passed is never null and the type should be a RenderElement as the argument is a container.

[Chris Dumez]

Created attachment 239840 [details]
Patch

[Chris Dumez]

Created attachment 239843 [details]
Patch

[Benjamin Poulain]

Comment on attachment 239843 [details]
Patch

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

rs=me

> Source/WebCore/rendering/RenderBox.cpp:1915
> +    auto* container = this->container(repaintContainer, &containerSkipped);

IMHO, "auto" is bad here, the type is 100% non obvious.

> Source/WebCore/rendering/RenderBox.cpp:1964
> +    auto* container = this->container(ancestorToStopAt, &ancestorSkipped);

ditto

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2640
> +    auto* container = this->container();

ditto

> Source/WebCore/rendering/RenderObject.cpp:1874
> -        auto nextContainer = currContainer->container();
> +        auto* nextContainer = currContainer->container();

gosh I hate auto sometimes... It is just making code hard to follow :(

[Chris Dumez]

Comment on attachment 239843 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:1915
>> +    auto* container = this->container(repaintContainer, &containerSkipped);
> 
> IMHO, "auto" is bad here, the type is 100% non obvious.

I think Darin prefers auto* in this case because this way it always uses the same type returned by container(). For e.g., it is quite common for people to assign to a RenderObject* even though the function returns a RenderElement* nowadays. That said, I don't feel strongly either way. In this case, I do agree the type is not obvious.

[Chris Dumez]

Created attachment 239846 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 239846 [details]
Patch

Clearing flags on attachment: 239846

Committed r174722: <http://trac.webkit.org/changeset/174722>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious?

[Benjamin Poulain]

(In reply to comment #8)
> At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious?

For me it is all about reading/reviewing/understanding code. I would rather spend 5 minutes making the code explicit when writing it than creating cognitive load for anyone reading the code.

I like that "auto" always get the right type even after refactoring of disconnected parts of the code but the code readability suffers so much that the cost far outweigh the gains.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> > At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious?
> 
> For me it is all about reading/reviewing/understanding code. I would rather spend 5 minutes making the code explicit when writing it than creating cognitive load for anyone reading the code.

What I’m not clear on is whether this cognitive load is an intrinsic result of this coding style, or a result of the fact that we are accustomed to the other style. I often don’t need to know the types of things; the names of the functions we are calling on them seem sufficient. Every time I see an explicit type name such as RenderObject I wonder if the type is correct.

We do not see the type names of function results or data members each time we use them. Is there some reason we need these type names urgently for local variables, but not those other things?

[Benjamin Poulain]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > At some point we should talk this style point. I agree that the type is non-obvious, but I don’t agree that is a problem. Why does the type need to be obvious?
> > 
> > For me it is all about reading/reviewing/understanding code. I would rather spend 5 minutes making the code explicit when writing it than creating cognitive load for anyone reading the code.
> 
> What I’m not clear on is whether this cognitive load is an intrinsic result of this coding style, or a result of the fact that we are accustomed to the other style. I often don’t need to know the types of things; the names of the functions we are calling on them seem sufficient. Every time I see an explicit type name such as RenderObject I wonder if the type is correct.
> 
> We do not see the type names of function results or data members each time we use them. Is there some reason we need these type names urgently for local variables, but not those other things?

Funny you mention that, you always have me remove my local variables :) I do use local variables to make types explicit whenever I feel that it clarifies the flow.

Let's talk about this face to face when you have 5 minutes, this is not the best forum.