Bug 109825 – [Text Autosizing] Combine narrow descendants of a cluster into groups that should be autosized with the same multiplier.

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

[Anton Vayvod]

[Text Autosizing] Combine narrow descendants of a cluster into groups that should be autosized with the same multiplier.

[Anton Vayvod]

Created attachment 188339 [details]
Patch

[John Mellor]

Comment on attachment 188339 [details]
Patch

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

Useful addition, and code looks good (with nits), though I have this nagging feeling that CompareIndicesByWidthDifference/getNarrowDescendantsGroupedByDifferenceFromParentTextWidth is slightly overcomplicated.
Julien/Kenneth, what do you think?

> Source/WebCore/ChangeLog:29
> +            different groups if the difference between their deltas is greater than 100 (in CSS

That description doesn't seem quite right - if the deltas were [320, 400, 480], then those would all be the same group, right?

> Source/WebCore/rendering/TextAutosizer.cpp:579
> +void TextAutosizer::getNarrowDescendantsGroupedByDifferenceFromParentTextWidth(const TextAutosizingClusterInfo& parentClusterInfo, Vector<Vector<TextAutosizingClusterInfo> >& groups)

Nit: I wonder if this should return a Vector<Vector<TextAutosizingClusterInfo*> > to avoid copying the clusterinfos; they're moderately heavyweight, as they each contain a vector, though I guess in practice at this stage descendants are guaranteed to have empty narrowDescendants vectors so it's probably fine.

> Source/WebCore/rendering/TextAutosizer.cpp:585
> +    const float maxDifferenceWithinGroup = 100;

Probably want to explain the purpose of this constant, and mention that it was empirically determined. (might also be clearer to move it down to the bottom next to where it gets used?)

> Source/WebCore/rendering/TextAutosizer.cpp:588
> +    differencesFromParentTextWidth.reserveInitialCapacity(clusterInfos.size());

Nit: it doesn't seem necessary to actually compute the differences. Directly sorting and grouping the cluster widths should be exactly equivalent to sorting and grouping the differences (except you'd need to sort in the opposite order). Though if you want to compress clusterInfos to a Vector of floats anyway in order to make the sort comparisons cheaper, then I guess you may as well use (parentTextWidth - clusterWidth) rather than just clusterWidth. So do what feels best.

> Source/WebCore/rendering/TextAutosizer.cpp:595
> +        float parentTextWidth = parentClusterInfo.blockContainingAllText->contentLogicalWidth();

Move this out of the loop.

> Source/WebCore/rendering/TextAutosizer.cpp:603
> +            groups.append(Vector<TextAutosizingClusterInfo>());

Nit: Move this out of the loop (then instead of the repeated if, you just have if (!clusterIndices); and you could remove that if too by returning early towards the beginning of this function if narrowDescendants is empty). You might also want to change this to groups.grow(1) for conciseness.

> Source/WebCore/rendering/TextAutosizer.cpp:606
> +        const TextAutosizingClusterInfo& currentClusterInfo = clusterInfos[currentIndex];

Redundant variable.

> Source/WebCore/rendering/TextAutosizer.cpp:613
> +                groups.append(Vector<TextAutosizingClusterInfo>());

Nit: you could do groups.grow(groups.size() + 1), but I'm fine with this if you prefer.

> LayoutTests/fast/text-autosizing/narrow-descendants-combined.html:24
>      <div style="width: 320px">

Perhaps additionally add a 240px one, to show that 240 and 400 get grouped together because of the 320px one?

[Anton Vayvod]

Created attachment 188887 [details]
Patch

[Anton Vayvod]

Comment on attachment 188339 [details]
Patch

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

>> Source/WebCore/ChangeLog:29
>> +            different groups if the difference between their deltas is greater than 100 (in CSS
> 
> That description doesn't seem quite right - if the deltas were [320, 400, 480], then those would all be the same group, right?

Right, rephrased according to the new implementation.

>> Source/WebCore/rendering/TextAutosizer.cpp:579
>> +void TextAutosizer::getNarrowDescendantsGroupedByDifferenceFromParentTextWidth(const TextAutosizingClusterInfo& parentClusterInfo, Vector<Vector<TextAutosizingClusterInfo> >& groups)
> 
> Nit: I wonder if this should return a Vector<Vector<TextAutosizingClusterInfo*> > to avoid copying the clusterinfos; they're moderately heavyweight, as they each contain a vector, though I guess in practice at this stage descendants are guaranteed to have empty narrowDescendants vectors so it's probably fine.

I'd rather change it to Vector<Vector<TextAutosizingClusterInfo*> > in the following patch since narrowDescendants of clusterInfo's here are empty and changing to pointers will touch much unrelated code.

>> Source/WebCore/rendering/TextAutosizer.cpp:585
>> +    const float maxDifferenceWithinGroup = 100;
> 
> Probably want to explain the purpose of this constant, and mention that it was empirically determined. (might also be clearer to move it down to the bottom next to where it gets used?)

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:588
>> +    differencesFromParentTextWidth.reserveInitialCapacity(clusterInfos.size());
> 
> Nit: it doesn't seem necessary to actually compute the differences. Directly sorting and grouping the cluster widths should be exactly equivalent to sorting and grouping the differences (except you'd need to sort in the opposite order). Though if you want to compress clusterInfos to a Vector of floats anyway in order to make the sort comparisons cheaper, then I guess you may as well use (parentTextWidth - clusterWidth) rather than just clusterWidth. So do what feels best.

Done. Sorting by the width now. The code is much simpler.

>> Source/WebCore/rendering/TextAutosizer.cpp:595
>> +        float parentTextWidth = parentClusterInfo.blockContainingAllText->contentLogicalWidth();
> 
> Move this out of the loop.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:603
>> +            groups.append(Vector<TextAutosizingClusterInfo>());
> 
> Nit: Move this out of the loop (then instead of the repeated if, you just have if (!clusterIndices); and you could remove that if too by returning early towards the beginning of this function if narrowDescendants is empty). You might also want to change this to groups.grow(1) for conciseness.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:606
>> +        const TextAutosizingClusterInfo& currentClusterInfo = clusterInfos[currentIndex];
> 
> Redundant variable.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:613
>> +                groups.append(Vector<TextAutosizingClusterInfo>());
> 
> Nit: you could do groups.grow(groups.size() + 1), but I'm fine with this if you prefer.

Done.

>> LayoutTests/fast/text-autosizing/narrow-descendants-combined.html:24
>>      <div style="width: 320px">
> 
> Perhaps additionally add a 240px one, to show that 240 and 400 get grouped together because of the 320px one?

Done.

[Kenneth Rohde Christiansen]

Comment on attachment 188887 [details]
Patch

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

> Source/WebCore/rendering/TextAutosizer.cpp:564
> +bool greaterClusterInfoRootWidth(const TextAutosizingClusterInfo& first, const TextAutosizingClusterInfo& second)

I am not convinced about this name... what about something with compare... people are used to compare methods

[Anton Vayvod]

Created attachment 188893 [details]
Patch

[Anton Vayvod]

Comment on attachment 188887 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:564
>> +bool greaterClusterInfoRootWidth(const TextAutosizingClusterInfo& first, const TextAutosizingClusterInfo& second)
> 
> I am not convinced about this name... what about something with compare... people are used to compare methods

Ok, done. Do I need to reflect in the name that the method compares values in the descending order or will a comment suffice?

[John Mellor]

Comment on attachment 188893 [details]
Patch

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

Much simpler now, thanks. Looks good with teensy nits.

> I'd rather change it to Vector<Vector<TextAutosizingClusterInfo*> > in the
> following patch since narrowDescendants of clusterInfo's here are empty and
> changing to pointers will touch much unrelated code.

Oh, I'd forgotten that these get passed to processCompositeCluster. Definitely fine to keep it as non-pointers then.

> Source/WebCore/rendering/TextAutosizer.cpp:565
> +bool compareClusterInfosByRootWidth(const TextAutosizingClusterInfo& first, const TextAutosizingClusterInfo& second)

Hmm, how about just "clusterWiderThan"? Kenneth, what do you think?

> Source/WebCore/rendering/TextAutosizer.cpp:585
> +    // this value, the next element should start a new group.

Nit: s/value/empirically determined value/

[Kenneth Rohde Christiansen]

> Hmm, how about just "clusterWiderThan"? Kenneth, what do you think?

Sounds good

or clusterWiderThanComparisonFn()

[Anton Vayvod]

Created attachment 188924 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188924 [details]
Patch

Clearing flags on attachment: 188924

Committed r143318: <http://trac.webkit.org/changeset/143318>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.