cc: Don't use partial updates for scrollbars when they are not allowed.

cc: Don't use partial updates for scrollbars when they are not allowed.

Scrollbars always use partial updates, without claiming a partial update
slot from the LayerTreeHost, unlike content layers. However, when partial
updates are not allowed at all (under ubercomp), then the Scrollbar layer
should not do partial updates at all.

Tests:
LayerTreeHostTestAtomicCommit.runMultiThread
LayerTreeHostTestAtomicCommitWithPartialUpdate.runMultiThread

NOTRY=true
BUG=123444
Depends on: https://codereview.chromium.org/11609002

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174503

[danakj, 2012-12-08 03:16:50]

A 4-line change in the ubercomp CL becomes this to add tests:

29 files changed, 816 insertions(+), 185 deletions(-)

[enne (OOO), 2012-12-08 03:39:23]

So many fake classes and clients and painters.  :(

What was wrong with making WebScrollbarThemePainter abstract, and then just
implementing a single FakeWebScrollbarThemePainter?

[danakj, 2012-12-08 03:43:14]

/me defers to jamesr

[jamesr, 2012-12-10 01:38:32]

If adding an interface means that fewer cc:: classes have to be tied to WebKit
objects, then I think that's a win.  I can't tell that is the case here - it
looks like it's just adding things.  I'm not sure I'm getting the full picture,
though, there seems to be a lot of things going on here.

Could you split this up?  For instance, do you really need to rename
paint->Paint at the same time as everything else?  If you had just the renames,
just the test infrastructure additions, and just the actual bugfix as separate
patches it might be easier to see what's really going on.

https://codereview.chromium.org/11464041/diff/1030/cc/scrollbar_layer.h
File cc/scrollbar_layer.h (right):

https://codereview.chromium.org/11464041/diff/1030/cc/scrollbar_layer.h#newco...
cc/scrollbar_layer.h:30: WebKit::WebScrollbarThemePainter,
if you have LayerPainterClient can you try to get rid of this?

[danakj, 2012-12-10 03:37:10]

On 2012/12/10 01:38:32, jamesr wrote:
> If adding an interface means that fewer cc:: classes have to be tied to WebKit
> objects, then I think that's a win.  I can't tell that is the case here - it
> looks like it's just adding things.  I'm not sure I'm getting the full
picture,
> though, there seems to be a lot of things going on here.
> 
> Could you split this up?  For instance, do you really need to rename
> paint->Paint at the same time as everything else?  If you had just the
renames,
> just the test infrastructure additions, and just the actual bugfix as separate
> patches it might be easier to see what's really going on.

Ya I can, I just did that so I could have proper style in the new files.

> https://codereview.chromium.org/11464041/diff/1030/cc/scrollbar_layer.h
> File cc/scrollbar_layer.h (right):
> 
>
https://codereview.chromium.org/11464041/diff/1030/cc/scrollbar_layer.h#newco...
> cc/scrollbar_layer.h:30: WebKit::WebScrollbarThemePainter,
> if you have LayerPainterClient can you try to get rid of this?

I suppose the client could own it instead.

[danakj, 2012-12-10 15:08:16]

Oh I think I had an idea last night. If we use virtual factory methods on
scrollbar layer, we can avoid the client, keep the painter implementations
inside the scrollbar layer file, and only need a single fake painter, i
think.

Flying today. will do this when i can.

[enne (OOO), 2012-12-10 23:40:42]

Ok, I talked with jamesr and think I understand what's going on.  Only
webkit/compositor_bindings should depend on anything from WebKit to avoid a
dependency cycle.  cc just needs its own interfaces for these sorts of things.

Can I suggest that you add some cc::ScrollbarThemePainter that's exactly like
WebScrollbarThemePainter except it's pure virtual.   Something in the compositor
bindings can bridge between these two classes.  ScrollbarLayer then only depends
on cc::ScrollbarThemePainter, which can be faked for tests.

I don't think we necessarily need a client that can factory create painters, or
multiple painter types.

[danakj, 2012-12-11 06:06:55]

On 2012/12/10 23:40:42, enne wrote:
> Ok, I talked with jamesr and think I understand what's going on.  Only
> webkit/compositor_bindings should depend on anything from WebKit to avoid a
> dependency cycle.  cc just needs its own interfaces for these sorts of things.
> 
> Can I suggest that you add some cc::ScrollbarThemePainter that's exactly like
> WebScrollbarThemePainter except it's pure virtual.   Something in the
compositor
> bindings can bridge between these two classes.  ScrollbarLayer then only
depends
> on cc::ScrollbarThemePainter, which can be faked for tests.
> 
> I don't think we necessarily need a client that can factory create painters,
or
> multiple painter types.

Yaya! I like this. I was thinking to make the internal scrollbar painter class a
virtual interface, but making something like the WebScrollbarThemePainter makes
sense!

[danakj, 2012-12-17 18:49:23]

This is rebased on https://codereview.chromium.org/11609002

It's very simple now :) PTAL

[enne (OOO), 2012-12-17 20:59:01]

https://codereview.chromium.org/11464041/diff/15002/cc/scrollbar_layer.cc
File cc/scrollbar_layer.cc (right):

https://codereview.chromium.org/11464041/diff/15002/cc/scrollbar_layer.cc#new...
cc/scrollbar_layer.cc:309: m_dirtyRect.Union(m_updateRect);
What does adding m_dirtyRect enable you to do that m_updateRect didn't?

[danakj, 2012-12-17 21:27:06]

https://codereview.chromium.org/11464041/diff/15002/cc/scrollbar_layer.cc
File cc/scrollbar_layer.cc (right):

https://codereview.chromium.org/11464041/diff/15002/cc/scrollbar_layer.cc#new...
cc/scrollbar_layer.cc:309: m_dirtyRect.Union(m_updateRect);
On 2012/12/17 20:59:02, enne wrote:
> What does adding m_dirtyRect enable you to do that m_updateRect didn't?

it lets you return when visibleContentRect() is empty (ie scrollbar is not
visible) and then paint it when it becomes visible.

[enne (OOO), 2012-12-17 21:36:45]

lgtm

https://codereview.chromium.org/11464041/diff/15002/cc/scrollbar_layer.cc
File cc/scrollbar_layer.cc (right):

https://codereview.chromium.org/11464041/diff/15002/cc/scrollbar_layer.cc#new...
cc/scrollbar_layer.cc:309: m_dirtyRect.Union(m_updateRect);
On 2012/12/17 21:27:06, danakj wrote:
> On 2012/12/17 20:59:02, enne wrote:
> > What does adding m_dirtyRect enable you to do that m_updateRect didn't?
> 
> it lets you return when visibleContentRect() is empty (ie scrollbar is not
> visible) and then paint it when it becomes visible.

Ah, ok.  I see.  :)

[commit-bot: I haz the power, 2012-12-18 16:11:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11464041/15002

[commit-bot: I haz the power, 2012-12-18 16:11:08]

Failed to apply patch for cc/layer_tree_host_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layer_tree_host_unittest.cc
  Hunk #2 succeeded at 1202 (offset 26 lines).
  Hunk #3 succeeded at 1463 (offset 25 lines).
  Hunk #4 succeeded at 1471 (offset 25 lines).
  Hunk #5 FAILED at 1463.
  Hunk #6 FAILED at 1495.
  Hunk #7 succeeded at 1532 (offset 23 lines).
  Hunk #8 succeeded at 1542 (offset 23 lines).
  Hunk #9 succeeded at 1567 (offset 23 lines).
  Hunk #10 succeeded at 1583 (offset 23 lines).
  Hunk #11 FAILED at 1576.
  Hunk #12 succeeded at 1637 with fuzz 1 (offset 26 lines).
  Hunk #13 succeeded at 1658 with fuzz 2 (offset 30 lines).
  Hunk #14 succeeded at 1675 (offset 25 lines).
  Hunk #15 succeeded at 1710 (offset 27 lines).
  3 out of 15 hunks FAILED -- saving rejects to file
cc/layer_tree_host_unittest.cc.rej

Patch:       cc/layer_tree_host_unittest.cc
Index: cc/layer_tree_host_unittest.cc
diff --git a/cc/layer_tree_host_unittest.cc b/cc/layer_tree_host_unittest.cc
index
b8d79d9cf42289d77aa64fdbe7401c96fc173fa7..9e9d8588f5d239561b2e660d4099b4829016e82b
100644
--- a/cc/layer_tree_host_unittest.cc
+++ b/cc/layer_tree_host_unittest.cc
@@ -11,11 +11,15 @@
 #include "cc/layer_tree_host_impl.h"
 #include "cc/layer_tree_impl.h"
 #include "cc/output_surface.h"
+#include "cc/scrollbar_layer.h"
 #include "cc/single_thread_proxy.h"
 #include "cc/test/fake_content_layer_client.h"
 #include "cc/test/fake_layer_tree_host_client.h"
 #include "cc/test/fake_output_surface.h"
 #include "cc/test/fake_proxy.h"
+#include "cc/test/fake_scrollbar_theme_painter.h"
+#include "cc/test/fake_web_scrollbar.h"
+#include "cc/test/fake_web_scrollbar_theme_geometry.h"
 #include "cc/test/geometry_test_utils.h"
 #include "cc/test/layer_tree_test_common.h"
 #include "cc/test/occlusion_tracker_test_common.h"
@@ -1172,6 +1176,42 @@ private:
     int m_paintContentsCount;
 };
 
+class ScrollbarLayerWithUpdateTracking : public ScrollbarLayer {
+public:
+    static scoped_refptr<ScrollbarLayerWithUpdateTracking> create(int
scrollLayerId, bool paintContentsDuringUpdate)
+    {
+        return make_scoped_refptr(new
ScrollbarLayerWithUpdateTracking(scrollLayerId, paintContentsDuringUpdate));
+    }
+
+    int updateCount() { return m_updateCount; }
+    void resetUpdateCount() { m_updateCount = 0; }
+
+    virtual void update(ResourceUpdateQueue& queue, const OcclusionTracker*
occlusion, RenderingStats& stats) OVERRIDE
+    {
+        ScrollbarLayer::update(queue, occlusion, stats);
+        m_updateCount++;
+    }
+
+private:
+    explicit ScrollbarLayerWithUpdateTracking(int scrollLayerId, bool
paintContentsDuringUpdate)
+        : ScrollbarLayer(
+            FakeWebScrollbar::create().PassAs<WebKit::WebScrollbar>(),
+           
FakeScrollbarThemePainter::Create(paintContentsDuringUpdate).PassAs<ScrollbarThemePainter>(),
+           
FakeWebScrollbarThemeGeometry::create().PassAs<WebKit::WebScrollbarThemeGeometry>(),
+            scrollLayerId)
+        , m_updateCount(0)
+    {
+        setAnchorPoint(gfx::PointF(0, 0));
+        setBounds(gfx::Size(10, 10));
+        setIsDrawable(true);
+    }
+    virtual ~ScrollbarLayerWithUpdateTracking()
+    {
+    }
+
+    int m_updateCount;
+};
+
 // Layer opacity change during paint should not prevent compositor resources
from being updated during commit.
 class LayerTreeHostTestOpacityChange : public LayerTreeHostTest {
 public:
@@ -1398,6 +1438,7 @@ class LayerTreeHostTestAtomicCommit : public
LayerTreeHostTest {
 public:
     LayerTreeHostTestAtomicCommit()
         : m_layer(ContentLayerWithUpdateTracking::create(&m_client))
+        , m_scrollbar(ScrollbarLayerWithUpdateTracking::create(m_layer->id(),
true))
     {
         // Make sure partial texture updates are turned off.
         m_settings.maxPartialTextureUpdates = 0;
@@ -1405,6 +1446,7 @@ public:
 
     virtual void beginTest() OVERRIDE
     {
+        m_layer->addChild(m_scrollbar);
         m_layerTreeHost->setRootLayer(m_layer);
         m_layerTreeHost->setViewportSize(gfx::Size(10, 10), gfx::Size(10, 10));
 
@@ -1421,25 +1463,28 @@ public:
 
         switch (impl->activeTree()->source_frame_number()) {
         case 0:
-            // Number of textures should be one.
-            ASSERT_EQ(1, context->numTextures());
-            // Number of textures used for commit should be one.
-            EXPECT_EQ(1, context->numUsedTextures());
+            // Number of textures should be one for each layer
+            ASSERT_EQ(2, context->numTextures());
+            // Number of textures used for commit should be one for each layer.
+            EXPECT_EQ(2, context->numUsedTextures());
             // Verify that used texture is correct.
             EXPECT_TRUE(context->usedTexture(context->texture(0)));
+            EXPECT_TRUE(context->usedTexture(context->texture(1)));
 
             context->resetUsedTextures();
             break;
         case 1:
-            // Number of textures should be two as the first texture
-            // is used by impl thread and cannot by used for update.
-            ASSERT_EQ(2, context->numTextures());
-            // Number of textures used for commit should still be one.
-            EXPECT_EQ(1, context->numUsedTextures());
-            // First texture should not have been used.
+            // Number of textures should be doubled as the first textures
+            // are used by impl thread and cannot by used for update.
+            ASSERT_EQ(4, context->numTextures());
+            // Number of textures used for commit should still be one for each
layer.
+            EXPECT_EQ(2, context->numUsedTextures());
+            // First textures should not have been used.
             EXPECT_FALSE(context->usedTexture(context->texture(0)));
-            // New texture should have been used.
-            EXPECT_TRUE(context->usedTexture(context->texture(1)));
+            EXPECT_FALSE(context->usedTexture(context->texture(1)));
+            // New textures should have been used.
+            EXPECT_TRUE(context->usedTexture(context->texture(2)));
+            EXPECT_TRUE(context->usedTexture(context->texture(3)));
 
             context->resetUsedTextures();
             break;
@@ -1453,8 +1498,8 @@ public:
     {
         CompositorFakeWebGraphicsContext3DWithTextureTracking* context =
static_cast<CompositorFakeWebGraphicsContext3DWithTextureTracking*>(impl->outputSurface()->Context3D());
 
-        // Number of textures used for draw should always be one.
-        EXPECT_EQ(1, context->numUsedTextures());
+        // Number of textures used for draw should always be one for each
layer.
+        EXPECT_EQ(2, context->numUsedTextures());
 
         if (impl->activeTree()->source_frame_number() < 1) {
             context->resetUsedTextures();
@@ -1467,6 +1512,7 @@ public:
     virtual void layout() OVERRIDE
     {
         m_layer->setNeedsDisplay();
+        m_scrollbar->setNeedsDisplay();
     }
 
     virtual void afterTest() OVERRIDE
@@ -1476,6 +1522,7 @@ public:
 private:
     FakeContentLayerClient m_client;
     scoped_refptr<ContentLayerWithUpdateTracking> m_layer;
+    scoped_refptr<ScrollbarLayerWithUpdateTracking> m_scrollbar;
 };
 
 TEST_F(LayerTreeHostTestAtomicCommit, runMultiThread)
@@ -1500,6 +1547,8 @@ public:
     LayerTreeHostTestAtomicCommitWithPartialUpdate()
         : m_parent(ContentLayerWithUpdateTracking::create(&m_client))
         , m_child(ContentLayerWithUpdateTracking::create(&m_client))
+        ,
m_scrollbarWithPaints(ScrollbarLayerWithUpdateTracking::create(m_parent->id(),
true))
+        ,
m_scrollbarWithoutPaints(ScrollbarLayerWithUpdateTracking::create(m_parent->id(),
false))
         , m_numCommits(0)
     {
         // Allow one partial texture update.
@@ -1514,6 +1563,8 @@ public:
         gfx::Transform identityMatrix;
         setLayerPropertiesForTesting(m_parent.get(), 0, identityMatrix,
gfx::PointF(0, 0), gfx::PointF(0, 0), gfx::Size(10, 20), true);
         setLayerPropertiesForTesting(m_child.get(), m_parent.get(),
identityMatrix, gfx::PointF(0, 0), gfx::PointF(0, 10), gfx::Size(10, 10),
false);
+        setLayerPropertiesForTesting(m_scrollbarWithPaints.get(),
m_parent.get(), identityMatrix, gfx::PointF(0, 0), gfx::PointF(0, 10),
gfx::Size(10, 10), false);
+        setLayerPropertiesForTesting(m_scrollbarWithoutPaints.get(),
m_parent.get(), identityMatrix, gfx::PointF(0, 0), gfx::PointF(0, 10),
gfx::Size(10, 10), false);
 
         ASSERT_TRUE(m_layerTreeHost->initializeRendererIfNeeded());
         ResourceUpdateQueue queue;
@@ -1528,31 +1579,54 @@ public:
 
         switch (impl->activeTree()->source_frame_number()) {
         case 0:
-            // Number of textures should be two.
-            ASSERT_EQ(2, context->numTextures());
-            // Number of textures used for commit should be two.
-            EXPECT_EQ(2, context->numUsedTextures());
+            // Number of textures should be one for each layer.
+            ASSERT_EQ(4, context->numTextures());
+            // Number of textures used for commit should be one for each layer.
+            EXPECT_EQ(4, context->numUsedTextures());
             // Verify that used textures are correct.
             EXPECT_TRUE(context->usedTexture(context->texture(0)));
             EXPECT_TRUE(context->usedTexture(context->texture(1)));
+            EXPECT_TRUE(context->usedTexture(context->texture(2)));
+            EXPECT_TRUE(context->usedTexture(context->texture(3)));
 
             context->resetUsedTextures();
             break;
         case 1:
-            // Number of textures used for commit should still be two.
-            EXPECT_EQ(2, context->numUsedTextures());
-            // First two textures should not have been used.
+            // Number of textures should be two for each content layer and one
+            // for each scrollbar, since they always do a partial update.
+            ASSERT_EQ(6…
(message too large)

[commit-bot: I haz the power, 2012-12-18 16:27:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11464041/24001

[danakj, 2012-12-22 01:01:59]

ok android will pass now.

1. Moved the #ifdef OS_ANDROID stuff to layer_tree_settings.cc
2. Made use of FakeContentLayer in the unit tests
3. Added FakeScrollbarLayer to match and used it also.

[enne (OOO), 2012-12-22 01:17:18]

https://codereview.chromium.org/11464041/diff/41008/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11464041/diff/41008/cc/layer_impl.cc#newcode796
cc/layer_impl.cc:796: static const double fadeoutDelay = 0.3;
s/static const//

https://codereview.chromium.org/11464041/diff/41008/cc/resource_update_contro...
File cc/resource_update_controller.cc (left):

https://codereview.chromium.org/11464041/diff/41008/cc/resource_update_contro...
cc/resource_update_controller.cc:27: const size_t partialTextureUpdatesMax = 0;
I see that you moved this to settings, but where is the code that uses the value
from settings rather than from resource provider?

[danakj, 2012-12-22 01:22:38]

https://codereview.chromium.org/11464041/diff/41008/cc/resource_update_contro...
File cc/resource_update_controller.cc (left):

https://codereview.chromium.org/11464041/diff/41008/cc/resource_update_contro...
cc/resource_update_controller.cc:27: const size_t partialTextureUpdatesMax = 0;
On 2012/12/22 01:17:18, enne wrote:
> I see that you moved this to settings, but where is the code that uses the
value
> from settings rather than from resource provider?

LayerTreeHost takes the min of settings and the value from the proxy, and puts
that back in the settings.

[danakj, 2012-12-22 01:23:26]

https://codereview.chromium.org/11464041/diff/41008/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11464041/diff/41008/cc/layer_impl.cc#newcode796
cc/layer_impl.cc:796: static const double fadeoutDelay = 0.3;
On 2012/12/22 01:17:18, enne wrote:
> s/static const//

Mhm! Done.

[enne (OOO), 2012-12-22 01:39:08]

lgtm

[commit-bot: I haz the power, 2012-12-22 01:44:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11464041/43015

[commit-bot: I haz the power, 2012-12-22 06:45:00]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-22 06:46:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11464041/43015

[commit-bot: I haz the power, 2012-12-22 06:47:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11464041/43015

[commit-bot: I haz the power, 2012-12-22 07:04:59]

Change committed as 174503