New issue
Advanced search Search tips

Issue 695681 link

Starred by 1 user
Status: Started
Owner:
weiliangc@chromium.org
Cc:
danakj@chromium.org
ajuma@chromium.org
enne@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

SetNeeds on cc::Layer interface is confusing.

Project Member Reported by weiliangc@chromium.org, Feb 23 2017 
On cc::Layer there are different calls that gets triggered by property change, and sometimes confusing to which one should be called.

Currently there are:
SetNeedsUpdateLayer
SetNeedsCommit (w/ and w/o rebuilding tree, and full tree sync)
SetNeedsPushProperties

NeedsUpdateLayer leads to potential commit, and should live on LayerTreeHost instead of Layer.

NeedsPushProperties should lead to a commit, unless it is called inside a commit.

So step forward:
- Remove SetNeedsUpdateLayer
- Merge SetNeedsPushProperties and SetNeedsCommit use cases.

Comment 1 by ajuma@chromium.org, Feb 24 2017 

Another possible step forward: make the distinction between SetNeedsCommit and SetNeedsCommitNoRebuild more clear. Maybe the default SetNeedsCommit shouldn't rebuild, and we should have a SetNeedsRebuildPropertyTrees that both commits and rebuilds?

Comment 2 by weiliangc@chromium.org, Feb 24 2017 

I think of there are three levels of severity when SetNeedsCommit, though still trying to think of a way better than developer has to decide what needs rebuild at callsite.
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d914bf6e03df6f7649638827df8d2e4de95192c

commit 1d914bf6e03df6f7649638827df8d2e4de95192c
Author: weiliangc <weiliangc@chromium.org>
Date: Mon Feb 27 21:10:35 2017

cc: Remove SetNeedsUpdate function from cc::Layer

Remove SetNeedsUpdate call from cc::Layer to try reduce confusion.
Move animation use case to LayerTreeHost direct, and call LayerTreeHost
directly in one remaining case.

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

Review-Url: https://codereview.chromium.org/2716583004
Cr-Commit-Position: refs/heads/master@{#453335}

[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/BUILD.gn
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/layers/layer.cc
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/layers/layer.h
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/layers/layer_unittest.cc
[delete] https://crrev.com/028009c724049003f9e3e25cbcaef43f26bf59c8/cc/test/layer_internals_for_test.cc
[delete] https://crrev.com/028009c724049003f9e3e25cbcaef43f26bf59c8/cc/test/layer_internals_for_test.h
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/trees/layer_tree_host.h
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/trees/layer_tree_host_unittest.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d914bf6e03df6f7649638827df8d2e4de95192c

commit 1d914bf6e03df6f7649638827df8d2e4de95192c
Author: weiliangc <weiliangc@chromium.org>
Date: Mon Feb 27 21:10:35 2017

cc: Remove SetNeedsUpdate function from cc::Layer

Remove SetNeedsUpdate call from cc::Layer to try reduce confusion.
Move animation use case to LayerTreeHost direct, and call LayerTreeHost
directly in one remaining case.

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

Review-Url: https://codereview.chromium.org/2716583004
Cr-Commit-Position: refs/heads/master@{#453335}

[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/BUILD.gn
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/layers/layer.cc
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/layers/layer.h
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/layers/layer_unittest.cc
[delete] https://crrev.com/028009c724049003f9e3e25cbcaef43f26bf59c8/cc/test/layer_internals_for_test.cc
[delete] https://crrev.com/028009c724049003f9e3e25cbcaef43f26bf59c8/cc/test/layer_internals_for_test.h
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/trees/layer_tree_host.h
[modify] https://crrev.com/1d914bf6e03df6f7649638827df8d2e4de95192c/cc/trees/layer_tree_host_unittest.cc

Comment 5 by weiliangc@chromium.org, Mar 14 2017 

One idea to merge SetNeedsCommit w/ and w/o property tree rebuild is move logic that determines whether property tree needs rebuild to property trees.

This would involve some clean up on property trees. (moving id to index map to individual property tree and such)
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 14 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d18473730c4e536b78ad19abec5358945aaa757

commit 0d18473730c4e536b78ad19abec5358945aaa757
Author: weiliangc <weiliangc@chromium.org>
Date: Tue Mar 14 23:04:47 2017

cc: Remove friend class TransformTree from PropertyTree

In transform tree, call the base class instead of accessing private.

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

Review-Url: https://codereview.chromium.org/2750853003
Cr-Commit-Position: refs/heads/master@{#456879}

[modify] https://crrev.com/0d18473730c4e536b78ad19abec5358945aaa757/cc/trees/property_tree.cc
[modify] https://crrev.com/0d18473730c4e536b78ad19abec5358945aaa757/cc/trees/property_tree.h
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 17 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/42c074f058c39ca559d47e168dade668b5ec419e

commit 42c074f058c39ca559d47e168dade668b5ec419e
Author: weiliangc <weiliangc@chromium.org>
Date: Fri Mar 17 19:28:30 2017

cc: Move Layer Id to Node Map to Individual Property Tree Private

Move the layer id to node index map to individual property tree, and
make this map private. This helps control of access to these maps.

BUG=695681
R=ajuma
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2753933005
Cr-Commit-Position: refs/heads/master@{#457850}

[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/input/scrollbar_animation_controller.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/layers/layer.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/layers/layer_impl.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/layers/layer_impl.h
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/layers/render_surface_impl.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/trees/property_tree.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/trees/property_tree.h
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/cc/trees/property_tree_builder.cc
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
[modify] https://crrev.com/42c074f058c39ca559d47e168dade668b5ec419e/third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57

commit 1c0802c3c4c03b43efea23b82d7e624f1d8c7b57
Author: weiliangc <weiliangc@chromium.org>
Date: Tue Mar 21 19:53:17 2017

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}

[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/layer.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/layer.h
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/layer_impl.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/surface_layer.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/draw_property_utils.h
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/property_tree.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/property_tree.h
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/property_tree_builder.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57

commit 1c0802c3c4c03b43efea23b82d7e624f1d8c7b57
Author: weiliangc <weiliangc@chromium.org>
Date: Tue Mar 21 19:53:17 2017

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}

[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/layer.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/layer.h
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/layer_impl.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/layers/surface_layer.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/draw_property_utils.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/draw_property_utils.h
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host_common.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/property_tree.cc
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/property_tree.h
[modify] https://crrev.com/1c0802c3c4c03b43efea23b82d7e624f1d8c7b57/cc/trees/property_tree_builder.cc
►

Sign in to add a comment