Draw Transform of Render Surface From Property Trees

Draw Transform of Render Surface From Property Trees
BUG=497817
Committed: https://crrev.com/504a05053fffd4fd4bb0d9d58bb56b46bc2fe09f
Cr-Commit-Position: refs/heads/master@{#340142}

[jaydasika, 2015-07-23 15:24:10]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org

[jaydasika, 2015-07-23 15:25:42]

Patch 2 removes unnecessary transform tree walk and exposes only transform tree
index in render surface rather than the owning layer.

[ajuma, 2015-07-23 15:57:03]

Just a few nits, but otherwise this looks good.

https://codereview.chromium.org/1248433007/diff/20001/cc/layers/render_surfac...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/1248433007/diff/20001/cc/layers/render_surfac...
cc/layers/render_surface_impl.cc:75: return owning_layer_ ?
owning_layer_->transform_tree_index() : 0;
I don't think owning_layer_ will ever be null (despite the function above having
a null check; owning_layer_ is used elsewhere in this file without a null
check). (Aside: the 'null' index for property trees is -1, not 0.)

https://codereview.chromium.org/1248433007/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/1248433007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2443: void
VerifyPropertyTreeValuesForSurfaces(RenderSurfaceImpl* render_surface,
Nit: s/Surfaces/Surface

https://codereview.chromium.org/1248433007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2505: if (current_layer->parent() &&
current_layer->render_surface())
I think you can instead just check if (it.represents_target_render_surface())
(and then assume current_layer->render_surface() is non-null). Also, can we
remove the parent() check? The root render surface should still have a draw
transform, even if it always happens to be the identity.

[jaydasika, 2015-07-23 16:56:26]

https://codereview.chromium.org/1248433007/diff/20001/cc/layers/render_surfac...
File cc/layers/render_surface_impl.cc (right):

https://codereview.chromium.org/1248433007/diff/20001/cc/layers/render_surfac...
cc/layers/render_surface_impl.cc:75: return owning_layer_ ?
owning_layer_->transform_tree_index() : 0;
On 2015/07/23 15:57:03, ajuma wrote:
> I don't think owning_layer_ will ever be null (despite the function above
having
> a null check; owning_layer_ is used elsewhere in this file without a null
> check). (Aside: the 'null' index for property trees is -1, not 0.)

Done.

https://codereview.chromium.org/1248433007/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/1248433007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2443: void
VerifyPropertyTreeValuesForSurfaces(RenderSurfaceImpl* render_surface,
On 2015/07/23 15:57:03, ajuma wrote:
> Nit: s/Surfaces/Surface

Done.

https://codereview.chromium.org/1248433007/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2505: if (current_layer->parent() &&
current_layer->render_surface())
On 2015/07/23 15:57:03, ajuma wrote:
> I think you can instead just check if (it.represents_target_render_surface())
> (and then assume current_layer->render_surface() is non-null). Also, can we
> remove the parent() check? The root render surface should still have a draw
> transform, even if it always happens to be the identity.

Done.

[ajuma, 2015-07-23 17:28:29]

Thanks, lgtm.

[jaydasika, 2015-07-23 17:30:04]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2015-07-23 17:30:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1248433007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1248433007/40001

[commit-bot: I haz the power, 2015-07-23 18:20:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-23 18:20:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jaydasika, 2015-07-23 18:55:58]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2015-07-23 18:56:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1248433007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1248433007/40001

[commit-bot: I haz the power, 2015-07-23 19:25:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-23 19:26:54]

Patchset 3 (id:??) landed as
https://crrev.com/504a05053fffd4fd4bb0d9d58bb56b46bc2fe09f
Cr-Commit-Position: refs/heads/master@{#340142}