Use union type in ImageBitmapFactories.idl

Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

In this CL, the CanvasRenderingContext2D is excluded in the ImageBitmapSource.

BUG=555958
Committed: https://crrev.com/423cdccfe8d03c41c94697d64bcc34078ec65b33
Cr-Commit-Position: refs/heads/master@{#362048}

[xidachen, 2015-11-24 13:22:35]

Description was changed from

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

BUG=555958
==========

to

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

BUG=555958
==========

[xidachen, 2015-11-24 13:22:35]

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

[xidachen, 2015-11-24 13:24:36]

Adding junov@ who is owner of Source/core/ and haraken@ of Source/modules/.

Please review this CL. Thank you.

junov@: I don't see the need of "ImageBitmapModuleTest.cpp" under modules/ to
exist anymore. Please correct me if I am wrong and I will add it back.

[Justin Novosad, 2015-11-24 17:03:13]

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-colorClamping.html
(left):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-colorClamping.html:22:
createImageBitmap(aCtx, 100, 100, 100, 100).then(function (imageBitmap) {
The fact that we are excluding CRC2D should be documented in the CL description

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:638: LayoutSize lSize =
image->imageSizeForLayoutObject(layoutObject(), 1.0f);
This should not depend on layout. We want access to the raw image resource, not
the version that is resized for layout. An image element that is not attached to
the DOM could be used as a source, in which case layout would not even be
calculated (layoutObject() returns nullptr)

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:99: if
(bitmapSource->isHTMLImageElement()) {
This would be a good place to use polymorphism. Something like
bitmapSource->createImageBitmap(...)  And you could get rid of  is* methods on
ImageBitmapSource

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:160:
exceptionState.throwSecurityError("The source image contains image data from
multiple origins.");
The security model is wrong here. Please file a bug so we can follow-up on this
in a later patch. ImageBitmap creation with cross-origin content is meant to
succeed, but it should taint the image bitmap, by setting its origin clean flag
to false. And the tainting should propagate to an eventual canvas to with the
imageBitmap would be drawn.

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:174:
exceptionState.throwSecurityError("The source video contains image data from
multiple origins.");
Same here

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.idl
(right):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.idl:45:
[CallWith=ScriptState, RaisesException] Promise
createImageBitmap(ImageBitmapSource imageBitmap);
Nice!

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapModuleTest.cpp
(left):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapModuleTest.cpp:39:
TEST_F(ImageBitmapModuleTest, ImageResourceLifetime)
This test need to be re-located

[xidachen, 2015-11-24 18:01:47]

Description was changed from

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

BUG=555958
==========

to

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

In this CL, the CanvasRenderingContext2D is excluded in the ImageBitmapSource.

BUG=555958
==========

[davve, 2015-11-24 19:51:30]

davve@opera.com changed reviewers:
+ davve@opera.com

[davve, 2015-11-24 19:51:30]

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:638: LayoutSize lSize =
image->imageSizeForLayoutObject(layoutObject(), 1.0f);
On 2015/11/24 17:03:13, Justin Novosad wrote:
> This should not depend on layout. We want access to the raw image resource,
not
> the version that is resized for layout. An image element that is not attached
to
> the DOM could be used as a source, in which case layout would not even be
> calculated (layoutObject() returns nullptr)

ImageResource::imageSizeForLayoutObject() doesn't return anything layout related
(it used to do special stuff for SVG, but not anymore). It only uses
layoutObject() for LayoutObject::shouldRespectImageOrientation(). So it depends
if we want the image orientation to be respected here, otherwise we might as
well go to the raw image resource, as you say. Since multiplier is 1,
ImageResource::imageSizeForLayoutObject() doesn't add much more than the
orientation stuff.

(See https://codereview.chromium.org/1468023002/ in an attempt to make this less
confusing and perhaps review :)

[Justin Novosad, 2015-11-24 20:14:58]

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:638: LayoutSize lSize =
image->imageSizeForLayoutObject(layoutObject(), 1.0f);
On 2015/11/24 19:51:30, David Vest wrote:
> On 2015/11/24 17:03:13, Justin Novosad wrote:
> > This should not depend on layout. We want access to the raw image resource,
> not
> > the version that is resized for layout. An image element that is not
attached
> to
> > the DOM could be used as a source, in which case layout would not even be
> > calculated (layoutObject() returns nullptr)
> 
> ImageResource::imageSizeForLayoutObject() doesn't return anything layout
related
> (it used to do special stuff for SVG, but not anymore). It only uses
> layoutObject() for LayoutObject::shouldRespectImageOrientation(). So it
depends
> if we want the image orientation to be respected here, otherwise we might as
> well go to the raw image resource, as you say. Since multiplier is 1,
> ImageResource::imageSizeForLayoutObject() doesn't add much more than the
> orientation stuff.
> 
> (See https://codereview.chromium.org/1468023002/ in an attempt to make this
less
> confusing and perhaps review :)

Ah, that CL is a great readability improvement.  Xida, can you wait for that to
land and use the new imageSize() method? Also, the literal 1.0f is not readable,
please use a constant with a descriptive name.

[xidachen, 2015-11-24 20:19:27]

No problem :). I will work on other issues of this CL in the meantime.

[xidachen, 2015-11-25 17:01:54]

PTAL.

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-colorClamping.html
(left):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/canvas-createImageBitmap-colorClamping.html:22:
createImageBitmap(aCtx, 100, 100, 100, 100).then(function (imageBitmap) {
On 2015/11/24 17:03:13, Justin Novosad wrote:
> The fact that we are excluding CRC2D should be documented in the CL
description

Done.

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:638: LayoutSize lSize =
image->imageSizeForLayoutObject(layoutObject(), 1.0f);
On 2015/11/24 17:03:13, Justin Novosad wrote:
> This should not depend on layout. We want access to the raw image resource,
not
> the version that is resized for layout. An image element that is not attached
to
> the DOM could be used as a source, in which case layout would not even be
> calculated (layoutObject() returns nullptr)

Done.

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
(right):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:99: if
(bitmapSource->isHTMLImageElement()) {
On 2015/11/24 17:03:13, Justin Novosad wrote:
> This would be a good place to use polymorphism. Something like
> bitmapSource->createImageBitmap(...)  And you could get rid of  is* methods on
> ImageBitmapSource

This has been done, but with exception of the case  when it is Blob.

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp:160:
exceptionState.throwSecurityError("The source image contains image data from
multiple origins.");
On 2015/11/24 17:03:13, Justin Novosad wrote:
> The security model is wrong here. Please file a bug so we can follow-up on
this
> in a later patch. ImageBitmap creation with cross-origin content is meant to
> succeed, but it should taint the image bitmap, by setting its origin clean
flag
> to false. And the tainting should propagate to an eventual canvas to with the
> imageBitmap would be drawn.

Could you give me the link to the specs so that I can file a bug?
What I saw here: https://html.spec.whatwg.org/#imagebitmapfactories, says:
If the origin of the img element's image is not the same origin as the origin
specified by the entry settings object, then return a promise rejected with a
SecurityError exception and abort these steps.

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/imagebitmap/ImageBitmapModuleTest.cpp
(left):

https://codereview.chromium.org/1455763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/imagebitmap/ImageBitmapModuleTest.cpp:39:
TEST_F(ImageBitmapModuleTest, ImageResourceLifetime)
On 2015/11/24 17:03:13, Justin Novosad wrote:
> This test need to be re-located

I move this test to be under CanvasRenderingContext2DTest, hope that is fine.

https://codereview.chromium.org/1455763002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/1455763002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.h:120: IntSize
bitmapSourceSize() override { return IntSize(width(), height()); }
This implementation of bitmapSourceSize will be changed once the imageSize() CL
is landed. Because my implementation here makes its return value non-const.

[Justin Novosad, 2015-11-25 19:09:46]

> On 2015/11/24 17:03:13, Justin Novosad wrote:
> > This test need to be re-located
> 
> I move this test to be under CanvasRenderingContext2DTest, hope that is fine.

Yes, since it has a dependency on CRC2D it has to stay in modules.

[davve, 2015-11-27 11:19:43]

https://codereview.chromium.org/1455763002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapSource.h (right):

https://codereview.chromium.org/1455763002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapSource.h:22: virtual
PassRefPtrWillBeRawPtr<ImageBitmap> createImageBitmap(EventTarget&,
ImageBitmapSource*, int sx, int sy, int sw, int sh, ExceptionState&) { return
nullptr; }
Drive-by review: Why do you pass ImageBitmapSource* here? Won't the this pointer
do? The static_cast inside the non-static createImageBitmap implementations
looks a bit off.

https://codereview.chromium.org/1455763002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1455763002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:812: LayoutSize lSize =
image->imageSize(LayoutObject::shouldRespectImageOrientation(layoutObject()),
1.0f);
Since multiplier is 1 and last argument to imageSize is the default
(IntrinsicSize) the returned size should never be non-integer. Something we may
want to add asserts for here. Makes the truncation on the next line more
obvious. WDYT?

[xidachen, 2015-11-27 19:00:25]

https://codereview.chromium.org/1455763002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/imagebitmap/ImageBitmapSource.h (right):

https://codereview.chromium.org/1455763002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/imagebitmap/ImageBitmapSource.h:22: virtual
PassRefPtrWillBeRawPtr<ImageBitmap> createImageBitmap(EventTarget&,
ImageBitmapSource*, int sx, int sy, int sw, int sh, ExceptionState&) { return
nullptr; }
On 2015/11/27 11:19:43, David Vest wrote:
> Drive-by review: Why do you pass ImageBitmapSource* here? Won't the this
pointer
> do? The static_cast inside the non-static createImageBitmap implementations
> looks a bit off.

Great comments! I remove this parameter and there should be no static_cast in
the derived class now. Let me know when you spot one. Thanks.

https://codereview.chromium.org/1455763002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/1455763002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLImageElement.cpp:812: LayoutSize lSize =
image->imageSize(LayoutObject::shouldRespectImageOrientation(layoutObject()),
1.0f);
On 2015/11/27 11:19:43, David Vest wrote:
> Since multiplier is 1 and last argument to imageSize is the default
> (IntrinsicSize) the returned size should never be non-integer. Something we
may
> want to add asserts for here. Makes the truncation on the next line more
> obvious. WDYT?

Done.

[Justin Novosad, 2015-11-27 20:23:40]

lgtm

[xidachen, 2015-11-27 20:25:08]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2015-11-27 20:25:25]

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

[commit-bot: I haz the power, 2015-11-27 20:33:42]

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

[commit-bot: I haz the power, 2015-11-27 20:33:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xidachen, 2015-11-27 20:42:31]

On 2015/11/27 20:33:43, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

haraken@: Could you give me an LGTM under Source/modules/, I deleted several
files under the dir of modules/imagebitmap/, and the code won't pass presubmit
without an LGTM from an owner of modules/. Thank you.

[haraken, 2015-11-28 00:50:34]

On 2015/11/27 20:42:31, xidachen wrote:
> On 2015/11/27 20:33:43, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> haraken@: Could you give me an LGTM under Source/modules/, I deleted several
> files under the dir of modules/imagebitmap/, and the code won't pass presubmit
> without an LGTM from an owner of modules/. Thank you.

modules/ LGTM

[xidachen, 2015-11-28 01:16:38]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2015-11-28 01:16:59]

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

[commit-bot: I haz the power, 2015-11-28 03:34:00]

Description was changed from

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

In this CL, the CanvasRenderingContext2D is excluded in the ImageBitmapSource.

BUG=555958
==========

to

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

In this CL, the CanvasRenderingContext2D is excluded in the ImageBitmapSource.

BUG=555958
==========

[commit-bot: I haz the power, 2015-11-28 03:34:01]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-11-28 03:35:11]

Description was changed from

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

In this CL, the CanvasRenderingContext2D is excluded in the ImageBitmapSource.

BUG=555958
==========

to

==========
Use union type in ImageBitmapFactories.idl

The ImageBitmapFactories.idl should use typedef for its image source,
as specified in: https://html.spec.whatwg.org/#imagebitmapfactories.

In this CL, the CanvasRenderingContext2D is excluded in the ImageBitmapSource.

BUG=555958

Committed: https://crrev.com/423cdccfe8d03c41c94697d64bcc34078ec65b33
Cr-Commit-Position: refs/heads/master@{#362048}
==========

[commit-bot: I haz the power, 2015-11-28 03:35:12]

Patchset 13 (id:??) landed as
https://crrev.com/423cdccfe8d03c41c94697d64bcc34078ec65b33
Cr-Commit-Position: refs/heads/master@{#362048}

[Yuki, 2015-12-01 09:14:50]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2015-12-01 09:14:51]

xidachen@, junov@ and haraken@,

Is it really acceptable that we no longer support
createImageBitmap(CanvasRenderingContext2D)?  Isn't it a regression?

IIUC, it's not observable from user JavaScript whether we're using a union type
in the IDLs or not, while createImageBitmap(CanvasRenderingContext2D) is
observable from user JS.

Is this CL worth causing the regression?

[Justin Novosad, 2015-12-02 21:33:13]

On 2015/12/01 09:14:51, Yuki wrote:
> xidachen@, junov@ and haraken@,
> 
> Is it really acceptable that we no longer support
> createImageBitmap(CanvasRenderingContext2D)?  Isn't it a regression?

It is not a regression in Chrome, since this API has not yet been shipped.

> 
> IIUC, it's not observable from user JavaScript whether we're using a union
type
> in the IDLs or not, while createImageBitmap(CanvasRenderingContext2D) is
> observable from user JS.

I intend to propose removing createImageBitmap(CanvasRenderingContext2D) from
the spec. There is no reason for this API to exist: Since CanvasProxy is being
abandoned in favor of OffscreenCanvas, the notion of having a rendering context
that is not attached to a canvas is going away. Therefore it will always be
possible to create an image bitmap from a 2D context by going through the
HTMLCanvasElement or OffscreenCanvas object that the context is bound to.
Example: createImageBitmap(myCanvasRenderingContext2D.canvas)

[Yuki, 2015-12-03 04:19:01]

On 2015/12/02 21:33:13, Justin Novosad wrote:
> On 2015/12/01 09:14:51, Yuki wrote:
> > xidachen@, junov@ and haraken@,
> > 
> > Is it really acceptable that we no longer support
> > createImageBitmap(CanvasRenderingContext2D)?  Isn't it a regression?
> 
> It is not a regression in Chrome, since this API has not yet been shipped.
> 
> > 
> > IIUC, it's not observable from user JavaScript whether we're using a union
> type
> > in the IDLs or not, while createImageBitmap(CanvasRenderingContext2D) is
> > observable from user JS.
> 
> I intend to propose removing createImageBitmap(CanvasRenderingContext2D) from
> the spec. There is no reason for this API to exist: Since CanvasProxy is being
> abandoned in favor of OffscreenCanvas, the notion of having a rendering
context
> that is not attached to a canvas is going away. Therefore it will always be
> possible to create an image bitmap from a 2D context by going through the
> HTMLCanvasElement or OffscreenCanvas object that the context is bound to.
> Example: createImageBitmap(myCanvasRenderingContext2D.canvas)

Thanks for the clarification.  Got it.