Add some plumbing for the color management of canvases

Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/d5e03be7f3be92a1940c19aab0664ca02ce1011f
Cr-Commit-Position: refs/heads/master@{#414108}

[Justin Novosad, 2016-08-04 23:34:34]

Description was changed from

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
==========

to

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Justin Novosad, 2016-08-04 23:34:35]

Description was changed from

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Justin Novosad, 2016-08-04 23:44:32]

junov@chromium.org changed reviewers:
+ chrishtr@chromium.org, danakj@chromium.org, senorblanco@chromium.org

[Justin Novosad, 2016-08-04 23:44:32]

Reviewers:
danakj -> cc
senorblanco -> third_party/WebKit/Source/
chrishtr -> third_party/WebKit/public

[chrishtr, 2016-08-08 22:04:31]

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:911: :
CanvasRenderingContext(passedCanvas, passedOffscreenCanvas, "")
No namespace?

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

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.h:33:
private:
Useless, remove.

[chrishtr, 2016-08-08 22:04:50]

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:911: :
CanvasRenderingContext(passedCanvas, passedOffscreenCanvas, "")
On 2016/08/08 at 22:04:31, chrishtr wrote:
> No namespace?

s/namespace/colorspace/

[Justin Novosad, 2016-08-11 19:32:26]

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

[Justin Novosad, 2016-08-11 19:32:57]

Ping danakj + senorblanco

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(right):

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:911: :
CanvasRenderingContext(passedCanvas, passedOffscreenCanvas, "")
On 2016/08/08 22:04:50, chrishtr wrote:
> On 2016/08/08 at 22:04:31, chrishtr wrote:
> > No namespace?
> 
> s/namespace/colorspace/

My intention is to land in two phases. Phase two will add the plumbing to WebGL

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

https://codereview.chromium.org/2212163002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.h:33:
private:
On 2016/08/08 22:04:31, chrishtr wrote:
> Useless, remove.

Done.

[commit-bot: I haz the power, 2016-08-11 19:33:12]

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-08-11 19:45:22]

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

[commit-bot: I haz the power, 2016-08-11 19:45:24]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chrishtr, 2016-08-11 20:22:55]

lgtm

[Stephen White, 2016-08-11 20:41:27]

https://codereview.chromium.org/2212163002/diff/20001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/2212163002/diff/20001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:70: // assumption: using srgb or
linear-rgb
Assert that assumption?

https://codereview.chromium.org/2212163002/diff/20001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/2212163002/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1501: 
Nit: remove whitespace change, unless intentional.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:61:
CHECK(0);
Nit: CHECK(false) seems to be more common than CHECK(0).

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:86:
CHECK(0);
Ibid.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:147:
CanvasRenderingContext(HTMLCanvasElement*, OffscreenCanvas*, const String&
colorSpace);
As discussed, I think we should pass in (a ref to) the whole
CanvasContextCreationAttributes struct, and have the base class here pull out
the ones it's interested in. It'll keep the constructor args from ballooning in
the future, and allow us to centralize more attribute handling between 2D &
WebGL where that makes sense.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl:29:
[RuntimeEnabled=ExperimentalCanvasFeatures] DOMString colorSpace =
"legacy-srgb";
Let's log a bug to decide if we really want to ship "legacy-srgb" as an option,
or remove it before removing the flag.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:914:
attrs.setColorSpace(colorSpaceAsString());
Shouldn't the base class be responsible for this? (We'll have to virtualize
getContextAttributes() and call the base class from here, but that seems like
that would be the most scalable way to do it.)

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:78:
std::unique_ptr<ImageBufferSurface> surface(wrapUnique(new
UnacceleratedImageBufferSurface(size, opacityMode, initializationMode,
colorSpace)));
Could we use std::move() here too?

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:21:
: ImageBufferSurface(size, opacityMode, colorSpace)
std::move()?

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp:85:
return wrapUnique(new UnacceleratedImageBufferSurface(size, opacityMode,
InitializeImagePixels, colorSpace));
std::move()?

[Justin Novosad, 2016-08-12 17:32:47]

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

[commit-bot: I haz the power, 2016-08-12 17:33:14]

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

[Justin Novosad, 2016-08-12 17:40:17]

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

[Justin Novosad, 2016-08-12 17:40:18]

+bajones for modules/webgl
FYI: Changed context creation attribute plumbing in WebGL to defer the
conversion to WebGLContextAttributes. This allows the attributes to be stored
and managed in shared code in the base class (CanvasRenderingContext).

https://codereview.chromium.org/2212163002/diff/20001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/2212163002/diff/20001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:70: // assumption: using srgb or
linear-rgb
On 2016/08/11 20:41:27, Stephen White wrote:
> Assert that assumption?

Done.

https://codereview.chromium.org/2212163002/diff/20001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/2212163002/diff/20001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1501: 
On 2016/08/11 20:41:27, Stephen White wrote:
> Nit: remove whitespace change, unless intentional.

Done.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:61:
CHECK(0);
On 2016/08/11 20:41:27, Stephen White wrote:
> Nit: CHECK(false) seems to be more common than CHECK(0).

Done.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp:86:
CHECK(0);
On 2016/08/11 20:41:27, Stephen White wrote:
> Ibid.

Done.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:147:
CanvasRenderingContext(HTMLCanvasElement*, OffscreenCanvas*, const String&
colorSpace);
On 2016/08/11 20:41:27, Stephen White wrote:
> As discussed, I think we should pass in (a ref to) the whole
> CanvasContextCreationAttributes struct, and have the base class here pull out
> the ones it's interested in. It'll keep the constructor args from ballooning
in
> the future, and allow us to centralize more attribute handling between 2D &
> WebGL where that makes sense.

Done.  This was a bit more complicated than anticipated: I had to change all the
attribute plumbing on the WebGL side so that the generic attributes (not the
WebGL-specif ones) can be handed down to the base class constructor.  It's
better this way.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl:29:
[RuntimeEnabled=ExperimentalCanvasFeatures] DOMString colorSpace =
"legacy-srgb";
On 2016/08/11 20:41:27, Stephen White wrote:
> Let's log a bug to decide if we really want to ship "legacy-srgb" as an
option,
> or remove it before removing the flag.

Done. crbug.com/637288

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2D.cpp:914:
attrs.setColorSpace(colorSpaceAsString());
On 2016/08/11 20:41:27, Stephen White wrote:
> Shouldn't the base class be responsible for this? (We'll have to virtualize
> getContextAttributes() and call the base class from here, but that seems like
> that would be the most scalable way to do it.)

Nope.  The base class is in core, so it can't know about
Canvas2DContextAttributes, it only knows about CanvasContextCreationAttributes
(the non-specific flavor of the attributes).

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp (right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp:78:
std::unique_ptr<ImageBufferSurface> surface(wrapUnique(new
UnacceleratedImageBufferSurface(size, opacityMode, initializationMode,
colorSpace)));
On 2016/08/11 20:41:27, Stephen White wrote:
> Could we use std::move() here too?

Done.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp:21:
: ImageBufferSurface(size, opacityMode, colorSpace)
On 2016/08/11 20:41:27, Stephen White wrote:
> std::move()?

Done.

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurfaceTest.cpp:85:
return wrapUnique(new UnacceleratedImageBufferSurface(size, opacityMode,
InitializeImagePixels, colorSpace));
On 2016/08/11 20:41:27, Stephen White wrote:
> std::move()?

Done.

[bajones, 2016-08-12 17:50:14]

modules/webgl LGTM. kbr@ gave it a verbal thumbs up as well.

[commit-bot: I haz the power, 2016-08-12 17:50:30]

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

[commit-bot: I haz the power, 2016-08-12 17:50:31]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Justin Novosad, 2016-08-12 19:14:25]

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

[commit-bot: I haz the power, 2016-08-12 19:14:53]

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

[Justin Novosad, 2016-08-12 19:15:03]

New Patch

[commit-bot: I haz the power, 2016-08-12 19:28:06]

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

[commit-bot: I haz the power, 2016-08-12 19:28:07]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Stephen White, 2016-08-12 19:48:31]

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl
(right):

https://codereview.chromium.org/2212163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl:29:
[RuntimeEnabled=ExperimentalCanvasFeatures] DOMString colorSpace =
"legacy-srgb";
On 2016/08/12 17:40:18, Justin Novosad wrote:
> On 2016/08/11 20:41:27, Stephen White wrote:
> > Let's log a bug to decide if we really want to ship "legacy-srgb" as an
> option,
> > or remove it before removing the flag.
> 
> Done. crbug.com/637288

Add a comment here referencing that bug?

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:83: virtual
bool hasAlpha() const { return creationAttributes().alpha(); }
As discussed, let's see if we can get rid of this double-virtual hasAlpha().

(And long-term, let's see if there's a way to get rid of the spaghetti
inheritance!)

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/HTMLCanvasPainterTest.cpp (right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/HTMLCanvasPainterTest.cpp:59: size, 0,
NonOpaque, Canvas2DLayerBridge::ForceAccelerationForTesting, nullptr));
Nit: maybe wrap this line at 80 cols, since the previous one is wrapped as well?
(your call)

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl
(right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl:29:
[RuntimeEnabled=ExperimentalCanvasFeatures] DOMString colorSpace =
"legacy-srgb";
Nit: I'd still like a reference to the crbug in a comment here.

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebExternalTextureMailbox.h:48: const
SkColorSpace* colorSpace;
Maybe we should document that WebExternalTextureMailbox struct should never
outlive the SkColorSpace, since otherwise ownership gets murky.

[Justin Novosad, 2016-08-12 20:08:49]

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
(right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h:83: virtual
bool hasAlpha() const { return creationAttributes().alpha(); }
On 2016/08/12 19:48:31, Stephen White wrote:
> As discussed, let's see if we can get rid of this double-virtual hasAlpha().
> 
> (And long-term, let's see if there's a way to get rid of the spaghetti
> inheritance!)

Done.

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/HTMLCanvasPainterTest.cpp (right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/HTMLCanvasPainterTest.cpp:59: size, 0,
NonOpaque, Canvas2DLayerBridge::ForceAccelerationForTesting, nullptr));
On 2016/08/12 19:48:31, Stephen White wrote:
> Nit: maybe wrap this line at 80 cols, since the previous one is wrapped as
well?
> (your call)

Done.

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl
(right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/Canvas2DContextAttributes.idl:29:
[RuntimeEnabled=ExperimentalCanvasFeatures] DOMString colorSpace =
"legacy-srgb";
On 2016/08/12 19:48:31, Stephen White wrote:
> Nit: I'd still like a reference to the crbug in a comment here.

Done.

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/2212163002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebExternalTextureMailbox.h:48: const
SkColorSpace* colorSpace;
On 2016/08/12 19:48:31, Stephen White wrote:
> Maybe we should document that WebExternalTextureMailbox struct should never
> outlive the SkColorSpace, since otherwise ownership gets murky.

Done.

[Stephen White, 2016-08-12 20:11:57]

LGTM

[Justin Novosad, 2016-08-12 20:13:51]

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

[commit-bot: I haz the power, 2016-08-12 20:14:12]

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-08-12 20:27:47]

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

[commit-bot: I haz the power, 2016-08-12 20:27:48]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[danakj, 2016-08-12 21:03:48]

What's the plan for gfx::ColorSpace vs SkColorSpace? Is blink going to produce
gfx::ColorSpace or?

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:64: static gfx::ColorSpace
toGfxColorSpace(const SkColorSpace* skColorSpace) {
how about SkColorSpaceToColorSpace (like SkRectToRectF and friends)

also use chromium style please for function and variable names

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:66: // SkColorSpace and
gfx::ColorSpace
comments need punctuation and capitalization

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:70: // Assumption: using srgb or
linear-rgb
comments need punctuation, should be proper sentences. but this is literally
saying what the DCHECK is doing, not explaining it. remove?

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:74: skColorSpace->gammaNamed() ==
SkColorSpace::kSRGB_GammaNamed
can this be a switch instead? so new enum values force people to update here? or
would that be bad?

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:77: return
gfx::ColorSpace::ColorSpace(gfx::ColorSpace::PrimaryID::BT709,
what is gfx::ColorSpace::ColorSpace()? is that the constructor..? I didnt know
you can write it that way if so.

https://codereview.chromium.org/2212163002/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/2212163002/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:814: static sk_sp<SkColorSpace>
toSkColorSpace(const gfx::ColorSpace& color_space) {
chromium style function names plz.

ColorSpaceToSkColorSpace?

[Justin Novosad, 2016-08-15 18:07:04]

FYI: The plan we are considering for gfx::ColorSpace is to make it a wrapper of
SkColorSpace  See crbug.com/634102

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
File cc/blink/web_external_texture_layer_impl.cc (right):

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:64: static gfx::ColorSpace
toGfxColorSpace(const SkColorSpace* skColorSpace) {
On 2016/08/12 21:03:48, danakj wrote:
> how about SkColorSpaceToColorSpace (like SkRectToRectF and friends)
> 
> also use chromium style please for function and variable names

Done.

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:66: // SkColorSpace and
gfx::ColorSpace
On 2016/08/12 21:03:48, danakj wrote:
> comments need punctuation and capitalization

Done.

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:70: // Assumption: using srgb or
linear-rgb
On 2016/08/12 21:03:48, danakj wrote:
> comments need punctuation, should be proper sentences. but this is literally
> saying what the DCHECK is doing, not explaining it. remove?

Done.

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:74: skColorSpace->gammaNamed() ==
SkColorSpace::kSRGB_GammaNamed
On 2016/08/12 21:03:48, danakj wrote:
> can this be a switch instead? so new enum values force people to update here?
or
> would that be bad?

Done.

https://codereview.chromium.org/2212163002/diff/80001/cc/blink/web_external_t...
cc/blink/web_external_texture_layer_impl.cc:77: return
gfx::ColorSpace::ColorSpace(gfx::ColorSpace::PrimaryID::BT709,
On 2016/08/12 21:03:48, danakj wrote:
> what is gfx::ColorSpace::ColorSpace()? is that the constructor..? I didnt know
> you can write it that way if so.

This was bad code. somehow the compiler was letting me get away with it on
linux.  Try bots failed on some other platforms.

https://codereview.chromium.org/2212163002/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/2212163002/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:814: static sk_sp<SkColorSpace>
toSkColorSpace(const gfx::ColorSpace& color_space) {
On 2016/08/12 21:03:48, danakj wrote:
> chromium style function names plz.
> 
> ColorSpaceToSkColorSpace?
> 

Done.

[Justin Novosad, 2016-08-15 18:08:54]

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

[commit-bot: I haz the power, 2016-08-15 18:09:20]

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-08-15 18:31:15]

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

[commit-bot: I haz the power, 2016-08-15 18:31:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Justin Novosad, 2016-08-15 19:04:44]

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

[commit-bot: I haz the power, 2016-08-15 19:05:10]

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-08-15 20:33:59]

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

[commit-bot: I haz the power, 2016-08-15 20:34:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Justin Novosad, 2016-08-15 21:16:11]

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

[commit-bot: I haz the power, 2016-08-15 21:16:25]

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-08-15 22:34:59]

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

[commit-bot: I haz the power, 2016-08-15 22:35:00]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2016-08-18 19:29:10]

danakj, friendly ping.

[Justin Novosad, 2016-08-19 21:09:17]

Description was changed from

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Justin Novosad, 2016-08-19 21:09:25]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-08-19 21:09:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, bajones@chromium.org, senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/2212163002/#ps140001
(title: "sepculative fix for crashing tests")

[commit-bot: I haz the power, 2016-08-19 21:09:58]

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-08-19 23:30:29]

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

[commit-bot: I haz the power, 2016-08-19 23:30:30]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Justin Novosad, 2016-08-22 21:18:37]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-08-22 21:20:26]

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-08-22 21:26:56]

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

[commit-bot: I haz the power, 2016-08-22 21:26:57]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Justin Novosad, 2016-08-22 21:37:25]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-08-22 21:37:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org, chrishtr@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2212163002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-22 21:38:23]

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-08-22 21:56:40]

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

[commit-bot: I haz the power, 2016-08-22 21:56:43]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Justin Novosad, 2016-08-23 21:03:21]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-08-23 21:03:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org, chrishtr@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2212163002/#ps180001
(title: "big rebase after refactor")

[commit-bot: I haz the power, 2016-08-23 21:04:33]

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-08-23 22:03:45]

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

[commit-bot: I haz the power, 2016-08-23 22:03:49]

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

[Justin Novosad, 2016-08-24 14:23:30]

The CQ bit was checked by junov@chromium.org

[Justin Novosad, 2016-08-24 14:23:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org, chrishtr@chromium.org, bajones@chromium.org
Link to the patchset: https://codereview.chromium.org/2212163002/#ps200001
(title: "rebase again")

[commit-bot: I haz the power, 2016-08-24 14:23:46]

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

[Justin Novosad, 2016-08-24 18:03:03]

Description was changed from

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Justin Novosad, 2016-08-24 18:03:59]

The CQ bit was unchecked by junov@chromium.org

[Justin Novosad, 2016-08-24 18:04:10]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:04:52]

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

[Justin Novosad, 2016-08-24 18:05:07]

The CQ bit was unchecked by junov@chromium.org

[Justin Novosad, 2016-08-24 18:05:25]

The CQ bit was checked by junov@chromium.org

[commit-bot: I haz the power, 2016-08-24 18:06:21]

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-08-24 18:22:30]

Description was changed from

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-24 18:22:33]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-08-24 18:26:18]

Description was changed from

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Add some plumbing for the color management of canvases

This change adds an experimental canvas feature for specifying a
color space at context creation time. This change also adds the
necessary plumbing to propagate the color space information to the
SkSurface objects that back the canvas, as well as the mailboxes and
compositor resources that transport the canvas contents

BUG=634542
TBR=danakj
NOTRY=true
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[commit-bot: I haz the power, 2016-08-24 18:26:20]

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