TextAutosizing: adjust the maximum difference between cluster text width and its descendant width

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Anton Vayvod]

Adjust the maximum width difference between a narrow descendant and its parent cluster's text width if the current width difference is less than 50px bigger. This allows layouts like nested comments to be handled better: deep threads are still considered to be a part of the same cluster and the font is boosted equally for all narrow nodes.

[Anton Vayvod]

Created attachment 185641 [details]
Patch

[Anton Vayvod]

Created attachment 186169 [details]
Patch

[Anton Vayvod]

Hey, Julien! Could you take a look?

[Anton Vayvod]

Hi Kenneth, could you, please, take a look? This is the next change to the one you reviewed last week. Thanks!

[Kenneth Rohde Christiansen]

Comment on attachment 186169 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +        This allows layouts like nested comments to be handled better: deep threads are still considered to be a part
> +        of the same cluster and the font is boosted equally for all the arrow nodes.
> +        

I would try to keep the lines at max 100 chars

> Source/WebCore/ChangeLog:3653
> +        * rendering/TextAutosizer.cpp:
> +        (WebCore::TextAutosizingClusterInfo::TextAutosizingClusterInfo):
> +        (TextAutosizingClusterInfo):
> +        (WebCore):
> +        (WebCore::TextAutosizer::processSubtree):
> +        (WebCore::TextAutosizer::processCluster):
> +        (WebCore::TextAutosizer::processContainer):
> +        (WebCore::TextAutosizer::isAutosizingCluster):
> +        (WebCore::TextAutosizer::clusterShouldBeAutosized):
> +        (WebCore::TextAutosizer::measureDescendantTextWidth):
> +        * rendering/TextAutosizer.h:
> +        (WebCore):
> +
>  2013-01-30  Dominik Röttsches  <[e-mail]>

This seems unrelated... there is an entry by Pavel before this!

> Source/WebCore/rendering/TextAutosizer.cpp:54
> +        , maxAllowedDifferenceFromTextWidth(150)

what kind of difference? That is not obvious

[John Mellor]

Comment on attachment 186169 [details]
Patch

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

Code looks good and test looks good. Might be good to explain what's going on slightly more clearly though, as this is a rather subtle patch.

> Source/WebCore/ChangeLog:5
> +        This allows layouts like nested comments to be handled better: deep threads are still considered to be a part

Please make this description more detailed, as the purpose of the patch is somewhat subtle :)

> Source/WebCore/ChangeLog:29
> +        * rendering/TextAutosizer.cpp:

You have 4 copies of the list of changed files. This one, one above (that you've filled in), and 2 below, one of which in the middle of pfeldman's patch. Please remove the last 3.

> Source/WebCore/rendering/TextAutosizer.cpp:60
> +    float maxAllowedDifferenceFromTextWidth;

It might be nice to have some explanation of this property / starting value. There was a comment below ("Upper limit on the difference between the width of the parent block containing all text and that of a narrow child before the child becomes a cluster.") which gave a reasonable overview (though in this context, I'd probably s/the parent/a cluster's/ and s/a cluster/a separate cluster/).

> Source/WebCore/rendering/TextAutosizer.cpp:269
> +        const float maxWidthDifferenceDifference = 50;

This comment probably isn't still accurate (seeing as the variable name changed).

[Anton Vayvod]

Comment on attachment 186169 [details]
Patch

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

>> Source/WebCore/ChangeLog:29
>> +        * rendering/TextAutosizer.cpp:
> 
> You have 4 copies of the list of changed files. This one, one above (that you've filled in), and 2 below, one of which in the middle of pfeldman's patch. Please remove the last 3.

Fixed.

>> Source/WebCore/ChangeLog:3653
>>  2013-01-30  Dominik Röttsches  <[e-mail]>
> 
> This seems unrelated... there is an entry by Pavel before this!

Yep, thanks for catching!

>> Source/WebCore/rendering/TextAutosizer.cpp:54
>> +        , maxAllowedDifferenceFromTextWidth(150)
> 
> what kind of difference? That is not obvious

contentWidth()'s difference between the current RenderObject and blockContainingAllText. Added a comment per John's comment for the member definition below.
On a second thought, it would make sense to handle updating of this field and comparison in a method of this struct (it could even be a const method and the member could be mutable). What do you think?

>> Source/WebCore/rendering/TextAutosizer.cpp:60
>> +    float maxAllowedDifferenceFromTextWidth;
> 
> It might be nice to have some explanation of this property / starting value. There was a comment below ("Upper limit on the difference between the width of the parent block containing all text and that of a narrow child before the child becomes a cluster.") which gave a reasonable overview (though in this context, I'd probably s/the parent/a cluster's/ and s/a cluster/a separate cluster/).

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:269
>> +        const float maxWidthDifferenceDifference = 50;
> 
> This comment probably isn't still accurate (seeing as the variable name changed).

Done.

[Anton Vayvod]

Created attachment 186399 [details]
Patch

[John Mellor]

Comment on attachment 186399 [details]
Patch

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

Thanks, that's a lot clearer.

> Source/WebCore/ChangeLog:3
> +        TextAutosizing: adjust the maximum difference between cluster text width and its descendant width

You have two (slightly different) bug titles?

> Source/WebCore/ChangeLog:7
> +        a separate autosizing cluster. This doesn't work well for layouts when narrow nodes are

Please include a link to webkit.org/b/105188, which introduced the narrow test (and hence contains further details).

> Source/WebCore/rendering/TextAutosizer.cpp:61
> +    // Upper limit on the difference between the width of a cluster's block containing all

Nit: s/a cluster's/the cluster's/ (my bad!)

> Source/WebCore/rendering/TextAutosizer.cpp:270
> +        // The upper limit on how many pixels the difference between the object width

Nit: s/object/renderer/? Object is a bit vague...

> Source/WebCore/rendering/TextAutosizer.cpp:271
> +        // and its parent cluster width can exceed the current maximum difference

Nit: s/maximum difference/maximum difference by/. Still a little hard to read, but it's admittedly a complex thing to explain, and I don't have any immediate suggestions :)

[Build Bot]

Comment on attachment 186399 [details]
Patch

Attachment 186399 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16365440

[Build Bot]

Comment on attachment 186399 [details]
Patch

Attachment 186399 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16371417

[Anton Vayvod]

Comment on attachment 186399 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        TextAutosizing: adjust the maximum difference between cluster text width and its descendant width
> 
> You have two (slightly different) bug titles?

What can I say? Fixing two bugs with one patch ;)

>> Source/WebCore/ChangeLog:7
>> +        a separate autosizing cluster. This doesn't work well for layouts when narrow nodes are
> 
> Please include a link to webkit.org/b/105188, which introduced the narrow test (and hence contains further details).

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:61
>> +    // Upper limit on the difference between the width of a cluster's block containing all
> 
> Nit: s/a cluster's/the cluster's/ (my bad!)

I knew it!

>> Source/WebCore/rendering/TextAutosizer.cpp:270
>> +        // The upper limit on how many pixels the difference between the object width
> 
> Nit: s/object/renderer/? Object is a bit vague...

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:271
>> +        // and its parent cluster width can exceed the current maximum difference
> 
> Nit: s/maximum difference/maximum difference by/. Still a little hard to read, but it's admittedly a complex thing to explain, and I don't have any immediate suggestions :)

Done.

[Anton Vayvod]

Created attachment 186618 [details]
Patch

[Anton Vayvod]

Please, take another look?

[John Mellor]

Comment on attachment 186618 [details]
Patch

Looks good to me. Kenneth/Julien, what do you think?

[Kenneth Rohde Christiansen]

Comment on attachment 186618 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        each time the width difference is not greater than 50px from the previous one. This allows

these are pixels in CSS units right?

> Source/WebCore/rendering/TextAutosizer.cpp:273
> +        const float maxWidthDifferenceDifference = 50;

DifferenceDifference... not really clear...

maxDifferenceFromWidthDifference or differenceFromMaxWidthDifference...

[Anton Vayvod]

Created attachment 186646 [details]
Patch

[Anton Vayvod]

Comment on attachment 186618 [details]
Patch

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

>> Source/WebCore/ChangeLog:12
>> +        each time the width difference is not greater than 50px from the previous one. This allows
> 
> these are pixels in CSS units right?

Yes, clarified.

>> Source/WebCore/rendering/TextAutosizer.cpp:273
>> +        const float maxWidthDifferenceDifference = 50;
> 
> DifferenceDifference... not really clear...
> 
> maxDifferenceFromWidthDifference or differenceFromMaxWidthDifference...

The latter. Done.

[Anton Vayvod]

Created attachment 186648 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186648 [details]
Patch

Clearing flags on attachment: 186648

Committed r141901: <http://trac.webkit.org/changeset/141901>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Comment on attachment 186618 [details]
Patch

Cleared review? from obsolete attachment 186618 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).