Bug 63847 – [chromium] Externalize layer visibility calculation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Externalize layer visibility calculation

[James Robinson]

Created attachment 99526 [details]
Patch

[Adrienne Walker]

Comment on attachment 99526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review

A few small questions.  In general, I really like where this is going.  :)

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
>  TransformationMatrix ContentLayerChromium::tilingTransform()

Can this get pushed into LayerChromium? It'd help with your ImageLayer/ContentLayer split.  It's also not really a tiling transform anymore.  It's now the scale that needs to be applied to the contents, so maybe it needs a slightly different name.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:202
> -void ContentLayerChromium::draw(const IntRect& targetSurfaceRect)
> +void ContentLayerChromium::draw()
>  {
> -    const TransformationMatrix transform = tilingTransform();
> -    IntRect layerRect = visibleLayerRect(targetSurfaceRect);
> +    const IntRect& layerRect = visibleLayerRect();
>      if (!layerRect.isEmpty())
> -        m_tiler->draw(layerRect, transform, ccLayerImpl()->drawOpacity(), m_textureUpdater.get());
> +        m_tiler->draw(layerRect, tilingTransform(), ccLayerImpl()->drawOpacity(), m_textureUpdater.get());
>  }

Maybe I'm just not following this, but tilingTransform() appears to longer include the translation of half the layer bounds so that the origin is the top left corner.  How does this continue to work the same way as before this patch without any changes to LayerTilerChromium?

[James Robinson]

Comment on attachment 99526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:202
>>  }
> 
> Maybe I'm just not following this, but tilingTransform() appears to longer include the translation of half the layer bounds so that the origin is the top left corner.  How does this continue to work the same way as before this patch without any changes to LayerTilerChromium?

there's the translate in tilingTransform() line 159 in this patch. is that what you mean?

[Adrienne Walker]

(In reply to comment #3)
> (From update of attachment 99526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:202
> >>  }
> > 
> > Maybe I'm just not following this, but tilingTransform() appears to longer include the translation of half the layer bounds so that the origin is the top left corner.  How does this continue to work the same way as before this patch without any changes to LayerTilerChromium?
> 
> there's the translate in tilingTransform() line 159 in this patch. is that what you mean?

Oh, yes.  I was just misreading the diff.  :)

[James Robinson]

(In reply to comment #2)
> (From update of attachment 99526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> 
> A few small questions.  In general, I really like where this is going.  :)
> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
> >  TransformationMatrix ContentLayerChromium::tilingTransform()
> 
> Can this get pushed into LayerChromium? It'd help with your ImageLayer/ContentLayer split.  It's also not really a tiling transform anymore.  It's now the scale that needs to be applied to the contents, so maybe it needs a slightly different name.
> 

That's a good point.  The main reason I left it on ContentLayerChromium is I'm still not entirely sure how the tiling transform and the draw transform are supposed to interact.  My initial instinct was to just do all of this in the draw transform, but I couldn't get the math worked out properly for sublayers, and maybe they are separate concepts after all.

Anyway leaving it down on ContentLayerChromium is a slightly smaller delta from the status quo (which is nice) and it actually merged really well with the content/image layer split (I've merged together all the branches locally to make sure).

[Adrienne Walker]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 99526 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> > 
> > A few small questions.  In general, I really like where this is going.  :)
> > 
> > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
> > >  TransformationMatrix ContentLayerChromium::tilingTransform()
> > 
> > Can this get pushed into LayerChromium? It'd help with your ImageLayer/ContentLayer split.  It's also not really a tiling transform anymore.  It's now the scale that needs to be applied to the contents, so maybe it needs a slightly different name.
> > 
> 
> That's a good point.  The main reason I left it on ContentLayerChromium is I'm still not entirely sure how the tiling transform and the draw transform are supposed to interact.  My initial instinct was to just do all of this in the draw transform, but I couldn't get the math worked out properly for sublayers, and maybe they are separate concepts after all.

We could certainly clean up that distinction in a separate patch.

> Anyway leaving it down on ContentLayerChromium is a slightly smaller delta from the status quo (which is nice) and it actually merged really well with the content/image layer split (I've merged together all the branches locally to make sure).

In that case, that sounds fine to me.

Unofficially, it looks good to me.

[James Robinson]

Cool, thanks!  Ken or Stephen - would you mind giving this an official review?  Thanks.

[Stephen White]

Comment on attachment 99526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review

I leave it up to you to decide if my comments are relevant here.

Looks good.  r=me.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:156
> +    transform.scaleNonUniform(bounds().width() / static_cast<double>(contentBounds().width()),
> +                              bounds().height() / static_cast<double>(contentBounds().height()));

This could be a div-zero if contentBounds() has zero width or height.  Dunno if that's an issue here.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:370
> +    transform.scaleNonUniform(bounds.width() / static_cast<double>(contentBounds.width()),
> +                              bounds.height() / static_cast<double>(contentBounds.height()));

Same as above; potential div-zero.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:449
> +                    maskLayer->setVisibleLayerRect(IntRect(IntPoint(), maskLayer->contentBounds()));
> +                    maskLayer->paintContentsIfDirty();

This function-pair is called four times here; could potentially be refactored.

[James Robinson]

(In reply to comment #8)
> (From update of attachment 99526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> 
> I leave it up to you to decide if my comments are relevant here.
> 
> Looks good.  r=me.

Thanks!

> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:156
> > +    transform.scaleNonUniform(bounds().width() / static_cast<double>(contentBounds().width()),
> > +                              bounds().height() / static_cast<double>(contentBounds().height()));
> 
> This could be a div-zero if contentBounds() has zero width or height.  Dunno if that's an issue here.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:370
> > +    transform.scaleNonUniform(bounds.width() / static_cast<double>(contentBounds.width()),
> > +                              bounds.height() / static_cast<double>(contentBounds.height()));
> 
> Same as above; potential div-zero.

Good point on both counts, I'll add some early-outs for empty contentBounds().

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:449
> > +                    maskLayer->setVisibleLayerRect(IntRect(IntPoint(), maskLayer->contentBounds()));
> > +                    maskLayer->paintContentsIfDirty();
> 
> This function-pair is called four times here; could potentially be refactored.

I'll add a helper.

[James Robinson]

Committed r90492: <http://trac.webkit.org/changeset/90492>