cc: Add tiling raster tile iterators.

cc/resources/picture_layer_tiling_unittest.cc

Index: cc/resources/picture_layer_tiling_unittest.cc
diff --git a/cc/resources/picture_layer_tiling_unittest.cc b/cc/resources/picture_layer_tiling_unittest.cc
index 5fe1acd1031210a53e1fd61c1f14708e1b35ace2..71347265aa9b3250ca95149c6029482a608e6c04 100644
--- a/cc/resources/picture_layer_tiling_unittest.cc
+++ b/cc/resources/picture_layer_tiling_unittest.cc
@@ -5,10 +5,14 @@
 #include "cc/resources/picture_layer_tiling.h"
 
 #include <limits>
+#include <set>
 
 #include "cc/base/math_util.h"
 #include "cc/resources/picture_layer_tiling_set.h"
+#include "cc/test/fake_output_surface.h"
+#include "cc/test/fake_output_surface_client.h"
 #include "cc/test/fake_picture_layer_tiling_client.h"
+#include "cc/test/test_context_provider.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/rect_conversions.h"
 #include "ui/gfx/size_conversions.h"
@@ -492,16 +496,22 @@ TEST(PictureLayerTilingTest, ViewportDistanceWithScale) {
       TilePriority priority = tile->priority(ACTIVE_TREE);
 
       if (viewport_in_content_space.Intersects(tile->content_rect())) {
-        EXPECT_EQ(TilePriority::NOW, priority.priority_bin);
-        EXPECT_FLOAT_EQ(0.f, priority.distance_to_visible);
+        EXPECT_EQ(TilePriority::NOW, priority.priority_bin) << "i: " << i
+                                                            << " j: " << j;
+        EXPECT_FLOAT_EQ(0.f, priority.distance_to_visible) << "i: " << i
+                                                           << " j: " << j;
         have_now = true;
       } else if (skewport.Intersects(tile->content_rect())) {
-        EXPECT_EQ(TilePriority::SOON, priority.priority_bin);
-        EXPECT_GT(priority.distance_to_visible, 0.f);
+        EXPECT_EQ(TilePriority::SOON, priority.priority_bin) << "i: " << i
+                                                             << " j: " << j;
+        EXPECT_GT(priority.distance_to_visible, 0.f) << "i: " << i
+                                                     << " j: " << j;
         have_soon = true;
       } else {
-        EXPECT_EQ(TilePriority::EVENTUALLY, priority.priority_bin);
-        EXPECT_GT(priority.distance_to_visible, 0.f);
+        EXPECT_EQ(TilePriority::EVENTUALLY, priority.priority_bin)
+            << "i: " << i << " j: " << j;
+        EXPECT_GT(priority.distance_to_visible, 0.f) << "i: " << i
+                                                     << " j: " << j;
         have_eventually = true;
       }
     }
@@ -751,6 +761,150 @@ TEST(PictureLayerTilingTest, EmptyStartingRect) {
   EXPECT_TRUE(out.IsEmpty());
 }
 
+TEST(PictureLayerTilingTest, TilingRasterTileIteratorStaticViewport) {
+  FakePictureLayerTilingClient client;
+  scoped_ptr<TestablePictureLayerTiling> tiling;
+
+  gfx::Rect viewport(50, 50, 100, 100);
+  gfx::Size layer_bounds(200, 200);
+
+  client.SetTileSize(gfx::Size(10, 10));
+
+  tiling = TestablePictureLayerTiling::Create(0.25f, layer_bounds, &client);
+  tiling->UpdateTilePriorities(ACTIVE_TREE, viewport, 1.f, 1.0);
+
+  PictureLayerTiling::TilingRasterTileIterator empty_iterator;
+  EXPECT_FALSE(empty_iterator);
+
+  std::vector<Tile*> all_tiles = tiling->AllTilesForTesting();
+
+  // Sanity check.
+  EXPECT_EQ(36u, all_tiles.size());
+
+  for (int i = 0; i < 3; ++i) {

[epennerAtGoogle, 2014/03/05 00:05:50]

Might help to say why there is three passes.

[vmpstr, 2014/03/05 00:37:21]

I'll add a comment.

+    PictureLayerTiling::TilingRasterTileIterator it(tiling.get());
+
+    // There are 3 bins in TilePriority.

[epennerAtGoogle, 2014/03/05 00:05:50]

Can we add a TilePriority::NUM_PRIORITIES, or is that annoying for 'no default'
switch statements? Even if it was an constant (beside the enum) it would be
nicer IMO.

[vmpstr, 2014/03/05 00:37:21]

There was a bit of a discussion a while back on this
(https://codereview.chromium.org/22875045/). Without re-reading all of the
comments, I don't think we really agreed on any particular style or variable
name. 

In that spirit, and because this is only used in unittests (now and in
foreseeable future), I can define a constant right here for the size of the
enums. Do you think that would be an acceptable compromise? 

+    bool have_tiles[3] = {};
+
+    EXPECT_TRUE(it);
+    std::set<Tile*> unique_tiles;
+    unique_tiles.insert(*it);
+    Tile* last_tile = *it;
+    have_tiles[last_tile->priority(ACTIVE_TREE).priority_bin] = true;
+
+    // On the second iteration, mark everything as ready to draw (solid color).

[epennerAtGoogle, 2014/03/05 00:05:50]

It wasn't super clear to me what impact this should have on this test or how
that is tested (since no asserts have i==1).

[vmpstr, 2014/03/05 00:37:21]

I agree that this part is a bit confusing (and this goes along with your comment
that we don't filter tiles that are already rasterized as well):

The final stage of this iterator would be to only return tiles that need to be
rasterized. Initially, the third pass through the tiles just said
"EXPECT_FALSE(it)" right at the start, since everything is marked as ready to
draw on the second pass. However, because of the usage here (to update tile
priorities), it returns all tiles.

There are a couple of options:
1. Wait until I have a chance to write up more follow-up patches that will
eventually make the transition from all tiles to only raster-required tiles
(which would also update this test and remove the use in update tile priorities
in favour of more laziness)
2. Change this now so that update tile priorities uses the previous method of
updating the priorities and raster tile iterator goes back to being used only in
unittests.

I think I actually like option #2, since that would make this patch landable
without regression (well, setting three more rects will regress, but not
noticeably). 

+    if (i == 1) {
+      ManagedTileState::TileVersion& tile_version =
+          last_tile->GetTileVersionForTesting(
+              last_tile->DetermineRasterModeForTree(ACTIVE_TREE));
+      tile_version.SetSolidColorForTesting(SK_ColorRED);
+    }
+    ++it;
+    int eventually_bin_order_correct_count = 0;
+    int eventually_bin_order_incorrect_count = 0;
+    while (it) {
+      Tile* new_tile = *it;
+      ++it;
+      unique_tiles.insert(new_tile);

[epennerAtGoogle, 2014/03/05 00:05:50]

Should this check that the tile is not already in unique_tiles? I think std::Set
doesn't  mind multiple insertions.

[vmpstr, 2014/03/05 00:37:21]

This goes with the check on line 848 that ensures that unique tiles size is the
same as all tiles size. If there were duplicates, then unique tiles size would
be less.

+
+      TilePriority last_priority = last_tile->priority(ACTIVE_TREE);
+      TilePriority new_priority = new_tile->priority(ACTIVE_TREE);
+      EXPECT_LE(last_priority.priority_bin, new_priority.priority_bin);
+      if (last_priority.priority_bin == new_priority.priority_bin) {
+        if (last_priority.priority_bin == TilePriority::EVENTUALLY) {
+          bool order_correct = last_priority.distance_to_visible <=
+                               new_priority.distance_to_visible;
+          eventually_bin_order_correct_count += order_correct;
+          eventually_bin_order_incorrect_count += !order_correct;
+        } else {
+          EXPECT_LE(last_priority.distance_to_visible,
+                    new_priority.distance_to_visible);
+        }
+      }
+      have_tiles[new_priority.priority_bin] = true;
+
+      last_tile = new_tile;
+
+      // On the second iteration, mark everything as ready to draw (solid
+      // color).
+      if (i == 1) {
+        ManagedTileState::TileVersion& tile_version =
+            last_tile->GetTileVersionForTesting(
+                last_tile->DetermineRasterModeForTree(ACTIVE_TREE));
+        tile_version.SetSolidColorForTesting(SK_ColorRED);
+      }
+    }
+
+    EXPECT_GT(eventually_bin_order_correct_count,
+              eventually_bin_order_incorrect_count);

[epennerAtGoogle, 2014/03/05 00:05:50]

I've looked at the above code but this is still confusing me. Part of it is just
the naming (Two 'rights' don't fix two 'wrongs', so it could probably use a
better name). More importantly though I don't see why these distance comparisons
should balance one another in the EVENTUALLY bin. Maybe just a comment will
clear this up.

[vmpstr, 2014/03/05 00:37:21]

This part is a consequence of the spiral iterator. In a square spiral, the
corner tiles will be further away than the edge tiles relative to the rect that
is being spiraled. This means that we technically would return wrong order tiles
(ie, corners before some of the edges). This is partly because we use manhattan
distance, and partly because having a spiral iteration that considers distance
would be fairly complicated (that last bit is just my gut feeling). 

Personally, I feel that is an acceptable limitation. That is, we're still
perfect in the visible rect and in a single direction skewport. In the
eventually rect, we're "OK" as in we're mostly correct.

Anyway, the end result is that this is hard to test. This line just says that
for the eventually bin we're more right than wrong. Suggestions for something
better are more than welcome.

+
+    // We should have now and eventually tiles, but not soon tiles because the
+    // viewport is static.
+    EXPECT_TRUE(have_tiles[TilePriority::NOW]);
+    EXPECT_FALSE(have_tiles[TilePriority::SOON]);
+    EXPECT_TRUE(have_tiles[TilePriority::EVENTUALLY]);
+
+    EXPECT_EQ(unique_tiles.size(), all_tiles.size());
+    tiling->UpdateTilePriorities(ACTIVE_TREE, viewport, 1.f, i + 2);
+  }
+}
+
+TEST(PictureLayerTilingTest, TilingRasterTileIteratorMovingViewport) {
+  FakePictureLayerTilingClient client;
+  scoped_ptr<TestablePictureLayerTiling> tiling;
+
+  gfx::Rect viewport(50, 0, 100, 100);
+  gfx::Size layer_bounds(500, 500);
+
+  client.SetTileSize(gfx::Size(30, 30));
+
+  tiling = TestablePictureLayerTiling::Create(1.f, layer_bounds, &client);
+  tiling->UpdateTilePriorities(ACTIVE_TREE, viewport, 1.f, 1.0);
+
+  viewport = gfx::Rect(50, 50, 100, 100);
+  tiling->UpdateTilePriorities(ACTIVE_TREE, viewport, 1.f, 1.1);
+
+  // There are 3 bins in TilePriority.

[epennerAtGoogle, 2014/03/05 00:05:50]

Same here.

+  bool have_tiles[3] = {};
+  Tile* last_tile = NULL;
+  int eventually_bin_order_correct_count = 0;
+  int eventually_bin_order_incorrect_count = 0;
+  for (PictureLayerTiling::TilingRasterTileIterator it(tiling.get()); it;
+       ++it) {
+    if (!last_tile)
+      last_tile = *it;
+
+    Tile* new_tile = *it;
+
+    TilePriority last_priority = last_tile->priority(ACTIVE_TREE);
+    TilePriority new_priority = new_tile->priority(ACTIVE_TREE);
+
+    have_tiles[new_priority.priority_bin] = true;
+
+    EXPECT_LE(last_priority.priority_bin, new_priority.priority_bin);
+    if (last_priority.priority_bin == new_priority.priority_bin) {
+      if (last_priority.priority_bin == TilePriority::EVENTUALLY) {
+        bool order_correct = last_priority.distance_to_visible <=
+                             new_priority.distance_to_visible;
+        eventually_bin_order_correct_count += order_correct;
+        eventually_bin_order_incorrect_count += !order_correct;
+      } else {
+        EXPECT_LE(last_priority.distance_to_visible,
+                  new_priority.distance_to_visible);
+      }
+    }
+    last_tile = new_tile;
+  }
+
+  EXPECT_GT(eventually_bin_order_correct_count,
+            eventually_bin_order_incorrect_count);
+
+  EXPECT_TRUE(have_tiles[TilePriority::NOW]);
+  EXPECT_TRUE(have_tiles[TilePriority::SOON]);
+  EXPECT_TRUE(have_tiles[TilePriority::EVENTUALLY]);
+}
+
 static void TileExists(bool exists, Tile* tile,
                        const gfx::Rect& geometry_rect) {
   EXPECT_EQ(exists, tile != NULL) << geometry_rect.ToString();