[webkit-reviews] review denied: [Bug 21680] queryCommandValue("BackColor") returns rgb(0, 0, 0) for elements with transparent background : [Attachment 61137] fixed style and FIXME

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 21680: queryCommandValue("BackColor") returns rgb(0,0,0) for elements with
transparent background
https://bugs.webkit.org/show_bug.cgi?id=21680

Attachment 61137: fixed style and FIXME
https://bugs.webkit.org/attachment.cgi?id=61137&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good. A few problems.

> +    if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR) {
> +	   Color backgroundColor(value->getRGBA32Value());
> +	   return backgroundColor.hasAlpha() && !backgroundColor.alpha();
> +    }

There is no reason to construct a Color here. You can check the alpha value in
an RGBA 32-bit value with alphaChannel.

There is no reason to call hasAlpha here. hasAlpha is just "alpha != 256" and
alpha is "alpha == 0" There's no reason to check both.

    if (value->primitiveType() == CSSPrimitiveValue::CSS_RGBCOLOR)
	return !alphaChannel(value->getRGBA32Value());

> +    // FIXME: Rather than retrieving the style at the start of the current
selection,
> +    // we retrieve the style present throughout the selection.
> +    RefPtr<CSSStyleDeclaration> selectionStyle =
m_frame->selectionComputedStyle(nodeToRemove);

Is there a reason you need to comment on this, but not fix it here? Is there a
test covering this?

> +	   } while (isTransparent(selectionStyle.get()));

The function name isTransparent is unclear. I thin the intent is to check if
the background color is transparent. That's not the same as saying a style "is
transparent". I would call the function hasTransparentBackgroundColor because
it doesn't even check if the background is transparent, just if the background
*color* is transparent. You could have a non-transparent background image or a
transparent background image. It also doesn't return true if the object has no
background color at all, which seems like a mistake. Is the algorithm right for
those cases?

>      TriState selectionHasStyle(CSSStyleDeclaration*) const;
> +    String selectionCSSPropertyValue(int);

The int here needs an argument name. It's not clear what an int here is,
although I know it's a property ID.

I think it's unfortunate to add a new function here and have it be unclear if
it means the property value at the start of the selection or throughout the
selection. The old code didn't have this lack of clarity; it said "selection
start" in its function name. We are adding the lack of clarity now. Lets not!

review- because I think this could be easily improved. Sorry it took me so long
to get to reviewing this. There are a lot of patches!