[webkit-reviews] review denied: [Bug 103627] Text Autosizing: containers wider than their enclosing clusters should be autosized as separate clusters : [Attachment 179297] 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 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 179297: Patch
https://bugs.webkit.org/attachment.cgi?id=179297&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179297&action=review


Overall comment: this patch is 37k long, webkit-patch should have warned you
that it's too big for any reviewer to safely review. I barely scratched the
surface on this review so if you want your patch to be properly reviewed, split
it up. If not, I can guarantee that it will painful for both of us.

> Source/WebCore/ChangeLog:16
> +

What is missing here would be some context: what are you trying to achieve and
why?

> Source/WebCore/ChangeLog:20
> +	   (WebCore::TextAutosizer::processSubtree): calls processCluster with
new number of arguments.

Nit: Sentences starts with a capitalized letter. Also going to the next line
give me space to avoid wrapping on the right (the reviewing tool had to wrap
some of your massive lines).

> Source/WebCore/ChangeLog:21
> +	   (WebCore::TextAutosizer::processCluster):
parentBlockContainingAllText parameter (for assertion only), finds new block
containing all text nodes and passes it into processContainer.

I don't think it's a good argument to pass in an extra argument just for the
sake of ASSERTing.

> Source/WebCore/ChangeLog:34
> +	   (TextAutosizer): removed unused declarations, added a typedef for a
vector of RenderObjects.

We usually remove the (TextAutosizer): part as it has no value (script snafu).

> Source/WebCore/rendering/TextAutosizer.cpp:340
> +const RenderBlock* TextAutosizer::findDeepestBlockContainingAllText(const
RenderBlock* cluster, const RenderBlock* parentBlockContainingAllText)
> +{
> +    ASSERT_UNUSED(parentBlockContainingAllText,
isContainerAutosizingCluster(cluster, parentBlockContainingAllText));

There is only one call site for this function and the only reason you pass
|parentBlockContainingAllText| is for the ASSERT so the parameter should be
removed and if you follow the caller, the ASSERT is already there.

> Source/WebCore/rendering/TextAutosizer.cpp:400
> +	       RenderObjects nextPath;
> +	       while (next != stayWithin) {
> +		   nextPath.append(next);
> +		   next = next->parent();
> +	       }
> +	       nextPath.append(stayWithin);
> +	       nextPath.reverse();
>  
> -    return containingBlock;
> +	       path.swap(nextPath);
> +	       return;

I don't see the need for an extra variable here as you are swapping the Vector
anyway.