[webkit-reviews] review canceled: [Bug 91660] Text Autosizing: Increase line height in proportion to font size. : [Attachment 158298] Patch

Tue Aug 14 10:42:03 PDT 2012

Julien Chaffraix <jchaffraix at webkit.org> has canceled John Mellor
<johnme at chromium.org>'s request for review:
Bug 91660: Text Autosizing: Increase line height in proportion to font size.
https://bugs.webkit.org/show_bug.cgi?id=91660

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

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

> Source/WebCore/ChangeLog:59
> +	   (StyleVisualData):

Please fill those entries, an explanation at the beginning is not a replacement
for those. Especially since you are doing a couple of changes that are not
mentioned in the explanation.

> Source/WebCore/rendering/TextAutosizer.cpp:61
> +	   if (!isNotAnAutosizingContainer(descendant))

I find the double negation to be difficult to read. You can easily remove it:

if (isNotAnAutosizingContainer(descendant))
    continue;

processBlock(toRenderBlock(descendant), windowSize);

> Source/WebCore/rendering/TextAutosizer.cpp:76
> +    for (RenderObject* descendant = traverseNext(block, block,
isNotAnAutosizingContainer); descendant; descendant = traverseNext(descendant,
block, isNotAnAutosizingContainer)) {

traverseNext is pretty confusing but this is orthogonal: the name is bad (super
generic that doesn't really say *which* part of the tree you actually traverse)
but most the filter parameter doesn't add much.

> Source/WebCore/rendering/TextAutosizer.cpp:94
> +    return !renderer->isRenderBlock() || renderer->isListItem() ||
renderer->isInline();

A comment on what type of renderers you actually consider and *why* would be
useful.

> Source/WebCore/rendering/style/RenderStyle.cpp:1173
> +void RenderStyle::setFontSize(float specifiedSize)

I don't understand why you changed the parameter to float here.

> Source/WebCore/rendering/style/RenderStyle.cpp:1184
> +    if (desc.specifiedSize())
> +	   desc.setComputedSize(specifiedSize * desc.computedSize() /
desc.specifiedSize());
> +    else
> +	   desc.setComputedSize(specifiedSize);

This code doesn't look right. You are using specifiedSize *before* setting it.
This should be the 2 previous lines without any extra massaging or you need to
explain why we need that.

> Source/WebCore/rendering/style/RenderStyle.h:601
> +    // FIXME: This should probably return a float, i.e.
fontDescription().computedSize()

As mentioned I don't think this FIXME is right so let's not put it from this
change.

> Source/WebCore/rendering/style/RenderStyle.h:1146
> +    // Only used for blending font sizes when animating, or MathML anonymous
blocks, or text autosizing.

It's more 'and' than 'or':
// Only used for blending font sizes when animating, MathML anonymous blocks
and text autosizing.

> Source/WebCore/rendering/style/StyleVisualData.h:58
> +    float textAutosizingMultiplier;

Let's follow WebKit style here. This should be m_textAutosizingMultiplier.

> LayoutTests/fast/text-autosizing/wide-child.html:1
> +<html>

Please add a doctype here. We don't want to test in quirks mode. It should be
added to all tests.