cc: Split tiling set raster queues into all and required.

cc/resources/tiling_set_raster_queue_required.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "cc/resources/tiling_set_raster_queue_required.h"
+
+#include <utility>
+
+#include "cc/resources/picture_layer_tiling_set.h"
+#include "cc/resources/tile.h"
+#include "cc/resources/tile_priority.h"
+
+namespace cc {
+
+TilingSetRasterQueueRequired::TilingSetRasterQueueRequired(
+    PictureLayerTilingSet* tiling_set,
+    RasterTilePriorityQueue::Type type)
+    : type_(type) {
+  DCHECK(type != RasterTilePriorityQueue::Type::ALL);

[danakj, 2015/01/08 23:24:41]

DCHECK_NE?

[vmpstr, 2015/01/09 20:21:40]

Since it's an enum class it would require a non-existent operator<<. I'd prefer
to keep this here, since the type itself doesn't matter it just has to be not
ALL :)

[danakj, 2015/01/09 20:28:22]

Can you static cast them to ints? If that's terrible, can you DCHECK(...) <<
type?

[vmpstr, 2015/01/09 20:52:42]

Done.

+  DCHECK(tiling_set);

[danakj, 2015/01/08 23:24:41]

nit: not really doing anything, we'll crash when we deref it?

[vmpstr, 2015/01/09 20:21:40]

Done.

+
+  PictureLayerTiling* tiling =

[danakj, 2015/01/08 23:24:41]

can you leave a comment about why highres is enough? maybe point to other places
in the code that guarantee this assumption?

[vmpstr, 2015/01/09 20:21:41]

Done.

+      tiling_set->FindTilingWithResolution(HIGH_RESOLUTION);
+  DCHECK(tiling);
+  iterator_ = TilingIterator(tiling, &tiling->tiling_data_);
+  while (iterator_ && !IsTileRequired(*iterator_))
+    ++iterator_;
+}
+
+TilingSetRasterQueueRequired::~TilingSetRasterQueueRequired() {
+}
+
+bool TilingSetRasterQueueRequired::IsEmpty() const {
+  return !iterator_;
+}
+
+void TilingSetRasterQueueRequired::Pop() {
+  DCHECK(!IsEmpty());
+  ++iterator_;
+  while (iterator_ && !IsTileRequired(*iterator_))
+    ++iterator_;
+}
+
+Tile* TilingSetRasterQueueRequired::Top() {
+  DCHECK(!IsEmpty());
+  return *iterator_;
+}
+
+const Tile* TilingSetRasterQueueRequired::Top() const {
+  DCHECK(!IsEmpty());
+  return *iterator_;
+}
+
+bool TilingSetRasterQueueRequired::IsTileRequired(const Tile* tile) const {
+  return (type_ == RasterTilePriorityQueue::Type::REQUIRED_FOR_ACTIVATION &&
+          tile->required_for_activation()) ||
+         (type_ == RasterTilePriorityQueue::Type::REQUIRED_FOR_DRAW &&
+          tile->required_for_draw());
+}
+
+TilingSetRasterQueueRequired::TilingIterator::TilingIterator()
+    : tiling_(NULL), current_tile_(NULL) {

[danakj, 2015/01/08 23:24:41]

nullptr

[vmpstr, 2015/01/09 20:21:40]

Done.

+}
+
+TilingSetRasterQueueRequired::TilingIterator::TilingIterator(
+    PictureLayerTiling* tiling,
+    TilingData* tiling_data)
+    : tiling_(tiling), tiling_data_(tiling_data), current_tile_(NULL) {

[danakj, 2015/01/08 23:24:41]

nullptr

[vmpstr, 2015/01/09 20:21:40]

Done.

+  if (!tiling_->has_visible_rect_tiles())

[danakj, 2015/01/08 23:24:41]

Should we have some kinda tests for this condition? Or a DCHECK in the early out
that verifies all the tiles are not required?

[vmpstr, 2015/01/09 20:21:40]

I added a DCHECK, let me know if that's what you mean?

+    return;
+
+  visible_iterator_ =
+      TilingData::Iterator(tiling_data_, tiling_->current_visible_rect(),
+                           false /* include_borders */);
+  if (!visible_iterator_)
+    return;
+
+  current_tile_ =
+      tiling_->TileAt(visible_iterator_.index_x(), visible_iterator_.index_y());
+  if (current_tile_ && TileNeedsRaster(current_tile_)) {
+    tiling_->UpdateTileAndTwinPriority(current_tile_);
+    return;
+  }
+  ++(*this);
+}
+
+TilingSetRasterQueueRequired::TilingIterator::~TilingIterator() {
+}
+
+TilingSetRasterQueueRequired::TilingIterator&
+    TilingSetRasterQueueRequired::TilingIterator::
+    operator++() {
+  do {
+    ++visible_iterator_;
+    if (!visible_iterator_) {
+      current_tile_ = NULL;
+      return *this;
+    }
+    std::pair<int, int> next_index = visible_iterator_.index();
+    current_tile_ = tiling_->TileAt(next_index.first, next_index.second);
+  } while (!current_tile_ || !TileNeedsRaster(current_tile_));

[danakj, 2015/01/08 23:24:41]

TileNeedsRaster uses occlusion to determine if it needs raster. Occlusion still
has this duality where it's able to be determined on its own, but its also part
of the tile priority. So it's not clear that this check here isn't using the
priority that gets set down below on line 104.

Now.. I believe this is right as it is, cuz this is in the iterator, and
iterators genreate priorities, and its using the method that doesn't use the
stored priority state about occlusion..

But what I wanna say is that this is a lot of assumptions/dependencies/something
between this code and PLT/Tile. It would be nice if this was more obvious.
Adding comments here to explain that would help (but the callsite to occlusion
is in the header and they wouldnt really make sense here or there in that case).
Maybe getting rid of the help and do the check inline here with comments? Maybe
do them with breaks and make the loop while (true) to let you comment?

I'm not sure exactly what to suggest, but I feel like I get this but other
people reading this would have no idea without having reviewed previous iterator
CLs.

I guess same thoughts about line80.

[vmpstr, 2015/01/09 20:21:40]

I understand. I've changed this code around a bit an sprinkled some comments.
Let me know if that reads better now.

[danakj, 2015/01/12 18:23:13]

Ya this is great, thank you for nice comments.

+
+  if (current_tile_)
+    tiling_->UpdateTileAndTwinPriority(current_tile_);
+  return *this;
+}
+
+}  // namespace cc