cc: Pass priority rect information from iterators to 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 6f212152a98cfcfb3caebe1951cc4ff1a77f1158..a2ecffde7ee76da3d957b1542af8afc7b7e7f36a 100644
--- a/cc/resources/tiling_set_raster_queue_all.cc
+++ b/cc/resources/tiling_set_raster_queue_all.cc
@@ -153,28 +153,38 @@ void TilingSetRasterQueueAll::AdvanceToNextStage() {
 
 // OnePriorityRectIterator
 TilingSetRasterQueueAll::OnePriorityRectIterator::OnePriorityRectIterator()
-    : tile_(nullptr), tiling_(nullptr), tiling_data_(nullptr) {
+    : tile_(nullptr),
+      tiling_(nullptr),
+      pending_twin_tiling_(nullptr),
+      tiling_data_(nullptr) {
 }
 
 TilingSetRasterQueueAll::OnePriorityRectIterator::OnePriorityRectIterator(
     PictureLayerTiling* tiling,
-    TilingData* tiling_data)
-    : tile_(nullptr), tiling_(tiling), tiling_data_(tiling_data) {
+    TilingData* tiling_data,
+    PictureLayerTiling::PriorityRect priority_rect)
+    : tile_(nullptr),
+      tiling_(tiling),
+      pending_twin_tiling_(

[vmpstr, 2015/05/05 18:04:02]

Why are you adding this here? Why is getting tiling->pending_visible_rect not
sufficient? It should return the same value that you need if this is an active
tree, and it would return empty rect if this is the pending tree.

[USE eero AT chromium.org, 2015/05/06 12:05:20]

As a premature (size) optimisation. I removed it.

+          tiling->tree() == ACTIVE_TREE
+              ? tiling->client()->GetPendingOrActiveTwinTiling(tiling)
+              : nullptr),
+      tiling_data_(tiling_data),
+      priority_rect_(priority_rect) {
 }
 
 template <typename TilingIteratorType>
 void TilingSetRasterQueueAll::OnePriorityRectIterator::AdvanceToNextTile(
     TilingIteratorType* iterator) {
-  tile_ = nullptr;
-  while (!tile_ || !TileNeedsRaster(tile_)) {
+  bool found_tile = false;
+  while (!found_tile) {
     ++(*iterator);
     if (!(*iterator)) {
       tile_ = nullptr;
-      return;
+      break;
     }
-    tile_ = tiling_->TileAt(iterator->index_x(), iterator->index_y());
+    found_tile = GetFirstTileAndCheckIfValid(iterator);
   }
-  tiling_->UpdateTilePriority(tile_);
 }
 
 template <typename TilingIteratorType>
@@ -185,7 +195,13 @@ bool TilingSetRasterQueueAll::OnePriorityRectIterator::
     tile_ = nullptr;
     return false;
   }
-  tiling_->UpdateTilePriority(tile_);
+  if (priority_rect_ > PictureLayerTiling::PriorityRect::PENDING_VISIBLE_RECT &&
+      pending_twin_tiling_ &&
+      pending_twin_tiling_->current_visible_rect().Intersects(
+          tile_->content_rect())) {
+    return false;
+  }
+  tiling_->UpdateTilePriority(tile_, priority_rect_);
   return true;
 }
 
@@ -193,7 +209,9 @@ bool TilingSetRasterQueueAll::OnePriorityRectIterator::
 TilingSetRasterQueueAll::VisibleTilingIterator::VisibleTilingIterator(
     PictureLayerTiling* tiling,
     TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data) {
+    : OnePriorityRectIterator(tiling,
+                              tiling_data,
+                              PictureLayerTiling::VISIBLE_RECT) {
   if (!tiling_->has_visible_rect_tiles())
     return;
   iterator_ =
@@ -216,10 +234,14 @@ TilingSetRasterQueueAll::VisibleTilingIterator&
 TilingSetRasterQueueAll::PendingVisibleTilingIterator::
     PendingVisibleTilingIterator(PictureLayerTiling* tiling,
                                  TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data) {
-  iterator_ = TilingData::DifferenceIterator(tiling_data_,
-                                             tiling_->pending_visible_rect(),
-                                             tiling_->current_visible_rect());
+    : OnePriorityRectIterator(tiling,
+                              tiling_data,
+                              PictureLayerTiling::PENDING_VISIBLE_RECT) {
+  if (!pending_twin_tiling_ || !pending_twin_tiling_->has_visible_rect_tiles())
+    return;
+  iterator_ = TilingData::DifferenceIterator(
+      tiling_data_, pending_twin_tiling_->current_visible_rect(),
+      tiling_->current_visible_rect());
   if (!iterator_)
     return;
   if (!GetFirstTileAndCheckIfValid(&iterator_))
@@ -237,8 +259,9 @@ TilingSetRasterQueueAll::PendingVisibleTilingIterator&
 TilingSetRasterQueueAll::SkewportTilingIterator::SkewportTilingIterator(
     PictureLayerTiling* tiling,
     TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data),
-      pending_visible_rect_(tiling->pending_visible_rect()) {
+    : OnePriorityRectIterator(tiling,
+                              tiling_data,
+                              PictureLayerTiling::SKEWPORT_RECT) {
   if (!tiling_->has_skewport_rect_tiles())
     return;
   iterator_ = TilingData::SpiralDifferenceIterator(
@@ -246,11 +269,7 @@ TilingSetRasterQueueAll::SkewportTilingIterator::SkewportTilingIterator(
       tiling_->current_visible_rect(), tiling_->current_visible_rect());
   if (!iterator_)
     return;
-  if (!GetFirstTileAndCheckIfValid(&iterator_)) {
-    ++(*this);
-    return;
-  }
-  if (tile_->content_rect().Intersects(pending_visible_rect_))
+  if (!GetFirstTileAndCheckIfValid(&iterator_))
     ++(*this);
 }
 
@@ -258,11 +277,6 @@ TilingSetRasterQueueAll::SkewportTilingIterator&
     TilingSetRasterQueueAll::SkewportTilingIterator::
     operator++() {
   AdvanceToNextTile(&iterator_);
-  while (!done()) {
-    if (!tile_->content_rect().Intersects(pending_visible_rect_))
-      break;
-    AdvanceToNextTile(&iterator_);
-  }
   return *this;
 }
 
@@ -270,8 +284,9 @@ TilingSetRasterQueueAll::SkewportTilingIterator&
 TilingSetRasterQueueAll::SoonBorderTilingIterator::SoonBorderTilingIterator(
     PictureLayerTiling* tiling,
     TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data),
-      pending_visible_rect_(tiling->pending_visible_rect()) {
+    : OnePriorityRectIterator(tiling,
+                              tiling_data,
+                              PictureLayerTiling::SOON_BORDER_RECT) {
   if (!tiling_->has_soon_border_rect_tiles())
     return;
   iterator_ = TilingData::SpiralDifferenceIterator(
@@ -279,11 +294,7 @@ TilingSetRasterQueueAll::SoonBorderTilingIterator::SoonBorderTilingIterator(
       tiling_->current_skewport_rect(), tiling_->current_visible_rect());
   if (!iterator_)
     return;
-  if (!GetFirstTileAndCheckIfValid(&iterator_)) {
-    ++(*this);
-    return;
-  }
-  if (tile_->content_rect().Intersects(pending_visible_rect_))
+  if (!GetFirstTileAndCheckIfValid(&iterator_))
     ++(*this);
 }
 
@@ -291,11 +302,6 @@ TilingSetRasterQueueAll::SoonBorderTilingIterator&
     TilingSetRasterQueueAll::SoonBorderTilingIterator::
     operator++() {
   AdvanceToNextTile(&iterator_);
-  while (!done()) {
-    if (!tile_->content_rect().Intersects(pending_visible_rect_))
-      break;
-    AdvanceToNextTile(&iterator_);
-  }
   return *this;
 }
 
@@ -303,8 +309,9 @@ TilingSetRasterQueueAll::SoonBorderTilingIterator&
 TilingSetRasterQueueAll::EventuallyTilingIterator::EventuallyTilingIterator(
     PictureLayerTiling* tiling,
     TilingData* tiling_data)
-    : OnePriorityRectIterator(tiling, tiling_data),
-      pending_visible_rect_(tiling->pending_visible_rect()) {
+    : OnePriorityRectIterator(tiling,
+                              tiling_data,
+                              PictureLayerTiling::EVENTUALLY_RECT) {
   if (!tiling_->has_eventually_rect_tiles())
     return;
   iterator_ = TilingData::SpiralDifferenceIterator(
@@ -312,11 +319,7 @@ TilingSetRasterQueueAll::EventuallyTilingIterator::EventuallyTilingIterator(
       tiling_->current_skewport_rect(), tiling_->current_soon_border_rect());
   if (!iterator_)
     return;
-  if (!GetFirstTileAndCheckIfValid(&iterator_)) {
-    ++(*this);
-    return;
-  }
-  if (tile_->content_rect().Intersects(pending_visible_rect_))

[vmpstr, 2015/05/05 18:04:02]

I see that this is less code, but can you maybe do this as a separate patch? I'd
rather be a bit more explicit about checking pending visible rect in each
iterator for now. 

I don't think your main change here to not intersect in priority rects depends
on this, right? That is, we still have this intersect check in GetFirstTile...
function

[USE eero AT chromium.org, 2015/05/06 12:05:20]

Yes, of course.
My main change does not depend on removing these code blocks which lie after
GetFirstTileAndCheckIfValid and AdvanceToNextTile calls. However, my main change
does depend on adding the skipping logic to GetFirstTileAndCheckIfValid.

Previously, tile priories for pending visible tiles which were not visible tiles
were calculated twice. First as pending visible tiles and then as
skewport/soon-border/eventually tiles.

But now I want DCHECK in PictureLayerTiling::ComputePriorityForTile that the
function is called with a correct priority rect type (see
https://codereview.chromium.org/1126813002/diff/1/cc/resources/picture_layer_...).
For that DCHECK to pass, the tile priorities must be computed only once, thus
GetFirstTileAndCheckIfValid must skip pending visible tiles before updating tile
priorities.

You are correct that these code removals are not mandatory. So I left them
intact and added only TODO comments. Let's clean up them later.

+  if (!GetFirstTileAndCheckIfValid(&iterator_))
     ++(*this);
 }
 
@@ -324,11 +327,6 @@ TilingSetRasterQueueAll::EventuallyTilingIterator&
     TilingSetRasterQueueAll::EventuallyTilingIterator::
     operator++() {
   AdvanceToNextTile(&iterator_);
-  while (!done()) {
-    if (!tile_->content_rect().Intersects(pending_visible_rect_))
-      break;
-    AdvanceToNextTile(&iterator_);
-  }
   return *this;
 }
 
@@ -342,7 +340,7 @@ TilingSetRasterQueueAll::TilingIterator::TilingIterator(
     TilingData* tiling_data)
     : tiling_(tiling),
       tiling_data_(tiling_data),
-      phase_(VISIBLE_RECT),
+      phase_(Phase::VISIBLE_RECT),
       current_tile_(NULL) {
   visible_iterator_ = VisibleTilingIterator(tiling_, tiling_data_);
   if (visible_iterator_.done()) {
@@ -356,32 +354,32 @@ TilingSetRasterQueueAll::TilingIterator::~TilingIterator() {
 }
 
 void TilingSetRasterQueueAll::TilingIterator::AdvancePhase() {
-  DCHECK_LT(phase_, EVENTUALLY_RECT);
+  DCHECK_LT(phase_, Phase::EVENTUALLY_RECT);
 
   current_tile_ = nullptr;
-  while (!current_tile_ && phase_ < EVENTUALLY_RECT) {
+  while (!current_tile_ && phase_ < Phase::EVENTUALLY_RECT) {
     phase_ = static_cast<Phase>(phase_ + 1);
     switch (phase_) {
-      case VISIBLE_RECT:
+      case Phase::VISIBLE_RECT:
         NOTREACHED();
         return;
-      case PENDING_VISIBLE_RECT:
+      case Phase::PENDING_VISIBLE_RECT:
         pending_visible_iterator_ =
             PendingVisibleTilingIterator(tiling_, tiling_data_);
         if (!pending_visible_iterator_.done())
           current_tile_ = *pending_visible_iterator_;
         break;
-      case SKEWPORT_RECT:
+      case Phase::SKEWPORT_RECT:
         skewport_iterator_ = SkewportTilingIterator(tiling_, tiling_data_);
         if (!skewport_iterator_.done())
           current_tile_ = *skewport_iterator_;
         break;
-      case SOON_BORDER_RECT:
+      case Phase::SOON_BORDER_RECT:
         soon_border_iterator_ = SoonBorderTilingIterator(tiling_, tiling_data_);
         if (!soon_border_iterator_.done())
           current_tile_ = *soon_border_iterator_;
         break;
-      case EVENTUALLY_RECT:
+      case Phase::EVENTUALLY_RECT:
         eventually_iterator_ = EventuallyTilingIterator(tiling_, tiling_data_);
         if (!eventually_iterator_.done())
           current_tile_ = *eventually_iterator_;
@@ -394,7 +392,7 @@ TilingSetRasterQueueAll::TilingIterator&
     TilingSetRasterQueueAll::TilingIterator::
     operator++() {
   switch (phase_) {
-    case VISIBLE_RECT:
+    case Phase::VISIBLE_RECT:
       ++visible_iterator_;
       if (visible_iterator_.done()) {
         AdvancePhase();
@@ -402,7 +400,7 @@ TilingSetRasterQueueAll::TilingIterator&
       }
       current_tile_ = *visible_iterator_;
       break;
-    case PENDING_VISIBLE_RECT:
+    case Phase::PENDING_VISIBLE_RECT:
       ++pending_visible_iterator_;
       if (pending_visible_iterator_.done()) {
         AdvancePhase();
@@ -410,7 +408,7 @@ TilingSetRasterQueueAll::TilingIterator&
       }
       current_tile_ = *pending_visible_iterator_;
       break;
-    case SKEWPORT_RECT:
+    case Phase::SKEWPORT_RECT:
       ++skewport_iterator_;
       if (skewport_iterator_.done()) {
         AdvancePhase();
@@ -418,7 +416,7 @@ TilingSetRasterQueueAll::TilingIterator&
       }
       current_tile_ = *skewport_iterator_;
       break;
-    case SOON_BORDER_RECT:
+    case Phase::SOON_BORDER_RECT:
       ++soon_border_iterator_;
       if (soon_border_iterator_.done()) {
         AdvancePhase();
@@ -426,7 +424,7 @@ TilingSetRasterQueueAll::TilingIterator&
       }
       current_tile_ = *soon_border_iterator_;
       break;
-    case EVENTUALLY_RECT:
+    case Phase::EVENTUALLY_RECT:
       ++eventually_iterator_;
       if (eventually_iterator_.done()) {
         current_tile_ = nullptr;