[webkit-reviews] review granted: [Bug 197808] Move idempotent text autosizing to StyleTreeResolver : [Attachment 370617] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 197808: Move idempotent text autosizing to StyleTreeResolver
https://bugs.webkit.org/show_bug.cgi?id=197808

Attachment 370617: Patch

https://bugs.webkit.org/attachment.cgi?id=370617&action=review




--- Comment #21 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 370617
  --> https://bugs.webkit.org/attachment.cgi?id=370617
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370617&action=review

> Source/WebCore/css/StyleResolver.cpp:878
> +static inline float idempotentTextSize(float specifiedSize, float pageScale)

'static' is redundant

More of this code could be factored into TextSizeAdjustment files

> Source/WebCore/css/StyleResolver.cpp:1170
> +#if ENABLE(TEXT_AUTOSIZING)

Maybe factor this into a function like adjustRenderStyleForTextAutosizing

> Source/WebCore/css/StyleResolver.cpp:1171
> +    auto newAutosizeStatus = style.autosizeStatus().modifiedStatus(style);

This is bit odd factoring, with style appearing twice. It might read better for
example as a static function, AutosizeStatus::updatedStatusForStyle(style)

> Source/WebCore/css/StyleResolver.cpp:1175
> +    if (settings().textAutosizingEnabled() &&
settings().textAutosizingUsesIdempotentMode() && element &&
!newAutosizeStatus.shouldSkipSubtree() && !style.textSizeAdjust().isNone() &&
hasTextChildren(*element) && pageScale != 1.0f) {
> +	   auto fontDescription = style.fontDescription();

Big lists of conditions like here of often read best as bool returning lambdas.

> Source/WebCore/rendering/style/RenderStyle.h:1856
> +	   // This is the state that the text autosizing code uses to determine
whether or not to apply autosizing.
> +	   unsigned textAutosizingFoundOutOfFlowPosition : 1;
> +	   unsigned textAutosizingFoundInlineBlock : 1;
> +	   unsigned textAutosizingFoundFixedHeight : 1;
> +	   unsigned textAutosizingFoundDisplayNone : 1;

You could just store the OptionSet directly (with from/toRaw) to a single field

> Source/WebCore/rendering/style/TextSizeAdjustment.cpp:70
> +    return m_fields.contains(Fields::FoundOutOfFlowPosition)
> +	   || m_fields.contains(Fields::FoundInlineBlock)
> +	   || m_fields.contains(Fields::FoundFixedHeight)
> +	   || m_fields.contains(Fields::FoundDisplayNone);

m_fields.containsAny({...});