Bug 109093 - [Text Autosizing] Split isAutosizingCluster into three independent checks
Summary: [Text Autosizing] Split isAutosizingCluster into three independent checks
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: Anton Vayvod
URL:
Keywords:
Depends on:
Blocks: 107300 109573
  Show dependency treegraph
 
Reported: 2013-02-06 14:03 PST by Anton Vayvod
Modified: 2013-02-12 05:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (20.27 KB, patch)
2013-02-06 14:13 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (18.74 KB, patch)
2013-02-06 14:16 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (9.04 KB, patch)
2013-02-07 07:38 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (9.37 KB, patch)
2013-02-07 12:18 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anton Vayvod 2013-02-06 14:03:18 PST
[Text Autosizing] Introduce an enum representing different types of renderers
Comment 1 Anton Vayvod 2013-02-06 14:13:56 PST
Created attachment 186918 [details]
Patch
Comment 2 Anton Vayvod 2013-02-06 14:16:39 PST
Created attachment 186919 [details]
Patch
Comment 3 Julien Chaffraix 2013-02-06 18:31:55 PST
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?).
Comment 4 John Mellor 2013-02-07 03:20:45 PST
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?
Comment 5 Anton Vayvod 2013-02-07 07:38:37 PST
Created attachment 187108 [details]
Patch
Comment 6 Anton Vayvod 2013-02-07 08:59:45 PST
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.
Comment 7 John Mellor 2013-02-07 10:58:11 PST
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?).
Comment 8 Anton Vayvod 2013-02-07 12:18:44 PST
Created attachment 187145 [details]
Patch
Comment 9 Anton Vayvod 2013-02-07 12:20:36 PST
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.
Comment 10 John Mellor 2013-02-07 12:23:43 PST
Comment on attachment 187145 [details]
Patch

Looks good. What do you think Julien?
Comment 11 Anton Vayvod 2013-02-07 12:56:18 PST
Since Julien is OOO for a couple of days, adding Kenneth as a reviewer. Thanks!
Comment 12 Kenneth Rohde Christiansen 2013-02-08 07:25:56 PST
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
Comment 13 Anton Vayvod 2013-02-08 07:43:20 PST
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.
Comment 14 Kenneth Rohde Christiansen 2013-02-08 08:09:35 PST
Comment on attachment 187145 [details]
Patch

r=me as this is an improvement
Comment 15 WebKit Review Bot 2013-02-08 08:42:28 PST
Comment on attachment 187145 [details]
Patch

Clearing flags on attachment: 187145

Committed r142287: <http://trac.webkit.org/changeset/142287>
Comment 16 WebKit Review Bot 2013-02-08 08:42:33 PST
All reviewed patches have been landed.  Closing bug.