[webkit-reviews] review denied: [Bug 40629] GraphicsLayer: Allow more layers to be opaque when applicable : [Attachment 60201] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Sam Magnuson
<smagnuson at netflix.com>'s request for review:
Bug 40629: GraphicsLayer: Allow more layers to be opaque when applicable
https://bugs.webkit.org/show_bug.cgi?id=40629

Attachment 60201: Patch
https://bugs.webkit.org/attachment.cgi?id=60201&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> index
d531bb5d0a0e29abdfe930ec7bf2166d3b55b422..fcee1962310652d679e6977f1b587b9581242
dff 100644
> +	   No new tests: this is an optimization.	 

You can and should add tests that dump the layer tree
(layoutTestController.layerTreeAsText()). You should also add a test or two to
make sure that you don't hit this optimization when you don't intend to.

>  void GraphicsLayerQtImpl::paint(QPainter* painter, const
QStyleOptionGraphicsItem* option, QWidget* widget)
>  {
> -    if (m_currentContent.backgroundColor.isValid())
> -	   painter->fillRect(option->rect,
QColor(m_currentContent.backgroundColor));
> +//	  if (m_currentContent.backgroundColor.isValid())
> +//	      painter->fillRect(option->rect,
QColor(m_currentContent.backgroundColor));

Don't check in commented-out code.

> diff --git a/WebCore/rendering/RenderLayerBacking.cpp
b/WebCore/rendering/RenderLayerBacking.cpp

> +    bool contentsOpaque = false;
> +    if (rendererHasBackground()) {
> +	   const Color &bgColor = rendererBackgroundColor();

Color is sizeof(int), so no point in using a reference there.

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

Use !bgColor.hasAlpha().

> +	       const IntRect elementBounds = m_owningLayer->localBoundingBox(),

> +			       layerBounds = compositedBounds();

We don't use multiple declarations like this. Also no need for the IntRects to
be 'const'. Also elementBounds is a bad name; that's really layer bounds. So
use layerBounds and compositedBounds or something.

> +	       if (elementBounds.width() >= layerBounds.width() &&
elementBounds.height() >= layerBounds.height())
> +		   contentsOpaque = true;

You should just test for rect equality. The composited bounds will never be
smaller than m_owningLayer->localBoundingBox().

You should file a follow-up bug for other obvious enhancements (e.g. non-alpha
background image).

r- for lack of tests.