Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2878743002:
Set opacity and transform mutations directly on property tree for SPv2. (Closed)
DescriptionSet opacity and transform mutations directly on property tree for SPv2.
BUG=709137
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/2878743002
Cr-Commit-Position: refs/heads/master@{#471453}
Committed: https://chromium.googlesource.com/chromium/src/+/03635b38e709b9b8ef610cb955c376bbc8bcfde1
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add unit tests. #Patch Set 3 : Sync to head and share test logic. #
Messages
Total messages: 21 (13 generated)
Description was changed from ========== Set opacity and transform mutations directly on property tree for SPv2. BUG=709137 ========== to ========== Set opacity and transform mutations directly on property tree for SPv2. BUG=709137 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
wkorman@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/2878743002/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2878743002/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:1278: // TODO(wkorman): Do we need to do the update-node-from-owning-layer-id Seeking your input here. I'm not sure how to think about the calls to UpdateNodeFromOwningLayerId() in the SPv1 code path for both this method and the one below. See callers of: https://cs.chromium.org/chromium/src/cc/trees/property_tree.h?sq=package:chro... Aside from these two methods it's also called in many other Layer methods. I'll see whether any tests fail. Are we planning to remove owning layer id for effect/transform nodes? Should we still execute the owning node logic in both of these methods in the SPv2 code path? Can we retrieve the owning layer node by walking up the respective trees in some manner rather than using layer id?
Description was changed from ========== Set opacity and transform mutations directly on property tree for SPv2. BUG=709137 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Set opacity and transform mutations directly on property tree for SPv2. BUG=709137 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...
Code looks good, just needs a test. https://codereview.chromium.org/2878743002/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2878743002/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:1278: // TODO(wkorman): Do we need to do the update-node-from-owning-layer-id On 2017/05/11 at 18:19:14, wkorman wrote: > Seeking your input here. I'm not sure how to think about the calls to UpdateNodeFromOwningLayerId() in the SPv1 code path for both this method and the one below. See callers of: > > https://cs.chromium.org/chromium/src/cc/trees/property_tree.h?sq=package:chro... > > Aside from these two methods it's also called in many other Layer methods. I'll see whether any tests fail. > > Are we planning to remove owning layer id for effect/transform nodes? Should we still execute the owning node logic in both of these methods in the SPv2 code path? Can we retrieve the owning layer node by walking up the respective trees in some manner rather than using layer id? Yes, we should remove the owning node logic and map. See for example https://codereview.chromium.org/2866733002 (not ready for review yet, still has bugs).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL added unit test.
The CQ bit was checked by chrishtr@chromium.org
lgtm
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2878743002/#ps40001 (title: "Sync to head and share test logic.")
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": 40001, "attempt_start_ts": 1494614079335440,
"parent_rev": "a3b83e8b17ecd27413f8533c7ba60fb8b293d908", "commit_rev":
"03635b38e709b9b8ef610cb955c376bbc8bcfde1"}
Message was sent while issue was closed.
Description was changed from ========== Set opacity and transform mutations directly on property tree for SPv2. BUG=709137 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Set opacity and transform mutations directly on property tree for SPv2. BUG=709137 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/2878743002 Cr-Commit-Position: refs/heads/master@{#471453} Committed: https://chromium.googlesource.com/chromium/src/+/03635b38e709b9b8ef610cb955c3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/03635b38e709b9b8ef610cb955c3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
