[webkit-reviews] review canceled: [Bug 97025] Text Autosizing: Cluster text at flow roots, for consistency and to avoid autosizing headers/footers. : [Attachment 164586] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled John Mellor
<johnme at chromium.org>'s request for review:
Bug 97025: Text Autosizing: Cluster text at flow roots, for consistency and to
avoid autosizing headers/footers.
https://bugs.webkit.org/show_bug.cgi?id=97025

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

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


I haven't looked at the tests but we definitely want another round for the code
side of it.

> Source/WebCore/rendering/TextAutosizer.cpp:76
> +    if (!container) // This should be rare.
> +	   return false;

Are you sure it's not impossible? RenderView should always pass the isContainer
check and you always have a containing RenderView.

> Source/WebCore/rendering/TextAutosizer.cpp:82
> +    if (!cluster) // This should be rare.
> +	   return false;

Ditto.

> Source/WebCore/rendering/TextAutosizer.cpp:90
> +    // FIXME: Find lowest common ancestor of first and last descendant text
node (or return).

I don't understand this comment or what should be done here instead.

> Source/WebCore/rendering/TextAutosizer.cpp:184
> -bool TextAutosizer::isNotAnAutosizingContainer(const RenderObject* renderer)

> +bool TextAutosizer::isContainer(const RenderObject* renderer)

Please let's keep the Autosizing (actually AutoSizing) part in the name.
isContainer() is ambiguous.

> Source/WebCore/rendering/TextAutosizer.cpp:197
> +    // FIXME: Explain basis in flow roots.

How about now? Probably no one knows what 'basis' you are talking about.

> Source/WebCore/rendering/TextAutosizer.cpp:201
> +    if (!isContainer(renderer))
> +	   return false;

Most of the code path have done this check before, it may be worth either
ASSERT'ing or not doing them on the caller's side?

>> Source/WebCore/rendering/TextAutosizer.cpp:202
>> +	return !renderer->containingBlock()
> 
> I would add a new line after first return

Shouldn't !renderer->containingBlock() equates renderer->isRenderView() in your
case as the tree is assumed to be rooted?

> Source/WebCore/rendering/TextAutosizer.cpp:207
> +	   || (renderer->containingBlock() &&
renderer->containingBlock()->isHorizontalWritingMode() !=
renderer->isHorizontalWritingMode());

The renderer->containingBlock() check is unneeded as you do it above anyway.

> Source/WebCore/rendering/TextAutosizer.cpp:219
> +    // Don't autosize clusters that contain less than 4 lines of text (in
> +    // practice less lines are required, since measureDescendantTextWidth
assumes
> +    // that characters are 1em wide, but most characters are narrower than
that).

This comment is not consistent if most characters are *narrower*, you would
need *more* than 4 lines of text, not less.

> Source/WebCore/rendering/TextAutosizer.cpp:229
> +void TextAutosizer::measureDescendantTextWidth(const RenderBlock* container,
float minTextWidth, float* textWidth)

Let's use a reference, not a pointer as you don't want the parameter to be
optional (you never NULL-checked it).