1304767 - Rename transferImageBitmap to transferFromImageBitmap

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Simon Pieters [:zcorpan]]

In https://github.com/w3c/web-platform-tests/pull/3801 the following test fails:

ImageBitmapRenderingContext support for transferImageBitmap

It was renamed to transferFromImageBitmap in https://github.com/whatwg/html/commit/fcb0756dd94d96df9b8355741d82fcd5ca0a6154

Current spec: https://html.spec.whatwg.org/multipage/scripting.html#dom-imagebitmaprenderingcontext-transferfromimagebitmap

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: rename canvas transferImageBitmap to transferFromImageBitmap due to spec revision

This implements the renaming while keeping around the older alias as a deprecated version in case any end-users use it.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8794287 [details] [diff] [review]
rename canvas transferImageBitmap to transferFromImageBitmap due to spec revision

DOM people should probably review the interface change.

Comment 3

[(no longer active)]

Comment on attachment 8794287 [details] [diff] [review]
rename canvas transferImageBitmap to transferFromImageBitmap due to spec revision

Review of attachment 8794287 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/ImageBitmapRenderingContext.webidl
@@ +34,5 @@
>    // styles affecting the display of the canvas are applied as usual.
> +  void transferFromImageBitmap(ImageBitmap bitmap);
> +
> +  // deprecated due to renaming in the spec
> +  void transferImageBitmap(ImageBitmap bitmap); // now transferFromImageBitmap

This feature isn't shipped yet, right?  We don't need to keep the old method around.  The demos using this stuff need to change their code to match the current spec.

Comment 4

[Lee Salzman [:lsalzman]]

This API shipped in 46. So the cat is already out of the bag.

Comment 5

[(no longer active)]

(In reply to Lee Salzman [:lsalzman] from comment #4)
> This API shipped in 46. So the cat is already out of the bag.

Err, I thought that this context is only used for OffscreenCanvas, but it seems like calling getContext("bitmaprenderer") on a normal canvas happily returns a ImageBitmapRenderingContext, which means that someone decided to implement and ship this disregarding our exposure policy <https://wiki.mozilla.org/WebAPI/ExposureGuidelines>.  Given that this seems to have happened in bug 1172796, it's not even clear to me whether shipping this was intentional...  CCing Morris and smaug (the author and reviewer) to help us figure out why we ended up shipping this with the rest of the OffscreenCanvas support turned off...

If this shipping hasn't been done intentionally, please file a follow-up bug to fix the situation.  If it was indeed intentional, please at least send a post-facto intent email to dev-platform.

But back to the question of renaming the API.  We shouldn't keep the old name at any rate.  Either the Web already depends on this name, in which case we should revert the spec change and WONTFIX this, or it hasn't, in which case we should just rename our Web facing method.  I don't see any scenario where we'd actually want to keep both names around.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

So ImageBitmapRenderingContext is separate thing from OffscreenCanvas, but shouldn't perhaps have been enabled by default yet. But here we are.

zcorpan, I think we should undo the spec change here. Why was there a change anyhow?
transferImageBitmap makes more sense.

Comment 7

[Simon Pieters [:zcorpan]]

Rationale for the change is in https://github.com/whatwg/html/pull/1101

transferToImageBitmap is not in the HTML spec but is in https://wiki.whatwg.org/wiki/OffscreenCanvas and appears to exist in Chromium with "Experimental canvas features" enabled in chrome://flags/ .

I suggest we continue discussing the spec in https://github.com/whatwg/html/issues/1818

Comment 8

[(no longer active)]

OK, so https://github.com/whatwg/html/issues/1818 is closed now.  :/  Chrome has done this already: <https://chromium.googlesource.com/chromium/src/+/f42dfa82c14388c3dcd799b2a59e55b72f6336e0>.  We need to decide on something.  Here are the alternatives that I can think of:

a) Be brave, and just hide the old method name under a pref and ship that change.  If we end up breaking a site, ship a hotfix add-on to set the pref back on.  Remove the pref in a couple of releases.
b) Be conservative, and add a deprecation warning for the old method name, and also add a telemetry measuring its usage.  Once we're happy with the low usage numbers, remove the old method.

I would normally advocate for (b) but this is a new feature which seemingly only Gecko has shipped so far, so maybe we can get away with (a).  If we want to go down that route, maybe we should uplift the patch all the way to beta to ship it as soon as we can, and actually remove the old method on trunk.

Lee, how should we proceed here?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(b) is quite simple, and would be safer to push to aurora/beta too to get telemetry data from a bit larger population.

Comment 10

[Lee Salzman [:lsalzman]]

I would prefer to leave the decision to the team and author involved with this issue.

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I would prefer (b), too.

Comment 12

[(no longer active)]

Sounds great!

Comment 13

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

Deprecated ImageBitmapRenderingContext.transferImageBitmap and replaced
it with ImageBitmapRenderingContext.transferFromImageBitmap.

Review commit: https://reviewboard.mozilla.org/r/84202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84202/

Comment 14

[(no longer active)]

I'm starting to get confused as to what the grand plan is here.  I thought that we're going to unship in bug 1307628 first, and then just rename this method (since after unshipping, renaming it won't possibly break websites.)  I think this doesn't match the plan you have in mind, but I'd like to know what your plans are, so that we're on the same page.  Thanks!

Comment 15

[(no longer active)]

Just saw your dev-platform post, and the plan is outlines there, so please ignore the last comment!

Comment 16

[(no longer active)]

Comment on attachment 8798787 [details]
Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

https://reviewboard.mozilla.org/r/84202/#review82822

::: dom/locales/en-US/chrome/dom/dom.properties:255
(Diff revision 1)
>  # LOCALIZATION NOTE: Do not translate "ServiceWorker". %1$S is the ServiceWorker scope URL. %2$S is an error string.
>  PushMessageDecryptionFailure=The ServiceWorker for scope ‘%1$S’ encountered an error decrypting a push message: ‘%2$S’. For help with encryption, please see https://developer.mozilla.org/en-US/docs/Web/API/Push_API/Using_the_Push_API#Encryption
>  # LOCALIZATION NOTE: %1$S is the type of a DOM event. 'passive' is a literal parameter from the DOM spec.
>  PreventDefaultFromPassiveListenerWarning=Ignoring ‘preventDefault()’ call on event of type ‘%1$S’ from a listener registered as ‘passive’.
>  FileLastModifiedDateWarning=File.lastModifiedDate is deprecated. Use File.lastModified instead.
> +ImageBitmapRenderingContext_TransferImageBitmap=ImageBitmapRenderingContext.transferImageBitmap is deprecated. Use ImageBitmapRenderingContext.transferFromImageBitmap instead.

Please add a LOCALIZATION NOTE stating that 'ImageBitmapRenderingContext.transferImageBitmap' and 'ImageBitmapRenderingContext.transferFromImageBitmap' should not be translated, similar to that of PushMessageDecryptionFailure above.

Also, it would be nice to mention that this method is due for removal soon.

Comment 17

[(no longer active)]

Comment on attachment 8798787 [details]
Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

https://reviewboard.mozilla.org/r/84202/#review82824

Thanks for the patch!  r=me on this, but we also need to add telemetry for the usages of the now deprecated method.  Are you planning to do that in this bug?

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I think the DEPRECATED_OPERATION also record the user count information. See this http://searchfox.org/mozilla-central/source/dom/base/UseCounter.h#26

So I think the telemetry info is included in this patch. Is it correct?

Comment 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #18)
> I think the DEPRECATED_OPERATION also record the user count information. See
> this http://searchfox.org/mozilla-central/source/dom/base/UseCounter.h#26
> 
> So I think the telemetry info is included in this patch. Is it correct?

oops, /user count/use count/s

Comment 20

[(no longer active)]

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #18)
> I think the DEPRECATED_OPERATION also record the user count information. See
> this http://searchfox.org/mozilla-central/source/dom/base/UseCounter.h#26
> 
> So I think the telemetry info is included in this patch. Is it correct?

Yes, you're right.  Please ignore that comment.  :-)

Comment 21

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8798787 [details]
Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84202/diff/1-2/

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Try looks ok.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40074a01ad41
Deprecated ImageBitmapRenderingContext.transferImageBitmap. r=Ehsan

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40074a01ad41

Comment 25

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/offscreen-canvas-transferimagebitmap-has-been-renamed-imagebitmaprenderingcontext-may-be-disabled-soon/

Comment 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8798787 [details]
Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: User cannot use latest spec of ImageBitmapRenderingContext. The main reason for uplifting is we want more telemetry data for old method usage. So we want to uplift this patch to beta and aurora for collecting data on larger population.
[Describe test coverage new/current, TreeHerder]: Tested on local and try.
[Risks and why]: Low, only add a new function and add deprecated warning to old method. 
[String/UUID change made/needed]: N/A

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798787 [details]
Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

Deprecating an old method to be spec compliant, Aurora51+, Beta50+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Patches that add strings to dom.properties are definitely *not* "[String/UUID change made/needed]: N/A"

Comment 29

[Francesco Lodolo [:flod]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> Patches that add strings to dom.properties are definitely *not*
> "[String/UUID change made/needed]: N/A"

Exactly, and we're way too ahead in the cycle. If you want to uplift these, please create ad-hoc patches with the string hard-coded.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Francesco Lodolo [:flod] from comment #29)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> > Patches that add strings to dom.properties are definitely *not*
> > "[String/UUID change made/needed]: N/A"
> 
> Exactly, and we're way too ahead in the cycle. If you want to uplift these,
> please create ad-hoc patches with the string hard-coded.

Hi Morris, could you please address Flod's comment? Also, as RyanVM mentioned in comment 28, going forward please mention "yes" in the uplift requests if your patch has a string change.

Comment 31

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Francesco Lodolo [:flod] from comment #29)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> > Patches that add strings to dom.properties are definitely *not*
> > "[String/UUID change made/needed]: N/A"
> 
> Exactly, and we're way too ahead in the cycle. If you want to uplift these,
> please create ad-hoc patches with the string hard-coded.

The code that use this string is auto-generated by webidl code gen. So it's not easy to hard-coded string in the code gen. Do we have another way to do this? Thanks.

Comment 32

[Francesco Lodolo [:flod]]

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #31)
> The code that use this string is auto-generated by webidl code gen. So it's
> not easy to hard-coded string in the code gen. Do we have another way to do
> this? Thanks.

Not really. Here's what happens as soon as you land this:
* You trigger a run of compare-locales for all products and all locales, since this is a /dom string.
* That exposes one missing string for all products and all locales in dashboards and tools.
* This will invalidate all existing sign-offs (about 180), which means someone in l10n-drivers needs to go through each one of them to check if there's real activity, request and review all of them individually. That's a lot of hours and work, and can't be automated (on purpose).
* Localizers will be annoyed, because we just added strings to string frozen branches. Add to this that they'll have like 3 days to localize Beta before it's closed (Oct 19).

Multiply all this for 2 branches.

I don't know how complicated it would be to update the code to work with hard-coded strings, but if situations like this are likely to happen again, it's probably worth it.

Comment 33

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Ok! Thanks for the response. Ehsan, do you think it's ok to leave the patch in the nightly only and let it ride the train?

Comment 34

[(no longer active)]

I prefer if we uplifted the patch as soon as we can.

In order to be able to hardcode the string, please consider the following approach:

1. Remove the [Deprecated] annotation from the WebIDL.  That annotation essentially makes this function get called every time the method is involved: <http://searchfox.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#3420>

2. You need to roll your own version of nsIDocument::WarnOnceAbout() (perhaps by adding an overload that accepts a string argument).  If you look at <http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#9999>, you'll see that the function is pretty simple.  You essentially want to replace the call to nsContentUtils::ReportToConsole() with nsContentUtils::ReportToConsoleNonLocalized().  You can pass a hardcoded string to that function.

3. Call this modified WarnOnceAbout() inside the deprecated function manually.

Comment 35

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Deprecated ImageBitmapRenderingContext.transferImageBitmap.

I cannot use nsDocument::WarnOnceAbout directly because transferImageBitmap may be called from the worker.
Luckily, DeprecationWarning in BindingUtils handle the off main thread stuff for us. So I modified both nsDocument::WarnOnceAbout and DeprecationWarning then let transferImageBitmap called DeprecationWarning.

Comment 36

[(no longer active)]

Comment on attachment 8801707 [details] [diff] [review]
Deprecated ImageBitmapRenderingContext.transferImageBitmap.

Review of attachment 8801707 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thank you!

::: dom/bindings/BindingUtils.h
@@ +3184,5 @@
>  // Warnings
>  void
>  DeprecationWarning(JSContext* aCx, JSObject* aObject,
> +                   nsIDocument::DeprecatedOperations aOperation,
> +                   const nsAString& aErrorText = NS_LITERAL_STRING(""));

You can make this default to EmptyString() which is slightly more efficient.

Comment 37

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3db412ef03ac

Try link.

Comment 38

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Deprecated ImageBitmapRenderingContext.transferImageBitmap. (carry r+: ehsan)

Addressed ehsan's comment.

Comment 39

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8801774 [details] [diff] [review]
Deprecated ImageBitmapRenderingContext.transferImageBitmap. (carry r+: ehsan)

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: User cannot use latest spec of ImageBitmapRenderingContext. The main reason for uplifting is we want more telemetry data for old method usage. So we want to uplift this patch to beta and aurora for collecting data on larger population.
[Describe test coverage new/current, TreeHerder]: Tested on local and try.
[Risks and why]: Low, only add a new function and add deprecated warning to old method. 
[String/UUID change made/needed]: N/A

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798787 [details]
Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

Clearing the A+ as this patch needs to be redone to address the string changes and another patch is in progress.

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8801774 [details] [diff] [review]
Deprecated ImageBitmapRenderingContext.transferImageBitmap. (carry r+: ehsan)

This new patch addresses the string change concerns, Aurora51+, Beta50+

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Morris Tseng [:mtseng] [:Morris] from comment #37)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3db412ef03ac
> 
> Try link.

Are those test failures on Windows VM related to this patch?

Comment 43

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I submit an another without my patch. Looks like same failures. So it should not be relevant to my patch. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab255e1edc1e&selectedJob=29288863

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b03f083b880e

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0616db3d38ab

Comment 46

[Boris Zbarsky [:bzbarsky]]

Is there a followup bug to remove the deprecated thing once we have use counter data?

Comment 47

[(no longer active)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> Is there a followup bug to remove the deprecated thing once we have use
> counter data?

Bug 1307628 is on file for unshipping this API.  I was hoping removing the deprecation can also happen in the same bug.

Comment 48

[Chris Mills [:cmills]]

I've updated the necessary ref pages, mentioning that the old name is an alias for now:
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapRenderingContext
https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapRenderingContext/transferFromImageBitmap

I've also made sure this change is noted in the Fx 52 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/52#Others

Let me know if this looks OK. Thanks!