Change ScrollbarLayer to use UI resource

Change the ScrollbarLayer to use UI Resource as oppose to prioritized resource manager.  

First attemped as a part of the UI resource CL: 
https://chromiumcodereview.appspot.com/18191020

Reverted due to failed tests: 
https://chromiumcodereview.appspot.com/21555004

This patch clears the SkCanvas (bitmap) for the thumb/track before painting, and scales the canvas with respect to the content_scale (closer to the original scrollbar implementation that used cc/resources/content_layer_updater).

Also change the pixel swizzling format to best_texture_format().

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

[aelias_OOO_until_Jul13, 2013-08-03 01:18:19]

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
File cc/layers/scrollbar_layer_unittest.cc (left):

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer_unittest.cc:480: class
ScaledScrollbarLayerTestResourceCreation : public testing::Test {
OK, if you need this behavior after all, you should bring back the test?

[powei, 2013-08-03 01:23:53]

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
File cc/layers/scrollbar_layer_unittest.cc (left):

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer_unittest.cc:480: class
ScaledScrollbarLayerTestResourceCreation : public testing::Test {
On 2013/08/03 01:18:19, aelias wrote:
> OK, if you need this behavior after all, you should bring back the test?

Yes, you're right.  I'll bring this test back.

[aelias_OOO_until_Jul13, 2013-08-03 01:34:23]

The new scaling logic looks fine, I guess you based it off
ContentLayerUpdater::PaintContents().

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.cc:234: if (contents_scale_x() != 1.f ||
contents_scale_y() != 1.f)
This if() statement doesn't change any behavior so you can get rid of it.

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.cc:238: SkRect layer_skrect = SkRect::MakeXYWH(
Use RectToSkRect() in ui/gfx/skia_util.h instead.

[powei, 2013-08-05 23:49:29]

Update scrollbar_layer_unittest and added a check for empty rect in scrollbar
layer (without it mac tests crashes).

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer.cc
File cc/layers/scrollbar_layer.cc (right):

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.cc:234: if (contents_scale_x() != 1.f ||
contents_scale_y() != 1.f)
On 2013/08/03 01:34:23, aelias wrote:
> This if() statement doesn't change any behavior so you can get rid of it.

Done.

https://codereview.chromium.org/21917004/diff/11001/cc/layers/scrollbar_layer...
cc/layers/scrollbar_layer.cc:238: SkRect layer_skrect = SkRect::MakeXYWH(
On 2013/08/03 01:34:23, aelias wrote:
> Use RectToSkRect() in ui/gfx/skia_util.h instead.

Done.

[aelias_OOO_until_Jul13, 2013-08-06 03:28:05]

Looks like your new ScaledResourceUpload test is failing on all the _rel bots.

[powei, 2013-08-06 18:26:49]

On 2013/08/06 03:28:05, aelias wrote:
> Looks like your new ScaledResourceUpload test is failing on all the _rel bots.

Yes, I made a small change and forgot to re-run the tests.   Sorry.  The latest
patch should be good.

[powei, 2013-08-08 17:18:46]

On 2013/08/06 18:26:49, powei wrote:
> On 2013/08/06 03:28:05, aelias wrote:
> > Looks like your new ScaledResourceUpload test is failing on all the _rel
bots.
> 
> Yes, I made a small change and forgot to re-run the tests.   Sorry.  The
latest
> patch should be good.

PTAL.  Corrected resource format for the scrollbar textures and added pixel
swizzling checks in this patch.  

Things I'm not certain about: 
- Is LTHI is the right place for pixel swizzling?
- Is the swizzling condition sufficient for all cases?  (If format is BGRA &&
software rendering).
- Is there swizzling code already in the code base? (ConvertSkiaToRGBA in
skia_util is for decoding PNG, and does not produce the right result for
scrollbar textures due to an "unmultiply by alpha" step.)

[aelias_OOO_until_Jul13, 2013-08-08 19:04:56]

I don't think we should let UIResourceBitmap support more than one swizzling
format.  We should require that the pixels come in with the SkPMColor swizzling
(which is the same as SkBitmaps).

I wrote a summary of the different swizzling formats used in Chrome in the patch
description of https://codereview.chromium.org/14883004/, this may clarify
what's going on.  It shouldn't matter whether the renderer is software or not,
either way bitmap pixels come in as SkPMColor.

If you need to do an explicit swizzling loop, it's unsafe to do it using pure
math because of platform differences in swizzling, the safer way is
SkPreMultiplyColor().  But please try to find a way to structure things pipeline
so that no swizzling is needed.  I would have expected that the entire resource
pipeline would already be in SkPMColor swizzling.  Maybe there is a problem
elsewhere that should be fixed (I'm suspicious of Scrollbar->PaintPart()).

[powei, 2013-08-10 01:45:46]

On 2013/08/08 19:04:56, aelias wrote:
> I don't think we should let UIResourceBitmap support more than one swizzling
> format.  We should require that the pixels come in with the SkPMColor
swizzling
> (which is the same as SkBitmaps).
> 
> I wrote a summary of the different swizzling formats used in Chrome in the
patch
> description of https://codereview.chromium.org/14883004/, this may clarify
> what's going on.  It shouldn't matter whether the renderer is software or not,
> either way bitmap pixels come in as SkPMColor.
> 
> If you need to do an explicit swizzling loop, it's unsafe to do it using pure
> math because of platform differences in swizzling, the safer way is
> SkPreMultiplyColor().  But please try to find a way to structure things
pipeline
> so that no swizzling is needed.  I would have expected that the entire
resource
> pipeline would already be in SkPMColor swizzling.  Maybe there is a problem
> elsewhere that should be fixed (I'm suspicious of Scrollbar->PaintPart()).

PTAL.  This patch uses the best_texture_format() flag in ResourceProvider to
avoid choosing between GL_RGBA or GL_BGRA_EXT.  This is closer to what the old
scrollbar was doing.  

Curiously, the old scrollbar was using TextureDrawQuad, which does not check for
swizzling conditions, and yet seems to draw the right thing on all platforms.  I
think there are two possible reasons for this: 1. best_texture_format() is the
same as SkPMColor for most platforms.  
2. Most scrollbars (in test or not) are drawing gray scrollbars, where using
RGBA or BGRA does not make a difference.

One side point: I was looking at TextureLayerImpl, which is doing direct bit
manipulation to do swizzling.  This is probably unsafe as you said, but I'm
guessing SkPMColor for all tested platforms are either RGBA or BGRA, so turned
out to be ok.  

Here's a doc of my current understanding of pixel formats in CC:
https://docs.google.com/a/google.com/document/d/1gDkKUU-nI0LHvQMsJEjTDc6TJqOJ...

Maybe it'll help illuminate some of these issues for other people down the line.
 Please let me know if you have any comments or corrections.  

Thanks.

[aelias_OOO_until_Jul13, 2013-08-10 02:59:51]

On 2013/08/10 01:45:46, powei wrote:
> Curiously, the old scrollbar was using TextureDrawQuad, which does not check
for
> swizzling conditions, and yet seems to draw the right thing on all platforms. 
I
> think there are two possible reasons for this: 1. best_texture_format() is the
> same as SkPMColor for most platforms.  
> 2. Most scrollbars (in test or not) are drawing gray scrollbars, where using
> RGBA or BGRA does not make a difference.

I think your reason (1) is correct.  Jared observed that using UIResources on
Android with TextureDrawQuad gives correct colors with the blue overscroll
texture and we know Android has a different SkPMColor swizzling order than
desktop.

> 
> One side point: I was looking at TextureLayerImpl, which is doing direct bit
> manipulation to do swizzling.  This is probably unsafe as you said, but I'm
> guessing SkPMColor for all tested platforms are either RGBA or BGRA, so turned
> out to be ok.  

I'm thinking the BGRA extension is probably supported almost everywhere so this
fallback almost is never used in practice.

[aelias_OOO_until_Jul13, 2013-08-10 03:47:27]

lgtm, but please wait until after M30 branch point to land.

On 2013/08/10 02:59:51, aelias wrote:
> On 2013/08/10 01:45:46, powei wrote:
> > Curiously, the old scrollbar was using TextureDrawQuad, which does not check
> for
> > swizzling conditions, and yet seems to draw the right thing on all
platforms. 
> I
> > think there are two possible reasons for this: 1. best_texture_format() is
the
> > same as SkPMColor for most platforms.  
> > 2. Most scrollbars (in test or not) are drawing gray scrollbars, where using
> > RGBA or BGRA does not make a difference.
> 
> I think your reason (1) is correct.  Jared observed that using UIResources on
> Android with TextureDrawQuad gives correct colors with the blue overscroll
> texture and we know Android has a different SkPMColor swizzling order than
> desktop.

Indeed, looking at PlatformColor.h, best-texture-format explicitly comes out of
looking at the Skia #define, and the only time it would be different is if
EXT_BGRA isn't supported (and I strongly suspect it practically always is
supported).  So using best_texture_format() like you're doing here is the right
thing.  See some comments I made on your doc for some further cleanups we could
do in ResourceProvider about this though.

[commit-bot: I haz the power, 2013-08-13 18:31:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/21917004/127001

[commit-bot: I haz the power, 2013-08-13 19:42:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/21917004/127001

[commit-bot: I haz the power, 2013-08-13 19:43:02]

Failed to apply patch for cc/layers/scrollbar_layer_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layers/scrollbar_layer_unittest.cc
  Hunk #1 succeeded at 4 with fuzz 2.
  Hunk #2 FAILED at 17.
  1 out of 10 hunks FAILED -- saving rejects to file
cc/layers/scrollbar_layer_unittest.cc.rej

Patch:       cc/layers/scrollbar_layer_unittest.cc
Index: cc/layers/scrollbar_layer_unittest.cc
diff --git a/cc/layers/scrollbar_layer_unittest.cc
b/cc/layers/scrollbar_layer_unittest.cc
index
0c160c400d9b1fdc72de719fe598db04d5c51ef0..1fd7059771765e20548ac817462bb04464430ff5
100644
--- a/cc/layers/scrollbar_layer_unittest.cc
+++ b/cc/layers/scrollbar_layer_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "cc/layers/scrollbar_layer.h"
 
+#include "base/containers/hash_tables.h"
 #include "cc/animation/scrollbar_animation_controller.h"
 #include "cc/layers/append_quads_data.h"
 #include "cc/layers/scrollbar_layer_impl.h"
@@ -16,10 +17,12 @@
 #include "cc/test/fake_layer_tree_host_client.h"
 #include "cc/test/fake_layer_tree_host_impl.h"
 #include "cc/test/fake_scrollbar.h"
+#include "cc/test/fake_scrollbar_layer.h"
 #include "cc/test/geometry_test_utils.h"
 #include "cc/test/layer_tree_test.h"
 #include "cc/test/mock_quad_culler.h"
 #include "cc/test/test_web_graphics_context_3d.h"
+#include "cc/trees/layer_tree_host.h"
 #include "cc/trees/layer_tree_impl.h"
 #include "cc/trees/single_thread_proxy.h"
 #include "cc/trees/tree_synchronizer.h"
@@ -404,9 +407,48 @@ class MockLayerTreeHost : public LayerTreeHost {
  public:
   MockLayerTreeHost(LayerTreeHostClient* client,
                     const LayerTreeSettings& settings)
-      : LayerTreeHost(client, settings) {
+      : LayerTreeHost(client, settings),
+        next_id_(1),
+        total_ui_resource_created_(0),
+        total_ui_resource_deleted_(0) {
     Initialize(NULL);
   }
+
+  virtual UIResourceId CreateUIResource(UIResourceClient* content) OVERRIDE {
+    total_ui_resource_created_++;
+    UIResourceId nid = next_id_++;
+    ui_resource_bitmap_map_[nid] = content->GetBitmap(nid, false);
+    return nid;
+  }
+
+  // Deletes a UI resource.  May safely be called more than once.
+  virtual void DeleteUIResource(UIResourceId id) OVERRIDE {
+    UIResourceBitmapMap::iterator iter = ui_resource_bitmap_map_.find(id);
+    if (iter != ui_resource_bitmap_map_.end()) {
+      ui_resource_bitmap_map_.erase(iter);
+      total_ui_resource_deleted_++;
+    }
+  }
+
+  size_t UIResourceCount() { return ui_resource_bitmap_map_.size(); }
+  int TotalUIResourceDeleted() { return total_ui_resource_deleted_; }
+  int TotalUIResourceCreated() { return total_ui_resource_created_; }
+
+  gfx::Size ui_resource_size(UIResourceId id) {
+    UIResourceBitmapMap::iterator iter = ui_resource_bitmap_map_.find(id);
+    if (iter != ui_resource_bitmap_map_.end() && iter->second.get())
+      return iter->second->GetSize();
+    return gfx::Size();
+  }
+
+ private:
+  typedef base::hash_map<UIResourceId, scoped_refptr<UIResourceBitmap> >
+      UIResourceBitmapMap;
+  UIResourceBitmapMap ui_resource_bitmap_map_;
+
+  int next_id_;
+  int total_ui_resource_created_;
+  int total_ui_resource_deleted_;
 };
 
 
@@ -415,7 +457,10 @@ class ScrollbarLayerTestResourceCreation : public
testing::Test {
   ScrollbarLayerTestResourceCreation()
       : fake_client_(FakeLayerTreeHostClient::DIRECT_3D) {}
 
-  void TestResourceUpload(size_t expected_resources) {
+  void TestResourceUpload(int num_updates,
+                          size_t expected_resources,
+                          int expected_created,
+                          int expected_deleted) {
     layer_tree_host_.reset(
         new MockLayerTreeHost(&fake_client_, layer_tree_settings_));
 
@@ -423,7 +468,7 @@ class ScrollbarLayerTestResourceCreation : public
testing::Test {
     scoped_refptr<Layer> layer_tree_root = Layer::Create();
     scoped_refptr<Layer> content_layer = Layer::Create();
     scoped_refptr<Layer> scrollbar_layer =
-        ScrollbarLayer::Create(scrollbar.Pass(), layer_tree_root->id());
+      ScrollbarLayer::Create(scrollbar.Pass(), layer_tree_root->id());
     layer_tree_root->AddChild(content_layer);
     layer_tree_root->AddChild(scrollbar_layer);
 
@@ -447,16 +492,17 @@ class ScrollbarLayerTestResourceCreation : public
testing::Test {
     testing::Mock::VerifyAndClearExpectations(layer_tree_host_.get());
     EXPECT_EQ(scrollbar_layer->layer_tree_host(), layer_tree_host_.get());
 
-    PriorityCalculator calculator;
     ResourceUpdateQueue queue;
     OcclusionTracker occlusion_tracker(gfx::Rect(), false);
 
     scrollbar_layer->SavePaintProperties();
-    scrollbar_layer->SetTexturePriorities(calculator);
-    layer_tree_host_->contents_texture_manager()->PrioritizeTextures();
-    scrollbar_layer->Update(&queue, &occlusion_tracker);
-    EXPECT_EQ(0u, queue.FullUploadSize());
-    EXPECT_EQ(expected_resources, queue.PartialUploadSize());
+    for (int update_counter = 0; update_counter < num_updates;
update_counter++)
+      scrollbar_layer->Update(&queue, &occlusion_tracker);
+
+    // A non-solid-color scrollbar should have requested two textures.
+    EXPECT_EQ(expected_resources, layer_tree_host_->UIResourceCount());
+    EXPECT_EQ(expected_created, layer_tree_host_->TotalUIResourceCreated());
+    EXPECT_EQ(expected_deleted, layer_tree_host_->TotalUIResourceDeleted());
 
     testing::Mock::VerifyAndClearExpectations(layer_tree_host_.get());
 
@@ -471,12 +517,18 @@ class ScrollbarLayerTestResourceCreation : public
testing::Test {
 
 TEST_F(ScrollbarLayerTestResourceCreation, ResourceUpload) {
   layer_tree_settings_.solid_color_scrollbars = false;
-  TestResourceUpload(2);
+  TestResourceUpload(0, 0, 0, 0);
+  int num_updates[3] = {1, 5, 10};
+  for (int j = 0; j < 3; j++) {
+    TestResourceUpload(
+        num_updates[j], 2, num_updates[j] * 2, (num_updates[j] - 1) * 2);
+  }
 }
 
 TEST_F(ScrollbarLayerTestResourceCreation, SolidColorNoResourceUpload) {
   layer_tree_settings_.solid_color_scrollbars = true;
-  TestResourceUpload(0);
+  TestResourceUpload(0, 0, 0, 0);
+  TestResourceUpload(1, 0, 0, 0);
 }
 
 class ScaledScrollbarLayerTestResourceCreation : public testing::Test {
@@ -484,19 +536,16 @@ class ScaledScrollbarLayerTestResourceCreation : public
testing::Test {
   ScaledScrollbarLayerTestResourceCreation()
       : fake_client_(FakeLayerTreeHostClient::DIRECT_3D) {}
 
-  void TestResourceUpload(size_t expected_resources, const float test_scale) {
+  void TestResourceUpload(const float test_scale) {
     layer_tree_host_.reset(
         new MockLayerTreeHost(&fake_client_, layer_tree_settings_));
 
     gfx::Point scrollbar_location(0, 185);
-    scoped_ptr<FakeScrollbar> scrollbar(new FakeScrollbar(false, true, false));
-    scrollbar->set_location(scrollbar_location);
-
     scoped_refptr<Layer> layer_tree_root = Layer::Create();
     scoped_refptr<Layer> content_layer = Layer::Create();
-    scoped_refptr<Layer> scrollbar_layer =
-        ScrollbarLayer::Create(scrollbar.PassAs<cc::Scrollbar>(),
-                               layer_tree_root->id());
+    scoped_refptr<FakeScrollbarLayer> scrollbar_layer =
+        FakeScrollbarLayer::Create(false, true, layer_tree_root->id());
+
     layer_tree_root->AddChild(content_layer);
     layer_tree_root->AddChild(scrollbar_layer);
 
@@ -529,30 +578,23 @@ class ScaledScrollbarLayerTestResourceCreation : public
testing::Test {
     testing::Mock::VerifyAndClearExpectations(layer_tree_host_.get());
     EXPECT_EQ(scrollbar_layer->layer_tree_host(), layer_tree_host_.get());
 
-    PriorityCalculator calculator;
     ResourceUpdateQueue queue;
     OcclusionTracker occlusion_tracker(gfx::Rect(), false);
-
     scrollbar_layer->SavePaintProperties();
-    scrollbar_layer->SetTexturePriorities(calculator);
-    layer_tree_host_->contents_texture_manager()->PrioritizeTextures();
     scrollbar_layer->Update(&queue, &occlusion_tracker);
-    EXPECT_EQ(expected_resources, queue.PartialUploadSize());
 
     // Verify that we have not generated any content uploads that are larger
     // than their destination textures.
-    while (queue.HasMoreUpdates()) {
-      ResourceUpdate update = queue.TakeFirstPartialUpload();
-      EXPECT_LE(update.texture->size().width(),
-                scrollbar_layer->content_bounds().width());
-      EXPECT_LE(update.texture->size().height(),
-                scrollbar_layer->content_bounds().height());
-
-      EXPECT_LE(update.dest_offset.x() + update.content_rect.width(),
-                update.texture->size().width());
-      EXPECT_LE(update.dest_offset.y() + update.content_rect.height(),
-                update.texture->size().height());
-    }
+
+    gfx::Size track_size = layer_tree_host_->ui_resource_size(
+        scrollbar_layer->track_resource_id());
+    gfx::Size thumb_size = layer_tree_host_->ui_resource_size(
+        scrollbar_layer->thumb_resource_id());
+
+    EXPECT_LE(track_size.width(), scrollbar_layer->content_bounds().width());
+    EXPECT_LE(track_size.height(), scrollbar_layer->content_bounds().height());
+    EXPECT_LE(thumb_size.width(), scrollbar_layer->content_bounds().width());
+    EXPECT_LE(thumb_size.height(), scrollbar_layer->content_bounds().height());
 
     testing::Mock::VerifyAndClearExpectations(layer_tree_host_.get());
 
@@ -569,7 +611,9 @@ TEST_F(ScaledScrollbarLayerTestResourceCreation,
ScaledResourceUpload) {
   layer_tree_settings_.solid_color_scrollbars = false;
   // Pick a test scale that moves the scrollbar's (non-zero) position to
   // a non-pixel-aligned location.
-  TestResourceUpload(2, 1.41f);
+  TestResourceUpload(.041f);
+  TestResourceUpload(1.41f);
+  TestResourceUpload(4.1f);
 }
 
 }  // namespace

[commit-bot: I haz the power, 2013-08-13 22:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/powei@chromium.org/21917004/154001

[commit-bot: I haz the power, 2013-08-14 02:19:02]

Change committed as 217446