Bug 88145 - [chromium] Paint scrollbars on WebKit thread and composite those textures
Summary: [chromium] Paint scrollbars on WebKit thread and composite those textures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on: 88882 88887
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-01 16:10 PDT by Adrienne Walker
Modified: 2012-06-12 16:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (23.97 KB, patch)
2012-06-01 16:15 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (716.44 KB, application/zip)
2012-06-01 22:38 PDT, WebKit Review Bot
no flags Details
Fix tests (26.83 KB, patch)
2012-06-02 13:05 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (27.62 KB, patch)
2012-06-12 10:40 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
For landing (27.63 KB, patch)
2012-06-12 15:01 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-06-01 16:10:01 PDT
[chromium] Paint scrollbars on WebKit thread and composite those textures
Comment 1 Adrienne Walker 2012-06-01 16:15:04 PDT
Created attachment 145401 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-01 22:38:38 PDT
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
Comment 3 WebKit Review Bot 2012-06-01 22:38:42 PDT
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
Comment 4 Adrienne Walker 2012-06-02 13:05:11 PDT
Created attachment 145457 [details]
Fix tests
Comment 5 James Robinson 2012-06-04 16:32:23 PDT
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?
Comment 6 James Robinson 2012-06-04 18:02:07 PDT
https://bugs.webkit.org/show_bug.cgi?id=88274 is useful if you want to do side-by-side tests.
Comment 7 James Robinson 2012-06-07 15:36:35 PDT
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?
Comment 8 Adrienne Walker 2012-06-08 13:50:10 PDT
(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.
Comment 9 Adrienne Walker 2012-06-12 10:40:07 PDT
Created attachment 147112 [details]
Patch
Comment 10 Adrienne Walker 2012-06-12 10:42:20 PDT
(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.
Comment 11 James Robinson 2012-06-12 10:47:59 PDT
(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.
Comment 12 James Robinson 2012-06-12 10:49:42 PDT
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
Comment 13 Adrienne Walker 2012-06-12 15:01:01 PDT
Created attachment 147173 [details]
For landing
Comment 14 WebKit Review Bot 2012-06-12 16:04:35 PDT
Comment on attachment 147173 [details]
For landing

Clearing flags on attachment: 147173

Committed r120135: <http://trac.webkit.org/changeset/120135>
Comment 15 WebKit Review Bot 2012-06-12 16:04:41 PDT
All reviewed patches have been landed.  Closing bug.