cc: Pass ideal contents scale to PictureLayerTiling UpdateTilePriorities.

cc/layers/picture_layer_impl_unittest.cc

 // Copyright 2013 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/layers/picture_layer_impl.h"
 
 #include <algorithm>
 #include <limits>
 #include <set>
 #include <utility>
@@ skipping 1966 matching lines @@
            tilings.begin();
        tiling_iterator != tilings.end();
        ++tiling_iterator) {
     std::vector<Tile*> tiles = (*tiling_iterator)->AllTilesForTesting();
     std::copy(tiles.begin(), tiles.end(), std::back_inserter(all_tiles));
   }
 
   std::set<Tile*> all_tiles_set(all_tiles.begin(), all_tiles.end());
 
   bool mark_required = false;
-  for (std::vector<Tile*>::iterator it = all_tiles.begin();
-       it != all_tiles.end();
-       ++it) {
-    Tile* tile = *it;
-    if (mark_required)
-      tile->MarkRequiredForActivation();
-    mark_required = !mark_required;
+  for (size_t i = 0; i < tilings.size(); ++i) {
+    PictureLayerTiling* tiling = tilings.at(i);
+    for (PictureLayerTiling::CoverageIterator iter(
+             tiling,
+             pending_layer_->contents_scale_x(),
+             pending_layer_->visible_content_rect());
+         iter;
+         ++iter) {
+      if (mark_required)
+        iter->MarkRequiredForActivation();
+      mark_required = !mark_required;
+    }

[sohanjg, 2014/06/20 13:35:20]

I think we shouldnt be iterating over all the tiles, instead only in the visible
content rect for activating tiles here, hence the CoverageIterator. 

[enne (OOO), 2014/06/25 18:33:07]

I think that's an ok change.  This is just a test, so...

Can you count how many things are marked and how many things are not marked and
DCHECK that there's at least one thing in each category? (i.e. test that the
test is testing something?)

[vmpstr, 2014/06/25 21:08:52]

I agree with this change. I was considering making that myself, since required
tiles might be handled differently later on, and I would prefer to have an
assurance that those tiles would only be in the visible rect.

[sohanjg, 2014/06/26 08:30:51]

Done.

   }
 
   // Sanity checks.
   EXPECT_EQ(91u, all_tiles.size());
   EXPECT_EQ(91u, all_tiles_set.size());
 
   // Empty iterator.
   PictureLayerImpl::LayerEvictionTileIterator it;
   EXPECT_FALSE(it);
 
@@ skipping 27 matching lines @@
       last_tile = tile;
       break;
     }
 
     if (reached_required) {
       EXPECT_TRUE(tile->required_for_activation());
     } else if (tile->required_for_activation()) {
       reached_required = true;
       scale_index = 0;
     }
 

[sohanjg, 2014/06/20 13:35:20]

This check becomes redundant now, because the tiles activated are not iterated
by the LayerEvictionTileIterator, not sure whether LayerEvictionTileIterator
should have some visible rect concept ?
I can remove it, if you want.

[enne (OOO), 2014/06/25 18:33:07]

I don't understand why this is redundant.

[sohanjg, 2014/06/25 20:17:53]

Because none of the tiles iterated over by layerevictioniterator here are
activated by the CoverageIterator above, so neither of the if-else block is
executed, and consequently the reached_required dcheck becomes redundant.
So i was thinking whether eviction iterator shud have a visible/content rect
concept ?

[enne (OOO), 2014/06/25 20:20:05]

The iterator above is marking some of the tiles as required.  The eviction
iterator needs to take *all* tiles into account.  Both of those if-else
statements inside the eviction iterator should be hit in order for this test to
properly test what it's supposed to be doing?

[vmpstr, 2014/06/25 21:08:52]

This part of the loop break out as soon as a now bin is visited, and now bin
tiles are visited in the loop below. I'd like to see the if-else block replaced
with an EXPECT_FALSE(tile->required_for_activation()) instead.

[sohanjg, 2014/06/26 08:30:51]

Done.

     while (std::abs(tile->contents_scale() - expected_scales[scale_index]) >
            std::numeric_limits<float>::epsilon()) {
       ++scale_index;
       ASSERT_LT(scale_index, arraysize(expected_scales));
     }
 
     EXPECT_FLOAT_EQ(tile->contents_scale(), expected_scales[scale_index]);
     unique_tiles.insert(tile);
 
     // If the tile is the same rough bin as last tile (same activation, bin, and
     // scale), then distance should be decreasing.
     if (tile->required_for_activation() ==
             last_tile->required_for_activation() &&
         priority.priority_bin ==
             last_tile->priority(PENDING_TREE).priority_bin &&
         std::abs(tile->contents_scale() - last_tile->contents_scale()) <
             std::numeric_limits<float>::epsilon()) {
       EXPECT_LE(priority.distance_to_visible,
                 last_tile->priority(PENDING_TREE).distance_to_visible);
     }
 
     last_tile = tile;
   }
 
   EXPECT_TRUE(reached_visible);
-  EXPECT_TRUE(reached_required);

[sohanjg, 2014/06/20 13:35:20]

As mentioned above, this check is not required.

[enne (OOO), 2014/06/25 18:33:07]

This still seems valid to me.  I don't understand.

   EXPECT_EQ(65u, unique_tiles.size());
 
   scale_index = 0;
   reached_required = false;
   for (; it; ++it) {
     Tile* tile = *it;
     EXPECT_TRUE(tile);
 
     TilePriority priority = tile->priority(PENDING_TREE);
     EXPECT_EQ(TilePriority::NOW, priority.priority_bin);
@@ skipping 726 matching lines @@
 
   host_impl_.pending_tree()->SetRootLayer(root.Pass());
 
   EXPECT_FALSE(pending_mask_content->tilings());
   host_impl_.pending_tree()->UpdateDrawProperties();
   EXPECT_NE(0u, pending_mask_content->num_tilings());
 }
 
 }  // namespace
 }  // namespace cc