|
|
Created:
4 years, 11 months ago by sunxd Modified:
4 years, 11 months ago Reviewers:
ajuma, weiliangc, Ian Vollick, David Trainor- moved to gerrit, enne (OOO), jaydasika, dtapuska CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ScrollTree builder and unit test
Remove ScrollBlocksOn from ScrollTree:
It will be moved from the code base soon, will work
to integrate the "passive event handler" if that
depends on layer tree hierarchy.
BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/ea1df789e1afcf57a69769b4c4a1d9ef09d112ed
Cr-Commit-Position: refs/heads/master@{#371914}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rename other_criteria, remove is_root in GetScrollParent and fixed a previous mistake in unit test. #
Total comments: 2
Patch Set 3 : ScrollTreeBuilder unit test checks scroll_tree_index() of layers which do not own a node #Patch Set 4 : Change the protobuf indices #
Total comments: 4
Patch Set 5 : Move the NEXT ID comment to the top. #Patch Set 6 : Remove ScrollBlocksOn from ScrollTree #Patch Set 7 : Remove ScrollBlocksOn from ScrollTree #Patch Set 8 : Resolve conflicts when merging with origin/master #Patch Set 9 : Merge with master #
Messages
Total messages: 65 (34 generated)
Description was changed from ========== Add ScrollTree builder and unit test BUG=568830 ========== to ========== Add ScrollTree builder and unit test BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
[email protected] changed reviewers: + [email protected], [email protected], [email protected], [email protected], [email protected]
Hello, This change is to add scroll tree builder but not to use the scroll tree. The scroll tree is still on dead code path. Thanks!
https://codereview.chromium.org/1626513003/diff/1/cc/trees/layer_tree_host_co... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1626513003/diff/1/cc/trees/layer_tree_host_co... cc/trees/layer_tree_host_common_unittest.cc:9882: property_tree_root.data.transform_id = 0; Can you change this to root->transform_tree_index() instead of hard-coding the value? This is to prevent any future changes to the transform tree breaking this test. (Similarly, for all nodes) https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... cc/trees/property_tree_builder.cc:98: const bool inherits_scroll = !layer->parent() || !layer->scroll_parent(); Why do we need !layer->parent() here ? (The only case I think this is handling is, root is its own scroll parent, is that ever possible ?)
https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... cc/trees/property_tree_builder.cc:657: data_from_ancestor.scroll_other_criteria); What's the intuition behind passing scroll_other_criteria to children and using it this way to determine if a node is needed? (Would it help to use a more descriptive name for it?)
https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... cc/trees/property_tree_builder.cc:98: const bool inherits_scroll = !layer->parent() || !layer->scroll_parent(); On 2016/01/22 22:28:28, jaydasika wrote: > Why do we need !layer->parent() here ? (The only case I think this is handling > is, root is its own scroll parent, is that ever possible ?) I made this identical to GetClipParentId, is it the same story in that part? https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... cc/trees/property_tree_builder.cc:657: data_from_ancestor.scroll_other_criteria); On 2016/01/25 15:19:54, ajuma wrote: > What's the intuition behind passing scroll_other_criteria to children and using > it this way to determine if a node is needed? (Would it help to use a more > descriptive name for it?) The idea is that we do not create scroll nodes for: A layer that only should_scroll_on_main_thread, and the scroll_tree_parent is created only because of should_scroll_on_main_thread. The precise name would be scroll_node_uninheritable_criteria? And the name in DataForRecursion can be scroll_tree_parent_created_by_uninheritable_criteria?
https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1626513003/diff/1/cc/trees/property_tree_buil... cc/trees/property_tree_builder.cc:657: data_from_ancestor.scroll_other_criteria); On 2016/01/25 15:37:21, sunxd wrote: > On 2016/01/25 15:19:54, ajuma wrote: > > What's the intuition behind passing scroll_other_criteria to children and > using > > it this way to determine if a node is needed? (Would it help to use a more > > descriptive name for it?) > > The idea is that we do not create scroll nodes for: > > A layer that only should_scroll_on_main_thread, and the scroll_tree_parent is > created only because of should_scroll_on_main_thread. > > The precise name would be scroll_node_uninheritable_criteria? And the name in > DataForRecursion can be scroll_tree_parent_created_by_uninheritable_criteria? Thanks, those names sound clearer. Please also add a comment about this. https://codereview.chromium.org/1626513003/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1626513003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:9825: // SCROLL_BLOCKS_ON_WHEEL_EVENT Which check is this comment referring to?
https://codereview.chromium.org/1626513003/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1626513003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:9825: // SCROLL_BLOCKS_ON_WHEEL_EVENT On 2016/01/25 16:21:59, ajuma wrote: > Which check is this comment referring to? Sorry, I think I forgot it! I should check the scroll_tree_index of all layers who do not own a scroll node.
lgtm
Description was changed from ========== Add ScrollTree builder and unit test BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add ScrollTree builder and unit test BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
[email protected] changed reviewers: + [email protected]
Add dtrainor to the reviewers. Hello David, I have to update the layer properties in layer.proto in this CL. But I'm not sure whether shifting the protobuf indices could result in a problem in blimp. Please take a look at it. Thanks!
On 2016/01/26 15:16:40, sunxd wrote: > Add dtrainor to the reviewers. > > Hello David, > > I have to update the layer properties in layer.proto in this CL. But I'm not > sure whether shifting the protobuf indices could result in a problem in blimp. > Please take a look at it. > > Thanks! It won't break us now because we haven't shipped yet, but generally that will break us in the future wrt versioning. Would you mind just using 50 as the id for the scroll_tree_index and putting a comment up top with something like "// NEXT ID: 51"? I think nyquist@ is doing something like this in another patch as well but I guess it hasn't landed yet. That way future changes can see that it's okay if the ids are out of order :).
On 2016/01/26 15:16:40, sunxd wrote: > Add dtrainor to the reviewers. > > Hello David, > > I have to update the layer properties in layer.proto in this CL. But I'm not > sure whether shifting the protobuf indices could result in a problem in blimp. > Please take a look at it. > > Thanks! It won't break us now because we haven't shipped yet, but generally that will break us in the future wrt versioning. Would you mind just using 50 as the id for the scroll_tree_index and putting a comment up top with something like "// NEXT ID: 51"? I think nyquist@ is doing something like this in another patch as well but I guess it hasn't landed yet. That way future changes can see that it's okay if the ids are out of order :).
Thanks David! I have changed the protobuf indices for scroll tree data structures.
last nit then lgtm. thanks for checking with me! https://codereview.chromium.org/1626513003/diff/60001/cc/proto/layer.proto File cc/proto/layer.proto (right): https://codereview.chromium.org/1626513003/diff/60001/cc/proto/layer.proto#ne... cc/proto/layer.proto:79: // NEXT ID: 51 Ah sorry! Can you put this up top? https://codereview.chromium.org/1626513003/diff/60001/cc/proto/property_tree.... File cc/proto/property_tree.proto (right): https://codereview.chromium.org/1626513003/diff/60001/cc/proto/property_tree.... cc/proto/property_tree.proto:155: optional PropertyTree scroll_tree = 7; Does this one need to be changed? Maybe I'm misreading it.
https://codereview.chromium.org/1626513003/diff/60001/cc/proto/layer.proto File cc/proto/layer.proto (right): https://codereview.chromium.org/1626513003/diff/60001/cc/proto/layer.proto#ne... cc/proto/layer.proto:79: // NEXT ID: 51 On 2016/01/26 16:34:56, David Trainor wrote: > Ah sorry! Can you put this up top? My bad :), move it to the top now. Thanks for pointing out! https://codereview.chromium.org/1626513003/diff/60001/cc/proto/property_tree.... File cc/proto/property_tree.proto (right): https://codereview.chromium.org/1626513003/diff/60001/cc/proto/property_tree.... cc/proto/property_tree.proto:155: optional PropertyTree scroll_tree = 7; On 2016/01/26 16:34:56, David Trainor wrote: > Does this one need to be changed? Maybe I'm misreading it. Sorry this one is changed in the previous CL by me before we realized that it may break blimp in the future.
lgtm too
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/1626513003/#ps80001 (title: "Move the NEXT ID comment to the top.")
The CQ bit was unchecked by [email protected]
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
On 2016/01/26 17:18:55, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) Fixed it, will try it before adding to cq. (Variable defined but not used)
Patchset #6 (id:100001) has been deleted
Patchset #8 (id:160001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/1626513003/diff/340001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/1626513003/diff/340001/cc/layers/layer_impl.h... cc/layers/layer_impl.h:536: return (ScrollBlocksOn)((unsigned int)scroll_blocks_on_); These c-style casts look fishy; I'm also not sure why we need to cast at all. Isn't scroll_blocks_on_ already ScrollBlocksOn? https://codereview.chromium.org/1626513003/diff/340001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1626513003/diff/340001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9965: // if (!(expected_nodes[i] == nodes[i])) { Why the commented code? https://codereview.chromium.org/1626513003/diff/340001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1626513003/diff/340001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:646: << scroll_blocks_on; Should this LOG(ERROR) stuff actually be submitted? Is it just for debugging purposes? One thing you might try if you think that this instrumentation will be useful in the future is to do TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("cc-debug"), "AddScrollNodeIfNeeded", etc, etc); If you do this, you can run with --trace-to-console=-.*,disabled-by-default-cc-debug to send this output to the console.
Sorry but I'm running try jobs on this CL because of a possible bug on ScrollBlocksOn enum type. The details are in the comment in layer_impl.h https://codereview.chromium.org/1626513003/diff/340001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/1626513003/diff/340001/cc/layers/layer_impl.h... cc/layers/layer_impl.h:536: return (ScrollBlocksOn)((unsigned int)scroll_blocks_on_); On 2016/01/27 15:25:51, vollick wrote: > These c-style casts look fishy; I'm also not sure why we need to cast at all. > Isn't scroll_blocks_on_ already ScrollBlocksOn? So somehow only on windows_chromium_x64_rel_ng, I discovered a strange problem: when we call layer::SetScrollBlocksOn(SCROLL_BLOCKS_ON_SCROLL_EVENT), and call scroll_blocks_on() to get the value in the unit test, we end up with -4 instead of SCROLL_BLOCKS_ON_SCROLL_EVENT(which is 4). Probably it is because we store scroll_blocks_on_ as a 3-bit int in both Layer and LayerImpl class. I'm still investigating into it now. https://codereview.chromium.org/1626513003/diff/340001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/1626513003/diff/340001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_common_unittest.cc:9965: // if (!(expected_nodes[i] == nodes[i])) { On 2016/01/27 15:25:51, vollick wrote: > Why the commented code? Sorry, patch 7-14 will be removed after analyzing the try job. https://codereview.chromium.org/1626513003/diff/340001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/1626513003/diff/340001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:646: << scroll_blocks_on; On 2016/01/27 15:25:51, vollick wrote: > Should this LOG(ERROR) stuff actually be submitted? Is it just for debugging > purposes? > > One thing you might try if you think that this instrumentation will be useful in > the future is to do TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("cc-debug"), > "AddScrollNodeIfNeeded", etc, etc); > > If you do this, you can run with > --trace-to-console=-.*,disabled-by-default-cc-debug to send this output to the > console. Sorry this is for debugging that scroll_blocks_on bug, I will remove this code after figuring out what is wrong.
Patchset #14 (id:340001) has been deleted
Patchset #13 (id:320001) has been deleted
Patchset #12 (id:300001) has been deleted
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by [email protected] to run a CQ dry run
Patchset #10 (id:260001) has been deleted
Patchset #9 (id:240001) has been deleted
Patchset #8 (id:220001) has been deleted
Patchset #7 (id:200001) has been deleted
Patchset #6 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1626513003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1626513003/360001
Description was changed from ========== Add ScrollTree builder and unit test BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add ScrollTree builder and unit test Remove ScrollBlocksOn from ScrollTree: It will be moved from the code base soon, will work to integrate the "passive event handler" if that depends on layer tree hierarchy. BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
[email protected] changed reviewers: + [email protected]
The CQ bit was unchecked by [email protected]
Patchset #6 (id:360001) has been deleted
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1626513003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1626513003/380001
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by [email protected]
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1626513003/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1626513003/440001
I removed ScrollBlocksOn from ScrollTree. And because there are changes to master branch between patch 4 and patch 6, I merged the conflicts.
On 2016/01/27 22:22:08, sunxd wrote: > I removed ScrollBlocksOn from ScrollTree. > > And because there are changes to master branch between patch 4 and patch 6, I > merged the conflicts. lgtm
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/1626513003/#ps440001 (title: "Merge with master")
Message was sent while issue was closed.
Description was changed from ========== Add ScrollTree builder and unit test Remove ScrollBlocksOn from ScrollTree: It will be moved from the code base soon, will work to integrate the "passive event handler" if that depends on layer tree hierarchy. BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add ScrollTree builder and unit test Remove ScrollBlocksOn from ScrollTree: It will be moved from the code base soon, will work to integrate the "passive event handler" if that depends on layer tree hierarchy. BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #9 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Add ScrollTree builder and unit test Remove ScrollBlocksOn from ScrollTree: It will be moved from the code base soon, will work to integrate the "passive event handler" if that depends on layer tree hierarchy. BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add ScrollTree builder and unit test Remove ScrollBlocksOn from ScrollTree: It will be moved from the code base soon, will work to integrate the "passive event handler" if that depends on layer tree hierarchy. BUG=568830 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/ea1df789e1afcf57a69769b4c4a1d9ef09d112ed Cr-Commit-Position: refs/heads/master@{#371914} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/ea1df789e1afcf57a69769b4c4a1d9ef09d112ed Cr-Commit-Position: refs/heads/master@{#371914} |