cc: Remove SetNeedsCommitNoRebuild from Layer API

cc: Remove SetNeedsCommitNoRebuild from Layer API

Remove SetNeedsCommitNoRebuild, and Layer::SetNeedsCommit simply calls
LayerTreeHost::SetNeedsCommit.

This means now property trees should be resposible for its own rebuild
when trying to update a node that does not exist yet on main thread.

On Layer, now some property setting functions need to call property
trees to rebuild. These should no longer be needed from Blink once SPv2
is in place.

BUG=695681
R=ajuma, danakj
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2755803003
Cr-Commit-Position: refs/heads/master@{#458521}
Committed: https://chromium.googlesource.com/chromium/src/+/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57

[weiliangc, 2017-03-16 20:02:28]

Description was changed from

==========
cc: Remove SetNeedsCommitNoRebuild from Layer API

Remove SetNeedsCommitNoRebuild, and Layer::SetNeedsCommit simply calls
LayerTreeHost::SetNeedsCommit.

This means now property trees should be resposible for its own rebuild
when trying to update a node that does not exist yet on main thread.

On Layer, now some property setting functions need to call property
trees to rebuild. These should no longer be needed from Blink once SPv2
is in place.

BUG=695681
R=ajuma, danakj
==========

to

==========
cc: Remove SetNeedsCommitNoRebuild from Layer API

Remove SetNeedsCommitNoRebuild, and Layer::SetNeedsCommit simply calls
LayerTreeHost::SetNeedsCommit.

This means now property trees should be resposible for its own rebuild
when trying to update a node that does not exist yet on main thread.

On Layer, now some property setting functions need to call property
trees to rebuild. These should no longer be needed from Blink once SPv2
is in place.

BUG=695681
R=ajuma, danakj
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[weiliangc, 2017-03-16 20:02:51]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 20:03:26]

Dry run: 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, 2017-03-16 21:38:29]

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

[commit-bot: I haz the power, 2017-03-16 21:38:31]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-03-16 22:26:57]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 22:27:26]

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

[weiliangc, 2017-03-16 22:29:28]

Trying to make property tree understand more about when to rebuild.

[commit-bot: I haz the power, 2017-03-16 22:39:47]

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

[commit-bot: I haz the power, 2017-03-16 22:39:47]

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

[ajuma, 2017-03-16 23:42:12]

Really happy to see SetNeedsCommitNoRebuild getting deleted!

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:832: inputs_.user_scrollable_vertical = vertical;
These values go into ScrollNode, so we should update the node here (or trigger a
rebuild).

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1354: continue;
This check can go outside the switch, then it doesn't need to be repeated for
each property.

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:85: virtual void set_needs_update(bool needs_update) {
Since we don't ever have a base class pointer, do we need virtual here? (e.g.
see https://codereview.chromium.org/1940073002 which removed some virtuals from
PropertyTree.)

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:109: property_trees()->needs_rebuild = true;
Assuming the DCHECK doesn't always hold, this should be an 'if' so that
needs_rebuild is never true on the compositor thread outside of tests.

[ajuma, 2017-03-17 00:00:37]

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:85: virtual void set_needs_update(bool needs_update) {
On 2017/03/16 23:42:11, ajuma wrote:
> Since we don't ever have a base class pointer, do we need virtual here? (e.g.
> see https://codereview.chromium.org/1940073002 which removed some virtuals
from
> PropertyTree.)

Actually, after reading https://codereview.chromium.org/1940073002#msg11 I take
back my comment, using virtual here seems fine.

[danakj, 2017-03-17 14:17:47]

Compiler errors like

e:\b\c\b\win\src\third_party\webkit\source\platform\graphics\compositing\propertytreemanager.cpp(114):
error C2039: 'InsertOwningIdForNode': is not a member of 'cc::ClipTree'

[danakj, 2017-03-17 14:26:52]

Did you look thru other layer types that use SNC() right now to make sure they
don't need to set needs rebuild?

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:217: layer_tree_host_->property_trees()->needs_rebuild =
true;
Maybe change these places to use the new method to set this?

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:492: layer_tree_host_->property_trees()->needs_rebuild =
true;
Use the new method? Then you don't need to check if layer_tree_host_ here also
fwiw

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:651: layer_tree_host_->property_trees()->needs_rebuild =
true;
Use new method?

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:538: void PropertyTreesNeedRebuild();
This function needs a verb at the front of it. "Do" "Set" "Can" "Should" etc..
In this case it looks like Set would be right?

[weiliangc, 2017-03-17 20:57:54]

On 2017/03/17 at 14:26:52, danakj wrote:
> Did you look thru other layer types that use SNC() right now to make sure they
don't need to set needs rebuild?

Yeah, all the other layer types calls SNC() on property that does not exist on
Layer base class, and property builder only access base layer class.
> 
> https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
> cc/layers/layer.cc:217: layer_tree_host_->property_trees()->needs_rebuild =
true;
> Maybe change these places to use the new method to set this?
> 
>
https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
> cc/layers/layer.cc:492: layer_tree_host_->property_trees()->needs_rebuild =
true;
> Use the new method? Then you don't need to check if layer_tree_host_ here also
fwiw
> 
>
https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
> cc/layers/layer.cc:651: layer_tree_host_->property_trees()->needs_rebuild =
true;
> Use new method?
> 
> https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.h
> File cc/layers/layer.h (right):
> 
>
https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.h#newco...
> cc/layers/layer.h:538: void PropertyTreesNeedRebuild();
> This function needs a verb at the front of it. "Do" "Set" "Can" "Should" etc..
In this case it looks like Set would be right?

[weiliangc, 2017-03-17 20:58:42]

Address review comments. Compile failure came from me forgot to change some
function call on call site in previous patch. :D

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:492: layer_tree_host_->property_trees()->needs_rebuild =
true;
On 2017/03/17 at 14:26:51, danakj wrote:
> Use the new method? Then you don't need to check if layer_tree_host_ here also
fwiw

Done.

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:651: layer_tree_host_->property_trees()->needs_rebuild =
true;
On 2017/03/17 at 14:26:51, danakj wrote:
> Use new method?

Done.

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1354: continue;
On 2017/03/16 at 23:42:11, ajuma wrote:
> This check can go outside the switch, then it doesn't need to be repeated for
each property.

Done.

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:538: void PropertyTreesNeedRebuild();
On 2017/03/17 at 14:26:51, danakj wrote:
> This function needs a verb at the front of it. "Do" "Set" "Can" "Should" etc..
In this case it looks like Set would be right?

Done with Set.

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:85: virtual void set_needs_update(bool needs_update) {
On 2017/03/17 at 00:00:37, ajuma wrote:
> On 2017/03/16 23:42:11, ajuma wrote:
> > Since we don't ever have a base class pointer, do we need virtual here?
(e.g.
> > see https://codereview.chromium.org/1940073002 which removed some virtuals
from
> > PropertyTree.)
> 
> Actually, after reading https://codereview.chromium.org/1940073002#msg11 I
take back my comment, using virtual here seems fine.

Change the transform tree override to final.

https://codereview.chromium.org/2755803003/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.h:109: property_trees()->needs_rebuild = true;
On 2017/03/16 at 23:42:12, ajuma wrote:
> Assuming the DCHECK doesn't always hold, this should be an 'if' so that
needs_rebuild is never true on the compositor thread outside of tests.

Talked w/ Ali, can make the DCHECK work if we actually tick animation by
ElementID, which I'll do in another patch.

[weiliangc, 2017-03-17 20:58:48]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-17 20:59:17]

Dry run: 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, 2017-03-17 22:52:26]

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

[commit-bot: I haz the power, 2017-03-17 22:52:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[ajuma, 2017-03-17 23:32:17]

Just one more thing:

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:832: inputs_.user_scrollable_vertical = vertical;
On 2017/03/16 23:42:11, ajuma wrote:
> These values go into ScrollNode, so we should update the node here (or trigger
a
> rebuild).

Looks like this still needs to be added.

[danakj, 2017-03-20 15:58:12]

LGTM % ajuma

https://codereview.chromium.org/2755803003/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/40001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:795:
property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
Just checking but this is setting needs_rebuild true, but before we only set
needs_rebuild to true if we *didnt* go in this path?

[weiliangc, 2017-03-20 18:17:10]

Addressed review comments.

https://codereview.chromium.org/2755803003/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/40001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:795:
property_trees->transform_tree.UpdateNodeFromOwningLayerId(id())) {
On 2017/03/20 15:58:12, danakj wrote:
> Just checking but this is setting needs_rebuild true, but before we only set
> needs_rebuild to true if we *didnt* go in this path?

This sets needs_rebuild to true if we didn't find the transform node. Which is
equivalent of the old code, where if we find the transform node, don't set
needs_rebuild to true.

[ajuma, 2017-03-20 18:22:49]

Thanks, lgtm

https://codereview.chromium.org/2755803003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:834:
layer_tree_host_->property_trees()->scroll_tree.set_needs_update(true);
Is this line needed? The scroll tree update doesn't seem to depend on
user_scrollable_*.

[weiliangc, 2017-03-20 19:32:49]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 19:33:16]

Dry run: 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, 2017-03-20 20:12:11]

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

[commit-bot: I haz the power, 2017-03-20 20:12:13]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[ajuma, 2017-03-20 20:30:41]

https://codereview.chromium.org/2755803003/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2755803003/diff/60001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:830: if (ScrollNode* node =
layer_tree_host_->property_trees()
Also, need to check for a null layer_tree_host here.

[weiliangc, 2017-03-21 17:12:26]

The CQ bit was checked by weiliangc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 17:13:07]

Dry run: 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, 2017-03-21 19:45:42]

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

[commit-bot: I haz the power, 2017-03-21 19:45:43]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-03-21 19:45:52]

The CQ bit was checked by weiliangc@chromium.org

[weiliangc, 2017-03-21 19:45:53]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2755803003/#ps100001
(title: "check for LTH")

[commit-bot: I haz the power, 2017-03-21 19:46:29]

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, 2017-03-21 19:53:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490125552536800,
"parent_rev": "1cebb2d2559139ff720a3302aca1b9085e31b28c", "commit_rev":
"1c0802c3c4c03b43efea23b82d7e624f1d8c7b57"}

[commit-bot: I haz the power, 2017-03-21 19:54:06]

Description was changed from

==========
cc: Remove SetNeedsCommitNoRebuild from Layer API

Remove SetNeedsCommitNoRebuild, and Layer::SetNeedsCommit simply calls
LayerTreeHost::SetNeedsCommit.

This means now property trees should be resposible for its own rebuild
when trying to update a node that does not exist yet on main thread.

On Layer, now some property setting functions need to call property
trees to rebuild. These should no longer be needed from Blink once SPv2
is in place.

BUG=695681
R=ajuma, danakj
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Remove SetNeedsCommitNoRebuild from Layer API

Remove SetNeedsCommitNoRebuild, and Layer::SetNeedsCommit simply calls
LayerTreeHost::SetNeedsCommit.

This means now property trees should be resposible for its own rebuild
when trying to update a node that does not exist yet on main thread.

On Layer, now some property setting functions need to call property
trees to rebuild. These should no longer be needed from Blink once SPv2
is in place.

BUG=695681
R=ajuma, danakj
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2755803003
Cr-Commit-Position: refs/heads/master@{#458521}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c0802c3c4c03b43efea23b82d7e...
==========

[commit-bot: I haz the power, 2017-03-21 19:54:07]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/1c0802c3c4c03b43efea23b82d7e...