Use gfx::ColorSpace instead of SkColorSpace in Blink

Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

For CanvasRenderingContext and Canvas2DLayerBridge, create a
distinction between the gfx::ColorSpace that the canvas is to be
interpreted in by the compositor, and the SkColorSpace that is to be
used by SkSurfaces during rendering (for canvases that are not using
Skia's color conversion, but still need to specify their color space to
the compositor).

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2660393002
Cr-Commit-Position: refs/heads/master@{#447969}
Committed: https://chromium.googlesource.com/chromium/src/+/628896ee553990e2f0dc60a5a2c7b0b44535a10f

[ccameron, 2017-01-30 22:25:48]

Description was changed from

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

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

to

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ccameron, 2017-01-30 22:27:06]

Description was changed from

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ccameron, 2017-01-30 22:27:06]

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

[ccameron, 2017-01-30 22:33:03]

Description was changed from

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

For CanvasRenderingContext and Canvas2DLayerBridge, create a
distinction between the gfx::ColorSpace that the canvas is to be
interpreted in by the compositor, and the SkColorSpace that is to be
used by SkSurfaces during rendering.

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ccameron, 2017-01-30 22:34:36]

Description was changed from

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

For CanvasRenderingContext and Canvas2DLayerBridge, create a
distinction between the gfx::ColorSpace that the canvas is to be
interpreted in by the compositor, and the SkColorSpace that is to be
used by SkSurfaces during rendering.

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

For CanvasRenderingContext and Canvas2DLayerBridge, create a
distinction between the gfx::ColorSpace that the canvas is to be
interpreted in by the compositor, and the SkColorSpace that is to be
used by SkSurfaces during rendering (for canvases that are not using
Skia's color conversion, but still need to specify their color space to
the compositor).

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[ccameron, 2017-01-30 22:50:49]

ptal

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (left):

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:885: static
gfx::ColorSpace SkColorSpaceToColorSpace(
Also avoiding this conversion is part of the motivation.

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp
(right):

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp:92: if
(m_graphicsLayer->colorBehavior().isTransformToTargetColorSpace()) {
Avoid this extra contorted is one of the main motivations for this patch.

[Justin Novosad, 2017-01-30 23:58:28]

It seem like you are only going half way in the CL.  Is this because you are
landing incrementally?

[Justin Novosad, 2017-01-31 00:15:59]

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:67:
m_gfxColorSpace = gfx::ColorSpace(
I am a bit concerned about having this duplication between m_colorSpace and
m_gfxColorSpace. Would be a more maintainable design to have a single ground
truth, like we had before this change where the skColorSpace() method was
generating SkColorSpaces on the fly.  If you are concerned about the overhead of
creating gfx::ColorSpace, we could use lazily initialized static instances, but
I don't think that is necessary.

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h:300:
sk_sp<SkColorSpace> m_skSurfaceColorSpace;
Do we really need to store an SkColorSpace here? I'd rather we rely on
gfx::ColorSpace::ToSkColorSpace() if that works.

[ccameron, 2017-01-31 05:34:27]

Oh -- I think this is as far as I'm planning to push this.

Some of the sub-classes (e.g, ImageBufferSurface) still use SkSurface, but
that's because they're using this directly as an argument to Skia, so I don't
see the need to wrap it.

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:67:
m_gfxColorSpace = gfx::ColorSpace(
On 2017/01/31 00:15:58, Justin Novosad wrote:
> I am a bit concerned about having this duplication between m_colorSpace and
> m_gfxColorSpace. Would be a more maintainable design to have a single ground
> truth, like we had before this change where the skColorSpace() method was
> generating SkColorSpaces on the fly.  If you are concerned about the overhead
of
> creating gfx::ColorSpace, we could use lazily initialized static instances,
but
> I don't think that is necessary.

Yeah, that was unnecessary optimization. Got rid of it.

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h (right):

https://codereview.chromium.org/2660393002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h:300:
sk_sp<SkColorSpace> m_skSurfaceColorSpace;
On 2017/01/31 00:15:58, Justin Novosad wrote:
> Do we really need to store an SkColorSpace here? I'd rather we rely on
> gfx::ColorSpace::ToSkColorSpace() if that works.

I changed this to a bool, "m_skSurfacesUseColorSpace", which is all we need.

That, in turn, comes from whether or not we had a legacy canvas or not.

[ccameron, 2017-01-31 05:35:20]

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

[ccameron, 2017-01-31 05:35:25]

(new version uploaded, ptal)

[commit-bot: I haz the power, 2017-01-31 05:35:28]

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, 2017-01-31 05:54:54]

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

[commit-bot: I haz the power, 2017-01-31 05:54:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2017-01-31 09:24:20]

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

[commit-bot: I haz the power, 2017-01-31 09:24:26]

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, 2017-01-31 10:57:58]

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

[commit-bot: I haz the power, 2017-01-31 10:57:59]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[ccameron, 2017-02-02 17:22:03]

ping again

[Justin Novosad, 2017-02-02 18:35:43]

lgtm

[ccameron, 2017-02-03 03:41:39]

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

[commit-bot: I haz the power, 2017-02-03 03:42:16]

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, 2017-02-03 03:45:44]

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

[commit-bot: I haz the power, 2017-02-03 03:45:45]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[ccameron, 2017-02-03 07:28:38]

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

[commit-bot: I haz the power, 2017-02-03 07:28:46]

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

[ccameron, 2017-02-03 07:50:43]

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

[ccameron, 2017-02-03 07:50:44]

Adding thakis@ for third_party/WebKit/PRESUBMIT.py

[Nico, 2017-02-03 08:20:02]

LGTM

esprehn, what's the guideline for allowing gfx types in blink (see presubmit
change)?

(But given the CubicBezier precedent, this seems fine to me)

[ccameron, 2017-02-03 08:40:42]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2017-02-03 08:40:48]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-02-03 08:40:49]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org
Link to the patchset: https://codereview.chromium.org/2660393002/#ps80001
(title: "Rebase (again)")

[commit-bot: I haz the power, 2017-02-03 08:40:57]

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, 2017-02-03 09:29:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486111248811170,
"parent_rev": "7496904fab33e2c38f8c6ec6cfad27ebd158c1f4", "commit_rev":
"628896ee553990e2f0dc60a5a2c7b0b44535a10f"}

[commit-bot: I haz the power, 2017-02-03 09:29:48]

Description was changed from

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

For CanvasRenderingContext and Canvas2DLayerBridge, create a
distinction between the gfx::ColorSpace that the canvas is to be
interpreted in by the compositor, and the SkColorSpace that is to be
used by SkSurfaces during rendering (for canvases that are not using
Skia's color conversion, but still need to specify their color space to
the compositor).

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Use gfx::ColorSpace instead of SkColorSpace in Blink

gfx::ColorSpace is used as input (speciying monitor color space) and
output (compositor texture mailbox) format for color spaces. Avoid
conversions by using this directly instead of SkColorSpace.

The SkColorSpace to pass to Skia APIs is accessible via the method
gfx::ColorSpace::ToSkColorSpace.

Delete gfx::ColorSpace::FromSkColorSpace and
gfx::ICCProfile::FromSkColorSpace, since they could not be implemented
reliably.

For CanvasRenderingContext and Canvas2DLayerBridge, create a
distinction between the gfx::ColorSpace that the canvas is to be
interpreted in by the compositor, and the SkColorSpace that is to be
used by SkSurfaces during rendering (for canvases that are not using
Skia's color conversion, but still need to specify their color space to
the compositor).

BUG=634102
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2660393002
Cr-Commit-Position: refs/heads/master@{#447969}
Committed:
https://chromium.googlesource.com/chromium/src/+/628896ee553990e2f0dc60a5a2c7...
==========

[commit-bot: I haz the power, 2017-02-03 09:29:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/628896ee553990e2f0dc60a5a2c7...