update callers to not use SkColorProfileType

update callers to not use SkColorProfileType

Requires https://codereview.chromium.org/2087833002/ to land first.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2086583002

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

[reed1, 2016-06-20 17:37:34]

Description was changed from

==========
update callers to not use SkColorProfileType

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

to

==========
update callers to not use SkColorProfileType

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2086583002
==========

[reed1, 2016-06-20 17:37:42]

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

[commit-bot: I haz the power, 2016-06-20 17:37:45]

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

[commit-bot: I haz the power, 2016-06-20 17:44:55]

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

[commit-bot: I haz the power, 2016-06-20 17:44:55]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2016-06-20 18:54:51]

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

[msarett, 2016-06-20 19:41:05]

Woohoo!  Mostly nits.

https://codereview.chromium.org/2086583002/diff/1/bench/SkLinearBitmapPipelin...
File bench/SkLinearBitmapPipelineBench.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/bench/SkLinearBitmapPipelin...
bench/SkLinearBitmapPipelineBench.cpp:93:
SkColorSpace::NewNamed(SkColorSpace::kSRGB_Named) : nullptr);
nit: spacing

https://codereview.chromium.org/2086583002/diff/1/bench/SkLinearBitmapPipelin...
bench/SkLinearBitmapPipelineBench.cpp:216: const bool gIsSRGB = true;
nit: This is a little confusing.  Maybe gSRGB?

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpace.cpp#n...
src/core/SkColorSpace.cpp:1224: return a == b;
So this will only work when both are sRGB or Adobe RGB (because we have once
ptrs).

Other profiles could be equivalent (or nearly equivalent) which we could
determine with more work.

I think a TODO is fine.  We may want to consider FastEq, SlowEq, ApproxEq, etc.

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpacePriv.h
File src/core/SkColorSpacePriv.h (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpacePriv.h...
src/core/SkColorSpacePriv.h:13: class SkColorSpacePriv {
Why not just put this is SkColorSpace_Base?

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpacePriv.h...
src/core/SkColorSpacePriv.h:15: static bool EQ(SkColorSpace*, SkColorSpace*);
nit: Seems like a strange function name to me, but I get it...  Why not
Equals()?

https://codereview.chromium.org/2086583002/diff/1/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkDraw.cpp#newcode...
src/core/SkDraw.cpp:1564: if (!fDevice->imageInfo().gammaCloseToSRGB()) {
!gammaCloseToSRGB() can mean "linear" or "wacky".  I would guess that this is
fine here (meaning we are always linear or srgb), but we should be careful to
handle all three cases when the src is encoded data.

[Brian Osman, 2016-06-20 20:09:19]

https://codereview.chromium.org/2086583002/diff/1/src/core/SkBlitter_Sprite.cpp
File src/core/SkBlitter_Sprite.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkBlitter_Sprite.c...
src/core/SkBlitter_Sprite.cpp:63: if
(SkColorSpacePriv::EQ(dst.info().colorSpace(), src.info().colorSpace())) {
Should this be !EQ ?

https://codereview.chromium.org/2086583002/diff/1/src/core/SkBlitter_Sprite.c...
src/core/SkBlitter_Sprite.cpp:85: if (dst.colorType() != kN32_SkColorType ||
nullptr == dst.info().colorSpace()) {
What exactly was this checking for before? I don't think these conditions are
the same (need to check the pointer, and then check if it's linear, or sRGB, or
something).

[reed1, 2016-06-20 21:05:53]

https://codereview.chromium.org/2086583002/diff/1/bench/SkLinearBitmapPipelin...
File bench/SkLinearBitmapPipelineBench.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/bench/SkLinearBitmapPipelin...
bench/SkLinearBitmapPipelineBench.cpp:216: const bool gIsSRGB = true;
On 2016/06/20 19:41:05, msarett wrote:
> nit: This is a little confusing.  Maybe gSRGB?

Done.

https://codereview.chromium.org/2086583002/diff/1/src/core/SkBlitter_Sprite.cpp
File src/core/SkBlitter_Sprite.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkBlitter_Sprite.c...
src/core/SkBlitter_Sprite.cpp:63: if
(SkColorSpacePriv::EQ(dst.info().colorSpace(), src.info().colorSpace())) {
On 2016/06/20 20:09:19, Brian Osman wrote:
> Should this be !EQ ?

Done.

https://codereview.chromium.org/2086583002/diff/1/src/core/SkBlitter_Sprite.c...
src/core/SkBlitter_Sprite.cpp:85: if (dst.colorType() != kN32_SkColorType ||
nullptr == dst.info().colorSpace()) {
On 2016/06/20 20:09:19, Brian Osman wrote:
> What exactly was this checking for before? I don't think these conditions are
> the same (need to check the pointer, and then check if it's linear, or sRGB,
or
> something).

Not sure :) Seems like this class wants to be SRGB specific. I will investigate
more.

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpace.cpp
File src/core/SkColorSpace.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpace.cpp#n...
src/core/SkColorSpace.cpp:1224: return a == b;
On 2016/06/20 19:41:05, msarett wrote:
> So this will only work when both are sRGB or Adobe RGB (because we have once
> ptrs).
> 
> Other profiles could be equivalent (or nearly equivalent) which we could
> determine with more work.
> 
> I think a TODO is fine.  We may want to consider FastEq, SlowEq, ApproxEq,
etc.

Removed

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpacePriv.h
File src/core/SkColorSpacePriv.h (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkColorSpacePriv.h...
src/core/SkColorSpacePriv.h:13: class SkColorSpacePriv {
On 2016/06/20 19:41:05, msarett wrote:
> Why not just put this is SkColorSpace_Base?

Removed

https://codereview.chromium.org/2086583002/diff/1/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/2086583002/diff/1/src/core/SkDraw.cpp#newcode...
src/core/SkDraw.cpp:1564: if (!fDevice->imageInfo().gammaCloseToSRGB()) {
On 2016/06/20 19:41:05, msarett wrote:
> !gammaCloseToSRGB() can mean "linear" or "wacky".  I would guess that this is
> fine here (meaning we are always linear or srgb), but we should be careful to
> handle all three cases when the src is encoded data.

Will mark this with a TODO

[reed1, 2016-06-20 21:05:57]

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

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

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

[msarett, 2016-06-20 21:10:30]

lgtm + nit

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

https://codereview.chromium.org/2086583002/diff/20001/include/core/SkColorSpa...
include/core/SkColorSpace.h:87: friend class SkColorSpacePriv;
Remove this.

[reed1, 2016-06-20 21:17:58]

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

https://codereview.chromium.org/2086583002/diff/20001/include/core/SkColorSpa...
include/core/SkColorSpace.h:87: friend class SkColorSpacePriv;
On 2016/06/20 21:10:30, msarett wrote:
> Remove this.

Done.

[reed1, 2016-06-20 21:18:02]

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

[commit-bot: I haz the power, 2016-06-20 21:18:13]

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

[commit-bot: I haz the power, 2016-06-20 21:38:20]

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

[commit-bot: I haz the power, 2016-06-20 21:38:21]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-06-21 15:17:16]

Description was changed from

==========
update callers to not use SkColorProfileType

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2086583002
==========

to

==========
update callers to not use SkColorProfileType

Requires https://codereview.chromium.org/2087833002/ to land first.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2086583002
==========

[reed1, 2016-06-21 15:17:24]

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

[commit-bot: I haz the power, 2016-06-21 15:17:32]

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

[commit-bot: I haz the power, 2016-06-21 15:39:24]

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

[commit-bot: I haz the power, 2016-06-21 15:39:25]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-06-21 17:26:52]

The CQ bit was checked by reed@google.com

[reed1, 2016-06-21 17:26:53]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com
Link to the patchset: https://codereview.chromium.org/2086583002/#ps80001
(title: "rm flag instead of commenting it out")

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

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

[commit-bot: I haz the power, 2016-06-21 17:28:17]

Description was changed from

==========
update callers to not use SkColorProfileType

Requires https://codereview.chromium.org/2087833002/ to land first.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2086583002
==========

to

==========
update callers to not use SkColorProfileType

Requires https://codereview.chromium.org/2087833002/ to land first.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2086583002

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

[commit-bot: I haz the power, 2016-06-21 17:28:17]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/dabe5d3780dee956882c0d65c979ccd09a99ea8f