Bug 109825 - [Text Autosizing] Combine narrow descendants of a cluster into groups that should be autosized with the same multiplier.
Summary: [Text Autosizing] Combine narrow descendants of a cluster into groups that sh...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Unspecified
: P2 Normal
Assignee: Anton Vayvod
URL:
Keywords:
Depends on: 109573
Blocks: 107300
  Show dependency treegraph
 
Reported: 2013-02-14 06:36 PST by Anton Vayvod
Modified: 2013-02-19 04:47 PST (History)
5 users (show)

See Also:


Attachments
Patch (11.00 KB, patch)
2013-02-14 06:47 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (10.39 KB, patch)
2013-02-18 07:42 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (10.47 KB, patch)
2013-02-18 08:27 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (10.49 KB, patch)
2013-02-18 11:38 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-14 06:36:50 PST
[Text Autosizing] Combine narrow descendants of a cluster into groups that should be autosized with the same multiplier.
Comment 1 Anton Vayvod 2013-02-14 06:47:30 PST
Created attachment 188339 [details]
Patch
Comment 2 John Mellor 2013-02-17 16:21:06 PST
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?
Comment 3 Anton Vayvod 2013-02-18 07:42:12 PST
Created attachment 188887 [details]
Patch
Comment 4 Anton Vayvod 2013-02-18 07:47:45 PST
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.
Comment 5 Kenneth Rohde Christiansen 2013-02-18 08:24:49 PST
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
Comment 6 Anton Vayvod 2013-02-18 08:27:37 PST
Created attachment 188893 [details]
Patch
Comment 7 Anton Vayvod 2013-02-18 08:28:45 PST
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?
Comment 8 John Mellor 2013-02-18 08:44:47 PST
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/
Comment 9 Kenneth Rohde Christiansen 2013-02-18 11:21:42 PST
> Hmm, how about just "clusterWiderThan"? Kenneth, what do you think?

Sounds good

or clusterWiderThanComparisonFn()
Comment 10 Anton Vayvod 2013-02-18 11:38:04 PST
Created attachment 188924 [details]
Patch
Comment 11 WebKit Review Bot 2013-02-19 04:47:08 PST
Comment on attachment 188924 [details]
Patch

Clearing flags on attachment: 188924

Committed r143318: <http://trac.webkit.org/changeset/143318>
Comment 12 WebKit Review Bot 2013-02-19 04:47:13 PST
All reviewed patches have been landed.  Closing bug.