Bug 109093 – [Text Autosizing] Split isAutosizingCluster into three independent checks

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

[Anton Vayvod]

[Text Autosizing] Introduce an enum representing different types of renderers

[Anton Vayvod]

Created attachment 186918 [details]
Patch

[Anton Vayvod]

Created attachment 186919 [details]
Patch

[Julien Chaffraix]

Comment on attachment 186919 [details]
Patch

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

> Source/WebCore/rendering/TextAutosizer.cpp:122
> +    while (cluster && getRendererType(cluster) != IndependentAutosizingCluster)

We don't prefix getters with get (Rule 6 of the coding style: "Use bare words for getters.")

> Source/WebCore/rendering/TextAutosizer.cpp:155
> -void TextAutosizer::processContainer(float multiplier, RenderBlock* container, TextAutosizingClusterInfo* clusterInfo, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)
> +void TextAutosizer::processContainer(float multiplier, RenderBlock* container, TextAutosizingClusterInfo& clusterInfo, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)

Not a super fan of squashing the pointer -> reference change as part of this one.

> Source/WebCore/rendering/TextAutosizer.cpp:172
> +                {

This is not in line with our coding style: we don't indent the parentheses like that.

> Source/WebCore/rendering/TextAutosizer.cpp:184
> +                    break;

This code is now super hard to read. By exporting the reason, I didn't mean you can't use the previous boolean methods for clarity. It just gave you much more flexibility if you needed it.

> Source/WebCore/rendering/TextAutosizer.h:62
> +    enum RendererType {

Probably more ClusterType or ContainerType instead of RendererType which is already way overloaded.

> Source/WebCore/rendering/TextAutosizer.h:72
> +        NarrowDescendant,
> +        // A wider descendant, considered to be an autosizing cluster because of its width greater
> +        // than that of the text width of the parent autosizing cluster.
> +        WiderDescendant,

These 2 names really don't convey the idea that they are AutosizingContainer too. Suggestions: AutosizingContainerWithNarrawDescendant, AutosizingContainerWithWiderDescendant (isn't WiderDescendant just a synonym of OverflowingDescendant?).

[John Mellor]

Comment on attachment 186919 [details]
Patch

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

The overall change does seem to make things clearer. Two nits.

> Source/WebCore/rendering/TextAutosizer.cpp:313
>      // the |blockContainingAllText| of their enclosing cluster also become clusters,

"become clusters" is a bit unclear, since we're now just terming these "Narrow/WiderDescendant".

It might make sense to move most of the above comment to isIndependentAutosizingCluster? Though I'm not sure where you'd put this last paragraph then...

> Source/WebCore/rendering/TextAutosizer.cpp:456
> +            RendererType rendererType(getRendererType(descendant, &clusterInfo));

Micro-nit: nothing wrong with this, but as it's just an enum, perhaps clearer to assign the value using '=' instead of a copy constructor?

[Anton Vayvod]

Created attachment 187108 [details]
Patch

[Anton Vayvod]

Comment on attachment 186919 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:122
>> +    while (cluster && getRendererType(cluster) != IndependentAutosizingCluster)
> 
> We don't prefix getters with get (Rule 6 of the coding style: "Use bare words for getters.")

Sorry, mixed with another style guide.

>> Source/WebCore/rendering/TextAutosizer.cpp:155
>> +void TextAutosizer::processContainer(float multiplier, RenderBlock* container, TextAutosizingClusterInfo& clusterInfo, RenderObject* subtreeRoot, const TextAutosizingWindowInfo& windowInfo)
> 
> Not a super fan of squashing the pointer -> reference change as part of this one.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:172
>> +                {
> 
> This is not in line with our coding style: we don't indent the parentheses like that.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:184
>> +                    break;
> 
> This code is now super hard to read. By exporting the reason, I didn't mean you can't use the previous boolean methods for clarity. It just gave you much more flexibility if you needed it.

I think if we just have three separate methods with clear naming that indicate if one of the reasons is true, we don't need an enum at all. Enum will require a switch and readability will probably suffer in most cases.

>> Source/WebCore/rendering/TextAutosizer.cpp:313
>>      // the |blockContainingAllText| of their enclosing cluster also become clusters,
> 
> "become clusters" is a bit unclear, since we're now just terming these "Narrow/WiderDescendant".
> 
> It might make sense to move most of the above comment to isIndependentAutosizingCluster? Though I'm not sure where you'd put this last paragraph then...

Done. Removed the last paragraph and introduced comments in isNarrow/WiderDescendant

>> Source/WebCore/rendering/TextAutosizer.cpp:456
>> +            RendererType rendererType(getRendererType(descendant, &clusterInfo));
> 
> Micro-nit: nothing wrong with this, but as it's just an enum, perhaps clearer to assign the value using '=' instead of a copy constructor?

Ack

>> Source/WebCore/rendering/TextAutosizer.h:62
>> +    enum RendererType {
> 
> Probably more ClusterType or ContainerType instead of RendererType which is already way overloaded.

Removed the enum.

>> Source/WebCore/rendering/TextAutosizer.h:72
>> +        WiderDescendant,
> 
> These 2 names really don't convey the idea that they are AutosizingContainer too. Suggestions: AutosizingContainerWithNarrawDescendant, AutosizingContainerWithWiderDescendant (isn't WiderDescendant just a synonym of OverflowingDescendant?).

I think overflowing might be confused with overflow CSS property which is a different thing.

[John Mellor]

Comment on attachment 187108 [details]
Patch

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

This seems reasonable. Just a few minor comment nits.

> Source/WebCore/ChangeLog:3
> +        [Text Autosizing] Refactor isAutosizingCluster.

You should probably rename the bug to match this.

> Source/WebCore/ChangeLog:33
> +            Handful method to check all separate conditions together.

s/Handful/Handy/ ?

> Source/WebCore/rendering/TextAutosizer.cpp:241
> +    // their enclosing cluster need to be autosized separately and thus treated the same way as

It's a little confusing to talk about "treated the same way as autosizing clusters", since based on the new method names, being narrower makes it by definition a cluster (i.e. isAutosizingCluster will return true). Perhaps:

    // Autosizing containers that are significantly narrower than the |blockContainingAllText| of
    // their enclosing cluster may be acting as separate columns, hence must be autosized
    // separately. For example the 2nd div in:
    // <body>
    //     <div style="float: right; width: 50%;"></div>
    //     <div style="width: 50%;"></div>
    // <body>
    // becomes the left column, and should be autosized differently from the body.

> Source/WebCore/rendering/TextAutosizer.cpp:244
> +    // the enclosing cluster and this upper limit of 200 units is a

"is a" what?

> Source/WebCore/rendering/TextAutosizer.cpp:263
> +    // cluster are treated the same way as autosizing clusters to be autosized separately.

Similarly, it's a little confusing to talk about "treated the same way as autosizing clusters", since based on the new method names, being wider makes it by definition a cluster. Perhaps "Autosizing containers that are wider than the |blockContainingAllText| of their enclosing cluster violate our assumption that all the descendants are narrower than the |blockContainingAllText|, and hence must be autosized separately."

> Source/WebCore/rendering/TextAutosizer.cpp:304
> +bool TextAutosizer::isAutosizingCluster(const RenderBlock* renderer, TextAutosizingClusterInfo* parentClusterInfo)

It would be nice to have an |ASSERT(isAutosizingContainer(renderer));| at the start of this (or perhaps the 3 is*Descendant methods should each have the assert, since they can be called individually?).

[Anton Vayvod]

Created attachment 187145 [details]
Patch

[Anton Vayvod]

Comment on attachment 187108 [details]
Patch

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

>> Source/WebCore/ChangeLog:33
>> +            Handful method to check all separate conditions together.
> 
> s/Handful/Handy/ ?

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:241
>> +    // their enclosing cluster need to be autosized separately and thus treated the same way as
> 
> It's a little confusing to talk about "treated the same way as autosizing clusters", since based on the new method names, being narrower makes it by definition a cluster (i.e. isAutosizingCluster will return true). Perhaps:
> 
>     // Autosizing containers that are significantly narrower than the |blockContainingAllText| of
>     // their enclosing cluster may be acting as separate columns, hence must be autosized
>     // separately. For example the 2nd div in:
>     // <body>
>     //     <div style="float: right; width: 50%;"></div>
>     //     <div style="width: 50%;"></div>
>     // <body>
>     // becomes the left column, and should be autosized differently from the body.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:244
>> +    // the enclosing cluster and this upper limit of 200 units is a
> 
> "is a" what?

Finished the sentence.

>> Source/WebCore/rendering/TextAutosizer.cpp:263
>> +    // cluster are treated the same way as autosizing clusters to be autosized separately.
> 
> Similarly, it's a little confusing to talk about "treated the same way as autosizing clusters", since based on the new method names, being wider makes it by definition a cluster. Perhaps "Autosizing containers that are wider than the |blockContainingAllText| of their enclosing cluster violate our assumption that all the descendants are narrower than the |blockContainingAllText|, and hence must be autosized separately."

Done. Thanks for the suggestions!

>> Source/WebCore/rendering/TextAutosizer.cpp:304
>> +bool TextAutosizer::isAutosizingCluster(const RenderBlock* renderer, TextAutosizingClusterInfo* parentClusterInfo)
> 
> It would be nice to have an |ASSERT(isAutosizingContainer(renderer));| at the start of this (or perhaps the 3 is*Descendant methods should each have the assert, since they can be called individually?).

Done.

[John Mellor]

Comment on attachment 187145 [details]
Patch

Looks good. What do you think Julien?

[Anton Vayvod]

Since Julien is OOO for a couple of days, adding Kenneth as a reviewer. Thanks!

[Kenneth Rohde Christiansen]

Comment on attachment 187145 [details]
Patch

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

LGTM

> Source/WebCore/rendering/TextAutosizer.cpp:243
> +bool TextAutosizer::isNarrowDescendant(const RenderBlock* renderer, TextAutosizingClusterInfo* parentClusterInfo)
> +{
> +    ASSERT(isAutosizingContainer(renderer));
> +
> +    // Autosizing containers that are significantly narrower than the |blockContainingAllText| of
> +    // their enclosing cluster may be acting as separate columns, hence must be autosized

I like this change, makes the code much more clear

> Source/WebCore/rendering/TextAutosizer.cpp:319
> +    return isNarrowDescendant(renderer, parentClusterInfo)
> +        || isWiderDescendant(renderer, parentClusterInfo)
> +        || isIndependentDescendant(renderer);

Do you know if one of these is much more common that the others? Maybe the order matters

[Anton Vayvod]

Comment on attachment 187145 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:319
>> +        || isIndependentDescendant(renderer);
> 
> Do you know if one of these is much more common that the others? Maybe the order matters

At the first glance there're no expensive checks in either of these methods so it shouldn't matter much, I believe. Then since the first two methods were introduced to fix only certain types of sites, both cases should probably happen less frequent than the style checks combined.

[Kenneth Rohde Christiansen]

Comment on attachment 187145 [details]
Patch

r=me as this is an improvement

[WebKit Review Bot]

Comment on attachment 187145 [details]
Patch

Clearing flags on attachment: 187145

Committed r142287: <http://trac.webkit.org/changeset/142287>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.