Expose ImageBitmap to (Window,Worker)

This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

The thread of Intent to ship ImageBitmap can be found here:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jRzvehX9U1U/discussion

BUG=563984
Committed: https://crrev.com/0b95bd78dc3ac8936f7dec1dce92ec1118d04f95
Cr-Commit-Position: refs/heads/master@{#370179}

[xidachen, 2015-12-01 02:19:05]

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

[xidachen, 2015-12-01 02:19:05]

PTAL

https://codereview.chromium.org/1486663002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/canvas/resources/canvas-createImageBitmap-data-in-workers.js
(right):

https://codereview.chromium.org/1486663002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/canvas/resources/canvas-createImageBitmap-data-in-workers.js:16:
shouldBe("bitmapHeight", "50");
This will test that getting the width/height of ImageBitmap works in worker.
The case of getting these two attributes in window is already covered under
canvas-ImageBitmap-transferable.html

[Justin Novosad, 2015-12-01 04:39:40]

You should have a crbug for this.

[Justin Novosad, 2015-12-01 04:51:58]

On 2015/12/01 04:39:40, Justin Novosad wrote:
> You should have a crbug for this.

The description Of this CL is more fitting for an issue description. CL
descriptions should describe the code change.  
lgtm with CL description fixed, and a BUG= line

[xidachen, 2015-12-01 13:18:43]

Description was changed from

==========
Expose ImageBitmap to (Window,Worker)

The spec: https://html.spec.whatwg.org/#images has [Exposed=(Window,Worker)]
for ImageBitmap, but this line is currently commented out in our
implementation.
==========

to

==========
This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

BUG=563984
==========

[xidachen, 2015-12-01 13:19:04]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2015-12-01 13:19:48]

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

[xidachen, 2015-12-01 13:27:51]

The CQ bit was unchecked by xidachen@chromium.org

[commit-bot: I haz the power, 2015-12-01 13:27:57]

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

[commit-bot: I haz the power, 2015-12-01 13:27:58]

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-12-01 14:53:34]

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

[xidachen, 2015-12-01 14:54:38]

rbyers@: I changed several expected.txt for layout tests in this CL and requires
lgtm from an API_OWNER. PTAL. Thank you.

[Rick Byers, 2015-12-01 16:12:27]

On 2015/12/01 14:54:38, xidachen wrote:
> rbyers@: I changed several expected.txt for layout tests in this CL and
requires
> lgtm from an API_OWNER. PTAL. Thank you.

Technically this is exposing a new API to the web, and so is subject to the
launch process: http://www.chromium.org/blink#launch-process

I'm trying to figure out if this might qualify as "trivial".  createImageBitmap
is already exposed on workers, right?  So it's not like you couldn't use
ImageBitmap on workers, you just didn't have the interface object.

Justin / Xida: in what way do you think sites could be affected by this change? 
Eg. is it common to test for this interface as a form of feature detection or
something?

[xidachen, 2015-12-01 19:38:49]

On 2015/12/01 16:12:27, Rick Byers wrote:
> On 2015/12/01 14:54:38, xidachen wrote:
> > rbyers@: I changed several expected.txt for layout tests in this CL and
> requires
> > lgtm from an API_OWNER. PTAL. Thank you.
> 
> Technically this is exposing a new API to the web, and so is subject to the
> launch process: http://www.chromium.org/blink#launch-process
> 
> I'm trying to figure out if this might qualify as "trivial". 
createImageBitmap
> is already exposed on workers, right?  So it's not like you couldn't use
> ImageBitmap on workers, you just didn't have the interface object.
> 
> Justin / Xida: in what way do you think sites could be affected by this
change? 
> Eg. is it common to test for this interface as a form of feature detection or
> something?

Hi Rick, this feature is actually a dependency of the new ImageBitmap API
specified here: https://code.google.com/p/chromium/issues/detail?id=249384
Per discussion with junov@, we want this feature (ImageBitmap width&height
expose to worker) to be available before we ship the ImageBitmap API. Hope that
makes sense.

[Rick Byers, 2015-12-01 19:50:22]

On 2015/12/01 19:38:49, xidachen wrote:
> On 2015/12/01 16:12:27, Rick Byers wrote:
> > On 2015/12/01 14:54:38, xidachen wrote:
> > > rbyers@: I changed several expected.txt for layout tests in this CL and
> > requires
> > > lgtm from an API_OWNER. PTAL. Thank you.
> > 
> > Technically this is exposing a new API to the web, and so is subject to the
> > launch process: http://www.chromium.org/blink#launch-process
> > 
> > I'm trying to figure out if this might qualify as "trivial". 
> createImageBitmap
> > is already exposed on workers, right?  So it's not like you couldn't use
> > ImageBitmap on workers, you just didn't have the interface object.
> > 
> > Justin / Xida: in what way do you think sites could be affected by this
> change? 
> > Eg. is it common to test for this interface as a form of feature detection
or
> > something?
> 
> Hi Rick, this feature is actually a dependency of the new ImageBitmap API
> specified here: https://code.google.com/p/chromium/issues/detail?id=249384
> Per discussion with junov@, we want this feature (ImageBitmap width&height
> expose to worker) to be available before we ship the ImageBitmap API. Hope
that
> makes sense.

Ah, I missed that createImageBitmap was behind a
RuntimeEnabled=ExperimentalCanvasFeatures.  Is there a reason why the API to get
an ImageBitmap is behind this flag, but the ImageBitmap class itself isn't? 
I.e. is there some other API for getting an ImageBitmap (without enabling
ExperimentalCanvasFeatures)?

[xidachen, 2015-12-02 02:54:04]

> Ah, I missed that createImageBitmap was behind a
> RuntimeEnabled=ExperimentalCanvasFeatures.  Is there a reason why the API to
get
> an ImageBitmap is behind this flag, but the ImageBitmap class itself isn't? 
> I.e. is there some other API for getting an ImageBitmap (without enabling
> ExperimentalCanvasFeatures)?

Hi Rick, now that I made the ImageBitmap behind the
RuntimeEnabled=ExperimentalCanvasFeatures, it actually takes few lines out from
the virtual/stable/webexposed/global-interface-listing-expected.txt. Please take
a look. Thank you.

[Rick Byers, 2015-12-02 03:57:36]

On 2015/12/02 02:54:04, xidachen wrote:
> > Ah, I missed that createImageBitmap was behind a
> > RuntimeEnabled=ExperimentalCanvasFeatures.  Is there a reason why the API to
> get
> > an ImageBitmap is behind this flag, but the ImageBitmap class itself isn't? 
> > I.e. is there some other API for getting an ImageBitmap (without enabling
> > ExperimentalCanvasFeatures)?
> 
> Hi Rick, now that I made the ImageBitmap behind the
> RuntimeEnabled=ExperimentalCanvasFeatures, it actually takes few lines out
from
> the virtual/stable/webexposed/global-interface-listing-expected.txt. Please
take
> a look. Thank you.

Ok, so it was shipped only by accident then with no practical use, right?

But unshipping it poses a risk this will break sites that somehow expect 
ImageBitmap to exist!  The risk is probably small but we can't remove anything
without measuring the risk somehow and doing an intent-to-remove thread (you
should read http://www.chromium.org/blink#new-features if you haven't before).

I think you could go either way here.  Unshipping if possible is technically
better but more work.  It looks like there hasn't been an intent-to-implement
thread for these APIs yet, right?  I suggest you do that and mention that
ImageBitmap was previously shipped accidentally and you plan now to extend it to
workers too - a trivial change, still not useful without the
experimentalCanvasFeatures flag enabled.  If no-one objects then I'm happy
approving the original CL.  WDYT?

[xidachen, 2015-12-02 13:40:21]

> Ok, so it was shipped only by accident then with no practical use, right?
> 
> But unshipping it poses a risk this will break sites that somehow expect 
> ImageBitmap to exist!  The risk is probably small but we can't remove anything
> without measuring the risk somehow and doing an intent-to-remove thread (you
> should read http://www.chromium.org/blink#new-features if you haven't before).
> 
> I think you could go either way here.  Unshipping if possible is technically
> better but more work.  It looks like there hasn't been an intent-to-implement
> thread for these APIs yet, right?  I suggest you do that and mention that
> ImageBitmap was previously shipped accidentally and you plan now to extend it
to
> workers too - a trivial change, still not useful without the
> experimentalCanvasFeatures flag enabled.  If no-one objects then I'm happy
> approving the original CL.  WDYT?

Hmm...This is a bit more tricky than I thought. So what do you think of this
solution: the new APIs (including ImageBitmap, createImageBitmap, ImageBitmap
supported in WebGL) is scheduled to ship at the end of this quarter and we are
on the right track. So it is very likely that I will send out an "Intend to
ship" email few weeks from now. How about we leave this CL as what it is now,
and come back after my "Intend to ship" email gets approved. Once the "Intend to
ship" email is approved, then we will remove the experimental flag. Does that
make sense?

[Rick Byers, 2015-12-02 14:31:52]

On 2015/12/02 13:40:21, xidachen wrote:
> > Ok, so it was shipped only by accident then with no practical use, right?
> > 
> > But unshipping it poses a risk this will break sites that somehow expect 
> > ImageBitmap to exist!  The risk is probably small but we can't remove
anything
> > without measuring the risk somehow and doing an intent-to-remove thread (you
> > should read http://www.chromium.org/blink#new-features if you haven't
before).
> > 
> > I think you could go either way here.  Unshipping if possible is technically
> > better but more work.  It looks like there hasn't been an
intent-to-implement
> > thread for these APIs yet, right? 

I found the old intent to implement thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/4xQWqlTfmLI. 
Apparently it had expired from my gmail so I didn't find it there, sorry!

> > I suggest you do that and mention that
> > ImageBitmap was previously shipped accidentally and you plan now to extend
it
> to
> > workers too - a trivial change, still not useful without the
> > experimentalCanvasFeatures flag enabled.  If no-one objects then I'm happy
> > approving the original CL.  WDYT?
> 
> Hmm...This is a bit more tricky than I thought. So what do you think of this
> solution: the new APIs (including ImageBitmap, createImageBitmap, ImageBitmap
> supported in WebGL) is scheduled to ship at the end of this quarter and we are
> on the right track. So it is very likely that I will send out an "Intend to
> ship" email few weeks from now. How about we leave this CL as what it is now,
> and come back after my "Intend to ship" email gets approved. Once the "Intend
to
> ship" email is approved, then we will remove the experimental flag. Does that
> make sense?

Sure, if there's no real benefit to landing this CL before shipping the rest
then I agree doing that at the same time is the simplest solution.  If, however,
you end up deciding that there's some benefit (eg. for better testing) to
landing this CL first then I'm OK calling the additional exposure "trivial" and
just landing ps#3.  I don't want to slow you down with process.  Mainly I just
wanted to make sure I understood the intent and risk here...

[xidachen, 2016-01-19 16:35:28]

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

[commit-bot: I haz the power, 2016-01-19 16:36:23]

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

[xidachen, 2016-01-19 18:37:28]

Hi Rick,

Now that I got 3 LGTMs on ImageBitmap, let's land this CL first.

I updated the expected output for some layout tests. The trybots here seems to
be fine.

[Justin Novosad, 2016-01-19 18:58:30]

Sorry I did not chime in earlier. I can answer Rick's questions.

Due to a limitation in the IDL parser, we "shipped" ImageBitmap though all the
methods for creating ImageBitmaps were not shipped, which means ImageBitmap
remained unshipped in practice (phantome feature?).  This was 2.5 years ago.  If
I recall correctly the issue had to do with CanvasImageSource inheritance and
the fact that drawImage uses the same entry point for ImageBitmap as it does for
other CanvasImageSource types, and there was no way to put RuntimeEnabled on an
"or-member" of a union typedef in our IDL implementation.

The current CL is a prerequisite for shipping IMHO because without this CL, the
feature is in a weird inconsistent state where it is possible to transfer or
post an ImageBitmap to a Worker, it is event possible to create an ImageBitmap
on a worker (Because WorkerGlobalScope implements ImageBitmapFactories), but we
can't access .width and .height attribute from a Worker.

One of the important use cases for this feature is for apps to implement
resource loader functionality in a Worker, so having ImageBitmap fully
functional in workers is important.

[Rick Byers, 2016-01-19 19:04:26]

On 2016/01/19 18:58:30, Justin Novosad wrote:
> Sorry I did not chime in earlier. I can answer Rick's questions.
> 
> Due to a limitation in the IDL parser, we "shipped" ImageBitmap though all the
> methods for creating ImageBitmaps were not shipped, which means ImageBitmap
> remained unshipped in practice (phantome feature?).  This was 2.5 years ago. 
If
> I recall correctly the issue had to do with CanvasImageSource inheritance and
> the fact that drawImage uses the same entry point for ImageBitmap as it does
for
> other CanvasImageSource types, and there was no way to put RuntimeEnabled on
an
> "or-member" of a union typedef in our IDL implementation.
> 
> The current CL is a prerequisite for shipping IMHO because without this CL,
the
> feature is in a weird inconsistent state where it is possible to transfer or
> post an ImageBitmap to a Worker, it is event possible to create an ImageBitmap
> on a worker (Because WorkerGlobalScope implements ImageBitmapFactories), but
we
> can't access .width and .height attribute from a Worker.
> 
> One of the important use cases for this feature is for apps to implement
> resource loader functionality in a Worker, so having ImageBitmap fully
> functional in workers is important.

I see, thanks for the history.  Now that you've passed an intent-to-ship, this
is fine to land: LGTM.
Please add a link to the intent-to-ship thread in the CL description.

[xidachen, 2016-01-19 19:11:05]

Description was changed from

==========
This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

BUG=563984
==========

to

==========
This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

The thread of Intent to ship ImageBitmap can be found here:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jRzvehX9U1U/discus...

BUG=563984
==========

[xidachen, 2016-01-19 19:11:38]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2016-01-19 19:11:39]

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/1486663002/#ps120001
(title: "More on updating layout test results")

[commit-bot: I haz the power, 2016-01-19 19:12:29]

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

[commit-bot: I haz the power, 2016-01-19 20:25:08]

Description was changed from

==========
This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

The thread of Intent to ship ImageBitmap can be found here:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jRzvehX9U1U/discus...

BUG=563984
==========

to

==========
This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

The thread of Intent to ship ImageBitmap can be found here:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jRzvehX9U1U/discus...

BUG=563984
==========

[commit-bot: I haz the power, 2016-01-19 20:25:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-19 20:26:54]

Description was changed from

==========
This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

The thread of Intent to ship ImageBitmap can be found here:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jRzvehX9U1U/discus...

BUG=563984
==========

to

==========
This CL put [Exposed=(Window,Worker)] in the ImageBitmap.idl

Along with that, some of the layout tests results has to be updated.

Also, a layout test has been updated to show that the worker is
able to get width&height for an ImageBitmap.

The thread of Intent to ship ImageBitmap can be found here:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/jRzvehX9U1U/discus...

BUG=563984

Committed: https://crrev.com/0b95bd78dc3ac8936f7dec1dce92ec1118d04f95
Cr-Commit-Position: refs/heads/master@{#370179}
==========

[commit-bot: I haz the power, 2016-01-19 20:26:55]

Patchset 7 (id:??) landed as
https://crrev.com/0b95bd78dc3ac8936f7dec1dce92ec1118d04f95
Cr-Commit-Position: refs/heads/master@{#370179}