Bug 109054 – [Text Autosizing] Collect narrow descendants and process them separately. Refactoring for a change to follow.

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

[Anton Vayvod]

[Text Autosizing] Collect narrow descendants and process them separately. Refactoring for a change to follow.

[Anton Vayvod]

Created attachment 186856 [details]
Patch

[Build Bot]

Comment on attachment 186856 [details]
Patch

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

[Julien Chaffraix]

Comment on attachment 186856 [details]
Patch

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

> Source/WebCore/rendering/TextAutosizer.cpp:256
> +    return isNarrowDescendant(renderer, parentClusterInfo) || isCertainlyAutosizingCluster(renderer, parentClusterInfo);

I don't like the naming at all. isAutosizingCluster vs isCertainlyAutosizingCluster is a confusing distinction.

I would prefer if you started returning an enum with the reason for being an autosizing cluster: NarrawDescendantReason, RegularAutosizingCluster (don't like 2nd name though).

> Source/WebCore/rendering/TextAutosizer.cpp:261
> +    TextAutosizingClusterInfo emptyClusterInfo(0);

It's probably better to just pass NULL than an emptyClusterInfo. That would avoid having 2 overloaded versions of isAutosizingCluster.

[Anton Vayvod]

Created attachment 187539 [details]
Patch

[John Mellor]

Comment on attachment 187539 [details]
Patch

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

Looks good to me (with minor nits). Kenneth/Julien, what do you think?

> Source/WebCore/rendering/TextAutosizer.cpp:67
> +    // processed as a separate cluster or clusters.

Is there a typo in "cluster or clusters"? That bit doesn't seem too clear.

> Source/WebCore/rendering/TextAutosizer.cpp:68
> +    Vector<TextAutosizingClusterInfo> narrowDescendants;

Can you add a comment above the declaration of struct TextAutosizingClusterInfo, to clarify that clusterInfo objects are temporary and do not persist between calls to processSubtree?

> Source/WebCore/rendering/TextAutosizer.cpp:159
> +    Vector<TextAutosizingClusterInfo>& narrowDescendants = clusterInfo.narrowDescendants;

Nit: you could probably just use this directly?

[John Mellor]

Comment on attachment 187539 [details]
Patch

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

Additional nit.

> Source/WebCore/ChangeLog:9
> +        the parent autosizing cluster. The groups will be autosized with the same multiplier.

You might want to expand on this a little, to explain what problem you're trying to solve, e.g. "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.".

[Anton Vayvod]

Comment on attachment 187539 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        the parent autosizing cluster. The groups will be autosized with the same multiplier.
> 
> You might want to expand on this a little, to explain what problem you're trying to solve, e.g. "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.".

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:67
>> +    // processed as a separate cluster or clusters.
> 
> Is there a typo in "cluster or clusters"? That bit doesn't seem too clear.

The idea was that all narrow descendants can become either a group of clusters or a single one.

>> Source/WebCore/rendering/TextAutosizer.cpp:68
>> +    Vector<TextAutosizingClusterInfo> narrowDescendants;
> 
> Can you add a comment above the declaration of struct TextAutosizingClusterInfo, to clarify that clusterInfo objects are temporary and do not persist between calls to processSubtree?

Done.

>> Source/WebCore/rendering/TextAutosizer.cpp:159
>> +    Vector<TextAutosizingClusterInfo>& narrowDescendants = clusterInfo.narrowDescendants;
> 
> Nit: you could probably just use this directly?

Well, it's used twice below.

[Anton Vayvod]

Created attachment 187635 [details]
Patch

[John Mellor]

Comment on attachment 187635 [details]
Patch

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

> Source/WebCore/rendering/TextAutosizer.cpp:51
> +// Represents cluster related data. Objects should not persist between the calls to processSubtree.

Nit: s/Objects/Instances/ and s/the calls/calls/

[Anton Vayvod]

Created attachment 187638 [details]
Patch

[John Mellor]

Comment on attachment 187638 [details]
Patch

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

[Julien Chaffraix]

Comment on attachment 187638 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 187638 [details]
Patch

Clearing flags on attachment: 187638

Committed r142534: <http://trac.webkit.org/changeset/142534>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

(In reply to comment #14)
> All reviewed patches have been landed.  Closing bug.

I think this may be responsible for for crashers in WebCore::TreeScopeAdopter::updateTreeScope, based on  it being the first build to have these crashes. (See the build bot).

[Peter Beverloo]

Oliver, on which bot are you seeing issues?  This code is isolated to Text Autosizing, which, to my knowledge, is only enabled for certain Chromium bots.

The mention of TreeScopeAdopter makes me think it may be related to some of the ongoing Web Component/Shadow DOM refactorings instead.  Were there nearby commits, perhaps?