Make SkColorSpace a public API

Make SkColorSpace a public API

I've trimmed uniqueID for now, and added some comments.

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

Committed: https://skia.googlesource.com/skia/+/8cc209111876b7c78b5ec577c9221d8ed5e21024

[msarett, 2016-05-20 15:32:59]

Description was changed from

==========
Make SkColorSpace a public API

I've trimmed uniqueID for now, and added some comments.

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

to

==========
Make SkColorSpace a public API

I've trimmed uniqueID for now, and added some comments.

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

[msarett, 2016-05-20 15:36:07]

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

[msarett, 2016-05-20 15:36:08]

Let's go for it...

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h
File include/core/SkColorSpace.h (left):

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h...
include/core/SkColorSpace.h:11: // Some terms
I don't feel strongly about removing these comments - just seems like they might
belong somewhere else.

[reed1, 2016-05-20 21:10:43]

reed@google.com changed reviewers:
+ brianosman@google.com, herb@google.com

[reed1, 2016-05-20 21:10:44]

I bet we could eliminate the forward declarations for colortables and
gammacurves if we really wanted (e.g. by having a private "base" class that we
always cast-to when we want to access private things.

That said, this looks pretty good as is.

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h
File include/core/SkColorSpace.h (right):

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h...
include/core/SkColorSpace.h:59: Named named() const { return fNamed; }
do we need this guy?

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h...
include/core/SkColorSpace.h:60: 
Do we have an idea for the srgb-like query we're gonna want?

e.g.

bool isGammaCloseTo2Dot2() const;

[msarett, 2016-05-23 13:59:01]

Patchset #2 (id:20001) has been deleted

[msarett, 2016-05-23 14:00:02]

Patchset #2 (id:40001) has been deleted

[msarett, 2016-05-23 14:03:49]

"I bet we could eliminate the forward declarations for colortables and
gammacurves if we really wanted (e.g. by having a private "base" class that we
always cast-to when we want to access private things."

Added a private base class, just to see how it looks.

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h
File include/core/SkColorSpace.h (right):

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h...
include/core/SkColorSpace.h:59: Named named() const { return fNamed; }
On 2016/05/20 21:10:44, reed1 wrote:
> do we need this guy?

Not right now, I'm deleting it because no one is using it.

Since we go to the trouble of checking if the xyz values match srgb (or adobe
rgb) at create time, it may make sense to expose that info to the client at some
point (so they don't also have to check).

https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h...
include/core/SkColorSpace.h:60: 
On 2016/05/20 21:10:44, reed1 wrote:
> Do we have an idea for the srgb-like query we're gonna want?
> 
> e.g.
> 
> bool isGammaCloseTo2Dot2() const;

Yeah I had an idea.  I'll pull it in from another CL.  It returns an enum
(rather than a bool), so we can say "2.2", "linear", or "complicated".

For images, we don't really need the "linear" so the bool would be fine, but I
think that other sources may be linear?

[Brian Osman, 2016-05-23 14:10:30]

On 2016/05/23 14:03:49, msarett wrote:
>
https://codereview.chromium.org/1996973002/diff/1/include/core/SkColorSpace.h...
> include/core/SkColorSpace.h:60: 
> On 2016/05/20 21:10:44, reed1 wrote:
> > Do we have an idea for the srgb-like query we're gonna want?
> > 
> > e.g.
> > 
> > bool isGammaCloseTo2Dot2() const;
> 
> Yeah I had an idea.  I'll pull it in from another CL.  It returns an enum
> (rather than a bool), so we can say "2.2", "linear", or "complicated".
> 
> For images, we don't really need the "linear" so the bool would be fine, but I
> think that other sources may be linear?

Yeah. And despite the absence of linear images on common end-user websites,
there are
definitely linear images in the wild. Certainly, people doing advanced graphics
work
are likely to use linear encoded images for things like mathematical lookup
tables,
normal maps, gloss maps, etc... Those are the kinds of images that may not be
published on the web, except as examples in very specific situations (like a
graphics
blog, where someone might be showing what one of those images looks like).

[msarett, 2016-05-23 15:22:51]

https://codereview.chromium.org/1996973002/diff/60001/include/core/SkColorSpa...
File include/core/SkColorSpace.h (right):

https://codereview.chromium.org/1996973002/diff/60001/include/core/SkColorSpa...
include/core/SkColorSpace.h:15: class SkColorSpace : public SkRefCnt {
Should this be SK_API?

[reed1, 2016-05-23 15:26:45]

lgtm w/ nit about parameter name in _Base.h

https://codereview.chromium.org/1996973002/diff/60001/src/core/SkColorSpace_B...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/1996973002/diff/60001/src/core/SkColorSpace_B...
src/core/SkColorSpace_Base.h:113: static inline SkColorSpace_Base*
as_CSB(SkColorSpace* image) {
image?

[msarett, 2016-05-23 15:31:46]

https://codereview.chromium.org/1996973002/diff/60001/src/core/SkColorSpace_B...
File src/core/SkColorSpace_Base.h (right):

https://codereview.chromium.org/1996973002/diff/60001/src/core/SkColorSpace_B...
src/core/SkColorSpace_Base.h:113: static inline SkColorSpace_Base*
as_CSB(SkColorSpace* image) {
On 2016/05/23 15:26:45, reed1 wrote:
> image?

Thanks for catching!  Changed to colorSpace.

[msarett, 2016-05-23 15:31:53]

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

[commit-bot: I haz the power, 2016-05-23 15:31:58]

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

[commit-bot: I haz the power, 2016-05-23 15:36:08]

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

[commit-bot: I haz the power, 2016-05-23 15:36:09]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[msarett, 2016-05-23 15:39:21]

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

[commit-bot: I haz the power, 2016-05-23 15:39:28]

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

[commit-bot: I haz the power, 2016-05-23 15:43:38]

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

[commit-bot: I haz the power, 2016-05-23 15:43:39]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[msarett, 2016-05-23 16:09:36]

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

[commit-bot: I haz the power, 2016-05-23 16:09:44]

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

[commit-bot: I haz the power, 2016-05-23 16:27:35]

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

[commit-bot: I haz the power, 2016-05-23 16:27:36]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-05-23 16:28:31]

The CQ bit was checked by msarett@google.com

[msarett, 2016-05-23 16:28:32]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1996973002/#ps100001
(title: "Remove SkColorSpace.h from gyp")

[commit-bot: I haz the power, 2016-05-23 16:28:34]

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

[commit-bot: I haz the power, 2016-05-23 16:29:32]

Description was changed from

==========
Make SkColorSpace a public API

I've trimmed uniqueID for now, and added some comments.

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

to

==========
Make SkColorSpace a public API

I've trimmed uniqueID for now, and added some comments.

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/+/8cc209111876b7c78b5ec577c9221d8ed5e21024
==========

[commit-bot: I haz the power, 2016-05-23 16:29:33]

Committed patchset #4 (id:100001) as
https://skia.googlesource.com/skia/+/8cc209111876b7c78b5ec577c9221d8ed5e21024