[webkit-reviews] review denied: [Bug 109054] [Text Autosizing] Collect narrow descendants and process them separately. Refactoring for a change to follow. : [Attachment 186856] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Anton Vayvod
<avayvod at chromium.org>'s request for review:
Bug 109054: [Text Autosizing] Collect narrow descendants and process them
separately. Refactoring for a change to follow.
https://bugs.webkit.org/show_bug.cgi?id=109054

Attachment 186856: Patch
https://bugs.webkit.org/attachment.cgi?id=186856&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.