Bug 89031 – [chromium] Move opaque rectangle tracking logic out of compositor core

• Component: New Bugs
• Status: RESOLVED FIXED

[James Robinson]

[chromium] Move opaque rectangle tracking logic out of compositor core

[James Robinson]

Created attachment 147399 [details]
Patch

[Adrienne Walker]

Comment on attachment 147399 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/OpaqueRectTrackingContentLayerDelegate.h:49
> +    explicit OpaqueRectTrackingContentLayerDelegate(GraphicsContextPainter* something);

I'm sure you can come up with something better than this parameter name.

> Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.cpp:51
>  void SkPictureCanvasLayerTextureUpdater::prepareToUpdate(const IntRect& contentRect, const IntSize& /* tileSize */, int borderTexels, float contentsScale, IntRect& resultingOpaqueRect)

Can the borderTexels parameter get removed too?

[James Robinson]

(In reply to comment #2)
> (From update of attachment 147399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147399&action=review
> 
> > Source/WebCore/platform/graphics/chromium/OpaqueRectTrackingContentLayerDelegate.h:49
> > +    explicit OpaqueRectTrackingContentLayerDelegate(GraphicsContextPainter* something);
> 
> I'm sure you can come up with something better than this parameter name.

Whoops, fixed.

> 
> > Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.cpp:51
> >  void SkPictureCanvasLayerTextureUpdater::prepareToUpdate(const IntRect& contentRect, const IntSize& /* tileSize */, int borderTexels, float contentsScale, IntRect& resultingOpaqueRect)
> 
> Can the borderTexels parameter get removed too?

Yes! Removed and added lots of OVERRIDEs to this class hierarchy to make it less brittle.

[James Robinson]

Created attachment 147445 [details]
Patch

[Adrienne Walker]

Comment on attachment 147445 [details]
Patch

Thanks! R=me, assuming Clang builds with all of those overrides.

[Adrienne Walker]

Comment on attachment 147445 [details]
Patch

Actually, I'm going to rescind my R+.  You need to update the code I landed in ScrollbarlayerChromium that uses LayerPainterChromium.

[James Robinson]

Yeah, I noticed when syncing.  The changes look pretty mechanical though - just update the signature so it compiles.  Do you want to take another look at the updated patch?

[James Robinson]

I think this does the trick (hopefully layout tests will let me know if I'm wrong):


diff --git a/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp b/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp
index f061580..8ceba89 100644
--- a/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp
+++ b/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp
@@ -29,7 +29,9 @@
 #include "ScrollbarLayerChromium.h"
 
 #include "BitmapCanvasLayerTextureUpdater.h"
+#include "GraphicsContext.h"
 #include "LayerPainterChromium.h"
+#include "PlatformContextSkia.h"
 #include "Scrollbar.h"
 #include "ScrollbarThemeComposite.h"
 #include "cc/CCLayerTreeHost.h"
@@ -109,8 +111,11 @@ public:
         return adoptPtr(new ScrollbarBackgroundPainter(scrollbar, theme));
     }
 
-    virtual void paint(GraphicsContext& context, const IntRect& contentRect)
+    virtual void paint(SkCanvas* canvas, const IntRect& contentRect, IntRect&) OVERRIDE
     {
+        PlatformContextSkia platformContext(canvas);
+        GraphicsContext context(&platformContext);
+
         // The following is a simplification of ScrollbarThemeComposite::paint.
         m_theme->paintScrollbarBackground(&context, m_scrollbar);
 
@@ -162,8 +167,11 @@ public:
         return adoptPtr(new ScrollbarThumbPainter(scrollbar, theme));
     }
 
-    virtual void paint(GraphicsContext& context, const IntRect& contentRect)
+    virtual void paint(SkCanvas* canvas, const IntRect& contentRect, IntRect& opaque) OVERRIDE
     {
+        PlatformContextSkia platformContext(canvas);
+        GraphicsContext context(&platformContext);
+
         context.clearRect(contentRect);
 
         // Consider the thumb to be at the origin when painting.
@@ -229,7 +237,7 @@ void ScrollbarLayerChromium::updatePart(LayerTextureUpdater* painter, LayerTextu
 
     // Paint and upload the entire part.
     IntRect paintedOpaqueRect;
-    painter->prepareToUpdate(rect, rect.size(), false, 1, paintedOpaqueRect);
+    painter->prepareToUpdate(rect, rect.size(), false, paintedOpaqueRect);
     texture->prepareRect(rect);
 
     IntRect destRect(IntPoint(), rect.size());


one side effect is it's not doing opaque rect tracking at all - I could use the opaque rect tracking stuff here, or perhaps set the opaque rects more specifically if we want them, but I don't think that we will be able to do much useful culling based on 15px wide opaque rects.

[James Robinson]

Hah nope, that doesn't work (the scrollbars are just all black in DRT)....hmmmm

[James Robinson]

Ah whoops, I mixed up the "borderTexels" and "contentsScale" factor on the call to prepareToUpdate and was passing "false" in as the scale.  D'oh!

This appears to pass all tests and is sane with some manual smoke testing:


diff --git a/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp b/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp
index f061580..006c92f 100644
--- a/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp
+++ b/Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp
@@ -29,7 +29,9 @@
 #include "ScrollbarLayerChromium.h"
 
 #include "BitmapCanvasLayerTextureUpdater.h"
+#include "GraphicsContext.h"
 #include "LayerPainterChromium.h"
+#include "PlatformContextSkia.h"
 #include "Scrollbar.h"
 #include "ScrollbarThemeComposite.h"
 #include "cc/CCLayerTreeHost.h"
@@ -109,8 +111,14 @@ public:
         return adoptPtr(new ScrollbarBackgroundPainter(scrollbar, theme));
     }
 
-    virtual void paint(GraphicsContext& context, const IntRect& contentRect)
+    virtual void paint(SkCanvas* canvas, const IntRect& contentRect, IntRect&) OVERRIDE
     {
+        PlatformContextSkia platformContext(canvas);
+        platformContext.setDrawingToImageBuffer(true);
+        GraphicsContext context(&platformContext);
+        context.clearRect(contentRect);
+        context.clip(contentRect);
+
         // The following is a simplification of ScrollbarThemeComposite::paint.
         m_theme->paintScrollbarBackground(&context, m_scrollbar);
 
@@ -162,8 +170,12 @@ public:
         return adoptPtr(new ScrollbarThumbPainter(scrollbar, theme));
     }
 
-    virtual void paint(GraphicsContext& context, const IntRect& contentRect)
+    virtual void paint(SkCanvas* canvas, const IntRect& contentRect, IntRect& opaque) OVERRIDE
     {
+        PlatformContextSkia platformContext(canvas);
+        platformContext.setDrawingToImageBuffer(true);
+        GraphicsContext context(&platformContext);
+
         context.clearRect(contentRect);
 
         // Consider the thumb to be at the origin when painting.
@@ -229,7 +241,7 @@ void ScrollbarLayerChromium::updatePart(LayerTextureUpdater* painter, LayerTextu
 
     // Paint and upload the entire part.
     IntRect paintedOpaqueRect;
-    painter->prepareToUpdate(rect, rect.size(), false, 1, paintedOpaqueRect);
+    painter->prepareToUpdate(rect, rect.size(), 1, paintedOpaqueRect);
     texture->prepareRect(rect);
 
     IntRect destRect(IntPoint(), rect.size());


will upload a new patch for EWS and posterity

[James Robinson]

Created attachment 147458 [details]
Patch

[Adrienne Walker]

Comment on attachment 147458 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 147458 [details]
Patch

Clearing flags on attachment: 147458

Committed r120346: <http://trac.webkit.org/changeset/120346>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.