[webkit-reviews] review granted: [Bug 199975] WebKit should strip away system font names from the pasted content : [Attachment 374646] Updated per review comments

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 199975: WebKit should strip away system font names from the pasted content
https://bugs.webkit.org/show_bug.cgi?id=199975

Attachment 374646: Updated per review comments

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




--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 374646
  --> https://bugs.webkit.org/attachment.cgi?id=374646
Updated per review comments

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

> Source/WebCore/editing/EditingStyle.cpp:1288
> +    auto* firstItem = downcast<CSSValueList>(value).item(0);

Since we’ve already checked that the length is 1, I don’t think we need to call
this "firstItem" because we know it’s the *only* item.

> Source/WebCore/editing/EditingStyle.cpp:1290
> +    if (!firstItem)
> +	   return false;

This can’t happen. We don’t allow null pointers in CSS value lists; the
function only returns null if we’re off the end of the list, which can’t happen
here. Could even use itemWithoutBoundsCheck here if you like since we already
checked the length. In fact, I suggest using a reference for the local
variable, rather than a pointer, to avoid bogus null checks below.

> Source/WebCore/editing/EditingStyle.cpp:1292
> +    String stringValue;

This is an unused local variable.

> Source/WebCore/editing/EditingStyle.cpp:1294
> +    if (!is<CSSPrimitiveValue>(firstItem))
> +	   return false;

Written this way it includes a null check, so is redundant with the null check
above.

> Source/WebCore/editing/EditingStyle.cpp:1299
> +    auto result = downcast<CSSPrimitiveValue>(firstItem)->getStringValue();
> +    if (result.hasException())
> +	   return false;
> +    auto fontFamily = result.releaseReturnValue();

The getStringValue function is designed for use in JavaScript. The plain old
stringValue function is easier to use in C++ code. Since it returns the null
string when it's not a string you don’t even have to check, can just use the
string.

    auto fontFamilyName =
downcast<CSSPrimitiveValue>(*firstItem).stringValue();

But I think we know that this is a font family, so we don’t need to use the
polymorphic stringValue(), instead we could just use this:

    auto fontFamilyName = downcast<CSSPrimitiveValue>(*firstItem).
fontFamily().familyName();

Code would be slightly bigger and slightly faster. Not sure which is better.