Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543723003:
cc : Seperate adding clip node and setting render surface is_clipped (Closed)
Descriptioncc : Seperate adding clip node and setting render surface is_clipped
Computing surface_is_clipped on effect node depends on creating a clip
node for render surface. This CL puts the code that computes
surface_is_clipped into its own method in such a way that it can be
called even from the path where we don't create clip nodes for render
surfaces
BUG=594675
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/1a837b8028867b9ef99248fe67acdd6aa66a4701
Cr-Commit-Position: refs/heads/master@{#437447}
Patch Set 1 #Patch Set 2 : . #
Total comments: 5
Patch Set 3 : comments #
Dependent Patchsets: Messages
Total messages: 30 (17 generated)
Description was changed from ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL separates puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 ========== to ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL separates puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL separates puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by jaydasika@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...
jaydasika@chromium.org changed reviewers: + ajuma@chromium.org, weiliangc@chromium.org
PTAL https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; apply_ancestor_clip here still depends on having a clip node for render surfaces as it uses parent->layers_are_clipped. But, in the code where we do not create clip nodes for render surfaces, it will not depend on it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Is it possible to write a unittest for this? https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:365: ClipNode* parent = GetClipParent(data_from_ancestor, layer); Since GetClipParent() also has two code path depends on ClipParent(), maybe remove this helper function and merge with apply_ancestor_clip calculation? https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; On 2016/12/01 09:12:54, jaydasika wrote: > apply_ancestor_clip here still depends on having a clip node for render surfaces > as it uses parent->layers_are_clipped. But, in the code where we do not create > clip nodes for render surfaces, it will not depend on it. Could you clarify when apply_ancestor_clip would be different from old ancestor_clips_subtree?
On 2016/12/01 20:09:26, weiliangc wrote: > Is it possible to write a unittest for this? I don't think its required as this does not change the logic of computation, it only changes how its computed, so current tests should suffice ? > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > File cc/trees/property_tree_builder.cc (right): > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > cc/trees/property_tree_builder.cc:365: ClipNode* parent = > GetClipParent(data_from_ancestor, layer); > Since GetClipParent() also has two code path depends on ClipParent(), maybe > remove this helper function and merge with apply_ancestor_clip calculation? > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; > On 2016/12/01 09:12:54, jaydasika wrote: > > apply_ancestor_clip here still depends on having a clip node for render > surfaces > > as it uses parent->layers_are_clipped. But, in the code where we do not create > > clip nodes for render surfaces, it will not depend on it. > > Could you clarify when apply_ancestor_clip would be different from old > ancestor_clips_subtree?
https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:365: ClipNode* parent = GetClipParent(data_from_ancestor, layer); On 2016/12/01 20:09:26, weiliangc wrote: > Since GetClipParent() also has two code path depends on ClipParent(), maybe > remove this helper function and merge with apply_ancestor_clip calculation? Done. https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; On 2016/12/01 20:09:26, weiliangc wrote: > On 2016/12/01 09:12:54, jaydasika wrote: > > apply_ancestor_clip here still depends on having a clip node for render > surfaces > > as it uses parent->layers_are_clipped. But, in the code where we do not create > > clip nodes for render surfaces, it will not depend on it. > > Could you clarify when apply_ancestor_clip would be different from old > ancestor_clips_subtree? Its the same. Its just computed differently now. This is required because when we stop creating clip nodes for render surfaces, there won't be layers_are_clipped anymore (as it will always be true) and so we have to track whether to apply ancestor clip using a recursion variable (which this CL does). When we have clip parent we still use parent's layers_are_clipped value here, but it won't be the case in the new code path.
The CQ bit was checked by jaydasika@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: This issue passed the CQ dry run.
On 2016/12/02 at 05:20:26, jaydasika wrote: > On 2016/12/01 20:09:26, weiliangc wrote: > > Is it possible to write a unittest for this? > I don't think its required as this does not change the logic of computation, it only changes how its computed, so current tests should suffice ? > This logic looks good. I am curious as to what it looks like to stop creating clip node for Render Surface and how this CL relates to that, and whether there are more logic changes in same area. Would you please make new CL that stop creating clip node depended on this? It is good to have smaller CLs, just want to look more directly at how this CL helps stop create clip node for RS. I think it should be fine to keep this CL around, shouldn't really be more changes in PTreeBuilder in the meantime. Thanks! > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > File cc/trees/property_tree_builder.cc (right): > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > cc/trees/property_tree_builder.cc:365: ClipNode* parent = > > GetClipParent(data_from_ancestor, layer); > > Since GetClipParent() also has two code path depends on ClipParent(), maybe > > remove this helper function and merge with apply_ancestor_clip calculation? > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; > > On 2016/12/01 09:12:54, jaydasika wrote: > > > apply_ancestor_clip here still depends on having a clip node for render > > surfaces > > > as it uses parent->layers_are_clipped. But, in the code where we do not create > > > clip nodes for render surfaces, it will not depend on it. > > > > Could you clarify when apply_ancestor_clip would be different from old > > ancestor_clips_subtree?
On 2016/12/02 17:17:44, weiliangc wrote: > On 2016/12/02 at 05:20:26, jaydasika wrote: > > On 2016/12/01 20:09:26, weiliangc wrote: > > > Is it possible to write a unittest for this? > > I don't think its required as this does not change the logic of computation, > it only changes how its computed, so current tests should suffice ? > > > > This logic looks good. I am curious as to what it looks like to stop creating > clip node for Render Surface and how this CL relates to that, and whether there > are more logic changes in same area. > > Would you please make new CL that stop creating clip node depended on this? It > is good to have smaller CLs, just want to look more directly at how this CL > helps stop create clip node for RS. > > I think it should be fine to keep this CL around, shouldn't really be more > changes in PTreeBuilder in the meantime. > > Thanks! > > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > File cc/trees/property_tree_builder.cc (right): > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > cc/trees/property_tree_builder.cc:365: ClipNode* parent = > > > GetClipParent(data_from_ancestor, layer); > > > Since GetClipParent() also has two code path depends on ClipParent(), maybe > > > remove this helper function and merge with apply_ancestor_clip calculation? > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; > > > On 2016/12/01 09:12:54, jaydasika wrote: > > > > apply_ancestor_clip here still depends on having a clip node for render > > > surfaces > > > > as it uses parent->layers_are_clipped. But, in the code where we do not > create > > > > clip nodes for render surfaces, it will not depend on it. > > > > > > Could you clarify when apply_ancestor_clip would be different from old > > > ancestor_clips_subtree? https://codereview.chromium.org/2554833002 has the code for what I think the new clip tree logic will look like.
On 2016/12/06 10:20:21, jaydasika wrote: > On 2016/12/02 17:17:44, weiliangc wrote: > > On 2016/12/02 at 05:20:26, jaydasika wrote: > > > On 2016/12/01 20:09:26, weiliangc wrote: > > > > Is it possible to write a unittest for this? > > > I don't think its required as this does not change the logic of computation, > > it only changes how its computed, so current tests should suffice ? > > > > > > > This logic looks good. I am curious as to what it looks like to stop creating > > clip node for Render Surface and how this CL relates to that, and whether > there > > are more logic changes in same area. > > > > Would you please make new CL that stop creating clip node depended on this? It > > is good to have smaller CLs, just want to look more directly at how this CL > > helps stop create clip node for RS. > > > > I think it should be fine to keep this CL around, shouldn't really be more > > changes in PTreeBuilder in the meantime. > > > > Thanks! > > > > > > > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > > File cc/trees/property_tree_builder.cc (right): > > > > > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > > cc/trees/property_tree_builder.cc:365: ClipNode* parent = > > > > GetClipParent(data_from_ancestor, layer); > > > > Since GetClipParent() also has two code path depends on ClipParent(), > maybe > > > > remove this helper function and merge with apply_ancestor_clip > calculation? > > > > > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > > cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; > > > > On 2016/12/01 09:12:54, jaydasika wrote: > > > > > apply_ancestor_clip here still depends on having a clip node for render > > > > surfaces > > > > > as it uses parent->layers_are_clipped. But, in the code where we do not > > create > > > > > clip nodes for render surfaces, it will not depend on it. > > > > > > > > Could you clarify when apply_ancestor_clip would be different from old > > > > ancestor_clips_subtree? > > https://codereview.chromium.org/2554833002 has the code for what I think the new > clip tree logic will look like. ping
On 2016/12/06 at 10:20:21, jaydasika wrote: > On 2016/12/02 17:17:44, weiliangc wrote: > > On 2016/12/02 at 05:20:26, jaydasika wrote: > > > On 2016/12/01 20:09:26, weiliangc wrote: > > > > Is it possible to write a unittest for this? > > > I don't think its required as this does not change the logic of computation, > > it only changes how its computed, so current tests should suffice ? > > > > > > > This logic looks good. I am curious as to what it looks like to stop creating > > clip node for Render Surface and how this CL relates to that, and whether there > > are more logic changes in same area. > > > > Would you please make new CL that stop creating clip node depended on this? It > > is good to have smaller CLs, just want to look more directly at how this CL > > helps stop create clip node for RS. > > > > I think it should be fine to keep this CL around, shouldn't really be more > > changes in PTreeBuilder in the meantime. > > > > Thanks! > > > > > > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > > File cc/trees/property_tree_builder.cc (right): > > > > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > > cc/trees/property_tree_builder.cc:365: ClipNode* parent = > > > > GetClipParent(data_from_ancestor, layer); > > > > Since GetClipParent() also has two code path depends on ClipParent(), maybe > > > > remove this helper function and merge with apply_ancestor_clip calculation? > > > > > > > > > > https://codereview.chromium.org/2543723003/diff/20001/cc/trees/property_tree_... > > > > cc/trees/property_tree_builder.cc:372: : parent->layers_are_clipped; > > > > On 2016/12/01 09:12:54, jaydasika wrote: > > > > > apply_ancestor_clip here still depends on having a clip node for render > > > > surfaces > > > > > as it uses parent->layers_are_clipped. But, in the code where we do not > > create > > > > > clip nodes for render surfaces, it will not depend on it. > > > > > > > > Could you clarify when apply_ancestor_clip would be different from old > > > > ancestor_clips_subtree? > > https://codereview.chromium.org/2554833002 has the code for what I think the new clip tree logic will look like. LGTM. That CL looks really clean. :)
The CQ bit was checked by jaydasika@chromium.org
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jaydasika@chromium.org
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": 1481252996014560,
"parent_rev": "c5318e8e7e426ada6bb5c5822c057c2a485ecab7", "commit_rev":
"c8da1a34adc33585c3de4504dfc542e1258d9aef"}
Message was sent while issue was closed.
Description was changed from ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Seperate adding clip node and setting render surface is_clipped Computing surface_is_clipped on effect node depends on creating a clip node for render surface. This CL puts the code that computes surface_is_clipped into its own method in such a way that it can be called even from the path where we don't create clip nodes for render surfaces BUG=594675 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/1a837b8028867b9ef99248fe67acdd6aa66a4701 Cr-Commit-Position: refs/heads/master@{#437447} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1a837b8028867b9ef99248fe67acdd6aa66a4701 Cr-Commit-Position: refs/heads/master@{#437447} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
