[Webkit-unassigned] [Bug 205770] new FontFace() should not throw when failing to parse arguments

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=205770

Myles C. Maxfield <mmaxfield at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #390338|review?                     |review-
              Flags|                            |

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

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

> Source/WebCore/ChangeLog:10
> +        No longer throws, rejects promise, sets status to error, and 

"(No longer throws), (rejects promise), (sets status to error)" ..... or "No longer (throws, rejects promise, sets status to error)" ?

> Source/WebCore/css/CSSFontFace.cpp:719
> +    setStatus(Status::Loading);
> +    setStatus(Status::Failure);
> +    m_inErrorState = true;

Is this legal regardless of what state the CSSFontFace happens to be in? It seems wrong to not even consult the current state. Perhaps ASSERT that there is a path from whatever the current state is to ::Failure.

> Source/WebCore/css/CSSFontFace.h:86
> +    const Optional<CSSValueList*> families() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<CSSValueList*>>(m_families.get()); }
> +    Optional<FontSelectionRange> weight() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeWeight()); }
> +    Optional<FontSelectionRange> stretch() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeWidth()); }
> +    Optional<FontSelectionRange> italic() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeSlope()); }
> +    Optional<FontSelectionCapabilities> fontSelectionCapabilities() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontSelectionCapabilities>>(m_fontSelectionCapabilities.computeFontSelectionCapabilities()); }
> +    const Optional<Vector<UnicodeRange>> ranges() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<Vector<UnicodeRange>>>(m_ranges); }
> +    const Optional<FontFeatureSettings> featureSettings() const { return m_inErrorState ? WTF::nullopt : static_cast<Optional<FontFeatureSettings>>(m_featureSettings); }
> +    Optional<FontLoadingBehavior> loadingBehavior() const { return m_inErrorState ? WTF::nullopt :  static_cast<Optional<FontLoadingBehavior>>(m_loadingBehavior); }

I'd appreciate a comment describing what it means for these accessors to return nullopt. (It means that the fields represent the empty string, but the backing store can't actually represent the empty string, so we use optionals here.) If the backing store *can* represent an empty string, there's no need to use an Optional here.

> Source/WebCore/css/CSSFontFace.h:196
> +    bool m_inErrorState { false };

The spec doesn't say anything about a new state. We already have CSSFontFace::Status::Failure. Why do we need a new, super-failure, state? What's the difference between this bool and being in the failure state? Why do these two situations need to be distinct?

I also think this doesn't quite follow the spec. It says "If any of them fail to parse correctly, ... set font face’s corresponding attributes to the empty string." I think "corresponding attributes" means "just the ones that failed to parse." There should be tests added for this (hopefully contributed to WPT).

> Source/WebCore/css/CSSFontFaceSet.cpp:163
> +    if (!familiesWrapped.hasValue())

I would just merge this with the (!face.families()) guard above.

> Source/WebCore/css/CSSFontFaceSet.cpp:244
> +    if (face.families().hasValue())

You removed the check for whether or not the CSSValueList* is null. Please add a test which would have caught this bug.

> Source/WebCore/css/CSSFontFaceSet.cpp:461
> +            if (!fontSelectionCapabilitiesWrapped.hasValue())
> +                continue;

This should just be an ASSERT(). You already made sure that nothing gets added to candidateFontFaces which has a nullopt fontSelectionCapabilities.

> Source/WebCore/css/CSSFontFaceSet.cpp:472
> +            auto firstCapabilitiesWrapped = first.fontSelectionCapabilities();
> +            auto secondCapabilitiesWrapped = second.fontSelectionCapabilities();
> +            if (!firstCapabilitiesWrapped.hasValue())
> +                return false;
> +            if (!secondCapabilitiesWrapped.hasValue())
> +                return false;

Ditto.

> Source/WebCore/css/CSSSegmentedFontFace.cpp:123
> +            if (!selectionCapabilitiesWrapped.hasValue())
> +                continue;

You should be able to ASSERT(selectionCapabilitiesWrapped.hasValue()) here. The code you wrote in CSSFontFaceSet::fontFace() should disallow any error-ed fonts to make it into this collection.

> Source/WebCore/css/FontFace.cpp:271
> +    auto familiesUnrwapped = families.value();

typo: unrwapped

> Source/WebCore/css/FontFace.cpp:390
> +    auto featureSettings = featureSettingsWrapped.value();

auto&

No need to make a copy

Also applicable to everywhere above

> LayoutTests/http/tests/css/font-face-constructor-expected.txt:11
> +
> +
> +
> +
> +
> +
> +

This indicates a bad design. A test should say PASS for each check it makes.

> LayoutTests/http/tests/css/font-face-constructor.html:26
> +        debug(testFontFace.family);
> +        debug(testFontFace.style);
> +        debug(testFontFace.weight);
> +        debug(testFontFace.stretch);
> +        debug(testFontFace.unicodeRange);
> +        debug(testFontFace.featureSettings);
> +        debug(testFontFace.display);
> +        document.fonts.add(testFontFace);
> +        debug("PASS: Did not throw");

Instead of "debug" you should use "shouldBeEqualToString" instead.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200211/27fca6da/attachment-0001.htm>