[webkit-reviews] review denied: [Bug 121415] The size of the background colour is not properly adjusted when the font size is being changed : [Attachment 234410] Solution update for the backgroundcolor while the fontsize varies

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 234410: Solution update for the backgroundcolor while the fontsize
varies
https://bugs.webkit.org/attachment.cgi?id=234410&action=review

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


Quite a few mistakes here, and not sure if the basic approach is right. We need
test cases demonstrating why it’s OK for this to work only on span and font
elements. Why not work on <div> and <p> elements too? What about <b> elements?

> Source/WebCore/editing/ApplyStyleCommand.cpp:556
> +    if (!style)
> +	   return;

Seems strange to handle null here. I don’t believe the functions below handle
null. I suggest that this function take EditingStyle& instead of EditingStyle&.


> Source/WebCore/editing/ApplyStyleCommand.cpp:564
> +    if (node->isTextNode() && node->parentNode())
> +	   node = node->parentNode();
> +    if (node->hasTagName(spanTag) || node->hasTagName(fontTag)) {

The behavior here is bizarre. This function works only on span elements, font
elements, and text elements that are just inside one of those two. How could
that possibly be correct? The text node part seems particularly peculiar. If
the function doesn’t work on all nodes, why would it specifically do the parent
thing for text nodes?

> Source/WebCore/editing/ApplyStyleCommand.cpp:565
> +	   String backgroundColorString = mutableStyle ?
mutableStyle->getPropertyValue(CSSPropertyBackgroundColor) : emptyString();

No need to re-check mutableStyle for null here since you are already doing it
above.

> Source/WebCore/editing/ApplyStyleCommand.cpp:568
> +	       if (backgroundColor.isValid() && backgroundColor.alpha()) {

Don’t need to also check isValid if we are checking alpha for zero since all
invalid colors have RGBA of 0.

> Source/WebCore/editing/ApplyStyleCommand.cpp:572
> +		   if (backgroundColorString.length() && htmlElement)

It doesn’t make sense to check htmlElement for null here. It will only be null
if node was null, and we have already dereferenced null.

Should be using isEmpty here rather than checking length.

> Source/WebCore/editing/ApplyStyleCommand.cpp:576
> +		   }
> +	       }
> +	   }

These braces are indented incorrectly.

> Source/WebCore/editing/EditingStyle.cpp:392
> +    if (!colorValue.isValid() || !colorValue.alpha())
> +	   return Color::transparent;

This code is repeated twice, here and in colorValueToRGBA. We don’t need it
twice.

If we’re checking for alpha of zero, we don’t also have to check isValid;
invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.cpp:439
> +	   if (value.isValid() && value.alpha())

If we’re checking for alpha of zero, we don’t also have to check isValid;
invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.cpp:1291
> +	   if (value.isValid() && value.alpha())

If we’re checking for alpha of zero, we don’t also have to check isValid;
invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.cpp:1647
> +	   if (!ancestor || !ancestor->renderStyle())
> +	       continue;

There’s no need to check !ancestor here. That’s already in the loop condition.

It doesn’t make much sense to check renderStyle for null, and then using
computedStyle. The main point of computedStyle is that it can give us a color
even when the render style is not available. If we require a renderStyle then
we should just use it.

What guarantees that style is updated? One of this things that
ComputedStyleExtractor::propertyValue does is call the appropriate layout
functions, such as updateLayoutIgnorePendingStylesheets and such. If you are
removing it, then the new code has to handle that issue somehow. Style isn’t
guaranteed to already be computed when this function is called.

> Source/WebCore/editing/EditingStyle.cpp:1648
> +	   RenderStyle* style = ancestor->computedStyle();

I don’t think we should be using computedStyle at all.

> Source/WebCore/editing/EditingStyle.cpp:1652
> +	   if (color.isValid() && color.alpha())

If we’re checking for alpha of zero, we don’t also have to check isValid;
invalid colors have RGBA of 0.

> Source/WebCore/editing/EditingStyle.h:37
> +#include "Color.h"

A return value of Color does not require including the entire Color header. A
forward class declaration will suffice.