Update Mojo surfaces type converters to use RP for generating DQ and SQS

Update Mojo surfaces type converters to use RP for generating DQ and SQS

In order for RenderPass to own allocation of DrawQuads and
SharedQuadState, update surface type converter code in mojo so when
convert to DrawQuad and SharedQuadState, function takes in pointer to RenderPass
as parameter. Also in unittest, create RenderPass to deal with DrawQuad and
SharedQuadState creation.

Follows 398533002.

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

[weiliangc, 2014-07-15 22:00:59]

mojo: jamesr
overall: danakj

[danakj, 2014-07-15 22:02:34]

Use full words instead of acronyms in your patch description. Maybe you can get
away with it in the patch title but if you can avoid it bonus points.

[danakj, 2014-07-15 22:05:52]

LGTM

https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
File mojo/services/public/cpp/surfaces/surfaces_type_converters.h (right):

https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
mojo/services/public/cpp/surfaces/surfaces_type_converters.h:54: //
TypeConverter for surfaces::Pass calls this explicitly. Also RenderPass owns
I feel like this comment could use some refactoring but I'm not sure so I'll let
jamesr comment.

[jamesr, 2014-07-15 22:48:01]

https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
File mojo/services/public/cpp/surfaces/surfaces_type_converters.h (right):

https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
mojo/services/public/cpp/surfaces/surfaces_type_converters.h:54: //
TypeConverter for surfaces::Pass calls this explicitly. Also RenderPass owns
On 2014/07/15 22:05:51, danakj wrote:
> I feel like this comment could use some refactoring but I'm not sure so I'll
let
> jamesr comment.

Since the ownership is getting more complicated here and this function can't
stand on its own at all maybe you should hide it in the .cc instead of having it
be here in the public header

https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
mojo/services/public/cpp/surfaces/surfaces_type_converters.h:70:
MOJO_SURFACES_EXPORT cc::SharedQuadState* ConvertTo(
ditto with this, since you can no longer convert a SharedQuadState in isolation

[weiliangc, 2014-07-15 22:56:40]

https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
File mojo/services/public/cpp/surfaces/surfaces_type_converters.h (right):

https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
mojo/services/public/cpp/surfaces/surfaces_type_converters.h:54: //
TypeConverter for surfaces::Pass calls this explicitly. Also RenderPass owns
On 2014/07/15 22:48:01, jamesr wrote:
> On 2014/07/15 22:05:51, danakj wrote:
> > I feel like this comment could use some refactoring but I'm not sure so I'll
> let
> > jamesr comment.
> 
> Since the ownership is getting more complicated here and this function can't
> stand on its own at all maybe you should hide it in the .cc instead of having
it
> be here in the public header

What about its usage in surface_unittest? (Where it does
DrawQuad->MojoQuad->DrawQuad)

[jamesr, 2014-07-15 22:57:18]

On 2014/07/15 22:56:40, weiliangc wrote:
>
https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
> File mojo/services/public/cpp/surfaces/surfaces_type_converters.h (right):
> 
>
https://codereview.chromium.org/393103003/diff/1/mojo/services/public/cpp/sur...
> mojo/services/public/cpp/surfaces/surfaces_type_converters.h:54: //
> TypeConverter for surfaces::Pass calls this explicitly. Also RenderPass owns
> On 2014/07/15 22:48:01, jamesr wrote:
> > On 2014/07/15 22:05:51, danakj wrote:
> > > I feel like this comment could use some refactoring but I'm not sure so
I'll
> > let
> > > jamesr comment.
> > 
> > Since the ownership is getting more complicated here and this function can't
> > stand on its own at all maybe you should hide it in the .cc instead of
having
> it
> > be here in the public header
> 
> What about its usage in surface_unittest? (Where it does
> DrawQuad->MojoQuad->DrawQuad)

We should be able to write these unit tests in terms of passes, right?

[jamesr, 2014-07-15 22:58:07]

In other words, with these changes it doesn't make sense to talk about a
DrawQuad in isolation since it always has ownership held by the pass, if I
understand this patch series correctly.  We can do the round-trip tests by
having a pretty trivial pass but I think the unit test usage should match our
expected ownership.

[weiliangc, 2014-07-15 23:15:10]

On 2014/07/15 22:58:07, jamesr wrote:
> In other words, with these changes it doesn't make sense to talk about a
> DrawQuad in isolation since it always has ownership held by the pass, if I
> understand this patch series correctly.  We can do the round-trip tests by
> having a pretty trivial pass but I think the unit test usage should match our
> expected ownership.

Yeah got it. Will change unittest to test mojo to DQ/SQS conversion in
round-trip render pass test.

[jamesr, 2014-07-15 23:21:31]

Thanks!
On Jul 15, 2014 4:15 PM, <weiliangc@chromium.org> wrote:

> On 2014/07/15 22:58:07, jamesr wrote:
>
>> In other words, with these changes it doesn't make sense to talk about a
>> DrawQuad in isolation since it always has ownership held by the pass, if I
>> understand this patch series correctly.  We can do the round-trip tests by
>> having a pretty trivial pass but I think the unit test usage should match
>> our
>> expected ownership.
>>
>
> Yeah got it. Will change unittest to test mojo to DQ/SQS conversion in
> round-trip render pass test.
>
> https://codereview.chromium.org/393103003/
>

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

[weiliangc, 2014-07-16 18:02:21]

On 2014/07/15 23:21:31, jamesr wrote:
> Thanks!
> On Jul 15, 2014 4:15 PM, <mailto:weiliangc@chromium.org> wrote:
> 
> > On 2014/07/15 22:58:07, jamesr wrote:
> >
> >> In other words, with these changes it doesn't make sense to talk about a
> >> DrawQuad in isolation since it always has ownership held by the pass, if I
> >> understand this patch series correctly.  We can do the round-trip tests by
> >> having a pretty trivial pass but I think the unit test usage should match
> >> our
> >> expected ownership.
> >>
> >
> > Yeah got it. Will change unittest to test mojo to DQ/SQS conversion in
> > round-trip render pass test.
> >
> > https://codereview.chromium.org/393103003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Addressed comments, PTAL.

[jamesr, 2014-07-17 20:25:25]

lgtm

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:81:
texture_quad_state->vertex_opacity.storage().data(),
unfortunately std::vector<>::data() is c++11 and not supported on all of our
toolchains. type &..storage()[0] instead.

we should be checking for this array not being present, but that's a separate
issue so you don't have to do that as part of this CL

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/surfaces/surfaces_type_converters.h (right):

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/surfaces/surfaces_type_converters.h:51: // conversion
function. Functionality is covered by RenderPass ConvertTo.
not sure this comment needs to be here - several converter types are asymmetric,
I don't think that's particularly unusual.

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/surfaces/tests/surface_unittest.cc (right):

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/surfaces/tests/surface_unittest.cc:108:
cc::SharedQuadState* sqs = render_pass->CreateAndAppendSharedQuadState();
nit: you could probably move these two lines into the SurfaceLibQuadTest fixture
since that's a base class of the type this function ends up being in and make
the different SurfaceLibQuadTest instances simple

[jamesr, 2014-07-17 20:52:23]

https://codereview.chromium.org/393583003/ has the std::vector::data stuff.  I
thought that patch had landed already, but guess not? Hmm..

[weiliangc, 2014-07-17 21:27:10]

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc (right):

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/surfaces/lib/surfaces_type_converters.cc:81:
texture_quad_state->vertex_opacity.storage().data(),
On 2014/07/17 20:25:24, jamesr wrote:
> unfortunately std::vector<>::data() is c++11 and not supported on all of our
> toolchains. type &..storage()[0] instead.
> 
> we should be checking for this array not being present, but that's a separate
> issue so you don't have to do that as part of this CL

Will leave to crrev.com/393583003

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/surfaces/tests/surface_unittest.cc (right):

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/surfaces/tests/surface_unittest.cc:191:
pass->SetAll(pass_id,
If set up SharedQuadState in SurfaceLibQuadTest, this test would fail because in
RenderPass SetAll() there is a DCHECK for no quad or SQS has been added yet.

So kill DCHECK or only setup RenderPass in SurfaceLibQuadTest?

[jamesr, 2014-07-17 21:29:37]

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/surfaces/tests/surface_unittest.cc (right):

https://codereview.chromium.org/393103003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/surfaces/tests/surface_unittest.cc:191:
pass->SetAll(pass_id,
On 2014/07/17 21:27:10, weiliangc wrote:
> If set up SharedQuadState in SurfaceLibQuadTest, this test would fail because
in
> RenderPass SetAll() there is a DCHECK for no quad or SQS has been added yet.
> 
> So kill DCHECK or only setup RenderPass in SurfaceLibQuadTest?

Or move this test out of the SurfaceLibQuadTest fixture and just make it be a
SurfaceLibTest.  The only thing it gets from the fixture are the default values
of rect/opaque_rect/etc, which aren't that important

[weiliangc, 2014-07-18 14:14:51]

The CQ bit was checked by weiliangc@chromium.org

[commit-bot: I haz the power, 2014-07-18 14:15:56]

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

[commit-bot: I haz the power, 2014-07-18 15:53:44]

Change committed as 284104