Implement ImageBitmap options premultiplyAlpha

Implement ImageBitmap options premultiplyAlpha

In this CL, we implement the option of premultiplyAlpha for ImageBitmap.
At this moment, it works for createImageBitmap from HTMLImageElement,
HTMLCanvasElement and Blob. The cases of from ImageBitmap, HTMLVideoElemtn
and ImageData will be solved in future CL(s).

This CL also includes modifies a current layout tests, which tests
the combination of ImageBitmap options "flipY" and premultiplyAlpha

BUG=249382
Committed: https://crrev.com/dfbf8dd24ba36edab0e567c3f98f4ed6ad8dd1e2
Cr-Commit-Position: refs/heads/master@{#374132}

[xidachen, 2016-01-20 02:29:06]

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

[xidachen, 2016-01-20 02:29:07]

PTAL.

https://codereview.chromium.org/1609763002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/1609763002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:82:
surface->getCanvas()->drawImage(skiaImage.get(), dstLeft, dstTop);
This program could crash at this point. It is because this skiaImage is created
from a SkBitmap in the case when premultiplyAlpha=false, and this line of code
will trigger a call to SkBitmapDevice::SkBitmapDevice and goes to line 81 of
SkBitmapDevice which is a SkASSERT, that SkASSERT always fail because it
requires the alpha type is either kPremul_SkAlphaType or kOpaque_SkAlphaType.
But in our case, it is kUnpremul_SkAlphaType.

Any suggestions?

https://codereview.chromium.org/1609763002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right):

https://codereview.chromium.org/1609763002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:7: };
I was really surprised that we didn't catch this typo in our first CL.

[Justin Novosad, 2016-01-20 16:41:30]

We are getting ahead of the spec here. I would like us to first converge on the
feature proposal.

[xidachen, 2016-02-05 14:24:27]

Description was changed from

==========
Implement ImageBitmap options premultiplyAlpha=false

In this CL, we implement the option of premultiplyAlpha=false for ImageBitmap.
At this moment, it works for createImageBitmap from HTMLImageElement,
HTMLVideoElement and HTMLCanvasElement. The cases of from Blob and ImageData
will be solved in another one or two CLs.

This CL also includes two layout tests. The layout tests are very similar
to the existing ones, but with an extra parameter of premultiplyAlpha=false.

BUG=249382
==========

to

==========
Implement ImageBitmap options premultiplyAlpha

In this CL, we implement the option of premultiplyAlpha for ImageBitmap.
At this moment, it works for createImageBitmap from HTMLImageElement,
HTMLCanvasElement and Blob. The cases of from ImageBitmap, HTMLVideoElemtn
and ImageData will be solved in future CL(s).

This CL also includes modifies a current layout tests, which tests
the combination of ImageBitmap options "flipY" and premultiplyAlpha

BUG=249382
==========

[xidachen, 2016-02-05 14:26:42]

PTAL

[Justin Novosad, 2016-02-05 14:46:41]

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-with-options.html
(right):

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-drawImage-with-options.html:165:
imageOrientationOptions = ["none", "flipY"];
You should try passing invalid values like "bogus".

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

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:24:
imageOrientationFlipYFlag = false;
ASSERT(options.imageOrientation() == imageBitmapOptionNone);

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:28:
premultiplyAlphaEnabledFlag = true;
ASSERT(options.premultiplyAlpha() == premultiplyAlphaDefault);

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:45: &&
frameBitmap.colorType() == kN32_SkColorType;
Should some of these conditions be ASSERTs (i.e. the ones we don't expect to
ever occur)?

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right):

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:7: dictionary
ImageBitmapOptions {
I think this interface should be hidden behind the experimental flag. The enums
as well. Missed this in previous CL

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

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:456:
ImageBitmapOptions options;
Here, you are not setting any options, and you are doing this in several places.
I would suggest calling the variable 'defaultOptions' and make it const. Since
this is used in many places, you could also declare it a global scope. Same
thing in ImageBitmapTest.cpp

[xidachen, 2016-02-05 16:47:37]

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

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:24:
imageOrientationFlipYFlag = false;
On 2016/02/05 14:46:41, Justin Novosad wrote:
> ASSERT(options.imageOrientation() == imageBitmapOptionNone);

Acknowledged.

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:28:
premultiplyAlphaEnabledFlag = true;
On 2016/02/05 14:46:41, Justin Novosad wrote:
> ASSERT(options.premultiplyAlpha() == premultiplyAlphaDefault);

Acknowledged.

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:45: &&
frameBitmap.colorType() == kN32_SkColorType;
On 2016/02/05 14:46:41, Justin Novosad wrote:
> Should some of these conditions be ASSERTs (i.e. the ones we don't expect to
> ever occur)?

Acknowledged.

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl (right):

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapOptions.idl:7: dictionary
ImageBitmapOptions {
On 2016/02/05 14:46:41, Justin Novosad wrote:
> I think this interface should be hidden behind the experimental flag. The
enums
> as well. Missed this in previous CL

Acknowledged.

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

https://codereview.chromium.org/1609763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas2d/CanvasRenderingContext2DTest.cpp:456:
ImageBitmapOptions options;
On 2016/02/05 14:46:41, Justin Novosad wrote:
> Here, you are not setting any options, and you are doing this in several
places.
> I would suggest calling the variable 'defaultOptions' and make it const. Since
> this is used in many places, you could also declare it a global scope. Same
> thing in ImageBitmapTest.cpp

I tried to define this local variable as a global variable, it crash chrome
under component build...

[Justin Novosad, 2016-02-05 22:33:01]

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

https://codereview.chromium.org/1609763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:25:
ASSERT(options.imageOrientation() == imageBitmapOptionNone);
The assert goes in the 'else'. You need to use braces. This will assert in a
debug build.

https://codereview.chromium.org/1609763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:30:
ASSERT(options.premultiplyAlpha() == "default");
same here

[xidachen, 2016-02-06 01:18:41]

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

https://codereview.chromium.org/1609763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:25:
ASSERT(options.imageOrientation() == imageBitmapOptionNone);
On 2016/02/05 22:33:01, Justin Novosad wrote:
> The assert goes in the 'else'. You need to use braces. This will assert in a
> debug build.

Acknowledged.

https://codereview.chromium.org/1609763002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:30:
ASSERT(options.premultiplyAlpha() == "default");
On 2016/02/05 22:33:01, Justin Novosad wrote:
> same here

Acknowledged.

[Justin Novosad, 2016-02-08 14:54:06]

On 2016/02/06 01:18:41, xidachen wrote:
>
https://codereview.chromium.org/1609763002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):
> 
>
https://codereview.chromium.org/1609763002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/ImageBitmap.cpp:25:
> ASSERT(options.imageOrientation() == imageBitmapOptionNone);
> On 2016/02/05 22:33:01, Justin Novosad wrote:
> > The assert goes in the 'else'. You need to use braces. This will assert in a
> > debug build.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1609763002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/frame/ImageBitmap.cpp:30:
> ASSERT(options.premultiplyAlpha() == "default");
> On 2016/02/05 22:33:01, Justin Novosad wrote:
> > same here
> 
> Acknowledged.

lgtm

[xidachen, 2016-02-08 14:57:57]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-02-08 14:58:27]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-02-08 14:58:37]

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

[commit-bot: I haz the power, 2016-02-08 14:59:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609763002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609763002/80001

[xidachen, 2016-02-08 16:21:45]

The CQ bit was unchecked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-02-08 16:22:27]

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

[xidachen, 2016-02-08 16:22:30]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-02-08 16:24:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1609763002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1609763002/80001

[commit-bot: I haz the power, 2016-02-08 16:29:54]

Description was changed from

==========
Implement ImageBitmap options premultiplyAlpha

In this CL, we implement the option of premultiplyAlpha for ImageBitmap.
At this moment, it works for createImageBitmap from HTMLImageElement,
HTMLCanvasElement and Blob. The cases of from ImageBitmap, HTMLVideoElemtn
and ImageData will be solved in future CL(s).

This CL also includes modifies a current layout tests, which tests
the combination of ImageBitmap options "flipY" and premultiplyAlpha

BUG=249382
==========

to

==========
Implement ImageBitmap options premultiplyAlpha

In this CL, we implement the option of premultiplyAlpha for ImageBitmap.
At this moment, it works for createImageBitmap from HTMLImageElement,
HTMLCanvasElement and Blob. The cases of from ImageBitmap, HTMLVideoElemtn
and ImageData will be solved in future CL(s).

This CL also includes modifies a current layout tests, which tests
the combination of ImageBitmap options "flipY" and premultiplyAlpha

BUG=249382
==========

[commit-bot: I haz the power, 2016-02-08 16:29:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-08 16:30:53]

Description was changed from

==========
Implement ImageBitmap options premultiplyAlpha

In this CL, we implement the option of premultiplyAlpha for ImageBitmap.
At this moment, it works for createImageBitmap from HTMLImageElement,
HTMLCanvasElement and Blob. The cases of from ImageBitmap, HTMLVideoElemtn
and ImageData will be solved in future CL(s).

This CL also includes modifies a current layout tests, which tests
the combination of ImageBitmap options "flipY" and premultiplyAlpha

BUG=249382
==========

to

==========
Implement ImageBitmap options premultiplyAlpha

In this CL, we implement the option of premultiplyAlpha for ImageBitmap.
At this moment, it works for createImageBitmap from HTMLImageElement,
HTMLCanvasElement and Blob. The cases of from ImageBitmap, HTMLVideoElemtn
and ImageData will be solved in future CL(s).

This CL also includes modifies a current layout tests, which tests
the combination of ImageBitmap options "flipY" and premultiplyAlpha

BUG=249382

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

[commit-bot: I haz the power, 2016-02-08 16:30:53]

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