Implement cc::SharedQuadState StructTraits

Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4690c5bb2208c108dfd76a210cd2bec61c67c884
Cr-Commit-Position: refs/heads/master@{#397812}

[Fady Samuel, 2016-06-01 19:00:14]

Description was changed from

==========
Implement SharedQuadState StructTraits

BUG=
==========

to

==========
Implement SharedQuadState StructTraits

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-06-01 20:27:22]

Description was changed from

==========
Implement SharedQuadState StructTraits

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-06-01 20:28:00]

Description was changed from

==========
Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement cc::SharedQuadState StructTraits

cc::SharedQuadState does not have a copy constructor and so
I'm mapping to and from std::unique_ptr<cc::SharedQuadState>.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-06-01 20:28:06]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 20:28:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032643002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2032643002/60001

[Fady Samuel, 2016-06-01 20:29:24]

fsamuel@chromium.org changed reviewers:
+ ben@chromium.org, danakj@chromium.org, tsepez@chromium.org

[Fady Samuel, 2016-06-01 20:29:25]

+tsepez@ for ipc
+danakj@ for cc in general (fyi)
+ben@ for mus.

[Tom Sepez, 2016-06-01 20:41:28]

lgtm

[danakj, 2016-06-01 20:54:11]

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
File cc/ipc/shared_quad_state.mojom (right):

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
cc/ipc/shared_quad_state.mojom:12: enum SkXfermode {
Erm. Include skia's header? Why are we reproducing this enum? Skia folks will
surely not keep this up to date if it changes.

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
File cc/ipc/shared_quad_state.typemap (right):

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
cc/ipc/shared_quad_state.typemap:10: type_mappings = [
"cc.mojom.SharedQuadState=std::unique_ptr<cc::SharedQuadState>" ]
why the unique_ptr?

[Fady Samuel, 2016-06-01 21:02:49]

PTAL

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
File cc/ipc/shared_quad_state.mojom (right):

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
cc/ipc/shared_quad_state.mojom:12: enum SkXfermode {
On 2016/06/01 20:54:11, danakj wrote:
> Erm. Include skia's header? Why are we reproducing this enum? Skia folks will
> surely not keep this up to date if it changes.

In the long term, enums should be represented only in mojoms. For now, we're
going to have to deal with this :(

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
File cc/ipc/shared_quad_state.typemap (right):

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
cc/ipc/shared_quad_state.typemap:10: type_mappings = [
"cc.mojom.SharedQuadState=std::unique_ptr<cc::SharedQuadState>" ]
On 2016/06/01 20:54:11, danakj wrote:
> why the unique_ptr?


cc::SharedQuadState is missing a copy constructor. We don't allow the default
constructor it seems for complex types so I solved that by passing it around via
unique_ptr. I could give cc::SharedQuadState a copy constructor? WDYT?

I think this becomes a problem with mojo::Array<cc::SharedQuadState> which uses
std::vector under the hood and I believe all the entries in an std::vector are
copied when it resizes.

[danakj, 2016-06-01 21:04:20]

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
File cc/ipc/shared_quad_state.mojom (right):

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
cc/ipc/shared_quad_state.mojom:12: enum SkXfermode {
On 2016/06/01 21:02:49, Fady Samuel wrote:
> On 2016/06/01 20:54:11, danakj wrote:
> > Erm. Include skia's header? Why are we reproducing this enum? Skia folks
will
> > surely not keep this up to date if it changes.
> 
> In the long term, enums should be represented only in mojoms. For now, we're
> going to have to deal with this :(

Skia enums are going to be in mojoms? How are you going to deal with third_party
stuff?

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
File cc/ipc/shared_quad_state.typemap (right):

https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
cc/ipc/shared_quad_state.typemap:10: type_mappings = [
"cc.mojom.SharedQuadState=std::unique_ptr<cc::SharedQuadState>" ]
On 2016/06/01 21:02:49, Fady Samuel wrote:
> On 2016/06/01 20:54:11, danakj wrote:
> > why the unique_ptr?
> 
> 
> cc::SharedQuadState is missing a copy constructor. We don't allow the default
> constructor it seems for complex types so I solved that by passing it around
via
> unique_ptr. I could give cc::SharedQuadState a copy constructor? WDYT?
> 
> I think this becomes a problem with mojo::Array<cc::SharedQuadState> which
uses
> std::vector under the hood and I believe all the entries in an std::vector are
> copied when it resizes.

vector moves things when it resizes if there is a move constructor. Maybe you
need to add that.

[Ken Rockot(use gerrit already), 2016-06-01 21:11:35]

On 2016/06/01 at 21:04:20, danakj wrote:
>
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> File cc/ipc/shared_quad_state.mojom (right):
> 
>
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> cc/ipc/shared_quad_state.mojom:12: enum SkXfermode {
> On 2016/06/01 21:02:49, Fady Samuel wrote:
> > On 2016/06/01 20:54:11, danakj wrote:
> > > Erm. Include skia's header? Why are we reproducing this enum? Skia folks
will
> > > surely not keep this up to date if it changes.
> > 
> > In the long term, enums should be represented only in mojoms. For now, we're
> > going to have to deal with this :(
> 
> Skia enums are going to be in mojoms? How are you going to deal with
third_party stuff?

Maybe a good question for skia owners, but I recall hearing support for using
mojom upstream. I think there's interest in generating the same symbols for
other languages.

> 
>
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> File cc/ipc/shared_quad_state.typemap (right):
> 
>
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> cc/ipc/shared_quad_state.typemap:10: type_mappings = [
"cc.mojom.SharedQuadState=std::unique_ptr<cc::SharedQuadState>" ]
> On 2016/06/01 21:02:49, Fady Samuel wrote:
> > On 2016/06/01 20:54:11, danakj wrote:
> > > why the unique_ptr?
> > 
> > 
> > cc::SharedQuadState is missing a copy constructor. We don't allow the
default
> > constructor it seems for complex types so I solved that by passing it around
via
> > unique_ptr. I could give cc::SharedQuadState a copy constructor? WDYT?
> > 
> > I think this becomes a problem with mojo::Array<cc::SharedQuadState> which
uses
> > std::vector under the hood and I believe all the entries in an std::vector
are
> > copied when it resizes.
> 
> vector moves things when it resizes if there is a move constructor. Maybe you
need to add that.

[danakj, 2016-06-01 21:13:19]

On Wed, Jun 1, 2016 at 2:11 PM, <rockot@chromium.org> wrote:

> On 2016/06/01 at 21:04:20, danakj wrote:
> >
>
>
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> > File cc/ipc/shared_quad_state.mojom (right):
> >
> >
>
>
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> > cc/ipc/shared_quad_state.mojom:12: enum SkXfermode {
> > On 2016/06/01 21:02:49, Fady Samuel wrote:
> > > On 2016/06/01 20:54:11, danakj wrote:
> > > > Erm. Include skia's header? Why are we reproducing this enum? Skia
> folks
> will
> > > > surely not keep this up to date if it changes.
> > >
> > > In the long term, enums should be represented only in mojoms. For now,
> we're
> > > going to have to deal with this :(
> >
> > Skia enums are going to be in mojoms? How are you going to deal with
> third_party stuff?
>
> Maybe a good question for skia owners, but I recall hearing support for
> using
> mojom upstream. I think there's interest in generating the same symbols for
> other languages.
>

I'd like to at the least understand where we're going before thinking of
duplicating enums from third_party/ in our mojoms.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-06-01 21:21:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-01 21:21:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2016-06-01 21:24:03]

rockot@chromium.org changed reviewers:
+ yzshen@chromium.org

[Ken Rockot(use gerrit already), 2016-06-01 21:25:03]

On 2016/06/01 at 21:13:19, danakj wrote:
> On Wed, Jun 1, 2016 at 2:11 PM, <rockot@chromium.org> wrote:
> 
> > On 2016/06/01 at 21:04:20, danakj wrote:
> > >
> >
> >
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> > > File cc/ipc/shared_quad_state.mojom (right):
> > >
> > >
> >
> >
https://codereview.chromium.org/2032643002/diff/60001/cc/ipc/shared_quad_stat...
> > > cc/ipc/shared_quad_state.mojom:12: enum SkXfermode {
> > > On 2016/06/01 21:02:49, Fady Samuel wrote:
> > > > On 2016/06/01 20:54:11, danakj wrote:
> > > > > Erm. Include skia's header? Why are we reproducing this enum? Skia
> > folks
> > will
> > > > > surely not keep this up to date if it changes.
> > > >
> > > > In the long term, enums should be represented only in mojoms. For now,
> > we're
> > > > going to have to deal with this :(
> > >
> > > Skia enums are going to be in mojoms? How are you going to deal with
> > third_party stuff?
> >
> > Maybe a good question for skia owners, but I recall hearing support for
> > using
> > mojom upstream. I think there's interest in generating the same symbols for
> > other languages.
> >
> 
> I'd like to at the least understand where we're going before thinking of
> duplicating enums from third_party/ in our mojoms.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to chromium-reviews+unsubscribe@chromium.org.
> 

It does seem likely that even if skia would upstream mojom usage some day, there
will be other third party libraries that won't, and they'll define enums that
need to be transported. And even supporting upstream mojom would require us to
export the bindings generator, which is a gnarly task in itself.

On one hand we could add native enum mapping support so you could declare a
mojom alias for an existing enum type. This has a major disadvantage in that it
precludes multilingual bindings generation for that type. On the other hand we
could suffer duplicate definitions.

So far we've opted for the latter, but maybe it's better to support native
mapping for cases where multilingual bindings are uninteresting (which remains
the case for skia in chrome today.) If support is needed for other languages,
duplicate definitions can be introduced when that need arises. +yzshen for
thoughts

[Fady Samuel, 2016-06-01 22:35:50]

PTAL Dana. I now avoid the unique_ptr typemapping. This is diffed against this:
https://codereview.chromium.org/2029813002/

Let me know if you'd prefer I to merge the two CLs into one. Thanks!

[danakj, 2016-06-01 22:38:40]

On Wed, Jun 1, 2016 at 3:35 PM, <fsamuel@chromium.org> wrote:

> PTAL Dana. I now avoid the unique_ptr typemapping. This is diffed against
> this:
> https://codereview.chromium.org/2029813002/
>
> Let me know if you'd prefer I to merge the two CLs into one. Thanks!
>

We need to figure out enums first.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Fady Samuel, 2016-06-02 14:49:01]

On 2016/06/01 22:38:40, danakj wrote:
> On Wed, Jun 1, 2016 at 3:35 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > PTAL Dana. I now avoid the unique_ptr typemapping. This is diffed against
> > this:
> > https://codereview.chromium.org/2029813002/
> >
> > Let me know if you'd prefer I to merge the two CLs into one. Thanks!
> >
> 
> We need to figure out enums first.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hi Dana, as per the offline thread, I think we might have a path forward for
enums. Given the scope of the problem, I don't think we're going to solve it
within this one CL. However, we are now actively thinking about it! Given that,
in the interest of continuing to make forward progress, do you mind letting me
land this patch? Thanks!

[Fady Samuel, 2016-06-03 01:38:54]

PTAL Dana, I've made blend_mode an int in the short term with a TODO. Are you OK
with that? Thanks!

[Fady Samuel, 2016-06-03 01:45:20]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 01:46:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032643002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2032643002/100001

[commit-bot: I haz the power, 2016-06-03 05:40:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-03 05:40:15]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2016-06-03 12:43:13]

Tom, I've made blend_mode an int as per discussion with Dana. Could you please
re-review the ipc bits of this patch? Thanks!

[Tom Sepez, 2016-06-03 16:48:22]

https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
File cc/ipc/shared_quad_state_struct_traits.h (right):

https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
cc/ipc/shared_quad_state_struct_traits.h:57: out->blend_mode =
static_cast<SkXfermode::Mode>(data.blend_mode());
You need to manually validate that blend_mode is within bounds since it is no
longer an enum (and mojo no longer does this for you automatically).

[Fady Samuel, 2016-06-03 18:01:56]

PTAL Tom!

https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
File cc/ipc/shared_quad_state_struct_traits.h (right):

https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
cc/ipc/shared_quad_state_struct_traits.h:57: out->blend_mode =
static_cast<SkXfermode::Mode>(data.blend_mode());
On 2016/06/03 16:48:22, Tom Sepez wrote:
> You need to manually validate that blend_mode is within bounds since it is no
> longer an enum (and mojo no longer does this for you automatically).

Added DCHECK. Done.

[Ken Rockot(use gerrit already), 2016-06-03 18:03:43]

rockot@chromium.org changed reviewers:
+ rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-06-03 18:03:44]

https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
File cc/ipc/shared_quad_state_struct_traits.h (right):

https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
cc/ipc/shared_quad_state_struct_traits.h:57: out->blend_mode =
static_cast<SkXfermode::Mode>(data.blend_mode());
On 2016/06/03 at 18:01:55, Fady Samuel wrote:
> On 2016/06/03 16:48:22, Tom Sepez wrote:
> > You need to manually validate that blend_mode is within bounds since it is
no
> > longer an enum (and mojo no longer does this for you automatically).
> 
> Added DCHECK. Done.

Drive-by comment: DCHECK is not validation. This should test the range and
return false if out of range.

[Fady Samuel, 2016-06-03 18:07:00]

On 2016/06/03 18:03:44, Ken Rockot wrote:
>
https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
> File cc/ipc/shared_quad_state_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2032643002/diff/100001/cc/ipc/shared_quad_sta...
> cc/ipc/shared_quad_state_struct_traits.h:57: out->blend_mode =
> static_cast<SkXfermode::Mode>(data.blend_mode());
> On 2016/06/03 at 18:01:55, Fady Samuel wrote:
> > On 2016/06/03 16:48:22, Tom Sepez wrote:
> > > You need to manually validate that blend_mode is within bounds since it is
> no
> > > longer an enum (and mojo no longer does this for you automatically).
> > 
> > Added DCHECK. Done.
> 
> Drive-by comment: DCHECK is not validation. This should test the range and
> return false if out of range.

Indeed. Thanks Ken! Fixed!

[Fady Samuel, 2016-06-03 18:14:06]

The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-03 18:14:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032643002/140001

[Tom Sepez, 2016-06-03 18:14:28]

https://codereview.chromium.org/2032643002/diff/140001/cc/ipc/shared_quad_sta...
File cc/ipc/shared_quad_state_struct_traits.h (right):

https://codereview.chromium.org/2032643002/diff/140001/cc/ipc/shared_quad_sta...
cc/ipc/shared_quad_state_struct_traits.h:57: if (data.blend_mode() >
SkXfermode::kLastMode)
blend_mode is signed, so it could be < 0 (at least looking at static int32_t
blend_mode(const cc::SharedQuadState& sqs) above).  Can we make it uint32_t
throughout?

[danakj, 2016-06-03 18:53:37]

LGTM

[Fady Samuel, 2016-06-03 19:00:29]

PTAL Tom!

[Tom Sepez, 2016-06-03 19:03:20]

lgtm

[Fady Samuel, 2016-06-03 19:05:44]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2016-06-03 19:05:45]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2032643002/#ps160001
(title: "Updated to uint")

[Fady Samuel, 2016-06-03 19:05:52]

The CQ bit was unchecked by fsamuel@chromium.org

[Fady Samuel, 2016-06-03 19:06:45]

Description was changed from

==========
Implement cc::SharedQuadState StructTraits

cc::SharedQuadState does not have a copy constructor and so
I'm mapping to and from std::unique_ptr<cc::SharedQuadState>.

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Fady Samuel, 2016-06-03 19:09:49]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-06-03 19:10:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032643002/160001

[Ben Goodger (Google), 2016-06-03 19:27:37]

lgtm

[commit-bot: I haz the power, 2016-06-03 21:35:52]

Description was changed from

==========
Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-03 21:35:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-03 21:37:15]

Description was changed from

==========
Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Implement cc::SharedQuadState StructTraits

BUG=611802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4690c5bb2208c108dfd76a210cd2bec61c67c884
Cr-Commit-Position: refs/heads/master@{#397812}
==========

[commit-bot: I haz the power, 2016-06-03 21:37:16]

Patchset 9 (id:??) landed as
https://crrev.com/4690c5bb2208c108dfd76a210cd2bec61c67c884
Cr-Commit-Position: refs/heads/master@{#397812}