[webkit-reviews] review denied: [Bug 39123] Unneeded fillRect call in RenderBoxModelObject::paintFillLayerExtended : [Attachment 57679] Added comments to proposed patch. Removed Color.isValid check

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Pat Galizia
<pgalizia at codeaurora.org>'s request for review:
Bug 39123: Unneeded fillRect call in
RenderBoxModelObject::paintFillLayerExtended
https://bugs.webkit.org/show_bug.cgi?id=39123

Attachment 57679: Added comments to proposed patch. Removed Color.isValid check
https://bugs.webkit.org/attachment.cgi?id=57679&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/rendering/RenderBoxModelObject.cpp
> ===================================================================

> +		   // If the bgColor alpha isn't 255, then we can apply the
> +		   // baseColor to fillRect.  If bgColor's alpha is 255, then
> +		   // this would be overwritten anyway, so there's not much
> +		   // sense in doing it.  It slightly helps performance in
> +		   // smaller devices.

The comment is slightly verbose. I think it would be better as:
 // Don't bother filling with the base color if the background color is opaque.


You don't need to explain the performanc implications.

> +		   if (bgColor.alpha() != 255) {

Better as

 if (!bgColor.hasAlpha())

> +		       context->save();
> +		       context->setCompositeOperation(CompositeCopy);
> +		       context->fillRect(rect, baseColor,
style()->colorSpace());
> +		       context->restore();
> +		   }
>	       } else
>		   context->clearRect(rect);

Can't you also avoid the clearRect() if the bgColor is opaque?