Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1840883002:
cc : Calculate jitter without using layer hierarchy (Closed)
Descriptioncc : Calculate jitter without using layer hierarchy
BUG=557194
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/eaa8d0855c8b55c74d2c86449154d4d4316d6182
Cr-Commit-Position: refs/heads/master@{#383870}
Patch Set 1 #Patch Set 2 : #
Total comments: 9
Patch Set 3 : #Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Messages
Total messages: 20 (6 generated)
Description was changed from ========== cc : Calculate jitter without using layer hierarchy BUG= ========== to ========== cc : Calculate jitter without using layer hierarchy BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
jaydasika@chromium.org changed reviewers: + ajuma@chromium.org, vollick@chromium.org, weiliangc@chromium.org
PTAL
https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:623: int LayerTreeHostCommon::CalculateFrameJitter(LayerImpl* layer) { "CalculateLayerJitter" or "CalculateJitter"? https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:1081: // is enough to calculate jitter on one of these layers. Please clarify this comment to say that once we've found a jittering layer, we don't need to consider other layers with the same scroll tree index (but until then, we compute the jitter for each layer). https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; We already have a currently_scrolling_node_id on the scroll tree, so do we still need this id?
https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common.cc (right): https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:623: int LayerTreeHostCommon::CalculateFrameJitter(LayerImpl* layer) { On 2016/03/29 14:24:02, ajuma wrote: > "CalculateLayerJitter" or "CalculateJitter"? Done. https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common.cc:1081: // is enough to calculate jitter on one of these layers. On 2016/03/29 14:24:02, ajuma wrote: > Please clarify this comment to say that once we've found a jittering layer, we > don't need to consider other layers with the same scroll tree index (but until > then, we compute the jitter for each layer). Done. https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; On 2016/03/29 14:24:02, ajuma wrote: > We already have a currently_scrolling_node_id on the scroll tree, so do we still > need this id? I think we need this as the one on scroll tree is reset when ScrollEnd is called
https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; On 2016/03/29 15:57:24, jaydasika wrote: > On 2016/03/29 14:24:02, ajuma wrote: > > We already have a currently_scrolling_node_id on the scroll tree, so do we > still > > need this id? > > I think we need this as the one on scroll tree is reset when ScrollEnd is called Ah, makes sense. Does this need to get updated on each activation then? (Since, unlike layer ids, scroll node ids aren't stable.)
https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; On 2016/03/29 16:52:07, ajuma wrote: > On 2016/03/29 15:57:24, jaydasika wrote: > > On 2016/03/29 14:24:02, ajuma wrote: > > > We already have a currently_scrolling_node_id on the scroll tree, so do we > > still > > > need this id? > > > > I think we need this as the one on scroll tree is reset when ScrollEnd is > called > > Ah, makes sense. Does this need to get updated on each activation then? (Since, > unlike layer ids, scroll node ids aren't stable.) It gets updated every activation here : https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; On 2016/03/29 18:30:19, jaydasika wrote: > On 2016/03/29 16:52:07, ajuma wrote: > > On 2016/03/29 15:57:24, jaydasika wrote: > > > On 2016/03/29 14:24:02, ajuma wrote: > > > > We already have a currently_scrolling_node_id on the scroll tree, so do we > > > still > > > > need this id? > > > > > > I think we need this as the one on scroll tree is reset when ScrollEnd is > > called > > > > Ah, makes sense. Does this need to get updated on each activation then? > (Since, > > unlike layer ids, scroll node ids aren't stable.) > > It gets updated every activation here : > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... When the layer passed to SetCurrentlyScrollingLayer is null, last_scrolled_scroll_node_id_ isn't updated.
On 2016/03/29 18:50:04, ajuma wrote: > https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h > File cc/trees/layer_tree_impl.h (right): > > https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... > cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; > On 2016/03/29 18:30:19, jaydasika wrote: > > On 2016/03/29 16:52:07, ajuma wrote: > > > On 2016/03/29 15:57:24, jaydasika wrote: > > > > On 2016/03/29 14:24:02, ajuma wrote: > > > > > We already have a currently_scrolling_node_id on the scroll tree, so do > we > > > > still > > > > > need this id? > > > > > > > > I think we need this as the one on scroll tree is reset when ScrollEnd is > > > called > > > > > > Ah, makes sense. Does this need to get updated on each activation then? > > (Since, > > > unlike layer ids, scroll node ids aren't stable.) > > > > It gets updated every activation here : > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > > When the layer passed to SetCurrentlyScrollingLayer is null, > last_scrolled_scroll_node_id_ isn't updated. SetCurrentlyScrollingLayer(null) is called when scrolling ends. So, we don't want last_scrolled_scroll_node_id_ to be updated in that case.
On 2016/03/29 19:48:07, jaydasika wrote: > On 2016/03/29 18:50:04, ajuma wrote: > > > https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h > > File cc/trees/layer_tree_impl.h (right): > > > > > https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... > > cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; > > On 2016/03/29 18:30:19, jaydasika wrote: > > > On 2016/03/29 16:52:07, ajuma wrote: > > > > On 2016/03/29 15:57:24, jaydasika wrote: > > > > > On 2016/03/29 14:24:02, ajuma wrote: > > > > > > We already have a currently_scrolling_node_id on the scroll tree, so > do > > we > > > > > still > > > > > > need this id? > > > > > > > > > > I think we need this as the one on scroll tree is reset when ScrollEnd > is > > > > called > > > > > > > > Ah, makes sense. Does this need to get updated on each activation then? > > > (Since, > > > > unlike layer ids, scroll node ids aren't stable.) > > > > > > It gets updated every activation here : > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > > > > When the layer passed to SetCurrentlyScrollingLayer is null, > > last_scrolled_scroll_node_id_ isn't updated. > > SetCurrentlyScrollingLayer(null) is called when scrolling ends. So, we don't > want > last_scrolled_scroll_node_id_ to be updated in that case. Right, the problem is that if the currently scrolling layer is null during activation, then we won't update the last_scrolled_scroll_node_id_ during activation, so if the scroll tree has changed, we could have a stale id.
On 2016/03/29 21:22:02, ajuma wrote: > On 2016/03/29 19:48:07, jaydasika wrote: > > On 2016/03/29 18:50:04, ajuma wrote: > > > > > > https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_impl.h > > > File cc/trees/layer_tree_impl.h (right): > > > > > > > > > https://codereview.chromium.org/1840883002/diff/20001/cc/trees/layer_tree_imp... > > > cc/trees/layer_tree_impl.h:513: int last_scrolled_scroll_node_id_; > > > On 2016/03/29 18:30:19, jaydasika wrote: > > > > On 2016/03/29 16:52:07, ajuma wrote: > > > > > On 2016/03/29 15:57:24, jaydasika wrote: > > > > > > On 2016/03/29 14:24:02, ajuma wrote: > > > > > > > We already have a currently_scrolling_node_id on the scroll tree, so > > do > > > we > > > > > > still > > > > > > > need this id? > > > > > > > > > > > > I think we need this as the one on scroll tree is reset when ScrollEnd > > is > > > > > called > > > > > > > > > > Ah, makes sense. Does this need to get updated on each activation then? > > > > (Since, > > > > > unlike layer ids, scroll node ids aren't stable.) > > > > > > > > It gets updated every activation here : > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre... > > > > > > When the layer passed to SetCurrentlyScrollingLayer is null, > > > last_scrolled_scroll_node_id_ isn't updated. > > > > SetCurrentlyScrollingLayer(null) is called when scrolling ends. So, we don't > > want > > last_scrolled_scroll_node_id_ to be updated in that case. > > Right, the problem is that if the currently scrolling layer is null during > activation, then we won't update the last_scrolled_scroll_node_id_ during > activation, so if the scroll tree has changed, we could have a stale id. Done.
Patchset #9 (id:160001) has been deleted
Brought back last_scrolled_layer_id to avoid the complexity.
Thanks, lgtm.
The CQ bit was checked by jaydasika@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840883002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840883002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== cc : Calculate jitter without using layer hierarchy BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc : Calculate jitter without using layer hierarchy BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/eaa8d0855c8b55c74d2c86449154d4d4316d6182 Cr-Commit-Position: refs/heads/master@{#383870} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/eaa8d0855c8b55c74d2c86449154d4d4316d6182 Cr-Commit-Position: refs/heads/master@{#383870}
Message was sent while issue was closed.
Description was changed from ========== cc : Calculate jitter without using layer hierarchy BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/eaa8d0855c8b55c74d2c86449154d4d4316d6182 Cr-Commit-Position: refs/heads/master@{#383870} ========== to ========== cc : Calculate jitter without using layer hierarchy BUG=557194 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/eaa8d0855c8b55c74d2c86449154d4d4316d6182 Cr-Commit-Position: refs/heads/master@{#383870} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
