[webkit-reviews] review denied: [Bug 205770] new FontFace() should not throw when failing to parse arguments : [Attachment 390338] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has denied Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 205770: new FontFace() should not throw when failing to parse arguments
https://bugs.webkit.org/show_bug.cgi?id=205770

Attachment 390338: Patch

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




--- 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.computeWe
ight()); }
> +    Optional<FontSelectionRange> stretch() const { return m_inErrorState ?
WTF::nullopt :
static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeWi
dth()); }
> +    Optional<FontSelectionRange> italic() const { return m_inErrorState ?
WTF::nullopt :
static_cast<Optional<FontSelectionRange>>(m_fontSelectionCapabilities.computeSl
ope()); }
> +    Optional<FontSelectionCapabilities> fontSelectionCapabilities() const {
return m_inErrorState ? WTF::nullopt :
static_cast<Optional<FontSelectionCapabilities>>(m_fontSelectionCapabilities.co
mputeFontSelectionCapabilities()); }
> +    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.