Prepare SkColorSpace to be a public API

Prepare SkColorSpace to be a public API

Moves implementation details into SkColorSpacePriv.h

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1985903002

Committed: https://skia.googlesource.com/skia/+/bb9f77437dad6a127840e6898edb3f811e3e9e94

[msarett, 2016-05-16 22:26:48]

Description was changed from

==========
Prepare SkColorSpace to be a public API

Moves implementation details into SkColorSpacePriv.h

BUG=skia:
==========

to

==========
Prepare SkColorSpace to be a public API

Moves implementation details into SkColorSpacePriv.h

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-05-16 22:35:52]

msarett@google.com changed reviewers:
+ reed@google.com, scroggo@google.com

[reed1, 2016-05-17 13:59:24]

like it

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h
File src/core/SkColorSpace.h (right):

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h...
src/core/SkColorSpace.h:49: SkGammas* gammas() const { return fGammas.get(); }
Do we need this getter to be public? I guess its ok if we do, but since SkGammas
is private, its a little funny.

Related: do we have a way for clients to identify the gamma ...
- precisely (if they care)?
- roughly sRGB-ish or not?

[msarett, 2016-05-17 15:40:53]

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h
File src/core/SkColorSpace.h (right):

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h...
src/core/SkColorSpace.h:49: SkGammas* gammas() const { return fGammas.get(); }
On 2016/05/17 13:59:24, reed1 wrote:
> Do we need this getter to be public? I guess its ok if we do, but since
SkGammas
> is private, its a little funny.
> 

Agreed that it's funny, but it's useful for test code.

> Related: do we have a way for clients to identify the gamma ...
> - precisely (if they care)?
> - roughly sRGB-ish or not?

As we discuseed, I think it'd be nice to add an API that returns an enum... 
Something like kSRGBish, kComplicated, kLinear.  I'll leave that for a follow
up.

[reed1, 2016-05-17 15:46:28]

lgtm

[msarett, 2016-05-17 15:47:15]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 15:47:17]

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

[commit-bot: I haz the power, 2016-05-17 16:06:46]

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

[commit-bot: I haz the power, 2016-05-17 16:06:47]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-05-17 16:30:12]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-05-17 16:30:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1985903002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1985903002/20001

[commit-bot: I haz the power, 2016-05-17 16:31:22]

Description was changed from

==========
Prepare SkColorSpace to be a public API

Moves implementation details into SkColorSpacePriv.h

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Prepare SkColorSpace to be a public API

Moves implementation details into SkColorSpacePriv.h

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/bb9f77437dad6a127840e6898edb3f811e3e9e94
==========

[commit-bot: I haz the power, 2016-05-17 16:31:23]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/bb9f77437dad6a127840e6898edb3f811e3e9e94

[scroggo, 2016-05-17 16:54:28]

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h
File src/core/SkColorSpace.h (right):

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h...
src/core/SkColorSpace.h:26: #include "SkTemplates.h"
I think the new way to include this file is "../private/SkTemplates.h"

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h...
src/core/SkColorSpace.h:49: SkGammas* gammas() const { return fGammas.get(); }
On 2016/05/17 15:40:52, msarett wrote:
> On 2016/05/17 13:59:24, reed1 wrote:
> > Do we need this getter to be public? I guess its ok if we do, but since
> SkGammas
> > is private, its a little funny.
> > 
> 
> Agreed that it's funny, but it's useful for test code.

Maybe we should document that it is only meant to be used in tests?

> 
> > Related: do we have a way for clients to identify the gamma ...
> > - precisely (if they care)?
> > - roughly sRGB-ish or not?
> 
> As we discuseed, I think it'd be nice to add an API that returns an enum... 
> Something like kSRGBish, kComplicated, kLinear.  I'll leave that for a follow
> up.

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpacePr...
File src/core/SkColorSpacePriv.h (right):

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpacePr...
src/core/SkColorSpacePriv.h:49: struct SkGammas : public SkRefCnt {
Why is gamma ref counted? I guess it's because of SkGammaCurve::fTable that you
don't want to worry about copying?

It seems potentially problematic because it has public members that can be
modified.

[msarett, 2016-05-17 17:27:13]

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h
File src/core/SkColorSpace.h (right):

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h...
src/core/SkColorSpace.h:26: #include "SkTemplates.h"
On 2016/05/17 16:54:27, scroggo wrote:
> I think the new way to include this file is "../private/SkTemplates.h"

Acknowledged.

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpace.h...
src/core/SkColorSpace.h:49: SkGammas* gammas() const { return fGammas.get(); }
On 2016/05/17 16:54:27, scroggo wrote:
> On 2016/05/17 15:40:52, msarett wrote:
> > On 2016/05/17 13:59:24, reed1 wrote:
> > > Do we need this getter to be public? I guess its ok if we do, but since
> > SkGammas
> > > is private, its a little funny.
> > > 
> > 
> > Agreed that it's funny, but it's useful for test code.
> 
> Maybe we should document that it is only meant to be used in tests?
> 
> > 
> > > Related: do we have a way for clients to identify the gamma ...
> > > - precisely (if they care)?
> > > - roughly sRGB-ish or not?
> > 
> > As we discuseed, I think it'd be nice to add an API that returns an enum... 
> > Something like kSRGBish, kComplicated, kLinear.  I'll leave that for a
follow
> > up.
> 

sgtm

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpacePr...
File src/core/SkColorSpacePriv.h (right):

https://codereview.chromium.org/1985903002/diff/20001/src/core/SkColorSpacePr...
src/core/SkColorSpacePriv.h:49: struct SkGammas : public SkRefCnt {
On 2016/05/17 16:54:27, scroggo wrote:
> Why is gamma ref counted? I guess it's because of SkGammaCurve::fTable that
you
> don't want to worry about copying?
> 

A couple reasons:
(1) As you say, it's nice to not have to std::move all the time.
(2) Now that it's a malloced ptr, it's nice to share a ref [ 2.2f, 2.2f, 2.2f ]
for the many images with standard gamma.

> It seems potentially problematic because it has public members that can be
> modified.

Ahh yes that's bad.  I think I can make these const, I'll follow up.