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

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


This is getting close. I think another round and it will be OK to land.

> Source/WebCore/rendering/TextAutosizer.cpp:90
> +    renderer->setStyle(newStyle.release());
> +    renderer->setNeedsLayoutAndPrefWidthsRecalc();

You shouldn't need to call setNeedsLayoutAndPrefWidthsRecalc as setStyle should
do the proper invalidation (due to the check in RenderStyle::diff)

> Source/WebCore/rendering/style/RenderStyle.cpp:1198
> +    desc.setComputedSize(specifiedSize * textAutosizingMultiplier()); //
FIXME: This is overly simplistic.

That's not really an actionable FIXME, what really needs to be fixed?

> Source/WebCore/rendering/style/RenderStyle.h:210
> +    // FIXME: Where should this go? Doesn't seem worth creating a
StyleTextAutosizingData
> +    // for just one float, but putting it in e.g. StyleRareNonInheritedData
seems costly
> +    // as it's not rare, so most RenderText and their parents would need an
instance of it.
> +    // Perhaps I should create a StyleNonInheritedData, and move m_box,
visual,
> +    // m_background and surround to be members of that (like
StyleRareNonInheritedData has
> +    // a bunch of DataRef members)?
> +    float m_textAutosizingMultiplier;

That's a tricky question. You don't want to increase the size of RenderStyle as
you do now so that rules out a new DataRef pointer (which would increase
RenderStyle more than just adding the member) or this approach.

I would add it to StyleVisualData for those reason: it's a non-inherited visual
information akin to zoom, the structure is small and it's also supposed to be
not-rare.

> Source/WebCore/rendering/style/RenderStyle.h:632
> +    Length specifiedLineHeight() const { return inherited->line_height; }

Exposing this function would remove some #if guard in the code so I would
rather expose it and remove the #if-def casing.

> Source/WebCore/rendering/style/RenderStyle.h:639
> +	   if (textAutosizingMultiplier() > 1 && lh.isFixed())
> +	       return Length(lh.value() * textAutosizingMultiplier(), Fixed);

You never actually scale any percent or calc values, is that really fine?

By the way, you could wrap only those 2 lines with the #if guard to limit the
difference.

> Source/WebCore/rendering/style/RenderStyle.h:1155
> +    void setFontSize(float specifiedSize);

Bug 18091 introduced this function with the intent that it should be used only
during animating. By removing the 'blend' part of it, you are basically
allowing anyone to override the font-size. I think it's fine to do this
renaming but I would like a comment left in its place to underline that it
shouldn't be done lightly.