Bug 109054

Summary: [Text Autosizing] Collect narrow descendants and process them separately. Refactoring for a change to follow.
Product: WebKit Reporter: Anton Vayvod <avayvod>
Component: Layout and RenderingAssignee: Anton Vayvod <avayvod>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jchaffraix, johnme, kenneth, ojan.autocc, oliver, peter, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 107300, 109573    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Anton Vayvod 2013-02-06 07:37:45 PST
[Text Autosizing] Collect narrow descendants and process them separately. Refactoring for a change to follow.
Comment 1 Anton Vayvod 2013-02-06 07:49:06 PST
Created attachment 186856 [details]
Patch
Comment 2 Build Bot 2013-02-06 09:28:16 PST
Comment on attachment 186856 [details]
Patch

Attachment 186856 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16400159
Comment 3 Julien Chaffraix 2013-02-06 11:46:07 PST
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.
Comment 4 Anton Vayvod 2013-02-11 03:30:26 PST
Created attachment 187539 [details]
Patch
Comment 5 John Mellor 2013-02-11 09:41:29 PST
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?
Comment 6 John Mellor 2013-02-11 09:46:19 PST
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.".
Comment 7 Anton Vayvod 2013-02-11 12:10:35 PST
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.
Comment 8 Anton Vayvod 2013-02-11 12:10:52 PST
Created attachment 187635 [details]
Patch
Comment 9 John Mellor 2013-02-11 12:27:30 PST
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/
Comment 10 Anton Vayvod 2013-02-11 12:29:10 PST
Created attachment 187638 [details]
Patch
Comment 11 John Mellor 2013-02-11 12:30:55 PST
Comment on attachment 187638 [details]
Patch

Looks good to me. Julien/Kenneth, what do you think?
Comment 12 Julien Chaffraix 2013-02-11 13:29:23 PST
Comment on attachment 187638 [details]
Patch

r=me
Comment 13 WebKit Review Bot 2013-02-11 15:50:29 PST
Comment on attachment 187638 [details]
Patch

Clearing flags on attachment: 187638

Committed r142534: <http://trac.webkit.org/changeset/142534>
Comment 14 WebKit Review Bot 2013-02-11 15:50:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Oliver Hunt 2013-02-25 11:41:25 PST
(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).
Comment 16 Peter Beverloo 2013-02-26 09:33:39 PST
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?