cc: Simplify the logic for deciding to update tile priorities.

cc: Simplify the logic for deciding to update tile priorities.

Only update priorities for a layer if it has at least one tiling that
has not had its priorities updated yet for the current impl-side
frame.

Commits happens at the start of an impl frame, so they are guaranteed to
get updated priorities. Any tilings that are newly created are also
guaranteed since they have no frame timestamp saved in them. When we
activate and draw the pending tree, we will only update the tile
priorities a single time.

R=enne
BUG=176312
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197976

[danakj, 2013-02-14 17:59:00]

Also added an early-out in PLImpl::UpdateTilePriorities if the layer has no
tilings, to avoid the project etc it that method.

[ccameron, 2013-02-14 18:35:10]

This should help some, though we'll still end up having to update priorities
while zooming/scrolling. It touches things I know little enough about that I'd
prefer enne to do the lg.

https://codereview.chromium.org/12259027/diff/10001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12259027/diff/10001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:241: 
It'd be really nice to (in a separate change) have something where we can see
how far the layer is from the viewport, and if sufficiently far away, just say
tilings_->SetTilePrioritiesToUnimportant(). If we could do that, it would get
Digg going much faster.

I'd also like to get those unimportant tiles out of the TileManager as well
(though that is less clear).

[danakj, 2013-02-14 18:41:24]

https://codereview.chromium.org/12259027/diff/10001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12259027/diff/10001/cc/picture_layer_impl.cc#...
cc/picture_layer_impl.cc:241: 
On 2013/02/14 18:35:10, ccameron1 wrote:
> It'd be really nice to (in a separate change) have something where we can see
> how far the layer is from the viewport, and if sufficiently far away, just say
> tilings_->SetTilePrioritiesToUnimportant(). If we could do that, it would get
> Digg going much faster.
> 
> I'd also like to get those unimportant tiles out of the TileManager as well
> (though that is less clear).

Yeh, this is just a way to avoid the matrix inverse and project for layers
without tilings, and to not update tile properties so much.

You're right if you're dirty every frame from a scroll this isn't a silver
bullet (tho will still help for layers without content).

[enne (OOO), 2013-02-14 20:40:01]

lgtm

[commit-bot: I haz the power, 2013-02-14 20:44:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/10001

[commit-bot: I haz the power, 2013-02-14 22:29:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/10001

[danakj, 2013-02-15 18:07:59]

One change PTAL.

When PLImpl does AddTiling, it sets needs_update_tile_priorities. This ensures
that if you create a new tiling on the active tree during calcDrawProps, you
will update its new tiles' priorities before you finally draw the frame.

[danakj, 2013-02-15 18:24:13]

I think we could also avoid the matrix inverse/project a bit more aggressively.
If there's no tiling that needs an update, then we don't need to
inverse/project, which we do for every layer right now. Added an early out for
that.

Also, if we aren't computing the tile priorities, I don't think we should save
the current state as last_foo_ so that we only save that data once per frame
(just like we compute prios once per frame).

[danakj, 2013-03-03 19:26:10]

review ping, enne can you PTAL at this? it changed a bunch since your first
look.

[enne (OOO), 2013-03-04 20:16:39]

https://codereview.chromium.org/12259027/diff/23001/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/12259027/diff/23001/cc/layer_tree_impl.cc#new...
cc/layer_tree_impl.cc:224: DCHECK_EQ(UPDATE_ACTIVE_TREE_FOR_DRAW, reason);
The change that added this UpdateDrawProperties reason had good performance
benefits, but I still feel like it's a lot of complexity to keep in your head
about at what point what gets updated and why.

Given that this patch provides needs_update_tile_priorities, can you get rid of
UpdateDrawPropertiesReason using the new flag instead? For instance, it's a
little weird that you have needs_update_tile_priorities, but
update_tile_priorities isn't always equal to it.

[danakj, 2013-04-19 23:24:45]

PTAL.

The "set_needs_update_tile_priorities" was really there to make sure new tilings
got prioritized immediately. But if we make a new tiling, that means we're
inside CalcDrawProps, and we're going to have a chance to UpdateTilePriorities()
later in that method. So just do it directly there to avoid an extra tree walk
just for updating tile prios.

Now, instead of having fancy logic distributed across 3 classes to decide when
to update tile priorities, we just pass it in explicitly when calling
UpdateDrawProps.

[danakj, 2013-04-19 23:27:12]

On Fri, Apr 19, 2013 at 7:24 PM, <danakj@chromium.org> wrote:

> PTAL.
>
> The "set_needs_update_tile_**priorities" was really there to make sure
> new tilings
> got prioritized immediately. But if we make a new tiling, that means we're
> inside CalcDrawProps, and we're going to have a chance to
> UpdateTilePriorities()
> later in that method. So just do it directly there to avoid an extra tree
> walk
> just for updating tile prios.
>
> Now, instead of having fancy logic distributed across 3 classes to decide
> when
> to update tile priorities, we just pass it in explicitly when calling
> UpdateDrawProps.
>

Hm.... but maybe this isn't quite great yet cuz it'll still update tile
prios on the pending tree, activate, and do it again to draw. /me thinks
more.


>
>
https://codereview.chromium.**org/12259027/<https://codereview.chromium.org/1...
>

[danakj, 2013-04-19 23:48:09]

On 2013/04/19 23:27:12, danakj wrote:
> Hm.... but maybe this isn't quite great yet cuz it'll still update tile
> prios on the pending tree, activate, and do it again to draw. /me thinks
> more.

Ok I solved this with a bool on LTHI instead of obtuse timestamp comparisons in
every picture layer (impl|tiling).

[enne (OOO), 2013-04-22 17:03:48]

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1403:
NeedsFirstTimeUpdateTilePrioritiesForLayer(layer))
I'm a little ಠ_ಠ at adding yet another virtual function to LayerImpl.  If you
have to call a virtual function to check if calling a virtual function is
needed, you might as well just do all the work in the second function and
early-out.

I think all of the "needs update tile priorities" could get pushed out of
LayerTreeImpl and get checked from PictureLayerImpl/Tiling.

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (left):

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_impl....
cc/trees/layer_tree_impl.h:122: enum UpdateDrawPropertiesReason {
<3

[danakj, 2013-04-22 18:37:30]

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1403:
NeedsFirstTimeUpdateTilePrioritiesForLayer(layer))
On 2013/04/22 17:03:48, enne wrote:
> I'm a little ಠ_ಠ at adding yet another virtual function to LayerImpl.  If you
> have to call a virtual function to check if calling a virtual function is
> needed, you might as well just do all the work in the second function and
> early-out.

I could certainly call the current function always and pass in "I want you to do
something" and it can decide to do something even if I said not to, if it has a
tiling that's never been prioritized.

> 
> I think all of the "needs update tile priorities" could get pushed out of
> LayerTreeImpl and get checked from PictureLayerImpl/Tiling.

Ah, I was happy about making this a single variable on the LTI instead of a
variable in every tiling, it seemed much simpler logic that way along with
smaller tilings. But I can go the other way instead - it doesn't seem required
tho to avoid the extra virtual. Do you prefer it in the tilings instead?

[danakj, 2013-04-22 18:44:07]

Updated the CL without the extra virtual.

[enne (OOO), 2013-04-22 18:44:13]

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1403:
NeedsFirstTimeUpdateTilePrioritiesForLayer(layer))
On 2013/04/22 18:37:30, danakj wrote:
> Ah, I was happy about making this a single variable on the LTI instead of a
> variable in every tiling, it seemed much simpler logic that way along with
> smaller tilings. But I can go the other way instead - it doesn't seem required
> tho to avoid the extra virtual. Do you prefer it in the tilings instead?

You have LayerTreeImpl::frame_time_last_update_tile_priorities_ and
PictureLayerTiling::last_impl_frame_time_.  How do these differ?  Don't you
already have a variable on the tiling?

[danakj, 2013-04-22 18:59:38]

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12259027/diff/46001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:1403:
NeedsFirstTimeUpdateTilePrioritiesForLayer(layer))
On 2013/04/22 18:44:13, enne wrote:
> On 2013/04/22 18:37:30, danakj wrote:
> > Ah, I was happy about making this a single variable on the LTI instead of a
> > variable in every tiling, it seemed much simpler logic that way along with
> > smaller tilings. But I can go the other way instead - it doesn't seem
required
> > tho to avoid the extra virtual. Do you prefer it in the tilings instead?
> 
> You have LayerTreeImpl::frame_time_last_update_tile_priorities_ and
> PictureLayerTiling::last_impl_frame_time_.  How do these differ?  Don't you
> already have a variable on the tiling?

Hm... right. Okay! Just using that now.

PTAL.

[enne (OOO), 2013-04-22 19:19:40]

I do like this a lot better, thanks.  I assume the perf test is still happy?

https://codereview.chromium.org/12259027/diff/57010/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/12259027/diff/57010/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:268: double current_frame_time =
(layer_tree_impl()->CurrentFrameTimeTicks() -
current_frame_time_in_seconds?

Also, can you use layerTreeImpl()->CurrentFrameTime().InSecondsF()?

[danakj, 2013-04-22 19:26:53]

https://codereview.chromium.org/12259027/diff/57010/cc/layers/picture_layer_i...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/12259027/diff/57010/cc/layers/picture_layer_i...
cc/layers/picture_layer_impl.cc:268: double current_frame_time =
(layer_tree_impl()->CurrentFrameTimeTicks() -
On 2013/04/22 19:19:40, enne wrote:
> current_frame_time_in_seconds?

ok

> Also, can you use layerTreeImpl()->CurrentFrameTime().InSecondsF()?

I don't see that function. You need a TimeDelta to get InSecondsF AFAICT.

[danakj, 2013-04-22 19:29:09]

Before:

[==========] Running 3 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 2 tests from LayerTreeHostPerfTestJsonReader
[ RUN      ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread
*RESULT 10_10_layer_tree: frames= 5102.81 runs/s
[       OK ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread (2015 ms)
[ RUN      ]
LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame
*RESULT 10_10_layer_tree: frames= 3426.38 runs/s
[       OK ]
LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame (2007 ms)
[----------] 2 tests from LayerTreeHostPerfTestJsonReader (4022 ms total)

[----------] 1 test from ScrollingLayerTreePerfTest
[ RUN      ] ScrollingLayerTreePerfTest.LongScrollablePage
*RESULT long_scrollable_page: frames= 18030.85 runs/s
[       OK ] ScrollingLayerTreePerfTest.LongScrollablePage (2004 ms)
[----------] 1 test from ScrollingLayerTreePerfTest (2004 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 2 test cases ran. (6026 ms total)
[  PASSED  ] 3 tests.

After:

[==========] Running 3 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 2 tests from LayerTreeHostPerfTestJsonReader
[ RUN      ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread
*RESULT 10_10_layer_tree: frames= 5163.51 runs/s
[       OK ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread (2015 ms)
[ RUN      ]
LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame
*RESULT 10_10_layer_tree: frames= 3414.79 runs/s
[       OK ]
LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame (2005 ms)
[----------] 2 tests from LayerTreeHostPerfTestJsonReader (4020 ms total)

[----------] 1 test from ScrollingLayerTreePerfTest
[ RUN      ] ScrollingLayerTreePerfTest.LongScrollablePage
*RESULT long_scrollable_page: frames= 17917.68 runs/s
[       OK ] ScrollingLayerTreePerfTest.LongScrollablePage (2003 ms)
[----------] 1 test from ScrollingLayerTreePerfTest (2003 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 2 test cases ran. (6024 ms total)
[  PASSED  ] 3 tests.

[danakj, 2013-04-22 19:31:00]

Renamed current_frame_time_in_seconds, PTAL

[danakj, 2013-04-22 19:37:48]

Updated the description too.

[enne (OOO), 2013-04-22 19:44:17]

lgtm

https://codereview.chromium.org/12259027/diff/68001/cc/resources/picture_laye...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/12259027/diff/68001/cc/resources/picture_laye...
cc/resources/picture_layer_tiling.h:153: return frame_time_in_seconds !=
last_impl_frame_time_;
last_impl_frame_time => last_frame_time_in_seconds?

[danakj, 2013-04-22 19:47:59]

https://codereview.chromium.org/12259027/diff/68001/cc/resources/picture_laye...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/12259027/diff/68001/cc/resources/picture_laye...
cc/resources/picture_layer_tiling.h:153: return frame_time_in_seconds !=
last_impl_frame_time_;
On 2013/04/22 19:44:17, enne wrote:
> last_impl_frame_time => last_frame_time_in_seconds?

Done.

[commit-bot: I haz the power, 2013-04-22 19:49:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/61018

[commit-bot: I haz the power, 2013-04-22 19:50:43]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-22 19:51:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/61018

[commit-bot: I haz the power, 2013-04-23 03:33:47]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-23 14:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/61018

[commit-bot: I haz the power, 2013-04-23 14:45:19]

Failed to apply patch for cc/trees/layer_tree_host_common.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host_common.cc
  Hunk #1 FAILED at 412.
  Hunk #2 FAILED at 440.
  Hunk #3 succeeded at 776 (offset -1 lines).
  Hunk #4 succeeded at 1254 (offset -1 lines).
  Hunk #5 succeeded at 1388 (offset -1 lines).
  Hunk #6 succeeded at 1438 (offset -1 lines).
  Hunk #7 succeeded at 1461 (offset -1 lines).
  Hunk #8 succeeded at 1477 (offset -1 lines).
  Hunk #9 succeeded at 1513 (offset -1 lines).
  2 out of 9 hunks FAILED -- saving rejects to file
cc/trees/layer_tree_host_common.cc.rej

Patch:       cc/trees/layer_tree_host_common.cc
Index: cc/trees/layer_tree_host_common.cc
diff --git a/cc/trees/layer_tree_host_common.cc
b/cc/trees/layer_tree_host_common.cc
index
7548cd293bbf1a2212062e961bc269fa34b0f08e..310e17ad7845482fbbacf3799f919ed3f67bdc4a
100644
--- a/cc/trees/layer_tree_host_common.cc
+++ b/cc/trees/layer_tree_host_common.cc
@@ -412,10 +412,10 @@ gfx::Transform ComputeSizeDeltaCompensation(
   // Apply step 3
   gfx::Transform container_layer_space_to_target_surface_space;
   if (target_surface_space_to_container_layer_space.GetInverse(
-      &container_layer_space_to_target_surface_space))
+          &container_layer_space_to_target_surface_space)) {
     result_transform.PreconcatTransform(
         container_layer_space_to_target_surface_space);
-  else {
+  } else {
     // FIXME: A non-invertible matrix could still make meaningful projection.
     // For example ScaleZ(0) is non-invertible but the layer is still visible.
     return gfx::Transform();
@@ -440,8 +440,7 @@ void ApplyPositionAdjustment(
     LayerImpl* layer,
     LayerImpl* container,
     const gfx::Transform& scroll_compensation,
-    gfx::Transform* combined_transform)
-{
+    gfx::Transform* combined_transform) {
   if (!layer->position_constraint().is_fixed_position())
     return;
 
@@ -777,8 +776,7 @@ static void CalculateDrawPropertiesInternal(
     float device_scale_factor,
     float page_scale_factor,
     bool subtree_can_use_lcd_text,
-    gfx::Rect* drawable_content_rect_of_subtree,
-    bool update_tile_priorities) {
+    gfx::Rect* drawable_content_rect_of_subtree) {
   // This function computes the new matrix transformations recursively for this
   // layer and all its descendants. It also computes the appropriate render
   // surfaces.
@@ -1256,8 +1254,7 @@ static void CalculateDrawPropertiesInternal(
         device_scale_factor,
         page_scale_factor,
         subtree_can_use_lcd_text,
-        &drawable_content_rect_of_child_subtree,
-        update_tile_priorities);
+        &drawable_content_rect_of_child_subtree);
     if (!drawable_content_rect_of_child_subtree.IsEmpty()) {
       accumulated_drawable_content_rect_of_children.Union(
           drawable_content_rect_of_child_subtree);
@@ -1391,8 +1388,7 @@ static void CalculateDrawPropertiesInternal(
     }
   }
 
-  if (update_tile_priorities)
-    UpdateTilePrioritiesForLayer(layer);
+  UpdateTilePrioritiesForLayer(layer);
 
   // If neither this layer nor any of its children were added, early out.
   if (sorting_start_index == descendants.size())
@@ -1442,7 +1438,6 @@ void LayerTreeHostCommon::CalculateDrawProperties(
   // initial clip rect.
   bool subtree_should_be_clipped = true;
   gfx::Rect device_viewport_rect(device_viewport_size);
-  bool update_tile_priorities = false;
 
   // This function should have received a root layer.
   DCHECK(IsRootLayer(root_layer));
@@ -1466,8 +1461,7 @@ void LayerTreeHostCommon::CalculateDrawProperties(
                                                  device_scale_factor,
                                                  page_scale_factor,
                                                  can_use_lcd_text,
-                                                 &total_drawable_content_rect,
-                                                 update_tile_priorities);
+                                                 &total_drawable_content_rect);
 
   // The dummy layer list should not have been used.
   DCHECK_EQ(0u, dummy_layer_list.size());
@@ -1483,8 +1477,7 @@ void LayerTreeHostCommon::CalculateDrawProperties(
     float page_scale_factor,
     int max_texture_size,
     bool can_use_lcd_text,
-    LayerImplList* render_surface_layer_list,
-    bool update_tile_priorities) {
+    LayerImplList* render_surface_layer_list) {
   gfx::Rect total_drawable_content_rect;
   gfx::Transform identity_matrix;
   gfx::Transform device_scale_transform;
@@ -1520,8 +1513,7 @@ void LayerTreeHostCommon::CalculateDrawProperties(
       device_scale_factor,
       page_scale_factor,
       can_use_lcd_text,
-      &total_drawable_content_rect,
-      update_tile_priorities);
+      &total_drawable_content_rect);
 
   // The dummy layer list should not have been used.
   DCHECK_EQ(0u, dummy_layer_list.size());

[commit-bot: I haz the power, 2013-04-23 14:47:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/83001

[commit-bot: I haz the power, 2013-04-23 14:47:08]

Failed to apply patch for cc/trees/layer_tree_host_common.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/layer_tree_host_common.cc
  Hunk #1 FAILED at 412.
  Hunk #2 FAILED at 440.
  Hunk #3 succeeded at 776 (offset -1 lines).
  Hunk #4 succeeded at 1254 (offset -1 lines).
  Hunk #5 succeeded at 1388 (offset -1 lines).
  Hunk #6 succeeded at 1438 (offset -1 lines).
  Hunk #7 succeeded at 1461 (offset -1 lines).
  Hunk #8 succeeded at 1477 (offset -1 lines).
  Hunk #9 succeeded at 1513 (offset -1 lines).
  2 out of 9 hunks FAILED -- saving rejects to file
cc/trees/layer_tree_host_common.cc.rej

Patch:       cc/trees/layer_tree_host_common.cc
Index: cc/trees/layer_tree_host_common.cc
diff --git a/cc/trees/layer_tree_host_common.cc
b/cc/trees/layer_tree_host_common.cc
index
7548cd293bbf1a2212062e961bc269fa34b0f08e..310e17ad7845482fbbacf3799f919ed3f67bdc4a
100644
--- a/cc/trees/layer_tree_host_common.cc
+++ b/cc/trees/layer_tree_host_common.cc
@@ -412,10 +412,10 @@ gfx::Transform ComputeSizeDeltaCompensation(
   // Apply step 3
   gfx::Transform container_layer_space_to_target_surface_space;
   if (target_surface_space_to_container_layer_space.GetInverse(
-      &container_layer_space_to_target_surface_space))
+          &container_layer_space_to_target_surface_space)) {
     result_transform.PreconcatTransform(
         container_layer_space_to_target_surface_space);
-  else {
+  } else {
     // FIXME: A non-invertible matrix could still make meaningful projection.
     // For example ScaleZ(0) is non-invertible but the layer is still visible.
     return gfx::Transform();
@@ -440,8 +440,7 @@ void ApplyPositionAdjustment(
     LayerImpl* layer,
     LayerImpl* container,
     const gfx::Transform& scroll_compensation,
-    gfx::Transform* combined_transform)
-{
+    gfx::Transform* combined_transform) {
   if (!layer->position_constraint().is_fixed_position())
     return;
 
@@ -777,8 +776,7 @@ static void CalculateDrawPropertiesInternal(
     float device_scale_factor,
     float page_scale_factor,
     bool subtree_can_use_lcd_text,
-    gfx::Rect* drawable_content_rect_of_subtree,
-    bool update_tile_priorities) {
+    gfx::Rect* drawable_content_rect_of_subtree) {
   // This function computes the new matrix transformations recursively for this
   // layer and all its descendants. It also computes the appropriate render
   // surfaces.
@@ -1256,8 +1254,7 @@ static void CalculateDrawPropertiesInternal(
         device_scale_factor,
         page_scale_factor,
         subtree_can_use_lcd_text,
-        &drawable_content_rect_of_child_subtree,
-        update_tile_priorities);
+        &drawable_content_rect_of_child_subtree);
     if (!drawable_content_rect_of_child_subtree.IsEmpty()) {
       accumulated_drawable_content_rect_of_children.Union(
           drawable_content_rect_of_child_subtree);
@@ -1391,8 +1388,7 @@ static void CalculateDrawPropertiesInternal(
     }
   }
 
-  if (update_tile_priorities)
-    UpdateTilePrioritiesForLayer(layer);
+  UpdateTilePrioritiesForLayer(layer);
 
   // If neither this layer nor any of its children were added, early out.
   if (sorting_start_index == descendants.size())
@@ -1442,7 +1438,6 @@ void LayerTreeHostCommon::CalculateDrawProperties(
   // initial clip rect.
   bool subtree_should_be_clipped = true;
   gfx::Rect device_viewport_rect(device_viewport_size);
-  bool update_tile_priorities = false;
 
   // This function should have received a root layer.
   DCHECK(IsRootLayer(root_layer));
@@ -1466,8 +1461,7 @@ void LayerTreeHostCommon::CalculateDrawProperties(
                                                  device_scale_factor,
                                                  page_scale_factor,
                                                  can_use_lcd_text,
-                                                 &total_drawable_content_rect,
-                                                 update_tile_priorities);
+                                                 &total_drawable_content_rect);
 
   // The dummy layer list should not have been used.
   DCHECK_EQ(0u, dummy_layer_list.size());
@@ -1483,8 +1477,7 @@ void LayerTreeHostCommon::CalculateDrawProperties(
     float page_scale_factor,
     int max_texture_size,
     bool can_use_lcd_text,
-    LayerImplList* render_surface_layer_list,
-    bool update_tile_priorities) {
+    LayerImplList* render_surface_layer_list) {
   gfx::Rect total_drawable_content_rect;
   gfx::Transform identity_matrix;
   gfx::Transform device_scale_transform;
@@ -1520,8 +1513,7 @@ void LayerTreeHostCommon::CalculateDrawProperties(
       device_scale_factor,
       page_scale_factor,
       can_use_lcd_text,
-      &total_drawable_content_rect,
-      update_tile_priorities);
+      &total_drawable_content_rect);
 
   // The dummy layer list should not have been used.
   DCHECK_EQ(0u, dummy_layer_list.size());

[commit-bot: I haz the power, 2013-04-23 14:47:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/82003

[commit-bot: I haz the power, 2013-04-23 14:59:23]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-23 15:32:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/82003

[commit-bot: I haz the power, 2013-04-23 22:03:08]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-29 19:48:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/82003

[commit-bot: I haz the power, 2013-04-29 19:48:59]

Failed to apply patch for cc/layers/layer_position_constraint_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layers/layer_position_constraint_unittest.cc
  Hunk #1 succeeded at 6 with fuzz 2 (offset 2 lines).
  Hunk #2 FAILED at 46.
  Hunk #3 FAILED at 120.
  2 out of 3 hunks FAILED -- saving rejects to file
cc/layers/layer_position_constraint_unittest.cc.rej

Patch:       cc/layers/layer_position_constraint_unittest.cc
Index: cc/layers/layer_position_constraint_unittest.cc
diff --git a/cc/layers/layer_position_constraint_unittest.cc
b/cc/layers/layer_position_constraint_unittest.cc
index
409c63492ca4d9a7ffc5d0deb6196f720530763b..4d8c5702857cbbf30f4f48f97d288dca5f5c00d4
100644
--- a/cc/layers/layer_position_constraint_unittest.cc
+++ b/cc/layers/layer_position_constraint_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "cc/layers/layer_position_constraint.h"
 
+#include <vector>
+
 #include "cc/layers/layer_impl.h"
 #include "cc/test/fake_impl_proxy.h"
 #include "cc/test/fake_layer_tree_host_impl.h"
@@ -44,14 +46,14 @@ void ExecuteCalculateDrawProperties(LayerImpl* root_layer,
   // We are probably not testing what is intended if the root_layer bounds are
   // empty.
   DCHECK(!root_layer->bounds().IsEmpty());
-  LayerTreeHostCommon::CalculateDrawProperties(root_layer,
-                                               device_viewport_size,
-                                               device_scale_factor,
-                                               page_scale_factor,
-                                               dummy_max_texture_size,
-                                               can_use_lcd_text,
-                                              
&dummy_render_surface_layer_list,
-                                               false);
+  LayerTreeHostCommon::CalculateDrawProperties(
+      root_layer,
+      device_viewport_size,
+      device_scale_factor,
+      page_scale_factor,
+      dummy_max_texture_size,
+      can_use_lcd_text,
+      &dummy_render_surface_layer_list);
 }
 
 void ExecuteCalculateDrawProperties(LayerImpl* root_layer) {
@@ -118,6 +120,7 @@ class LayerPositionConstraintTest : public testing::Test {
 
     return root.Pass();
   }
+
  protected:
   FakeImplProxy proxy_;
   FakeLayerTreeHostImpl host_impl_;

[commit-bot: I haz the power, 2013-04-30 00:55:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/102001

[commit-bot: I haz the power, 2013-04-30 01:10:26]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-30 01:15:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/102001

[commit-bot: I haz the power, 2013-04-30 06:44:35]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[commit-bot: I haz the power, 2013-04-30 18:15:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/102001

[danakj, 2013-04-30 21:13:40]

+aelias

This seems to be crashing the webview tests somehow, but there's not stacktrace
or anything in the bot logs. How do I see what's going on?

http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

C  364s Main 
********************************************************************************
C  364s Main  Detailed Logs
C  364s Main 
********************************************************************************
C  364s Main  [CRASH]
org.chromium.android_webview.test.AwContentsClientFullScreenVideoTest#testOnShowAndHideCustomView:
C  364s Main  no error message
C  364s Main  [CRASH]
org.chromium.android_webview.test.AwContentsClientGetVideoLoadingProgressViewTest#testGetVideoLoadingProgressView:
C  364s Main  no error message

[aelias_OOO_until_Jul13, 2013-04-30 21:29:52]

[+joth for help on WebView test failures]

[joth, 2013-04-30 22:10:44]

There should be some more info in the logcat dump: 
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...


08015:  04-30 02:27:12.929 24954 24991 F chromium:
[FATAL:picture_layer_impl.cc(608)] Check failed: false. All tiles considered for
activation should be live

give any clues?

[commit-bot: I haz the power, 2013-05-01 05:01:53]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is android_dbg, revision is HEAD

[enne (OOO), 2013-05-02 19:59:45]

https://codereview.chromium.org/12259027/diff/139001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12259027/diff/139001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:35: out->resolution_ = resolution_;
Looking at this code closer, but could this lead to multiple hi res tilings?

[danakj, 2013-05-02 20:02:04]

https://codereview.chromium.org/12259027/diff/139001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12259027/diff/139001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:35: out->resolution_ = resolution_;
On 2013/05/02 19:59:46, enne wrote:
> Looking at this code closer, but could this lead to multiple hi res tilings?

Hmm! It's kinda weird anyway right? You should manage tilings to decide the
resolution.

I'll make another CL to remove this with a test to verify it makes sense.

[enne (OOO), 2013-05-02 20:03:45]

sgtm and lgtm

[danakj, 2013-05-02 20:59:49]

Thanks joth, that helped out!

[commit-bot: I haz the power, 2013-05-02 21:00:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/139001

[commit-bot: I haz the power, 2013-05-02 21:54:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12259027/139001

[commit-bot: I haz the power, 2013-05-02 22:02:06]

Change committed as 197976