Bug 109054 - [Text Autosizing] Collect narrow descendants and process them separately. Refactoring for a change to follow.
Summary: [Text Autosizing] Collect narrow descendants and process them separately. Ref...
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:
Blocks: 107300 109573
  Show dependency treegraph
 
Reported: 2013-02-06 07:37 PST by Anton Vayvod
Modified: 2013-02-26 09:33 PST (History)
8 users (show)

See Also:


Attachments
Patch (9.99 KB, patch)
2013-02-06 07:49 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (4.17 KB, patch)
2013-02-11 03:30 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (4.89 KB, patch)
2013-02-11 12:10 PST, Anton Vayvod
no flags Details | Formatted Diff | Diff
Patch (4.89 KB, patch)
2013-02-11 12:29 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-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?