Push properties of a layer shouldn't depend on push properties of other layers |
|||
Issue descriptionScrollbar 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().
,
May 26 2016
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.
,
May 26 2016
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.
,
Jun 21 2016
This is fixed by https://codereview.chromium.org/2014533005 |
|||
►
Sign in to add a comment |
|||
Comment 1 by jaydasika@chromium.org
, May 25 2016