Bug 109054

Created attachment 186856 [details] Patch

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

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.

Created attachment 187539 [details] Patch

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 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 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.

Created attachment 187635 [details] Patch

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/

Created attachment 187638 [details] Patch

Comment on attachment 187638 [details] Patch Looks good to me. Julien/Kenneth, what do you think?

Comment on attachment 187638 [details] Patch r=me

Comment on attachment 187638 [details] Patch Clearing flags on attachment: 187638 Committed r142534: <http://trac.webkit.org/changeset/142534>

All reviewed patches have been landed. Closing bug.

(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).

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?