Fixing crash in PictureLayerImpl::MarkVisibleResourcesAsRequired when low-res tiles are disabled

cc/layers/picture_layer_impl.cc

Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index 85249812d5fbc9d5ea119537e014ae5e80eb80b2..b8e094c8d847ff09fa44bb3b7e529b30f68096f3 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -790,27 +790,46 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired() const {
   // active layer is also missing when both this layer and its twin have 2
   // tilings (high and low).  This avoids having to iterate/track coverage of
   // non-ideal tilings during the last draw call on the active layer.
-  if (high_res && low_res && tilings_->num_tilings() == 2 &&
-      twin_layer_ && twin_layer_->tilings_->num_tilings() == 2) {
-    twin_low_res = GetTwinTiling(low_res);
-    if (twin_low_res)
+  if (ShouldHaveLowResTiling()) {

[reveman, 2014/05/12 02:30:52]

why this change? is this broken before this change?

[Sergey, 2014/05/12 08:07:21]

If you set create_low_res_tiling=false, you will get
twin_layer_->tilings_->num_tilings() = 1. Thus, the original condition will turn
to false and there would not be a try to get high resolution tiling from twin
layer. Thus, made it two cases.

Logically this change seemed correct... Is there some misunderstanding from my
side?

[reveman, 2014/05/12 15:44:04]

Probably not but if you're fixing a bug in the implementation and not just some
unit tests you might want to do that separately or at least make it clear in the
description of the patch that this is currently broken. Does the patch currently
include a test that will fail without this change?

Is there any way you can make this look less like something that was tacked on
as side effect of making low res tiling optional and more like the initial
design?

[Sergey, 2014/05/12 23:37:47]

There was one, let me make sure we have it... Actually, I am not sure, that we
will have a failing test, as according to the MarkVisibleTilesAsRequired
function finding twin tiling is optional (we are searching here for a pointer,
that will be passed into the MarkVisibleTilesAsRequired as optional_twin_tiling
parameter). But I guess we should try our best to actually find it.
Agree, let me try.

[Sergey, 2014/05/13 05:37:44]

Found that test cases once again: NothingRequiredIfAllHighResTilesShared and
NothingRequiredIfActiveMissingTiles. Added them for low res disabled case.
Done. Please, check, if now it looks better.

+    if (high_res && low_res && tilings_->num_tilings() == 2 &&
+        twin_layer_ && twin_layer_->tilings_->num_tilings() == 2) {
+      twin_low_res = GetTwinTiling(low_res);
+      if (twin_low_res)
+        twin_high_res = GetTwinTiling(high_res);
+    }
+  } else {
+    if (high_res && tilings_->num_tilings() == 1 &&
+        twin_layer_ && twin_layer_->tilings_->num_tilings() == 1) {
       twin_high_res = GetTwinTiling(high_res);
+    }
   }
+
   // If this layer and its twin have different transforms, then don't compare
   // them and only allow activating to high res tiles, since tiles on each layer
   // will be in different places on screen.
-  if (!twin_high_res || !twin_low_res ||
-      twin_layer_->layer_tree_impl()->RequiresHighResToDraw() ||
-      draw_properties().screen_space_transform !=
-          twin_layer_->draw_properties().screen_space_transform) {
-    twin_high_res = NULL;
-    twin_low_res = NULL;
+
+  if (ShouldHaveLowResTiling()) {
+    if (!twin_high_res || !twin_low_res ||
+        twin_layer_->layer_tree_impl()->RequiresHighResToDraw() ||
+        draw_properties().screen_space_transform !=
+            twin_layer_->draw_properties().screen_space_transform) {
+      twin_high_res = NULL;
+      twin_low_res = NULL;
+    }
+  } else {
+    if (!twin_high_res ||
+        twin_layer_->layer_tree_impl()->RequiresHighResToDraw() ||
+        draw_properties().screen_space_transform !=
+            twin_layer_->draw_properties().screen_space_transform) {
+      twin_high_res = NULL;
+    }
   }
 
   // As a second pass, mark as required any visible high res tiles not filled in
   // by acceptable non-ideal tiles from the first pass.
   if (MarkVisibleTilesAsRequired(
-      high_res, twin_high_res, contents_scale_x(), rect, missing_region)) {
+      high_res, twin_high_res, contents_scale_x(), rect, missing_region) &&
+      ShouldHaveLowResTiling()) {

[reveman, 2014/05/12 15:44:04]

is this required?

[Sergey, 2014/05/12 23:37:47]

Actually, this one is more required, then the above :)
Without it we get into MarkVisibleTilesAsRequired with low_res = NULL and crash.

     // As an optional third pass, if a high res tile was skipped because its
     // twin was also missing, then fall back to mark low res tiles as required
     // in case the active twin is substituting those for missing high res