Implement canvas color space IDL format for 2D canvas

Implement canvas color space IDL format for 2D canvas

This patch updates the previous implementation of the canvas color space
to match the latest changes of the proposal in
https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpaceProposal.md

BUG=634542
Review-Url: https://codereview.chromium.org/2708403003
Cr-Commit-Position: refs/heads/master@{#453052}
Committed: https://chromium.googlesource.com/chromium/src/+/d0b25b0a993e6623ddf9deb789cbd6f9ae35435d

[zakerinasab, 2017-02-22 21:05:33]

Description was changed from

==========
Implement canvas color space IDL format for 2D canvas

This patch updates the previous implementation of the canvas color space
to match the latest changes of the proposal in
https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpacePropos...

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

to

==========
Implement canvas color space IDL format for 2D canvas

This patch updates the previous implementation of the canvas color space
to match the latest changes of the proposal in
https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpacePropos...

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

[zakerinasab, 2017-02-22 21:05:33]

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

[zakerinasab, 2017-02-22 21:07:34]

zakerinasab@chromium.org changed reviewers:
+ esprehn@chromium.org

[zakerinasab, 2017-02-23 17:46:25]

Patch uploaded to update the color managed canvas 2D code to match the new
changes in the proposal. PTAL.

[Justin Novosad, 2017-02-23 18:48:13]

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:769: # Layout tests from
fast/cavnas/color-space only pass on virtual/color_space and fail, as expected,
everywhere else
In this case, you should probably put the test directly under
virtual/color_space/fast/canvas. Then you won't need these lines in
TestExpectations

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/color-space/canvas-colorspace-arguments.html
(right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/color-space/canvas-colorspace-arguments.html:7:
["Test CanvasColorSpace value srgb", ["srgb"], ["srgb", "8-8-8-8", false]],
Would be better to use a dictionary instead of an array for the context creation
parameters.  This would eliminate the need for the code that construct the
dictionary in runTestScenario. It would also allow you to test more cases.
Example: {} and {pixelFormat: "8-8-8-8"}

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/canvas/CanvasContextCreationAttributes.idl
(right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasContextCreationAttributes.idl:25:
"legacy-srgb", // default
The revised proposal removes legacy srgb.  Basically, "srgb" matches the color
space used for CSS, so basically the color correct rendering  flag determines
whether or not we're in legacy mode.

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
(right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:124:
bool useF16 = false);
Better to use an enum than a bool for this. Makes the call site more readable.

[zakerinasab, 2017-02-23 20:22:33]

Comments addressed. New patch uploaded. PTAL.

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/TestExpectations:769: # Layout tests from
fast/cavnas/color-space only pass on virtual/color_space and fail, as expected,
everywhere else
On 2017/02/23 18:48:13, Justin Novosad wrote:
> In this case, you should probably put the test directly under
> virtual/color_space/fast/canvas. Then you won't need these lines in
> TestExpectations

It was like that in the beginning, but then I thought it may cause confusion
about where the fast/canvas/color-space tests are. I'll do that though.

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/color-space/canvas-colorspace-arguments.html
(right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/color-space/canvas-colorspace-arguments.html:7:
["Test CanvasColorSpace value srgb", ["srgb"], ["srgb", "8-8-8-8", false]],
On 2017/02/23 18:48:13, Justin Novosad wrote:
> Would be better to use a dictionary instead of an array for the context
creation
> parameters.  This would eliminate the need for the code that construct the
> dictionary in runTestScenario. It would also allow you to test more cases.
> Example: {} and {pixelFormat: "8-8-8-8"}

Done.

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/html/canvas/CanvasContextCreationAttributes.idl
(right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasContextCreationAttributes.idl:25:
"legacy-srgb", // default
On 2017/02/23 18:48:13, Justin Novosad wrote:
> The revised proposal removes legacy srgb.  Basically, "srgb" matches the color
> space used for CSS, so basically the color correct rendering  flag determines
> whether or not we're in legacy mode.

Done.

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp
(right):

https://codereview.chromium.org/2708403003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:124:
bool useF16 = false);
On 2017/02/23 18:48:13, Justin Novosad wrote:
> Better to use an enum than a bool for this. Makes the call site more readable.

Done.

[Justin Novosad, 2017-02-23 21:51:32]

So far so good. Will take another look once you've addressed the failing tests.

[zakerinasab, 2017-02-24 17:47:08]

On 2017/02/23 21:51:32, Justin Novosad wrote:
> So far so good. Will take another look once you've addressed the failing
tests.

It shall be fixed now. PTAL.

[Justin Novosad, 2017-02-24 18:16:44]

lgtm

Please file a bug for moving the test out of virtual/color_space once color
correct rendering is enabled by default.

[zakerinasab, 2017-02-24 18:19:53]

On 2017/02/24 18:16:44, Justin Novosad wrote:
> lgtm
> 
> Please file a bug for moving the test out of virtual/color_space once color
> correct rendering is enabled by default.

I'll file a general bug to move all the tests as I expect to have more tests
there by that time. I'll proceed with landing this one when I get a lgtm for
modules_idl_files.gni.

[zakerinasab, 2017-02-24 18:53:59]

zakerinasab@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-02-24 23:56:16]

LGTM

[zakerinasab, 2017-02-25 01:59:24]

The CQ bit was checked by zakerinasab@chromium.org

[commit-bot: I haz the power, 2017-02-25 01:59:41]

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-25 02:46:27]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487987964979380,
"parent_rev": "3fffa417af5b70accf1fe290b60de87c419be5df", "commit_rev":
"d0b25b0a993e6623ddf9deb789cbd6f9ae35435d"}

[commit-bot: I haz the power, 2017-02-25 02:47:01]

Description was changed from

==========
Implement canvas color space IDL format for 2D canvas

This patch updates the previous implementation of the canvas color space
to match the latest changes of the proposal in
https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpacePropos...

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

to

==========
Implement canvas color space IDL format for 2D canvas

This patch updates the previous implementation of the canvas color space
to match the latest changes of the proposal in
https://github.com/WICG/canvas-color-space/blob/master/CanvasColorSpacePropos...

BUG=634542

Review-Url: https://codereview.chromium.org/2708403003
Cr-Commit-Position: refs/heads/master@{#453052}
Committed:
https://chromium.googlesource.com/chromium/src/+/d0b25b0a993e6623ddf9deb789cb...
==========

[commit-bot: I haz the power, 2017-02-25 02:47:02]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d0b25b0a993e6623ddf9deb789cb...