New issue
Advanced search Search tips

Issue 655795 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Nov 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 653371



Sign in to add a comment

LayerTreeHostRemote doesn't build property tree

Project Member Reported by xingliu@chromium.org, Oct 13 2016

Issue description

In layer_tree_host_unittest.cc, some test cases will compare property tree states between LTH remote and LTH impl, these will fail since there is no property tree in LTH remote.

We probably want to figure out if we can modify code to enable those tests for LTH remote.
 
Blocking: 653371
So there can be a problem in 2 cases, if they end up comparing data in the PropertyTrees itself from the main copy to pending/active copy. Or if they end up comparing data set as a result of building PropertyTrees, for instance |transform_tree_index_| on the Layer with a LayerImpl, since the Layer on the engine will never have correct values for these.

We should make a note of which tests have this problem here and then we can come back to seeing if there is a clean strategy to try to enable them.

LayerTreeHostTestPushNodeOwnerToNodeIdMap -> compares tree indexes from Layers.
Also some cc::Layer subclass uses property tree in the main thread, which means the existence of property tree might be an underlying assumption. This issue will also cause the test crash since LTH remote doesn't build the property tree.

Such as PaintedScrollbarLayer, PaintedScrollbarLayer::Update will use the transform tree.


We don't need to worry about tests that have PaintedScrollbar layers. They are disabled in blimp. We should only use solid color scrollbar layers.

Though I ran into issues with blink not expecting cc to pull display lists for layers that were not drawable, at GraphicsLayer::getPaintController. So I'll look into building PropertyTrees on the engine side just for draw_property_utils::FindLayersThatNeedUpdates and so there are no other surprises. They still shouldn't need to be sent to the client.
This raised another issue in test case LayerTreeHostTestLayersPushProperties. (when num_commits_ is 12)

In normal LTH in process flow, property tree will be build and 
|subtree_property_changed_| will get propagated recursively through layer tree hierarchy. During the recursion, when a layer's |subtree_property_changed_| changed from false to true, it will call SetNeedsPushProperties.

In LTH remote, we don't build property tree on the engine, so |subtree_property_changed_| only will get propagate on the LTH in process on the client side, but the test is verifying the engine layer tree so the states will be incorrect. Also we don't clear |subtree_property_changed_| in Layer::ToLayerPropertiesProto. Changing this will cause another unit test to fail.

-------------------------------------------------------------------------------

Call stack where building property tree propagates |subtree_property_changed_|  and trigger SetNeedsPushProperties.

#2 0x000001076c61 cc::PushPropertiesCountingLayer::SetNeedsPushProperties()
#3 0x7f5d9fdebc5e cc::Layer::SetSubtreePropertyChanged()
#4 0x7f5da00fa79f cc::(anonymous namespace)::SetLayerPropertyChangedForChild()
#5 0x7f5da00f70e2 cc::(anonymous namespace)::BuildPropertyTreesInternal<>()
#6 0x7f5da00f711e cc::(anonymous namespace)::BuildPropertyTreesInternal<>()
#7 0x7f5da0100cd3 cc::BuildPropertyTreesTopLevelInternal<>()
#8 0x7f5da00f67ce cc::PropertyTreeBuilder::BuildPropertyTrees()
#9 0x7f5da00967ff cc::LayerTreeHostInProcess::DoUpdateLayers()

Also on test case LayerTreeHostTestImplLayersPushProperties.

Since remote LTH doesn't build property tree, the layers need to push properties on client and engine are actually different, so it doesn't make sense to compare the push properties count between engine and client(both main thread and impl thread tree).
Status: WontFix (was: Untriaged)

Sign in to add a comment