New issue
Advanced search Search tips

Issue 881011 link

Starred by 1 user
Status: Fixed
Owner:
bokan@chromium.org
Closed: Dec 11
Cc:
danakj@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Investigate whether LayerTreeHost::SetEventListenerProperties sets correct bits on layer tree

Project Member Reported by bokan@chromium.org, Sep 5 
Comment from danakj@ in https://chromium-review.googlesource.com/c/chromium/src/+/1207912/3/cc/trees/layer_tree_host.cc#1077

"""
Actually this doesn't seem true. SetNeedsFullTreeSync() will try to rebuild the actual layers. Since changing a property here does not add/remove a layer it will simply recycle all layers in the pending tree, reconstructing the same tree as before. It does not cause push properties to run at all. You might then think the SetSubtreePropertyChanged will cause PushPropertiesTo but that's also not the case. It just sets a bit on the layer saying it needs to be redrawn on the screen. It does not add it to the set of layers that need push properties. So basically I have no idea what any of this block of code is for. I thought what it's trying to do is set PushPropertiesTo on everything, without walking the layers directly here. But I can't see that it is doing that.
"""
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 11 

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

commit 6a4a425fdd7343dab3948c2dce293c109e0aabf8
Author: David Bokan <bokan@chromium.org>
Date: Tue Dec 11 18:00:32 2018

[BlinkGenPropertyTrees] Fix layer commit for wheel handler region

When a wheel event listener is added or removed on a page, we set each
layer as having a WheelEventHandlerRegion equal to the entire bounds of
the layer. This happens in Layer::PushPropertiesTo.

This means that when an event listener is changed, we need to cause all
layers to need a push properties and commit. Prior to this CL, this
happens in LayerTreeHost::SetEventListenerProperties by setting
SetNeedsFullTreeSync and SetSubtreePropertyChanged. However, as noted by
surrounding comments, this looks wrong; SetNeedsFullTreeSync only causes
the tree structure to be synced, properties aren't pushed.
SetSubtreePropertyChanged causes a property sync only on the root layer.

This works prior to BGPT because SetNeedsFullTreeSync causes a property
tree rebuild during which all layers are marked as needing a property
push. See PropertyTreeBuilderContext::BuildPropertyTreesInternal which
calls SetLayerPropertyTreeChangedForChild for all child layers as the
tree is built.

When BGPT is turned on, property trees are built in Blink independent of
the layer tree so this doesn't cause us to SetNeedsPushProperties. This
CL fixes SetEventListenerProperties to set the correct flags to
propagate the listener state to the active tree.

This CL fixes:

PointerLockBrowserTest.PointerLockWheelEventRouting
TouchpadPinchBrowserTest.WheelListenerAllowingPinch/0
TouchpadPinchBrowserTest.WheelListenerAllowingPinch/1

With BGPT turned on.

Bug:  912334 , 881011 
Change-Id: I4446ccfd32de12e618219e5fa15c48f5093bcc98
Reviewed-on: https://chromium-review.googlesource.com/c/1369059
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615579}
[modify] https://crrev.com/6a4a425fdd7343dab3948c2dce293c109e0aabf8/cc/trees/layer_tree_host.cc

Comment 2 by bokan@chromium.org, Dec 11

Status: Fixed (was: Assigned)
►

Sign in to add a comment