[webkit-reviews] review granted: [Bug 187722] [Web Animations] Interpolation between font-styles with a keyword value should be discrete : [Attachment 345205] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 17 17:27:36 PDT 2018


Myles C. Maxfield <mmaxfield at apple.com> has granted Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 187722: [Web Animations] Interpolation between font-styles with a keyword
value should be discrete
https://bugs.webkit.org/show_bug.cgi?id=187722

Attachment 345205: Patch

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




--- Comment #15 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 345205
  --> https://bugs.webkit.org/attachment.cgi?id=345205
Patch

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

> Source/WebCore/ChangeLog:3
> +	   [Web Animations] Interpolation between font-sizes with a keyword
value should be discrete

font-style

> Source/WebCore/ChangeLog:8
> +	   Animating between "font-size: normal" and another value should yield
a discrete interpolation where the from-value

"from-value" is "normal" here.

> Source/WebCore/ChangeLog:12
> +	   from an "oblique" value, we implement a custom PropertyWrapper for
the "font-size" property where we ensure the

font-style

> Source/WebCore/ChangeLog:15
> +	   base value for "normal".

What about blending from "italic" to "oblique"?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3601
> +	   && !isItalic(style.fontDescription().italic())

I don't think this is the same. Before it was comparing to 0, but now you're
comparing to 20.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2043
> +    return fontNonKeywordStyleFromStyleValue(italic.value());

Seems kind of brittle, relying on fontStyleKeyword() to catch the case where
italic is nullopt.

> Source/WebCore/css/CSSFontFaceSet.cpp:317
> +    auto styleSelectionValue =
StyleBuilderConverter::convertFontStyleFromValue(*styleValue);

What about lines 315-316?

> Source/WebCore/css/FontSelectionValueInlines.h:126
> +    if (!style || style.value() == normalItalicValue())

Now that we can distinguish between "font-style: oblique 0deg" and "font-style:
normal", I think this is not correct. I think it should be "if (!style)". We
should add a layout test to make sure getComputedStyle(normal) is not the same
as getComputedStyle(oblique 0deg).

> Source/WebCore/css/FontSelectionValueInlines.h:133
>  inline std::optional<FontSelectionValue> fontStyleValue(CSSValueID value)

I think this function is never called. It should be deleted.

> Source/WebCore/css/StyleBuilderConverter.h:1205
> +inline std::optional<FontSelectionValue>
StyleBuilderConverter::convertFontStyleFromValue(const CSSValue& value)

This is a little confusing because a naive programmer would think "optional"
means that the CSSValue is some unexpected Value. We should add a comment or
something to say that the input value needs to parsed and valid, and if the
function returns nullopt that means the input said "normal".

> Source/WebCore/css/StyleBuilderConverter.h:1211
>      if (valueID == CSSValueNormal)

Please add a FIXME saying that an optional is not the best data structure for
this. Instead it should be a tri-state (eventually).

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:418
> +static inline std::optional<FontSelectionValue> blendFunc(const
CSSPropertyBlendingClient* anim, std::optional<FontSelectionValue> from,
std::optional<FontSelectionValue> to, double progress)

This is kind of unfortunate because there's nothing saying this is specific to
font-style (and therefore nullopt has this special meaning).

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:420
> +    if (!from || !to)

What about blending from italic to oblique?

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1465
> +	   if (!fromFontItalic || !toFontItalic)

What about blending from italic to oblique?

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1466
> +	       blendedStyleAxis = (progress < 0.5 ? from :
to)->fontDescription().fontStyleAxis();

Why the inequality? The axis gets ignored (inside blendFunc()) if progress <
0.5, so you might as well set it correctly no matter what progress is.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:1470
> +	   description.setItalic(blendFunc(anim, fromFontItalic, toFontItalic,
progress));

Is the blendFunc() actually needed? Can't you just do it here? That would make
sure the blending is associated with font-style and not any
optional<FontSelectionValue>.

> Source/WebCore/platform/graphics/FontCache.h:113
> +	  
hasher.add(m_fontSelectionRequest.slope.value_or(normalItalicValue()));

I think we should distinguish between normal and oblique 0deg. It's just good
hygiene to make me less likely to create a bug when I modify this code in the
future.

> Source/WebCore/platform/graphics/FontCascade.h:155
> +    std::optional<FontSelectionValue> italic() const { return
m_fontDescription.italic(); }

Again, a comment might be helpful to describe what nullopt means.

> Source/WebCore/platform/graphics/FontDescription.cpp:-58
> -    : m_fontSelectionRequest { FontCascadeDescription::initialWeight(),
FontCascadeDescription::initialStretch(),
FontCascadeDescription::initialItalic() }

Shouldn't we make initialItalic() return nullopt?

> Source/WebCore/platform/graphics/FontDescription.cpp:98
> +    if (isItalic)
> +	   setItalic(italicValue());
> +    else
> +	   setItalic(std::nullopt);

Is this really better than the ternary?

> Source/WebCore/platform/graphics/FontDescription.h:-301
> -    static FontSelectionValue initialItalic() { return normalItalicValue();
}

I would just change the type of this.

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.cpp:69
> +    auto requestSlope = m_request.slope.value_or(normalItalicValue());

Cool.

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:346
> +inline TextStream& operator<<(TextStream& ts, const FontSelectionValue&
fontSelectionValue)
> +{
> +    ts <<
TextStream::FormatNumberRespectingIntegers(fontSelectionValue.rawValue());
> +    return ts;
> +}

Why wasn't this necessary before?

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:354
> +inline bool operator==(const FontSelectionRequest& a, const
FontSelectionRequest& b)

Does the build fail if you change this back?

> Source/WebCore/platform/graphics/FontSelectionAlgorithm.h:359
> +inline bool operator!=(const FontSelectionRequest& a, const
FontSelectionRequest& b)

Does the build fail if you change this back?

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:-626
> -    auto hfont = createGDIFont(family, weight, fontDescription.italic(),

Oh, jeez, this was silently comparing to 0 because of the implicit operator
float() inside FontSelectionValue? Scary.

Thanks for fixing!

> Source/WebKitLegacy/win/DOMCoreClasses.cpp:1298
> +    webFontDescription->italic = fontDescription.italic() &&
fontDescription.italic().value();

Pretty sure you want isItalic()

>
LayoutTests/imported/w3c/web-platform-tests/web-animations/animation-model/anim
ation-types/discrete-expected.txt:6
> -FAIL Test animating discrete values assert_equals: Animation produces 'from'
value just before the middle of the interval expected "normal" but got "oblique
9.75deg"
> +PASS Test animating discrete values 
>  PASS Test discrete animation is used when interpolation fails 
>  PASS Test discrete animation is used only for pairs of values that cannot be
interpolated 
> -FAIL Test the 50% switch point for discrete animation is based on the effect
easing assert_equals: Animation produces 'from' value at 94% of the iteration
time expected "normal" but got "oblique 8.5deg"
> -FAIL Test the 50% switch point for discrete animation is based on the
keyframe easing assert_equals: Animation produces 'from' value at 94% of the
iteration time expected "normal" but got "oblique 8.5deg"
> +PASS Test the 50% switch point for discrete animation is based on the effect
easing 
> +PASS Test the 50% switch point for discrete animation is based on the
keyframe easing 

YYYYAAAAYYYYYYYAYAYAYAYAY


More information about the webkit-reviews mailing list