[webkit-reviews] review granted: [Bug 103627] Text Autosizing: containers wider than their enclosing clusters should be autosized as separate clusters : [Attachment 179797] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix (vacations 20 dec -> 5 jan) <jchaffraix at webkit.org> has
granted Anton Vayvod <avayvod at chromium.org>'s request for review:
Bug 103627: Text Autosizing: containers wider than their enclosing clusters
should be autosized as separate clusters
https://bugs.webkit.org/show_bug.cgi?id=103627

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

------- Additional Comments from Julien Chaffraix (vacations 20 dec -> 5 jan)
<jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179797&action=review


r=me but I would like to see the updated patch before it lands.

> Source/WebCore/rendering/TextAutosizer.cpp:-97
> -    ASSERT(isAutosizingCluster(cluster));

Please add a comment in the ChangeLog about *why* we are removing these
ASSERTs.

> Source/WebCore/rendering/TextAutosizer.cpp:240
> +bool TextAutosizer::isAutosizingClusterIgnoringParentCluster(const
RenderObject* object)

This new naming is pretty confusing I have to say as you don't really check
your parent anywhere (it's implied as you define any auto-sizing cluster as
independent from its parent cluster). Here is some alternative names that
convey the idea better:
* isIndependentAutosizingCluster
* rendererIsAutosizingCluster

> Source/WebCore/rendering/TextAutosizer.cpp:265
> +

Extra line.

> Source/WebCore/rendering/TextAutosizer.h:81
> -    // Depending on the traversal direction specified, finds the first or
the last leaf text node child that doesn't
> -    // belong to any cluster.
> -    static const RenderObject* findFirstTextLeafNotInCluster(const
RenderObject*, size_t& depth, TraversalDirection);
> +    // Returns the first text leaf in the given direction and its depth
starting from the given parent.
> +    static const RenderObject* findFirstTextLeafNotInCluster(const
RenderObject* parent, size_t& depth, TraversalDirection);

Same comment as previously, this change doesn't add much as you don't touch
this function. The extra parameter name is not super helpful either so I would
leave this code alone.