New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 614840 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 614151



Sign in to add a comment

Push properties of a layer shouldn't depend on push properties of other layers

Project Member Reported by jaydasika@chromium.org, May 25 2016

Issue description

Scrollbar layers assume that the properties of the scroll layer (that they are assosiated with) are already pushed. This assumption was true when we were pushing layer properties by traversing the layer tree (as parent would get pushed before child). But now we maintain a list of ids of layers that need to push properties and we cannot guarantee any order in this.

More specifically, this call chain may or may not work based on which order we push layers:

ScrollbarLayerImplBase::PushPropertiesTo ->  ScrollbarLayerImplBase::SetScrollLayerId -> LTI::RegisterScrollbar ->
LTI::DidUpdateScrollState -> LayerById(scroll_layer_id)->scroll_clip_layer_id().


 
Cc: sunxd@chromium.org
Pushing layers using a set of ids is in M-51. But, we haven't seen any bug reports related to this so far(?). Its most likely because solid color scrollbars are hidden until first scroll and we update the scroll state as soon as we scroll, so things become right after that.

Having said that, this is a weird dependency to have. And also, we don't want the concept of scroll clip layers in SPv2, so this needs to be sorted out.

Possible ways of fixing this :
1) Put the info on scroll tree.

2) We already have a clip_scroll_map in LayerTreeImpl. Have the same map in LayerTreeHost and push it onto LayerTreeImpl (instead of constructing it on LayerTreeImpl directly) and use it instead of layer->scroll_clip_layer_id().

3) Don't add scrollbar layers to push properties set and push scrollbar layers whenever(and after) the associated scroll layer is pushed. This way we can ensure scroll layer is up-to-date before updating scrollbars.

1) seems like the best solution to me. It will play well with SPv2(?), but I don't know what's the complexity involved or if there are other dependencies that are blocking us from moving this onto the scroll tree.

For 2) we will need to push another map. Pushing maps at commit/activation seems(even small ones) like a costly operation ( crbug.com/613433 ) and it will be better if we can avoid that.

3) is more of a workaround than a solution, I think.
  
Also, 2 and 3 don't really solve the problem from SPv2 point of view.

Comment 3 by aelias@chromium.org, May 26 2016

Cc: bokan@chromium.org
FWIW, the mapping complexity around scrollbars is almost entirely for the root viewport.  There is a single pair of root scrollbars, but two viewports, the inner (a.k.a. visual viewport) and outer (a.k.a. layout viewport), and the scrollbars reflect the sum of both.  A very important nuance is that bounds of both viewports are adjusted dynamically on the impl side (the inner due to pinch zoom, the outer to reposition bottom-fixed elements during top controls hiding).

> we don't want the concept of scroll clip layers in SPv2

Well, if you want to kill the concept and represent the same info on the scroll tree, that sounds fine to me.  I also don't think this all needs to be expressed quite so generically as it is today -- more special-casing for the root viewport would make the code easier to understand.  Just make sure to preserve all their relevant properties, including the fact that they can resize on the impl side.  There are quite a few subtleties here so please involve bokan@ in reviewing your changes.
Owner: jaydasika@chromium.org
Status: Fixed (was: Available)
This is fixed by https://codereview.chromium.org/2014533005

Sign in to add a comment