[webkit-reviews] review granted: [Bug 121415] The size of the background colour is not properly adjusted when the font size is being changed : [Attachment 216786] Solution enhancement for the font size change and backgroundcolor issue

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Artur Moryc <a.moryc at samsung.com>'s
request for review:
Bug 121415: The size of the background colour is not properly adjusted when the
font size is being changed
https://bugs.webkit.org/show_bug.cgi?id=121415

Attachment 216786: Solution enhancement for the font size change and
backgroundcolor issue
https://bugs.webkit.org/attachment.cgi?id=216786&action=review

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


> Source/WebCore/editing/ApplyStyleCommand.cpp:1467
> +		   newCSSStyle.append(styleChange.cssStyle());

Does something guarantee that cssStyle ends with a semicolon if it’s not empty?
If not, don’t we need to add a semicolon?

What if the style attribute string already has background-color in it? Will we
just keep adding more? Could we add test cases that cover this?

> Source/WebCore/editing/ApplyStyleCommand.cpp:1468
> +		   newCSSStyle.append("background-color:" +
backgroundColorCSSValue->cssText());

This is wrong. If we already have a string builder we should append one string
at a time. This way does unnecessary allocation. Something like this:

    newCSSStyle.appendLiteral(“background-color:”);
    newCSSStyle.append(backgroundColorCSSValue->cssText());

Not sure I have the syntax exactly right.

> Source/WebCore/editing/ApplyStyleCommand.cpp:1469
> +		   setNodeAttribute(toHTMLElement(fontElement.get()),
styleAttr, newCSSStyle.toString());

Code above is just calling fontElement->setAttribute. Why does this code call
setNodeAttribute instead?

> Source/WebCore/editing/EditingStyle.cpp:1346
> +PassRefPtr<CSSValue> EditingStyle::backgroundColorInEffect(Node* node)

It’s ugly to have this return a CSSValue, although that’s not new to this
patch. For internal use in WebKit we should not be using DOM types like this.
The new caller wants only the serialized string form. The old caller wants only
an RGBA value, which does not require allocation of a CSSValue object at all.
The new caller wants the serialized color, and it also seems wasteful to
allocate a CSSValue for this.

> Source/WebCore/editing/EditingStyle.cpp:1350
> +	   if (RefPtr<CSSValue> value =
ComputedStyleExtractor(ancestor).propertyValue(CSSPropertyBackgroundColor)) {
> +	       if (!isTransparentColorValue(value.get()))

This is an inefficient approach. I don’t think we want to involve computed
style until we reach the item that has a background color. We should answer the
“does this have a non-transparent background color” using the lower level,
RenderStyle. It’s wasteful to construct a CSSValue for internal use.

> Source/WebCore/editing/EditingStyle.cpp:1354
> +    return 0;

nullptr