Fix ImageBitmap constructor from ImageData to consider the color space tags

Fix ImageBitmap constructor from ImageData to consider the color space tags

This CL fixes ImageBitmap constructor from ImageData such that the constructor
respects the color space tags of both ImageData and target ImageBitmap (as passed
in ImageBitmapOptions parameter).
This is a follow up for CL 2522693002, part of color management implementation
for ImageBitmap as described in proposal:
(github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md).

BUG=665919
Review-Url: https://codereview.chromium.org/2630563003
Cr-Commit-Position: refs/heads/master@{#445453}
Committed: https://chromium.googlesource.com/chromium/src/+/f5600ef4dc75558dd888842e742df522550fefc5

[zakerinasab1, 2017-01-12 19:09:45]

Description was changed from

==========
Fix ImageBitmap constructor from ImageData to consider the color space tags

This CL fixes ImageBitmap constructor from ImageData such that the constructor
respects the color space tags of both ImageData and target ImageBitmap (as
passed
in ImageBitmapOptions parameter).
This is a follow up for CL 2522693002, part of color management implementation
for ImageBitmap as described in proposal:
(github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md).

BUG=665919
==========

to

==========
Fix ImageBitmap constructor from ImageData to consider the color space tags

This CL fixes ImageBitmap constructor from ImageData such that the constructor
respects the color space tags of both ImageData and target ImageBitmap (as
passed
in ImageBitmapOptions parameter).
This is a follow up for CL 2522693002, part of color management implementation
for ImageBitmap as described in proposal:
(github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md).

BUG=665919
==========

[zakerinasab1, 2017-01-12 19:09:45]

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

[zakerinasab1, 2017-01-12 19:10:41]

zakerinasab@google.com changed reviewers:
+ zakerinasab@google.com

[zakerinasab1, 2017-01-12 19:10:42]

New CL submitted for ImageBitmap(ImageData) constructor. PTAL.

[Justin Novosad, 2017-01-17 20:00:09]

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:73: if
(options.colorSpaceConversion() != imageBitmapOptionNone) {
Please file a crbug to get rid of all the string literals in this file, in favor
of using constants that are defined at the top.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:331: sk_sp<SkData> srcData
= SkData::MakeUninitialized(size);
When out of memory, does this crash or return a null pointer?  If it can return
a null pointer, you have to check it.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:353: dstInfo, dstData,
image->width() * dstInfo.bytesPerPixel());
There is a potential security vulnerability here.  If the above call to
SkColorSpaceXform::apply fails, the this line will be generating an image from
uninitialized RAM, which may leak secrets.  You should check the return value of
apply() to know whether it succeeded.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:741: colorType =
kRGBA_F16_SkColorType;
This approach is fragile.  You should add a colorType argument to this function.
 The destination ColorType should be determined at the same place where we
determine the colorSpace (i.e. where we parse the options.)

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:912: SkImageInfo imageInfo
= SkImageInfo::Make(1, 1, SkColorType::kN32_SkColorType,
This variable appear to be unused.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:921: sk_sp<SkSurface>
surface = SkSurface::MakeRaster(SkImageInfo::MakeN32Premul(
I don'think it is Okay to assume N32 here.

[zakerinasab1, 2017-01-18 18:34:21]

Comments addressed. New patch set uploaded.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:73: if
(options.colorSpaceConversion() != imageBitmapOptionNone) {
On 2017/01/17 20:00:09, Justin Novosad wrote:
> Please file a crbug to get rid of all the string literals in this file, in
favor
> of using constants that are defined at the top.

Done. crbug.com/682321

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:331: sk_sp<SkData> srcData
= SkData::MakeUninitialized(size);
On 2017/01/17 20:00:09, Justin Novosad wrote:
> When out of memory, does this crash or return a null pointer?  If it can
return
> a null pointer, you have to check it.

When out of memory, it calls SK_ABORT("sk_throw") down the road. I added size
check just to be safe.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:353: dstInfo, dstData,
image->width() * dstInfo.bytesPerPixel());
On 2017/01/17 20:00:09, Justin Novosad wrote:
> There is a potential security vulnerability here.  If the above call to
> SkColorSpaceXform::apply fails, the this line will be generating an image from
> uninitialized RAM, which may leak secrets.  You should check the return value
of
> apply() to know whether it succeeded.

Done.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:741: colorType =
kRGBA_F16_SkColorType;
On 2017/01/17 20:00:09, Justin Novosad wrote:
> This approach is fragile.  You should add a colorType argument to this
function.
>  The destination ColorType should be determined at the same place where we
> determine the colorSpace (i.e. where we parse the options.)

Done.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:912: SkImageInfo imageInfo
= SkImageInfo::Make(1, 1, SkColorType::kN32_SkColorType,
On 2017/01/17 20:00:09, Justin Novosad wrote:
> This variable appear to be unused.

Ah, that was for debug. Removed.

https://codereview.chromium.org/2630563003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:921: sk_sp<SkSurface>
surface = SkSurface::MakeRaster(SkImageInfo::MakeN32Premul(
On 2017/01/17 20:00:09, Justin Novosad wrote:
> I don'think it is Okay to assume N32 here.

I think it is safe because this constructor always uses ImageData (not
Float32ImageData), so there is no need to consider non-N32 color types.

[Justin Novosad, 2017-01-23 15:55:07]

lgtm

[zakerinasab1, 2017-01-23 18:20:46]

The CQ bit was checked by zakerinasab@google.com

[zakerinasab1, 2017-01-23 18:20:46]

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/2630563003/#ps60001
(title: "Rebaseline")

[commit-bot: I haz the power, 2017-01-23 18:21: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, 2017-01-23 20:09:14]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485195646159670,
"parent_rev": "c6eede7d4c764d788ca02a02ab45c0f72309905b", "commit_rev":
"f5600ef4dc75558dd888842e742df522550fefc5"}

[commit-bot: I haz the power, 2017-01-23 20:09:50]

Description was changed from

==========
Fix ImageBitmap constructor from ImageData to consider the color space tags

This CL fixes ImageBitmap constructor from ImageData such that the constructor
respects the color space tags of both ImageData and target ImageBitmap (as
passed
in ImageBitmapOptions parameter).
This is a follow up for CL 2522693002, part of color management implementation
for ImageBitmap as described in proposal:
(github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md).

BUG=665919
==========

to

==========
Fix ImageBitmap constructor from ImageData to consider the color space tags

This CL fixes ImageBitmap constructor from ImageData such that the constructor
respects the color space tags of both ImageData and target ImageBitmap (as
passed
in ImageBitmapOptions parameter).
This is a follow up for CL 2522693002, part of color management implementation
for ImageBitmap as described in proposal:
(github.com/junov/CanvasColorSpace/blob/master/CanvasColorSpaceProposal.md).

BUG=665919

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

[commit-bot: I haz the power, 2017-01-23 20:09:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f5600ef4dc75558dd888842e742d...