[webkit-reviews] review granted: [Bug 53196] REGRESSION(71556, 68059): queryCommandValue screws up background color at collapsed cursor : [Attachment 81613] fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 53196: REGRESSION(71556,68059): queryCommandValue screws up background
color at collapsed cursor
https://bugs.webkit.org/show_bug.cgi?id=53196

Attachment 81613: fixes the bug
https://bugs.webkit.org/attachment.cgi?id=81613&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81613&action=review

> Source/WebCore/editing/ApplyStyleCommand.cpp:208
> +int legacyFontSizeFromCSSValue(Document* document, CSSPrimitiveValue*
cssValue, bool shouldUseFixedFontDefaultSize, LegacyFontSizeMode mode)

I would just call that argument “value”

> Source/WebCore/editing/ApplyStyleCommand.cpp:210
> +    if (cssValue->primitiveType() >= CSSPrimitiveValue::CSS_PX &&
cssValue->primitiveType() <= CSSPrimitiveValue::CSS_PC) {

Named function for this check taking either CSSPrimitiveValue or the primitive
type would be a good way to document what it is and why it makes sense to do
it.

> Source/WebCore/editing/ApplyStyleCommand.cpp:219
> +	       return legacyFontSize;
> +    } else if (CSSValueXSmall <= cssValue->getIdent() &&
cssValue->getIdent() <= CSSValueWebkitXxxLarge)
> +	   return cssValue->getIdent() - CSSValueXSmall + 1;
> +    return 0;

else here is pretty awkward. I think a return 0 inside that first if would be
clearer.

> Source/WebCore/editing/ApplyStyleCommand.cpp:279
> +	       m_applyFontSize = String::number(legacyFontSize);

m_applyFontSize is really such a terrible name. It’s a verb phrase!

> Source/WebCore/editing/Editor.cpp:1052
> +	   if (cssValue->isPrimitiveValue())
> +	       value =
String::number(legacyFontSizeFromCSSValue(m_frame->document(),
static_cast<CSSPrimitiveValue*>(cssValue.get()),
> +		   shouldUseFixedFontDefaultSize, AlwaysUseLegacyFontSize));

Need braces here.

> Source/WebCore/editing/Editor.cpp:3142
> -    RefPtr<EditingStyle> typingStyle = m_frame->selection()->typingStyle();
> -    typingStyle->removeNonEditingProperties();
> +    RefPtr<EditingStyle> typingStyle =
m_frame->selection()->typingStyle()->copy();

Change log does not make it clear why it’s better to omit the call to
removeNonEditingProperties. It states that it was wrong, but not why it was
bad.

Need a “why” in the change log.