Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

When a gfx::ICCProfile is created from a gfx::ColorSpace, store the
original gfx::ColorSpace that it was created from, so that when
gfx::ICCProfile::GetColorSpace is called, we can return the exact same
original object.

BUG=657946
Committed: https://crrev.com/b074f5f0a72f308e79120570fd3da24263f217c9
Cr-Commit-Position: refs/heads/master@{#432430}

[ccameron, 2016-11-14 19:37:22]

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

[ccameron, 2016-11-14 19:37:23]

rockot@: do we have any how-to guides about adding mojo type support?

In particular, for a private enum type nested in a class, how do we do this? Do
we next the mojo types (if so, how?)?

https://codereview.chromium.org/2502703002/diff/1/ui/gfx/mojo/icc_profile_str...
File ui/gfx/mojo/icc_profile_struct_traits.cc (right):

https://codereview.chromium.org/2502703002/diff/1/ui/gfx/mojo/icc_profile_str...
ui/gfx/mojo/icc_profile_struct_traits.cc:18: // out->color_space_ =
data.color_space();
I'll need these two entries to be communicated as well.

So far I've been cargo-culting them to the .mojom and .typemap files, but this
has not succeeded.

Is there documentation about what exactly we're supposed to do to semi-convert
these things to mojo?

Will I need to add mojo support for gfx::ColorSpace?

Is there an example of how to add private enum support to mojo?

[ccameron, 2016-11-14 20:23:13]

I just made the nested enum public ... I'm content to ignore that issue for now.
Now I'm just trying to get the ICCProfile::Type to be sent across the wire
(after that I can try to the color space). The spew I see is 

In file included from gen/ui/gfx/mojo/icc_profile.mojom.cc:15:
In file included from gen/ui/gfx/mojo/icc_profile.mojom.h:31:
gen/ui/gfx/mojo/icc_profile.mojom-shared.h:150:12: error: implicit instantiation
of undefined template 'mojo::EnumTraits<gfx::mojom::ICCProfileType,
gfx::ICCProfile::Type>'
    return Traits::FromMojom(static_cast<::gfx::mojom::ICCProfileType>(input),
output);
           ^
../../mojo/public/cpp/bindings/lib/serialization_forward.h:70:48: note: in
instantiation of member function
'mojo::internal::Serializer<gfx::mojom::ICCProfileType,
gfx::ICCProfile::Type>::Deserialize' requested here
  return Serializer<MojomType, InputUserType>::Deserialize(
                                               ^
gen/ui/gfx/mojo/icc_profile.mojom-shared.h:89:28: note: in instantiation of
function template specialization
'mojo::internal::Deserialize<gfx::mojom::ICCProfileType, int &,
gfx::ICCProfile::Type, nullptr>' requested here
    return mojo::internal::Deserialize<::gfx::mojom::ICCProfileType>(
                           ^
gen/ui/gfx/mojo/icc_profile.mojom.cc:64:18: note: in instantiation of function
template specialization
'gfx::mojom::ICCProfileDataView::ReadType<gfx::ICCProfile::Type>' requested here
      if (!input.ReadType(&result->type))
                 ^
../../mojo/public/cpp/bindings/enum_traits.h:23:8: note: template is declared
here
struct EnumTraits;

[ccameron, 2016-11-14 21:43:09]

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

[commit-bot: I haz the power, 2016-11-14 21:44:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2016-11-14 22:02:46]

Description was changed from

==========
Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

When a gfx::ICCProfile is created from a gfx::ColorSpace, store the
original gfx::ColorSpace that it was created from, so that when
gfx::ICCProfile::GetColorSpace is called, we can return the exact same
original object.

BUG=657946
==========

to

==========
Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

When a gfx::ICCProfile is created from a gfx::ColorSpace, store the
original gfx::ColorSpace that it was created from, so that when
gfx::ICCProfile::GetColorSpace is called, we can return the exact same
original object.

BUG=657946
==========

[ccameron, 2016-11-14 22:02:46]

ccameron@chromium.org changed reviewers:
+ dcheng@chromium.org
- rockot@chromium.org

[ccameron, 2016-11-14 22:04:45]

ccameron@chromium.org changed reviewers:
+ rockot@chromium.org
- dcheng@chromium.org

[ccameron, 2016-11-14 22:04:46]

ptal

fsamuel@ helped me out by pointing out the use of [Native], so this is building
now.

[ccameron, 2016-11-14 22:05:02]

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

[commit-bot: I haz the power, 2016-11-14 22:08:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 23:26:24]

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

[commit-bot: I haz the power, 2016-11-14 23:26:24]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-11-15 00:22:22]

On 2016/11/14 at 19:37:23, ccameron wrote:
> rockot@: do we have any how-to guides about adding mojo type support?
> 
> In particular, for a private enum type nested in a class, how do we do this?
Do we next the mojo types (if so, how?)?
> 
>
https://codereview.chromium.org/2502703002/diff/1/ui/gfx/mojo/icc_profile_str...
> File ui/gfx/mojo/icc_profile_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2502703002/diff/1/ui/gfx/mojo/icc_profile_str...
> ui/gfx/mojo/icc_profile_struct_traits.cc:18: // out->color_space_ =
data.color_space();
> I'll need these two entries to be communicated as well.
> 
> So far I've been cargo-culting them to the .mojom and .typemap files, but this
has not succeeded.
> 
> Is there documentation about what exactly we're supposed to do to semi-convert
these things to mojo?
> 
> Will I need to add mojo support for gfx::ColorSpace?
> 
> Is there an example of how to add private enum support to mojo?

It's not clear to me exactly what you're asking. Mojom doesn't have a notion of
private types and it wouldn't really make sense to. If you want to use a typemap
with an enum type, the enum either has to be publicly visible, or you need to
make the traits specialization a friend of the encapsulating class.

[ccameron, 2016-11-15 00:26:51]

On 2016/11/15 00:22:22, Ken Rockot wrote:
> On 2016/11/14 at 19:37:23, ccameron wrote:
> > rockot@: do we have any how-to guides about adding mojo type support?
> > 
> > In particular, for a private enum type nested in a class, how do we do this?
> Do we next the mojo types (if so, how?)?
> > 
> >
>
https://codereview.chromium.org/2502703002/diff/1/ui/gfx/mojo/icc_profile_str...
> > File ui/gfx/mojo/icc_profile_struct_traits.cc (right):
> > 
> >
>
https://codereview.chromium.org/2502703002/diff/1/ui/gfx/mojo/icc_profile_str...
> > ui/gfx/mojo/icc_profile_struct_traits.cc:18: // out->color_space_ =
> data.color_space();
> > I'll need these two entries to be communicated as well.
> > 
> > So far I've been cargo-culting them to the .mojom and .typemap files, but
this
> has not succeeded.
> > 
> > Is there documentation about what exactly we're supposed to do to
semi-convert
> these things to mojo?
> > 
> > Will I need to add mojo support for gfx::ColorSpace?
> > 
> > Is there an example of how to add private enum support to mojo?
> 
> It's not clear to me exactly what you're asking. Mojom doesn't have a notion
of
> private types and it wouldn't really make sense to. If you want to use a
typemap
> with an enum type, the enum either has to be publicly visible, or you need to
> make the traits specialization a friend of the encapsulating class.

I have it cleared up now. I was curious if there was a particular convention for
nested types in mojom, and WRT a private sub-type if there were conventions for
the expected types.

But, I've side-stepped those issues in the patch that is up.

[ccameron, 2016-11-15 22:41:45]

ping

[Ken Rockot(use gerrit already), 2016-11-15 22:59:42]

LGTM - sorry, I didn't realize you were just waiting for me to review

[ccameron, 2016-11-15 23:01:54]

On 2016/11/15 22:59:42, Ken Rockot wrote:
> LGTM - sorry, I didn't realize you were just waiting for me to review

Np -- sorry, I was unclear about the transition from "CL as a question" to "CL
to review".

[ccameron, 2016-11-15 23:06:59]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-11-15 23:07:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 23:30:51]

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

[commit-bot: I haz the power, 2016-11-15 23:30:52]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ccameron, 2016-11-15 23:42:11]

ccameron@chromium.org changed reviewers:
+ dcheng@chromium.org

[ccameron, 2016-11-15 23:42:12]

Adding dcheng@ for ipc OWNER

[dcheng, 2016-11-16 01:17:01]

LGTM

Just to confirm, this is still mostly about sending from browser to less
privileged processes? I notice there's a few COlorSpace things that aren't yet
fully implemented (and I just want to make sure the browser wont' end up
crashing if we call somethign like ColorSpace::ToSkColorSpace())

[ccameron, 2016-11-16 06:09:50]

On 2016/11/16 01:17:01, dcheng wrote:
> LGTM
> 
> Just to confirm, this is still mostly about sending from browser to less
> privileged processes? I notice there's a few COlorSpace things that aren't yet
> fully implemented (and I just want to make sure the browser wont' end up
> crashing if we call somethign like ColorSpace::ToSkColorSpace())

Yes, this is adding to ICCProfile, which is strictly browser->renderer. All
renderer->browser communication remains unchanged.

[ccameron, 2016-11-16 06:10:00]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2016-11-16 06:10:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 08:35:30]

Description was changed from

==========
Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

When a gfx::ICCProfile is created from a gfx::ColorSpace, store the
original gfx::ColorSpace that it was created from, so that when
gfx::ICCProfile::GetColorSpace is called, we can return the exact same
original object.

BUG=657946
==========

to

==========
Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

When a gfx::ICCProfile is created from a gfx::ColorSpace, store the
original gfx::ColorSpace that it was created from, so that when
gfx::ICCProfile::GetColorSpace is called, we can return the exact same
original object.

BUG=657946
==========

[commit-bot: I haz the power, 2016-11-16 08:35:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-16 08:38:30]

Description was changed from

==========
Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

When a gfx::ICCProfile is created from a gfx::ColorSpace, store the
original gfx::ColorSpace that it was created from, so that when
gfx::ICCProfile::GetColorSpace is called, we can return the exact same
original object.

BUG=657946
==========

to

==========
Add more robust gfx::ColorSpace<->gfx::ICCProfile conversion

When a gfx::ICCProfile is created from a gfx::ColorSpace, store the
original gfx::ColorSpace that it was created from, so that when
gfx::ICCProfile::GetColorSpace is called, we can return the exact same
original object.

BUG=657946

Committed: https://crrev.com/b074f5f0a72f308e79120570fd3da24263f217c9
Cr-Commit-Position: refs/heads/master@{#432430}
==========

[commit-bot: I haz the power, 2016-11-16 08:38:31]

Patchset 4 (id:??) landed as
https://crrev.com/b074f5f0a72f308e79120570fd3da24263f217c9
Cr-Commit-Position: refs/heads/master@{#432430}