Bug 109573 – [Text Autosizing] Process narrow descendants with the same multiplier for the font size.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Anton Vayvod]

[Text Autosizing] Process narrow descendants with the same multiplier for the font size.

[Anton Vayvod]

Created attachment 187849 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 187849 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        For example, on sites with a sidebar, sometimes the paragraphs next to the sidebar will have
> +        a large margin individually applied (via a CSS selector), causing them all to individually
> +        appear narrower than their enclosing blockContainingAllText. Rather than making each of
> +        these paragraphs into a separate cluster, we eventually want to be able to merge them back
> +        together into one (or a few) descendant clusters.

Do you have some kind of meta bug on what other kind of adaptations you have planned?

> Source/WebCore/ChangeLog:27
> +            Calculates the text width for a single cluster and if it should be autosized, then calls

s/if/whether/

> Source/WebCore/ChangeLog:28
> +            processCluster() to apply the multiplier and process the cluster's descendants.

processSingleCluster sounds like a specialized version of processCluster and it is not. Confusing

> Source/WebCore/rendering/TextAutosizer.cpp:161
> +    // Many pages set a max-width on their content. So especially for the
> +    // RenderView, instead of just taking the width of |cluster| we find
> +    // the lowest common ancestor of the first and last descendant text node of
> +    // the cluster (i.e. the deepest wrapper block that contains all the text),
> +    // and use its width instead.

I think it would be fine to make this comment around 100 chars before wrapping.

> Source/WebCore/rendering/TextAutosizer.cpp:168
> +void TextAutosizer::processCompositeCluster(Vector<TextAutosizingClusterInfo>& clusterInfos, const TextAutosizingWindowInfo& windowInfo)
> +{

So no way to avoid iterating the clusterinfos 3 times?

> Source/WebCore/rendering/TextAutosizer.h:65
> +    void processCluster(TextAutosizingClusterInfo&, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&, float textWidth, bool shouldBeAutosized);
> +    void processSingleCluster(TextAutosizingClusterInfo&, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&);

I think processCluster vs processSingleCluster is confusing... Like what is the difference; that is not obvious here

[John Mellor]

Comment on attachment 187849 [details]
Patch

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

Change looks generally good. A couple of comments though.

> Source/WebCore/ChangeLog:14
> +        these paragraphs into a separate cluster, we eventually want to be able to merge them back

s/we eventually want to be able to merge them back together into one (or a few) descendant clusters./we want them all to share the same multiplier, as if they were a single cluster/.

> Source/WebCore/rendering/TextAutosizer.cpp:177
> +    const float minLinesOfText = 4;

I'm not keen on the way this code is duplicated from clusterShouldBeAutosized. Could you not just make clusterShouldBeAutosized accept a list of clusterInfos (using the multi-clusterInfo aware code below)?

> Source/WebCore/rendering/TextAutosizer.cpp:178
> +    float minTextWidth = maxTextWidth * minLinesOfText;

Nit: perhaps rename these to minDescendantTextWidth and totalDescendantTextWidth, to clarify that these are measuring a different type of thing than maxTextWidth (otherwise it seems like minTextWidth..maxTextWidth is some kind of range).

> Source/WebCore/rendering/TextAutosizer.cpp:182
> +        measureDescendantTextWidth(clusterInfos[i].blockContainingAllText, clusterInfos[i], minTextWidth, textWidth);

Shouldn't you be passing total(Descendant)TextWidth directly to measureDescendantTextWidth instead of textWidth? There is logic in measureDescendantTextWidth to early out once textWidth >= minTextWidth, but by hiding the true total(Descendant)TextWidth from measureDescendantTextWidth that logic won't be able to work. And since it IIRC just adds to the number passed in by reference, it should be equivalent to let it directly increase total(Descendant)TextWidth.

[Anton Vayvod]

Comment on attachment 187849 [details]
Patch

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

>> Source/WebCore/ChangeLog:14
>> +        these paragraphs into a separate cluster, we eventually want to be able to merge them back
> 
> s/we eventually want to be able to merge them back together into one (or a few) descendant clusters./we want them all to share the same multiplier, as if they were a single cluster/.

Done.

>> Source/WebCore/ChangeLog:15
>> +        together into one (or a few) descendant clusters.
> 
> Do you have some kind of meta bug on what other kind of adaptations you have planned?

There's the complete patch I've been splitting into smaller ones and submitting over the last week. See wkbug.com/107300.
This should be the penultimate patch.

>> Source/WebCore/ChangeLog:27
>> +            Calculates the text width for a single cluster and if it should be autosized, then calls
> 
> s/if/whether/

Done.

>> Source/WebCore/ChangeLog:28
>> +            processCluster() to apply the multiplier and process the cluster's descendants.
> 
> processSingleCluster sounds like a specialized version of processCluster and it is not. Confusing

renamed back to processCluster and renamed the common code holder to processClusterInternal

>> Source/WebCore/rendering/TextAutosizer.cpp:161
>> +    // and use its width instead.
> 
> I think it would be fine to make this comment around 100 chars before wrapping.

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:168
>> +{
> 
> So no way to avoid iterating the clusterinfos 3 times?

I could probably move the first loop to the place where we add cluster info to the vector, the second one exits early as soon as we have enough text and the third one is unavoidable.
But the code will do the same amount of work so it shouldn't matter from the performance point of view. Having this code in the same place makes the code clearer to me.

>> Source/WebCore/rendering/TextAutosizer.cpp:177
>> +    const float minLinesOfText = 4;
> 
> I'm not keen on the way this code is duplicated from clusterShouldBeAutosized. Could you not just make clusterShouldBeAutosized accept a list of clusterInfos (using the multi-clusterInfo aware code below)?

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:178
>> +    float minTextWidth = maxTextWidth * minLinesOfText;
> 
> Nit: perhaps rename these to minDescendantTextWidth and totalDescendantTextWidth, to clarify that these are measuring a different type of thing than maxTextWidth (otherwise it seems like minTextWidth..maxTextWidth is some kind of range).

Since I moved it to modified version of shouldClusterBeAutosized, I've left the names as they were there.

>> Source/WebCore/rendering/TextAutosizer.cpp:182
>> +        measureDescendantTextWidth(clusterInfos[i].blockContainingAllText, clusterInfos[i], minTextWidth, textWidth);
> 
> Shouldn't you be passing total(Descendant)TextWidth directly to measureDescendantTextWidth instead of textWidth? There is logic in measureDescendantTextWidth to early out once textWidth >= minTextWidth, but by hiding the true total(Descendant)TextWidth from measureDescendantTextWidth that logic won't be able to work. And since it IIRC just adds to the number passed in by reference, it should be equivalent to let it directly increase total(Descendant)TextWidth.

Done.

>> Source/WebCore/rendering/TextAutosizer.h:65
>> +    void processSingleCluster(TextAutosizingClusterInfo&, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&);
> 
> I think processCluster vs processSingleCluster is confusing... Like what is the difference; that is not obvious here

Done.

[Anton Vayvod]

Created attachment 188074 [details]
Patch

[John Mellor]

Comment on attachment 188074 [details]
Patch

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

[Julien Chaffraix]

Comment on attachment 188074 [details]
Patch

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

r=me

> Source/WebCore/rendering/TextAutosizer.cpp:160
> +    // text), and use its width instead.

As you are touching this comment, why? (explaining the why here would be a nice addition to the previous comment)

> Source/WebCore/rendering/TextAutosizer.h:64
> +    void processClusterInternal(TextAutosizingClusterInfo&, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&, float textWidth, bool shouldBeAutosized);

Usually boolean arguments should be avoided as they make the code less readable. Here you never really use them directly so it's kinda OK.

[Anton Vayvod]

Comment on attachment 188074 [details]
Patch

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

>> Source/WebCore/rendering/TextAutosizer.cpp:160
>> +    // text), and use its width instead.
> 
> As you are touching this comment, why? (explaining the why here would be a nice addition to the previous comment)

Kenneth asked to rewrap it around 100 characters.

>> Source/WebCore/rendering/TextAutosizer.h:64
>> +    void processClusterInternal(TextAutosizingClusterInfo&, RenderBlock* container, RenderObject* subtreeRoot, const TextAutosizingWindowInfo&, float textWidth, bool shouldBeAutosized);
> 
> Usually boolean arguments should be avoided as they make the code less readable. Here you never really use them directly so it's kinda OK.

Thanks, I'll follow with a patch that fixes this.

[WebKit Review Bot]

Comment on attachment 188074 [details]
Patch

Clearing flags on attachment: 188074

Committed r142866: <http://trac.webkit.org/changeset/142866>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.