Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2619383004:
Revise LayerTreeHostImpl to use new element-id-to-node-index maps. (Closed)
DescriptionRevise LayerTreeHostImpl to use new element-id-to-node-index maps.
Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups.
BUG=674258
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2619383004
Cr-Commit-Position: refs/heads/master@{#442743}
Committed: https://chromium.googlesource.com/chromium/src/+/d7ab6d03146c909de6078e21b59a4cdeb0b233d2
Patch Set 1 #
Total comments: 5
Patch Set 2 : Integrate feedback. #Patch Set 3 : Fix missing _EQ. #Patch Set 4 : Sync to head. #Messages
Total messages: 26 (20 generated)
Description was changed from ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 ========== to ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel,master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wkorman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL can not be processed by CQ because of: * Failed to parse additional trybots
Description was changed from ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel,master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wkorman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wkorman@chromium.org changed reviewers: + ajuma@chromium.org
Dependent on the other referenced dependent patchset which I'll submit first. This change is step 2 from http://crbug.com/674258#c1. https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3897: const int effect_node_index = All unit and layout tests seem to pass (locally, CQ in flight) with this change, so I'm not sure whether we still need a DCHECK here (and similar below), thought to solicit feedback. The logic for the new maps only inserts an (element id -> node index) if the layer has an element id, so if it doesn't exist when we go to look it up I think we'd get a "default value" as 0 per: http://stackoverflow.com/a/25517832 It looks like these methods are only called if we actually have an animation and so an element id (see ElementAnimations::On{Filter,Opacity,Transform,ScrollOffset}Animated), which means we should have an associated effect/transform node. It's possible there is some edge case around animations being added/deleted in conjunction with layer/property state changing that could cause problems but I would expect that to show up as at least a few test failures. Since we're just switching from using one map to a different map I also don't see an obvious need to add/change unit tests but again feedback welcome.
https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3897: const int effect_node_index = On 2017/01/09 23:46:16, wkorman wrote: > All unit and layout tests seem to pass (locally, CQ in flight) with this change, > so I'm not > sure whether we still need a DCHECK here (and similar below), thought to solicit > feedback. > > The logic for the new maps only inserts an (element id -> node index) if the > layer has an element > id, so if it doesn't exist when we go to look it up I think we'd get a "default > value" as 0 per: > http://stackoverflow.com/a/25517832 What if we DCHECK before looking up? e.g.: DCHECK(element_id_to_effect_node_index.count(element_id) == 1); > It looks like these methods are only called if we actually have an animation and > so an element id > (see ElementAnimations::On{Filter,Opacity,Transform,ScrollOffset}Animated), > which means we > should have an associated effect/transform node. It's possible there is some > edge case around > animations being added/deleted in conjunction with layer/property state changing > that could > cause problems but I would expect that to show up as at least a few test > failures. > > Since we're just switching from using one map to a different map I also don't > see an obvious > need to add/change unit tests but again feedback welcome. Agreed. https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3926: const int layer_id = tree->LayerIdByElementId(element_id); You can use LayerByElementId here to get the layer directly (without layer_id as an intermediate step).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3897: const int effect_node_index = On 2017/01/10 at 00:39:11, ajuma wrote: > What if we DCHECK before looking up? e.g.: > DCHECK(element_id_to_effect_node_index.count(element_id) == 1); Done. https://codereview.chromium.org/2619383004/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:3926: const int layer_id = tree->LayerIdByElementId(element_id); On 2017/01/10 at 00:39:11, ajuma wrote: > You can use LayerByElementId here to get the layer directly (without layer_id as an intermediate step). Done.
The CQ bit was checked by wkorman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org Link to the patchset: https://codereview.chromium.org/2619383004/#ps50001 (title: "Sync to head.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 50001, "attempt_start_ts": 1484091217875530,
"parent_rev": "a37e7afabb6be7d7a12667fb29d9f74556589abf", "commit_rev":
"d7ab6d03146c909de6078e21b59a4cdeb0b233d2"}
Message was sent while issue was closed.
Description was changed from ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Revise LayerTreeHostImpl to use new element-id-to-node-index maps. Improves on element-id-to-layer-id followed by layer-id-to-node-index lookups. BUG=674258 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2619383004 Cr-Commit-Position: refs/heads/master@{#442743} Committed: https://chromium.googlesource.com/chromium/src/+/d7ab6d03146c909de6078e21b59a... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:50001) as https://chromium.googlesource.com/chromium/src/+/d7ab6d03146c909de6078e21b59a... |
