Bug 88145 – [chromium] Paint scrollbars on WebKit thread and composite those textures

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

[Adrienne Walker]

[chromium] Paint scrollbars on WebKit thread and composite those textures

[Adrienne Walker]

Created attachment 145401 [details]
Patch

[WebKit Review Bot]

Comment on attachment 145401 [details]
Patch

Attachment 145401 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12861863

New failing tests:
platform/chromium/virtual/threaded/compositing/visibility/visibility-image-layers-dynamic.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-simple-canvas2d-layer.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-image-layers.html
platform/chromium/virtual/threaded/compositing/visibility/compositing-and-visibility-turned-off-together.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-composited-transforms.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-simple-video-layer.html
platform/chromium/virtual/threaded/compositing/visibility/visibility-composited.html

[WebKit Review Bot]

Created attachment 145427 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adrienne Walker]

Created attachment 145457 [details]
Fix tests

[James Robinson]

Comment on attachment 145457 [details]
Fix tests

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

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:234
> +    // FIXME: Only really need to paint these if the theme changes or if
> +    // some of the scrollbar properties change. Maybe scrollbar invalidations
> +    // could be a stand in for repainting all the parts textures.
> +    if (contentBounds().isEmpty())

we should be getting invalidations on the scrollbar layer when it gets damaged today, which includes theme/property changing or the scroll offset changing.  it'd be good to avoid repainting+reuploading using this information currently, even if it does mean repainting+reuploading when the scroll offset and nothing else changes

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:32
> +#include "CCTextureDrawQuad.h"

nit: should be "cc/CCTexture..."

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:36
> +#include "Scrollbar.h"

what pulls this in?

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=88274 is useful if you want to do side-by-side tests.

[James Robinson]

Comment on attachment 145457 [details]
Fix tests

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1181
> +    WebTransformationMatrix quadTransform = quad->quadTransform();
> +    IntRect quadRect = quad->quadRect();
> +    quadTransform.translate(quadRect.x() + quadRect.width() / 2.0, quadRect.y() + quadRect.height() / 2.0);
>  
> -    drawTexturedQuad(quad->layerTransform(), bounds.width(), bounds.height(), quad->opacity(), sharedGeometryQuad(), binding.matrixLocation, binding.alphaLocation, -1);
> +    drawTexturedQuad(quadTransform, quadRect.width(), quadRect.height(), quad->opacity(), sharedGeometryQuad(), binding.matrixLocation, binding.alphaLocation, -1);

what's this?

[Adrienne Walker]

(In reply to comment #7)
> (From update of attachment 145457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145457&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1181
> > +    WebTransformationMatrix quadTransform = quad->quadTransform();
> > +    IntRect quadRect = quad->quadRect();
> > +    quadTransform.translate(quadRect.x() + quadRect.width() / 2.0, quadRect.y() + quadRect.height() / 2.0);
> >  
> > -    drawTexturedQuad(quad->layerTransform(), bounds.width(), bounds.height(), quad->opacity(), sharedGeometryQuad(), binding.matrixLocation, binding.alphaLocation, -1);
> > +    drawTexturedQuad(quadTransform, quadRect.width(), quadRect.height(), quad->opacity(), sharedGeometryQuad(), binding.matrixLocation, binding.alphaLocation, -1);
> 
> what's this?

This makes drawTextureQuad behave like the other drawFooQuad functions.

drawTexturedQuad draws a -0.5, 0.5 unit square transformed by the quad transform.  If you don't factor in quadRect.x() and .y(), then you get the quad centered at the layer origin.  Before this patch, nobody was using drawTextureQuad for anything other than a quad whose position was IntPoint().

This base draw function is a giant mess that needs to be cleaned up, but not here.

[Adrienne Walker]

Created attachment 147112 [details]
Patch

[Adrienne Walker]

(In reply to comment #9)
> Created an attachment (id=147112) [details]
> Patch

* Fixed includes.

* Added logic for only repainting during invalidation.  For what it's worth, in my limited testing on Linux, it looked like scrollbars had all-or-nothing invalidations.

* Turned on the use of ScrollbarLayerChromium even when using the single thread proxy to get the benefit of testing.  This turned up bug 88882 and bug 88887, which were pre-existing bugs with impl-side scrollbars.

[James Robinson]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=147112) [details] [details]
> > Patch
> 
> * Fixed includes.
> 
> * Added logic for only repainting during invalidation.  For what it's worth, in my limited testing on Linux, it looked like scrollbars had all-or-nothing invalidations.

That matches my understanding. I'd like to tighten it up someday (when it makes a difference).

> 
> * Turned on the use of ScrollbarLayerChromium even when using the single thread proxy to get the benefit of testing.  This turned up bug 88882 and bug 88887, which were pre-existing bugs with impl-side scrollbars.

[James Robinson]

Comment on attachment 147112 [details]
Patch

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

Wunderbar!

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:239
> +void ScrollbarLayerChromium::update(CCTextureUpdater& updater, const CCOcclusionTracker* occlusion)

if we're not using occlusion, just leave the parameter name off

[Adrienne Walker]

Created attachment 147173 [details]
For landing

[WebKit Review Bot]

Comment on attachment 147173 [details]
For landing

Clearing flags on attachment: 147173

Committed r120135: <http://trac.webkit.org/changeset/120135>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.