cc/blimp: Eliminate Layer updates for common cases.

cc/blimp: Eliminate Layer updates for common cases.

In the threaded compositor, we optimize out updates for layers that
will not be drawn after building/updating PropertyTrees. Use some of
that logic in the remote LayerTreeHost.

BUG=650885
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/a0679741aca67ad446f6262645d8d65e8d0ab802
Cr-Commit-Position: refs/heads/master@{#429165}

[Khushal, 2016-11-01 19:09:21]

Description was changed from

==========
cc/blimp: Eliminate Layer updates for common cases.

In the threaded compositor, we optimize out updates for layers that
will not be drawn after building/updating PropertyTrees. Use some of
that logic in the remote LayerTreeHost.

BUG=650885
==========

to

==========
cc/blimp: Eliminate Layer updates for common cases.

In the threaded compositor, we optimize out updates for layers that
will not be drawn after building/updating PropertyTrees. Use some of
that logic in the remote LayerTreeHost.

BUG=650885
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Khushal, 2016-11-01 19:11:32]

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

[Khushal, 2016-11-01 19:11:33]

I ran into a blink assert because we were trying to pull DisplayLists for layers
that were marked as not-drawable. But instead of building the complete
PropertyTrees server side just for
draw_property_utils::FindLayersThatNeedUpdates, I just isolated the parts that
we need.
Can you take a look and see if it looks correct?

[ajuma, 2016-11-01 19:27:58]

Looks good. Please add a test for this.

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:48: //    used by the renderer.
It's true that the renderer never uses the HideLayerAndSubtree call, but there's
still the case of a layer with opacity 0 causing its subtree to get skipped.
Those layers won't get skipped with the logic below (but that might be fine --
the amount of work saved by skipping is small).

[Khushal, 2016-11-01 21:09:01]

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:48: //    used by the renderer.
On 2016/11/01 19:27:58, ajuma wrote:
> It's true that the renderer never uses the HideLayerAndSubtree call, but
there's
> still the case of a layer with opacity 0 causing its subtree to get skipped.
> Those layers won't get skipped with the logic below (but that might be fine --
> the amount of work saved by skipping is small).

You're right, thanks! I've added that in the comment. Will come back here later,
I'm going to leave the bug open for now.

Also, while writing the test, I ran into a problem because
CallFunctionForEveryLayer wouldn't run on a mask layer's subtree. Is that
expected? Is the mask layer not supposed to have any children?

[ajuma, 2016-11-01 21:37:07]

lgtm

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:48: //    used by the renderer.
On 2016/11/01 21:09:01, Khushal wrote:
> On 2016/11/01 19:27:58, ajuma wrote:
> > It's true that the renderer never uses the HideLayerAndSubtree call, but
> there's
> > still the case of a layer with opacity 0 causing its subtree to get skipped.
> > Those layers won't get skipped with the logic below (but that might be fine
--
> > the amount of work saved by skipping is small).
> 
> You're right, thanks! I've added that in the comment. Will come back here
later,
> I'm going to leave the bug open for now.
> 
> Also, while writing the test, I ran into a problem because
> CallFunctionForEveryLayer wouldn't run on a mask layer's subtree. Is that
> expected? Is the mask layer not supposed to have any children?

Right, a mask layer shouldn't have children, the mask layer itself is supposed
to represent the entire mask.

https://codereview.chromium.org/2467043002/diff/20001/cc/blimp/layer_tree_hos...
File cc/blimp/layer_tree_host_remote_unittest.cc (right):

https://codereview.chromium.org/2467043002/diff/20001/cc/blimp/layer_tree_hos...
cc/blimp/layer_tree_host_remote_unittest.cc:423: scoped_refptr<MockLayer>
empty_bound_layer = new MockLayer(true);
nit: empty_bounds_layer

[Khushal, 2016-11-01 21:42:24]

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:48: //    used by the renderer.
On 2016/11/01 21:37:07, ajuma wrote:
> On 2016/11/01 21:09:01, Khushal wrote:
> > On 2016/11/01 19:27:58, ajuma wrote:
> > > It's true that the renderer never uses the HideLayerAndSubtree call, but
> > there's
> > > still the case of a layer with opacity 0 causing its subtree to get
skipped.
> > > Those layers won't get skipped with the logic below (but that might be
fine
> --
> > > the amount of work saved by skipping is small).
> > 
> > You're right, thanks! I've added that in the comment. Will come back here
> later,
> > I'm going to leave the bug open for now.
> > 
> > Also, while writing the test, I ran into a problem because
> > CallFunctionForEveryLayer wouldn't run on a mask layer's subtree. Is that
> > expected? Is the mask layer not supposed to have any children?
> 
> Right, a mask layer shouldn't have children, the mask layer itself is supposed
> to represent the entire mask.

Ohh, makes sense. Would it be a good idea to add a DCHECK in Layer::SetMaskLayer
for this? Makes it obvious to someone reading it.

[ajuma, 2016-11-01 21:47:20]

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:48: //    used by the renderer.
On 2016/11/01 21:42:23, Khushal wrote:
> On 2016/11/01 21:37:07, ajuma wrote:
> > On 2016/11/01 21:09:01, Khushal wrote:
> > > On 2016/11/01 19:27:58, ajuma wrote:
> > > > It's true that the renderer never uses the HideLayerAndSubtree call, but
> > > there's
> > > > still the case of a layer with opacity 0 causing its subtree to get
> skipped.
> > > > Those layers won't get skipped with the logic below (but that might be
> fine
> > --
> > > > the amount of work saved by skipping is small).
> > > 
> > > You're right, thanks! I've added that in the comment. Will come back here
> > later,
> > > I'm going to leave the bug open for now.
> > > 
> > > Also, while writing the test, I ran into a problem because
> > > CallFunctionForEveryLayer wouldn't run on a mask layer's subtree. Is that
> > > expected? Is the mask layer not supposed to have any children?
> > 
> > Right, a mask layer shouldn't have children, the mask layer itself is
supposed
> > to represent the entire mask.
> 
> Ohh, makes sense. Would it be a good idea to add a DCHECK in
Layer::SetMaskLayer
> for this? Makes it obvious to someone reading it.

Yes, that's an excellent idea!

[Khushal, 2016-11-01 23:20:42]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-11-01 23:20:43]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2467043002/#ps60001
(title: "test update")

[Khushal, 2016-11-01 23:21:00]

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2467043002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:48: //    used by the renderer.
On 2016/11/01 21:47:20, ajuma wrote:
> On 2016/11/01 21:42:23, Khushal wrote:
> > On 2016/11/01 21:37:07, ajuma wrote:
> > > On 2016/11/01 21:09:01, Khushal wrote:
> > > > On 2016/11/01 19:27:58, ajuma wrote:
> > > > > It's true that the renderer never uses the HideLayerAndSubtree call,
but
> > > > there's
> > > > > still the case of a layer with opacity 0 causing its subtree to get
> > skipped.
> > > > > Those layers won't get skipped with the logic below (but that might be
> > fine
> > > --
> > > > > the amount of work saved by skipping is small).
> > > > 
> > > > You're right, thanks! I've added that in the comment. Will come back
here
> > > later,
> > > > I'm going to leave the bug open for now.
> > > > 
> > > > Also, while writing the test, I ran into a problem because
> > > > CallFunctionForEveryLayer wouldn't run on a mask layer's subtree. Is
that
> > > > expected? Is the mask layer not supposed to have any children?
> > > 
> > > Right, a mask layer shouldn't have children, the mask layer itself is
> supposed
> > > to represent the entire mask.
> > 
> > Ohh, makes sense. Would it be a good idea to add a DCHECK in
> Layer::SetMaskLayer
> > for this? Makes it obvious to someone reading it.
> 
> Yes, that's an excellent idea!

Done. :)

[commit-bot: I haz the power, 2016-11-01 23:21:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 00:10:44]

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

[commit-bot: I haz the power, 2016-11-02 00:10:45]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Khushal, 2016-11-02 00:18:15]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-11-02 00:18:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 00:46:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-02 00:52:28]

Description was changed from

==========
cc/blimp: Eliminate Layer updates for common cases.

In the threaded compositor, we optimize out updates for layers that
will not be drawn after building/updating PropertyTrees. Use some of
that logic in the remote LayerTreeHost.

BUG=650885
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc/blimp: Eliminate Layer updates for common cases.

In the threaded compositor, we optimize out updates for layers that
will not be drawn after building/updating PropertyTrees. Use some of
that logic in the remote LayerTreeHost.

BUG=650885
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/a0679741aca67ad446f6262645d8d65e8d0ab802
Cr-Commit-Position: refs/heads/master@{#429165}
==========

[commit-bot: I haz the power, 2016-11-02 00:52:29]

Patchset 4 (id:??) landed as
https://crrev.com/a0679741aca67ad446f6262645d8d65e8d0ab802
Cr-Commit-Position: refs/heads/master@{#429165}