Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742743003:
Delete RenderPassSink (Closed)
DescriptionDelete RenderPassSink
Delete RenderPassSink because it is only used by FrameData.
AppendRenderPasses only create one RenderPass, so rename to CreateRenderPass.
BUG=None
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2742743003
Cr-Commit-Position: refs/heads/master@{#457109}
Committed: https://chromium.googlesource.com/chromium/src/+/e5394bb17d12ce01acf000f5017be4c4c28ff83c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rename AppendRenderPasses to CreateRenderPass #
Total comments: 5
Patch Set 3 : Delete RenderPassSink #
Total comments: 1
Patch Set 4 : Refine unittest #
Messages
Total messages: 40 (26 generated)
Description was changed from ========== Rename AppendRenderPasses to AppendRenderPass BUG=None ========== to ========== Rename AppendRenderPasses to AppendRenderPass BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by xing.xu@intel.com 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...
Description was changed from ========== Rename AppendRenderPasses to AppendRenderPass BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Rename AppendRenderPasses to AppendRenderPass AppendRenderPasses only create one RenderPass, so rename to AppendRenderPass. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
xing.xu@intel.com changed reviewers: + weiliangc@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL.
LGTM
The CQ bit was checked by xing.xu@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2742743003/diff/1/cc/layers/render_surface_im... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2742743003/diff/1/cc/layers/render_surface_im... cc/layers/render_surface_impl.cc:360: void RenderSurfaceImpl::AppendRenderPass(RenderPassSink* pass_sink) { Now that this is the case, this function doesn't use anything private to RenderSurfaceImpl. We could delete this function (and the whole RenderPassSink concept) and just do this based on the public members of RenderSurfaceImpl at the callsite, if you like.
https://codereview.chromium.org/2742743003/diff/1/cc/layers/render_surface_im... File cc/layers/render_surface_impl.cc (right): https://codereview.chromium.org/2742743003/diff/1/cc/layers/render_surface_im... cc/layers/render_surface_impl.cc:360: void RenderSurfaceImpl::AppendRenderPass(RenderPassSink* pass_sink) { On 2017/03/13 14:46:00, danakj wrote: > Now that this is the case, this function doesn't use anything private to > RenderSurfaceImpl. We could delete this function (and the whole RenderPassSink > concept) and just do this based on the public members of RenderSurfaceImpl at > the callsite, if you like. Or, if you like, this could just return a unique_ptr<RenderPass> to the caller if you wanna keep this code here. But either way RenderPassSink can go away now.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xing.xu@intel.com
The CQ bit was unchecked by xing.xu@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by xing.xu@intel.com 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...
Description was changed from ========== Rename AppendRenderPasses to AppendRenderPass AppendRenderPasses only create one RenderPass, so rename to AppendRenderPass. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Rename AppendRenderPasses to CreateRenderPass AppendRenderPasses only create one RenderPass, so rename to CreateRenderPass. Also delete parameter RenderPassSink. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/03/13 14:47:52, danakj wrote: > https://codereview.chromium.org/2742743003/diff/1/cc/layers/render_surface_im... > File cc/layers/render_surface_impl.cc (right): > > https://codereview.chromium.org/2742743003/diff/1/cc/layers/render_surface_im... > cc/layers/render_surface_impl.cc:360: void > RenderSurfaceImpl::AppendRenderPass(RenderPassSink* pass_sink) { > On 2017/03/13 14:46:00, danakj wrote: > > Now that this is the case, this function doesn't use anything private to > > RenderSurfaceImpl. We could delete this function (and the whole RenderPassSink > > concept) and just do this based on the public members of RenderSurfaceImpl at > > the callsite, if you like. > > Or, if you like, this could just return a unique_ptr<RenderPass> to the caller > if you wanna keep this code here. But either way RenderPassSink can go away now. Thanks, updated, PTAL. BTW, I prefer later, because this function has been used by LTHI and unitests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool, thanks. There's more you can delete :) https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... File cc/layers/render_surface_unittest.cc (right): https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... cc/layers/render_surface_unittest.cc:202: TestRenderPassSink pass_sink; Delete this. And delete the class from cc/test/ https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... cc/layers/render_surface_unittest.cc:206: ASSERT_EQ(1u, pass_sink.RenderPasses().size()); delete, there can only be 1 https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... cc/layers/render_surface_unittest.cc:209: EXPECT_EQ(2, pass->id); just check these on what was returned on 204 https://codereview.chromium.org/2742743003/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2742743003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:723: void LayerTreeHostImpl::FrameData::AppendRenderPass( Delete the RenderPassSink class https://cs.chromium.org/chromium/src/cc/layers/render_pass_sink.h?sq=package:... along with this method https://codereview.chromium.org/2742743003/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:840: frame->AppendRenderPass(render_surface->CreateRenderPass()); just push the new render pass onto frame->render_passes directly.
The CQ bit was checked by xing.xu@intel.com 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...
Description was changed from ========== Rename AppendRenderPasses to CreateRenderPass AppendRenderPasses only create one RenderPass, so rename to CreateRenderPass. Also delete parameter RenderPassSink. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Delete RenderPassSink Delete RenderPassSink because it is only used by FrameData. AppendRenderPasses only create one RenderPass, so rename to CreateRenderPass. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/14 15:14:35, danakj wrote: > Cool, thanks. There's more you can delete :) > > https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... > File cc/layers/render_surface_unittest.cc (right): > > https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... > cc/layers/render_surface_unittest.cc:202: TestRenderPassSink pass_sink; > Delete this. And delete the class from cc/test/ > > https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... > cc/layers/render_surface_unittest.cc:206: ASSERT_EQ(1u, > pass_sink.RenderPasses().size()); > delete, there can only be 1 > > https://codereview.chromium.org/2742743003/diff/20001/cc/layers/render_surfac... > cc/layers/render_surface_unittest.cc:209: EXPECT_EQ(2, pass->id); > just check these on what was returned on 204 > > https://codereview.chromium.org/2742743003/diff/20001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2742743003/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:723: void > LayerTreeHostImpl::FrameData::AppendRenderPass( > Delete the RenderPassSink class > https://cs.chromium.org/chromium/src/cc/layers/render_pass_sink.h?sq=package:... > along with this method > > https://codereview.chromium.org/2742743003/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:840: > frame->AppendRenderPass(render_surface->CreateRenderPass()); > just push the new render pass onto frame->render_passes directly. Deleted RenderPassSink related code, PTAL, thanks.
Thanks! LGTM https://codereview.chromium.org/2742743003/diff/40001/cc/layers/render_surfac... File cc/layers/render_surface_unittest.cc (right): https://codereview.chromium.org/2742743003/diff/40001/cc/layers/render_surfac... cc/layers/render_surface_unittest.cc:188: RenderPassList render_passes; you don't need this list, just auto pass = render_surface->CreateRenderPass()
The CQ bit was checked by xing.xu@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from weiliangc@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2742743003/#ps60001 (title: "Refine unittest")
On 2017/03/15 15:22:16, danakj wrote: > Thanks! LGTM > > https://codereview.chromium.org/2742743003/diff/40001/cc/layers/render_surfac... > File cc/layers/render_surface_unittest.cc (right): > > https://codereview.chromium.org/2742743003/diff/40001/cc/layers/render_surfac... > cc/layers/render_surface_unittest.cc:188: RenderPassList render_passes; > you don't need this list, just > > auto pass = render_surface->CreateRenderPass() Thanks, updated.
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": 60001, "attempt_start_ts": 1489593478626860,
"parent_rev": "3003da8fd0ca3a91e706f7b4c4218055783c509c", "commit_rev":
"e5394bb17d12ce01acf000f5017be4c4c28ff83c"}
Message was sent while issue was closed.
Description was changed from ========== Delete RenderPassSink Delete RenderPassSink because it is only used by FrameData. AppendRenderPasses only create one RenderPass, so rename to CreateRenderPass. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Delete RenderPassSink Delete RenderPassSink because it is only used by FrameData. AppendRenderPasses only create one RenderPass, so rename to CreateRenderPass. BUG=None CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2742743003 Cr-Commit-Position: refs/heads/master@{#457109} Committed: https://chromium.googlesource.com/chromium/src/+/e5394bb17d12ce01acf000f5017b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e5394bb17d12ce01acf000f5017b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
