Remove Float32ImageData and update ImageData for color management

Remove Float32ImageData and update ImageData for color management

Remove Float32ImageData and update ImageData to match the latest changes
in the color managed canvas proposal:
github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md

BUG=670712
Review-Url: https://codereview.chromium.org/2728373004
Cr-Commit-Position: refs/heads/master@{#456708}
Committed: https://chromium.googlesource.com/chromium/src/+/31afd3a4d2c0763304f080c519776cb37ee743a7

[zakerinasab, 2017-03-07 19:36:53]

Description was changed from

==========
Remove Float32ImageData and update ImageData for color management

Remove Float32ImageData and update ImageData to match the latest changes
in the color managed canvas proposal:
github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md

BUG=670712
==========

to

==========
Remove Float32ImageData and update ImageData for color management

Remove Float32ImageData and update ImageData to match the latest changes
in the color managed canvas proposal:
github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md

BUG=670712
==========

[zakerinasab, 2017-03-07 19:36:53]

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

[zakerinasab, 2017-03-10 15:51:31]

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

[Justin Novosad, 2017-03-10 20:30:26]

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:387: if
(m_colorSettings.storageFormat() == kUint8ClampedArrayStorageFormatName) {
This series of if/else would look better as a switch/case IMHO.

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.h (right):

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.h:129: DEFINE_INLINE_TRACE() {
This is big enough that it should no longer be inline.

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.idl (right):

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.idl:53:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute ImageDataArray
dataUnion;
Great that you figured out how to make this work!  Did you benchmark this?  I
would like to know whether accessing img.dataUnion[x] is any slower than
accessing img.data[x].

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.idl:54:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute
ImageDataColorSettings colorAttributes;
perhaps this should be called "settings"?

[zakerinasab, 2017-03-13 15:43:27]

New patch submitted.

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.cpp:387: if
(m_colorSettings.storageFormat() == kUint8ClampedArrayStorageFormatName) {
On 2017/03/10 20:30:26, Justin Novosad wrote:
> This series of if/else would look better as a switch/case IMHO.

Done.

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.h (right):

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.h:129: DEFINE_INLINE_TRACE() {
On 2017/03/10 20:30:26, Justin Novosad wrote:
> This is big enough that it should no longer be inline.

Done.

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageData.idl (right):

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.idl:53:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute ImageDataArray
dataUnion;
On 2017/03/10 20:30:26, Justin Novosad wrote:
> Great that you figured out how to make this work!  Did you benchmark this?  I
> would like to know whether accessing img.dataUnion[x] is any slower than
> accessing img.data[x].

A new layout test added to print out a performance comparison.

https://codereview.chromium.org/2728373004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageData.idl:54:
[RuntimeEnabled=ExperimentalCanvasFeatures] readonly attribute
ImageDataColorSettings colorAttributes;
On 2017/03/10 20:30:26, Justin Novosad wrote:
> perhaps this should be called "settings"?

My intuition was that you try to apply "settings", but then you will have the
"attributes", which might match or do not match the settings. Anyway, I changed
it to settings.

[Justin Novosad, 2017-03-13 18:41:06]

https://codereview.chromium.org/2728373004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/color-space/imageData-colorSpace-perf.html
(right):

https://codereview.chromium.org/2728373004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/color-space/imageData-colorSpace-perf.html:1:
<!DOCTYPE html>
Performance tests like this one belong under
third_party/WebKit/PerformanceTests/canvas please look at other tests in that
directory for examples of how to use the test harness.  The harness is designed
to work with our blink_perf telemetry tests, and will show up on the performance
dashboard, and it will be continuously monitored.

[zakerinasab, 2017-03-13 19:32:19]

Patchset #6 (id:100001) has been deleted

[zakerinasab, 2017-03-13 19:33:11]

New patch uploaded.

https://codereview.chromium.org/2728373004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/color-space/imageData-colorSpace-perf.html
(right):

https://codereview.chromium.org/2728373004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/color-space/imageData-colorSpace-perf.html:1:
<!DOCTYPE html>
On 2017/03/13 18:41:06, Justin Novosad wrote:
> Performance tests like this one belong under
> third_party/WebKit/PerformanceTests/canvas please look at other tests in that
> directory for examples of how to use the test harness.  The harness is
designed
> to work with our blink_perf telemetry tests, and will show up on the
performance
> dashboard, and it will be continuously monitored.

Done.

[jochen (gone - plz use gerrit), 2017-03-13 19:44:09]

as this removes a web-exposed feature it seems, there should be an intent to
remove. Can you link to it from the CL description please?

[Justin Novosad, 2017-03-13 19:47:45]

On 2017/03/13 19:44:09, jochen wrote:
> as this removes a web-exposed feature it seems, there should be an intent to
> remove. Can you link to it from the CL description please?

The interface that it removed was never shipped.  We are basically iterating on
the canvas color correction feature (intent to implement already sent)

[jochen (gone - plz use gerrit), 2017-03-13 19:50:55]

can you help me understand why virtual/stable/webexposed changes?

[zakerinasab, 2017-03-13 19:56:59]

On 2017/03/13 19:50:55, jochen wrote:
> can you help me understand why virtual/stable/webexposed changes?

The partial interface "interface ImageData { readonly attribute
ImageDataColorSpace colorSpace; }" was added in this issue:

https://codereview.chromium.org/2555213002

We are still implementing this, so this should have been put behind the
experimental-canvas-features flag at the first place. It was not, and that's the
reason that now virtual/stable/webexposed is changed.

[Justin Novosad, 2017-03-13 20:00:52]

https://codereview.chromium.org/2728373004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageData.idl (left):

https://codereview.chromium.org/2728373004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageData.idl:48: readonly attribute
ImageDataColorSpace colorSpace;
Whoops, looks like this attribute was accidentally shipped in a previous CL.  I
am pretty sure there are no consequences to unshipping it since it is completely
useless without the other APIs of the color correction ecosystem.

[jochen (gone - plz use gerrit), 2017-03-13 21:53:53]

ok, idl files and webexposed changes lgtm

[Justin Novosad, 2017-03-14 13:49:32]

lgtm

[zakerinasab, 2017-03-14 14:08:51]

The CQ bit was checked by zakerinasab@chromium.org

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

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-03-14 15:40:02]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489500531116520,
"parent_rev": "7eaf2657fde72f7cc64482cfb0d701bbaf34648a", "commit_rev":
"31afd3a4d2c0763304f080c519776cb37ee743a7"}

[commit-bot: I haz the power, 2017-03-14 15:40:40]

Description was changed from

==========
Remove Float32ImageData and update ImageData for color management

Remove Float32ImageData and update ImageData to match the latest changes
in the color managed canvas proposal:
github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md

BUG=670712
==========

to

==========
Remove Float32ImageData and update ImageData for color management

Remove Float32ImageData and update ImageData to match the latest changes
in the color managed canvas proposal:
github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md

BUG=670712

Review-Url: https://codereview.chromium.org/2728373004
Cr-Commit-Position: refs/heads/master@{#456708}
Committed:
https://chromium.googlesource.com/chromium/src/+/31afd3a4d2c0763304f080c51977...
==========

[commit-bot: I haz the power, 2017-03-14 15:40:44]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/31afd3a4d2c0763304f080c51977...