cc: Remove tile sharing from tilings.

cc/resources/tiling_set_raster_queue_all.cc

Index: cc/resources/tiling_set_raster_queue_all.cc
diff --git a/cc/resources/tiling_set_raster_queue_all.cc b/cc/resources/tiling_set_raster_queue_all.cc
index 2ccba67eeb31e637ff39644507d23e1c0bf83d31..6d7dc581e5bf5cd97fa36dbddc068c5d11347c37 100644
--- a/cc/resources/tiling_set_raster_queue_all.cc
+++ b/cc/resources/tiling_set_raster_queue_all.cc
@@ -173,11 +173,33 @@ TilingSetRasterQueueAll::VisibleTilingIterator&
   return *this;
 }
 
+// PendingVisibleTilingIterator.
+TilingSetRasterQueueAll::PendingVisibleTilingIterator::
+    PendingVisibleTilingIterator(PictureLayerTiling* tiling,
+                                 TilingData* tiling_data)
+    : OnePriorityRectIterator(tiling, tiling_data) {
+  iterator_ = TilingData::DifferenceIterator(tiling_data_,
+                                             tiling_->pending_visible_rect(),

[enne (OOO), 2015/04/09 17:54:24]

This is still a little brain-breaking to me.  PendingVisibleTilingIterator is
about iterating through active tree tilings with the pending visible rect. 
However, it's still called on the pending tree, which happens to have two
identical rects here, so it eventually is a noop?  O_O

Can this be more obvious? If you have an iterator that only goes on one tree's
tilings, maybe just ask if the tiling is on the pending or active tree and skip
it if it's on the wrong tree?

What about calling this RequiredForActivationActiveTilingIterator? That way
there's no Pending in the name, so it's clear that it's on the active tree? 
That name is hella unwieldy though, but it's less confusing to me.

[vmpstr, 2015/04/10 20:25:14]

Yep. The way I am meaning to write this is that a "PendingVisibleRect" is
another rect in our set of priority rects (along side of skewport, soon border
rect, visible rect). Perhaps the name is just not ideal. But given that it's
just a new rect called Foo, then both pending and active trees have a rect
called Foo. It just happens that in the pending tree case Foo == visible rect.
So this would be FooTilingIterator and would be a noop on the pending tree, and
do something on the active tree. 

I'm reluctant to start adding if (we're on one tree or the other) type of logic
here, since it can quickly become more confusing.

So I guess my question is: is there a name for Foo that would make this easier
to understand, or is your concern with the fact that this new region is only
"unique" on one of the trees?

[enne (OOO), 2015/04/10 21:16:28]

My concern is kind of both? Maybe a comment here or on the enum about when this
iterator does something real and when it subtly does nothing would help.

[vmpstr, 2015/04/10 22:24:29]

I've added a comment (next to the enum in the header). Let me know if you know
of a good name for this new region and I'll change it throughout. 

In another attempt for defense here, we have a similar issue with other regions
like the skewport. It overlaps the visible rect, and sometimes it's exactly the
same as the visible rect. However, we don't have explicit logic here to handle
the case when they are the same. It just helps not to think about it, but verify
that it does the right thing where the regions are the same. (as an aside, the
pending visible rect will also be the same as the current visible rect in some
cases on the active tree).

It is a different case, because on the pending tree, in this implementation, the
pending visible rect will always be the same as the visible rect. Adding an if
statement though  kind of accomplishes a task of a comment by explicitly saying
"we know that on the pending tree they are the same, skip the case" instead of a
comment that says "this is a noop on the pending tree". I don't know if putting
an if-statement in this case is something that I like.

+                                             tiling_->current_visible_rect());
+  if (!iterator_)
+    return;
+  if (!GetFirstTileAndCheckIfValid(&iterator_))
+    ++(*this);
+}
+
+TilingSetRasterQueueAll::PendingVisibleTilingIterator&
+    TilingSetRasterQueueAll::PendingVisibleTilingIterator::
+    operator++() {
+  AdvanceToNextTile(&iterator_);
+  return *this;
+}
+
 // SkewportTilingIterator.
 TilingSetRasterQueueAll::SkewportTilingIterator::SkewportTilingIterator(
     PictureLayerTiling* tiling,
     TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data) {
+    : OnePriorityRectIterator(tiling, tiling_data),
+      pending_visible_rect_(tiling->pending_visible_rect()) {
   if (!tiling_->has_skewport_rect_tiles())
     return;
   iterator_ = TilingData::SpiralDifferenceIterator(
@@ -185,7 +207,11 @@ TilingSetRasterQueueAll::SkewportTilingIterator::SkewportTilingIterator(
       tiling_->current_visible_rect(), tiling_->current_visible_rect());
   if (!iterator_)
     return;
-  if (!GetFirstTileAndCheckIfValid(&iterator_))
+  if (!GetFirstTileAndCheckIfValid(&iterator_)) {
+    ++(*this);
+    return;
+  }
+  if (tile_->content_rect().Intersects(pending_visible_rect_))
     ++(*this);
 }
 
@@ -193,6 +219,11 @@ TilingSetRasterQueueAll::SkewportTilingIterator&
     TilingSetRasterQueueAll::SkewportTilingIterator::
     operator++() {
   AdvanceToNextTile(&iterator_);
+  while (!done()) {
+    if (!tile_->content_rect().Intersects(pending_visible_rect_))
+      break;
+    AdvanceToNextTile(&iterator_);
+  }
   return *this;
 }
 
@@ -200,7 +231,8 @@ TilingSetRasterQueueAll::SkewportTilingIterator&
 TilingSetRasterQueueAll::SoonBorderTilingIterator::SoonBorderTilingIterator(
     PictureLayerTiling* tiling,
     TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data) {
+    : OnePriorityRectIterator(tiling, tiling_data),
+      pending_visible_rect_(tiling->pending_visible_rect()) {
   if (!tiling_->has_soon_border_rect_tiles())
     return;
   iterator_ = TilingData::SpiralDifferenceIterator(
@@ -208,7 +240,11 @@ TilingSetRasterQueueAll::SoonBorderTilingIterator::SoonBorderTilingIterator(
       tiling_->current_skewport_rect(), tiling_->current_visible_rect());
   if (!iterator_)
     return;
-  if (!GetFirstTileAndCheckIfValid(&iterator_))
+  if (!GetFirstTileAndCheckIfValid(&iterator_)) {
+    ++(*this);
+    return;
+  }
+  if (tile_->content_rect().Intersects(pending_visible_rect_))
     ++(*this);
 }
 
@@ -216,6 +252,11 @@ TilingSetRasterQueueAll::SoonBorderTilingIterator&
     TilingSetRasterQueueAll::SoonBorderTilingIterator::
     operator++() {
   AdvanceToNextTile(&iterator_);
+  while (!done()) {
+    if (!tile_->content_rect().Intersects(pending_visible_rect_))
+      break;
+    AdvanceToNextTile(&iterator_);
+  }
   return *this;
 }
 
@@ -223,7 +264,8 @@ TilingSetRasterQueueAll::SoonBorderTilingIterator&
 TilingSetRasterQueueAll::EventuallyTilingIterator::EventuallyTilingIterator(
     PictureLayerTiling* tiling,
     TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data) {
+    : OnePriorityRectIterator(tiling, tiling_data),
+      pending_visible_rect_(tiling->pending_visible_rect()) {
   if (!tiling_->has_eventually_rect_tiles())
     return;
   iterator_ = TilingData::SpiralDifferenceIterator(
@@ -231,7 +273,11 @@ TilingSetRasterQueueAll::EventuallyTilingIterator::EventuallyTilingIterator(
       tiling_->current_skewport_rect(), tiling_->current_soon_border_rect());
   if (!iterator_)
     return;
-  if (!GetFirstTileAndCheckIfValid(&iterator_))
+  if (!GetFirstTileAndCheckIfValid(&iterator_)) {
+    ++(*this);
+    return;
+  }
+  if (tile_->content_rect().Intersects(pending_visible_rect_))
     ++(*this);
 }
 
@@ -239,6 +285,11 @@ TilingSetRasterQueueAll::EventuallyTilingIterator&
     TilingSetRasterQueueAll::EventuallyTilingIterator::
     operator++() {
   AdvanceToNextTile(&iterator_);
+  while (!done()) {
+    if (!tile_->content_rect().Intersects(pending_visible_rect_))
+      break;
+    AdvanceToNextTile(&iterator_);
+  }
   return *this;
 }
 
@@ -262,6 +313,9 @@ TilingSetRasterQueueAll::TilingIterator::TilingIterator(
   current_tile_ = *visible_iterator_;
 }
 
+TilingSetRasterQueueAll::TilingIterator::~TilingIterator() {
+}
+
 void TilingSetRasterQueueAll::TilingIterator::AdvancePhase() {
   DCHECK_LT(phase_, EVENTUALLY_RECT);
 
@@ -272,6 +326,12 @@ void TilingSetRasterQueueAll::TilingIterator::AdvancePhase() {
       case VISIBLE_RECT:
         NOTREACHED();
         return;
+      case PENDING_VISIBLE_RECT:
+        pending_visible_iterator_ =
+            PendingVisibleTilingIterator(tiling_, tiling_data_);
+        if (!pending_visible_iterator_.done())
+          current_tile_ = *pending_visible_iterator_;
+        break;
       case SKEWPORT_RECT:
         skewport_iterator_ = SkewportTilingIterator(tiling_, tiling_data_);
         if (!skewport_iterator_.done())
@@ -303,6 +363,14 @@ TilingSetRasterQueueAll::TilingIterator&
       }
       current_tile_ = *visible_iterator_;
       break;
+    case PENDING_VISIBLE_RECT:
+      ++pending_visible_iterator_;
+      if (pending_visible_iterator_.done()) {
+        AdvancePhase();
+        return *this;
+      }
+      current_tile_ = *pending_visible_iterator_;
+      break;
     case SKEWPORT_RECT:
       ++skewport_iterator_;
       if (skewport_iterator_.done()) {