Use RenderPass to create DrawQuad in unittests

Use RenderPass to create DrawQuad in unittests

Instead of calling DQ::Create() function and then push it into
RenderPass in unittest, use RenderPass's function call to create and
append a DQ while only pass out a raw pointer instead of ownership.

Follow CL 369623002.

BUG=344962
TEST=cc_unittests, content_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282179

[weiliangc, 2014-07-07 22:07:36]

overall: danakj
cc_message_perftest: piman

[danakj, 2014-07-07 22:30:15]

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:399: RenderPass* render_pass,
nit: can you make render_pass the last param like you did in the others?

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:438: RenderPass* render_pass,
ditto

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:472: RenderPass* render_pass,
dittoditto

https://codereview.chromium.org/368403003/diff/50001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/368403003/diff/50001/cc/quads/render_pass.h#n...
cc/quads/render_pass.h:99: DrawQuadType* CopyFromAndAppendDrawQuad(const
DrawQuadType* quad) {
Can we do two methods:

1. that takes DrawQuad* and SQS* and does DrawQuad::Copy.
2. that takes RPDrawQuad* and SQS* and RPId and does RPDrawQuad::Copy.

and then we can make those methods not allocate either now or later.

https://codereview.chromium.org/368403003/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/368403003/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:187: frame->render_pass_list[0]
Can you put this RenderPass in a temp var for better readability?

[piman, 2014-07-07 22:57:42]

danakj@ has OWNERs super-powers, so her approval will be enough.


On Mon, Jul 7, 2014 at 3:30 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> output/renderer_pixeltest.cc
> File cc/output/renderer_pixeltest.cc (right):
>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> output/renderer_pixeltest.cc#newcode399
> cc/output/renderer_pixeltest.cc:399: RenderPass* render_pass,
> nit: can you make render_pass the last param like you did in the others?
>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> output/renderer_pixeltest.cc#newcode438
> cc/output/renderer_pixeltest.cc:438: RenderPass* render_pass,
> ditto
>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> output/renderer_pixeltest.cc#newcode472
> cc/output/renderer_pixeltest.cc:472: RenderPass* render_pass,
> dittoditto
>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> quads/render_pass.h
> File cc/quads/render_pass.h (right):
>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> quads/render_pass.h#newcode99
> cc/quads/render_pass.h:99: DrawQuadType* CopyFromAndAppendDrawQuad(const
> DrawQuadType* quad) {
> Can we do two methods:
>
> 1. that takes DrawQuad* and SQS* and does DrawQuad::Copy.
> 2. that takes RPDrawQuad* and SQS* and RPId and does RPDrawQuad::Copy.
>
> and then we can make those methods not allocate either now or later.
>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> trees/layer_tree_host_unittest_delegated.cc
> File cc/trees/layer_tree_host_unittest_delegated.cc (right):
>
> https://codereview.chromium.org/368403003/diff/50001/cc/
> trees/layer_tree_host_unittest_delegated.cc#newcode187
> cc/trees/layer_tree_host_unittest_delegated.cc:187:
> frame->render_pass_list[0]
> Can you put this RenderPass in a temp var for better readability?
>
> https://codereview.chromium.org/368403003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[weiliangc, 2014-07-08 21:54:38]

Address review comments

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:399: RenderPass* render_pass,
On 2014/07/07 22:30:15, danakj wrote:
> nit: can you make render_pass the last param like you did in the others?

Done.

Saw some SQS func that doesn't have render_pass as last param, changed those
too.

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:438: RenderPass* render_pass,
On 2014/07/07 22:30:14, danakj wrote:
> ditto

Done.

https://codereview.chromium.org/368403003/diff/50001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:472: RenderPass* render_pass,
On 2014/07/07 22:30:14, danakj wrote:
> dittoditto

Done.

https://codereview.chromium.org/368403003/diff/50001/cc/quads/render_pass.h
File cc/quads/render_pass.h (right):

https://codereview.chromium.org/368403003/diff/50001/cc/quads/render_pass.h#n...
cc/quads/render_pass.h:99: DrawQuadType* CopyFromAndAppendDrawQuad(const
DrawQuadType* quad) {
On 2014/07/07 22:30:15, danakj wrote:
> Can we do two methods:
> 
> 1. that takes DrawQuad* and SQS* and does DrawQuad::Copy.
> 2. that takes RPDrawQuad* and SQS* and RPId and does RPDrawQuad::Copy.
> 
> and then we can make those methods not allocate either now or later.

Done.

https://codereview.chromium.org/368403003/diff/50001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_delegated.cc (right):

https://codereview.chromium.org/368403003/diff/50001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_delegated.cc:187: frame->render_pass_list[0]
On 2014/07/07 22:30:15, danakj wrote:
> Can you put this RenderPass in a temp var for better readability?

Done.

[danakj, 2014-07-09 17:12:09]

Can the draw quad Copy methods go away or are they still used somewhere?

https://codereview.chromium.org/368403003/diff/90001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/368403003/diff/90001/cc/quads/render_pass.cc#...
cc/quads/render_pass.cc:203: const CheckerboardDrawQuad* original_quad =
I think you can move this material cast into the
CopyFromAndAppendTypedDrawQuad<T> method and just pass it a DrawQuad* to keep
things more concise

[weiliangc, 2014-07-09 17:55:04]

On 2014/07/09 17:12:09, danakj wrote:
> Can the draw quad Copy methods go away or are they still used somewhere?

Some. Once in delegated_renderer_layer_impl, once in surface_agg, and some in
DQ_unittests.

I have a git branch, can definitely rebase then merge with this?

> 
> https://codereview.chromium.org/368403003/diff/90001/cc/quads/render_pass.cc
> File cc/quads/render_pass.cc (right):
> 
>
https://codereview.chromium.org/368403003/diff/90001/cc/quads/render_pass.cc#...
> cc/quads/render_pass.cc:203: const CheckerboardDrawQuad* original_quad =
> I think you can move this material cast into the
> CopyFromAndAppendTypedDrawQuad<T> method and just pass it a DrawQuad* to keep
> things more concise

Done.

[danakj, 2014-07-09 17:55:51]

On 2014/07/09 17:55:04, weiliangc wrote:
> On 2014/07/09 17:12:09, danakj wrote:
> > Can the draw quad Copy methods go away or are they still used somewhere?
> 
> Some. Once in delegated_renderer_layer_impl, once in surface_agg, and some in
> DQ_unittests.
> 
> I have a git branch, can definitely rebase then merge with this?

Oh, no that's ok, keep the patches coming :)

[danakj, 2014-07-09 17:59:14]

LGTM

https://codereview.chromium.org/368403003/diff/130001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/368403003/diff/130001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:202: case DrawQuad::CHECKERBOARD: {
don't need the {}'s for each of these cases

https://codereview.chromium.org/368403003/diff/130001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:243: :  // RenderPass quads need to use typed copy from
function.
errr can this : be on the previous line?

[weiliangc, 2014-07-09 20:59:09]

https://codereview.chromium.org/368403003/diff/130001/cc/quads/render_pass.cc
File cc/quads/render_pass.cc (right):

https://codereview.chromium.org/368403003/diff/130001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:202: case DrawQuad::CHECKERBOARD: {
On 2014/07/09 17:59:14, danakj wrote:
> don't need the {}'s for each of these cases

Done.

https://codereview.chromium.org/368403003/diff/130001/cc/quads/render_pass.cc...
cc/quads/render_pass.cc:243: :  // RenderPass quads need to use typed copy from
function.
On 2014/07/09 17:59:14, danakj wrote:
> errr can this : be on the previous line?

Done.

[weiliangc, 2014-07-09 20:59:35]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:01:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/weiliangc@chromium.org/368403003/150001

[commit-bot: I haz the power, 2014-07-10 00:00:05]

Change committed as 282179