[webkit-reviews] review canceled: [Bug 91660] Text Autosizing: Increase line height in proportion to font size. : [Attachment 158830] 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 91660: Text Autosizing: Increase line height in proportion to font size.
https://bugs.webkit.org/show_bug.cgi?id=91660

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

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


> I find the changelog pretty good now.

I totally agree.

> Source/WebCore/rendering/RenderObject.cpp:371
> +RenderObject* RenderObject::nextInPreOrderMatchingFilter(const RenderObject*
stayWithin, RenderObjectFilterFunctor filter) const

The name is a bit of a misnomer: I would expect this function to be a full
preorder traversal, which it's not as it skips subtrees that don't match your
filter.

I thought of nextInPreOrderIgnoringNonMatchingSubtrees for a replacement but
it's long and again doesn't convey the idea well so we may as well stick with
your naming as it's better.

It's a pretty narrow use case which I am not convinced is worth exposing in
RenderObject. Let's keep it in TextAutosizer for now as this will force a world
rebuild for little value.

> Source/WebCore/rendering/style/RenderStyle.cpp:-1190
> -int RenderStyle::wordSpacing() const { return inherited->font.wordSpacing();
}
> -int RenderStyle::letterSpacing() const { return
inherited->font.letterSpacing(); }

Apparently unintended change (not explained anywhere).

> Source/WebCore/rendering/style/RenderStyle.cpp:1215
> +	   // Fall through, since Text Autosizing might not be runtime enabled.


I would rather have explicit curly braces around the 'else' body below to avoid
relying on the implicit rule that the 'else' should be 1 line long.

> Source/WebCore/rendering/style/RenderStyle.cpp:1221
> +    // Preserve existing ratio between specifiedSize and computedSize.
> +    if (desc.specifiedSize())
> +	   desc.setComputedSize(specifiedSize * desc.computedSize() /
desc.specifiedSize());
> +    else
> +	   desc.setComputedSize(specifiedSize);

> It's debatable whether this is really necessary, but doing everything in
specifiedSize certainly makes it easier for setBlendedFontSize to be compatible
with both text zoom and Text Autosizing.

If it's not strictly necessary, I would prefer for it to be removed from this
patch. This code is used by other code paths which makes the risk of
regressions real but also your path is big enough without this change.

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

> My understanding is that in the rare cases where public data members are
used, they should be unprefixed:
> https://lists.webkit.org/pipermail/webkit-dev/2010-June/013037.html

That's not my reading. Here you are in a class, not a struct so the discussion
doesn't apply. Looking at the code, there is a mix between the 2 styles but the
new code mostly uses m_ prefixing even if the variable is public so we should
use that for consistency.

> LayoutTests/fast/text-autosizing/em-margin-border-padding.html:15
> +    console.warn("This test depends on the Text Autosizing setting being
true, so run it in DumpRenderTree, or manually enable Text Autosizing, and
either use a mobile device with 320px device-width (like Nexus S or iPhone), or
define HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP.");

HACK_FORCE_TEXT_AUTOSIZING_ON_DESKTOP doesn't work in WebKit AFAICT so it would
be better if we didn't mention that (especially since there is HACK in it)