Closed Bug 1304767 Opened 3 years ago Closed 3 years ago

Rename transferImageBitmap to transferFromImageBitmap

Categories

(Core :: Canvas: 2D, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: zcorpan, Assigned: mtseng, NeedInfo)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

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
This implements the renaming while keeping around the older alias as a deprecated version in case any end-users use it.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8794287 - Flags: review?(jmuizelaar)
Priority: -- → P1
Whiteboard: [gfx-noted]
Comment on attachment 8794287 [details] [diff] [review]
rename canvas transferImageBitmap to transferFromImageBitmap due to spec revision

DOM people should probably review the interface change.
Attachment #8794287 - Flags: review?(jmuizelaar) → review?(ehsan)
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.
Attachment #8794287 - Flags: review?(ehsan) → review-
This API shipped in 46. So the cat is already out of the bag.
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
Assignee: lsalzman → mtseng
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.
Flags: needinfo?(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
Flags: needinfo?(zcorpan)
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?
Flags: needinfo?(lsalzman)
(b) is quite simple, and would be safer to push to aurora/beta too to get telemetry data from a bit larger population.
I would prefer to leave the decision to the team and author involved with this issue.
Flags: needinfo?(lsalzman) → needinfo?(mtseng)
I would prefer (b), too.
Flags: needinfo?(mtseng)
Sounds great!
See Also: → 1307628
Keywords: site-compat
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!
Flags: needinfo?(mtseng)
Just saw your dev-platform post, and the plan is outlines there, so please ignore the last comment!
Flags: needinfo?(mtseng)
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 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?
Attachment #8798787 - Flags: review?(ehsan) → review+
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?
Flags: needinfo?(ehsan)
(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
(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.  :-)
Flags: needinfo?(ehsan)
Try looks ok.
Pushed by mtseng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40074a01ad41
Deprecated ImageBitmapRenderingContext.transferImageBitmap. r=Ehsan
https://hg.mozilla.org/mozilla-central/rev/40074a01ad41
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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
Attachment #8798787 - Flags: approval-mozilla-beta?
Attachment #8798787 - Flags: approval-mozilla-aurora?
Comment on attachment 8798787 [details]
Bug 1304767 - Deprecated ImageBitmapRenderingContext.transferImageBitmap.

Deprecating an old method to be spec compliant, Aurora51+, Beta50+
Attachment #8798787 - Flags: approval-mozilla-beta?
Attachment #8798787 - Flags: approval-mozilla-beta+
Attachment #8798787 - Flags: approval-mozilla-aurora?
Attachment #8798787 - Flags: approval-mozilla-aurora+
Patches that add strings to dom.properties are definitely *not* "[String/UUID change made/needed]: N/A"
(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.
(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.
Flags: needinfo?(mtseng)
(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.
Flags: needinfo?(mtseng) → needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
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?
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
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.
Attachment #8801707 - Flags: review?(ehsan)
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.
Attachment #8801707 - Flags: review?(ehsan) → review+
Attachment #8801707 - Attachment is obsolete: true
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
Attachment #8801774 - Flags: approval-mozilla-beta?
Attachment #8801774 - Flags: approval-mozilla-aurora?
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.
Attachment #8798787 - Flags: approval-mozilla-beta+
Attachment #8798787 - Flags: approval-mozilla-aurora+
Comment on attachment 8801774 [details] [diff] [review]
Deprecated ImageBitmapRenderingContext.transferImageBitmap. (carry r+: ehsan)

This new patch addresses the string change concerns, Aurora51+, Beta50+
Attachment #8801774 - Flags: approval-mozilla-beta?
Attachment #8801774 - Flags: approval-mozilla-beta+
Attachment #8801774 - Flags: approval-mozilla-aurora?
Attachment #8801774 - Flags: approval-mozilla-aurora+
(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?
Flags: needinfo?(mtseng)
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
Flags: needinfo?(mtseng)
Is there a followup bug to remove the deprecated thing once we have use counter data?
Flags: needinfo?(mtseng)
(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.
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!
You need to log in before you can comment on or make changes to this bug.