Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled

RESOLVED FIXED in Firefox 51

Status

()

Core
Canvas: 2D
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: peterv, Assigned: Thomas Wisniewski)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {dev-doc-complete, site-compat})

Trunk
mozilla51
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [DocArea=Canvas])

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)

Updated

6 years ago
Blocks: 775235
Depends on: 696630
Keywords: dev-doc-needed
Whiteboard: [DocArea=Canvas]
Created attachment 8691870 [details] [diff] [review]
Unprefix mozImageSmoothingEnabled
Attachment #8691870 - Flags: superreview?
Attachment #8691870 - Flags: review?(gwright)
Attachment #8691870 - Flags: review?(bzbarsky)
Created attachment 8691871 [details] [diff] [review]
Unprefix mozImageSmoothingEnabled in devtools/
Created attachment 8691872 [details] [diff] [review]
Unprefix mozImageSmoothingEnabled in toolkit/
Attachment #8691872 - Attachment description: bug768072-toolkit.patch → Unprefix mozImageSmoothingEnabled in toolkit/
Comment on attachment 8691870 [details] [diff] [review]
Unprefix mozImageSmoothingEnabled

1) I don't recall seeing an intent to ship for this.  Was there one?  If not, please send one.

2) Typically we'd add the unprefixed version alongside the prefixed one for a bit before killing off the prefixed one, so we don't break existing consumers.  Is there a reason that's not being done here?  We've implemented mozImageSmoothingEnabled for a _long_ time now, so there is a good chance there are some pages depending on it.  There are _certainly_ extensions using it.  I suggest adding a deprecation warning for it and leaving it in for now, while adding the unprefixed version.  File a followup bug to remove the prefixed one, mark it as addon-compat, etc.

r=me with the prefixed version left in and the intent to ship sent.
Attachment #8691870 - Flags: review?(bzbarsky) → review+
Attachment #8691870 - Flags: review?(gwright) → review+
r=me for the test changes
Created attachment 8693342 [details] [diff] [review]
Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled. r=bz,gwright
Attachment #8691870 - Attachment is obsolete: true
Attachment #8691870 - Flags: superreview?
Attachment #8693342 - Flags: review+
Summary: Unprefix CanvasRenderingContext2D.imageSmoothingEnabled → Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled
Thanks Boris!

I updated the patch to keep the prefixed version and to present a warning.
Intent to ship sent: https://groups.google.com/forum/#!topic/mozilla.dev.platform/Nv4efVxrhCo

Follow-up bug to remove mozImageSmoothingEnabled filed: bug 1228850

Next step is sending the patch to try? Would anyone do that for me? Thanks!

Updated

2 years ago
Keywords: site-compat
Release Note Request (optional, but appreciated)
[Why is this notable]: web compat. Shipping ctx.imageSmoothingEnabled and deprecating ctx.mozImageSmoothingEnabled.
[Suggested wording]: Canvas2D context property ctx.mozImageSmoothingEnabled changed to standard ctx.imageSmoothingEnabled.
[Links (documentation, blog post, etc)]:
Spec https://html.spec.whatwg.org/multipage/scripting.html#image-smoothing
MDN https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/imageSmoothingEnabled

Blink and WebKit unprefixed awhile ago, too:
https://code.google.com/p/chromium/issues/detail?id=277199
https://bugs.webkit.org/show_bug.cgi?id=147803
relnote-firefox: --- → ?
Attachment #8691872 - Flags: review?(adw)
Attachment #8691871 - Flags: review?(pbrosset)
Comment on attachment 8691871 [details] [diff] [review]
Unprefix mozImageSmoothingEnabled in devtools/

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

Looks good to me.
Attachment #8691871 - Flags: review?(pbrosset) → review+

Updated

2 years ago
Attachment #8691872 - Flags: review?(adw) → review+
Status?
Assignee: nobody → fscholz
Flags: needinfo?(fscholz)
Sorry, I forgot about this.

In the intent to ship thread, it sounded necessary to land tests that show our implementation is interoperable with Blink's and WebKit's. However, no one came back to me there: https://github.com/w3c/web-platform-tests/pull/2416 (needs rebase now)

For the patches here I asked for a try-run as my web-platform-tests pass, but that also never happened.

I don't have the time to rebase these patches shortly I'm afraid.
Assignee: fscholz → nobody
Flags: needinfo?(fscholz)
(Assignee)

Comment 12

2 years ago
Created attachment 8782931 [details] [diff] [review]
768072-implement-imageSmoothingEnabled-and-deprecate-mozImageSmoothingEnabled.diff

I don't mind helping to nudge this one across the finish line.

Here's a (trivially) rebased patch combining the three patches that were here, as well as marking the two relevant web platform tests as passing.

Also, the other new WPT tests mentioned in comment 11 landed a couple of days ago, and this patch passes them as well.

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3275dc5ea236

I do suspect that it's worth giving these patches one more review, since it's been a long while since they were r+'d.
Assignee: nobody → wisniewskit
Attachment #8691871 - Attachment is obsolete: true
Attachment #8691872 - Attachment is obsolete: true
Attachment #8693342 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8782931 - Flags: review?(bas)
Comment on attachment 8782931 [details] [diff] [review]
768072-implement-imageSmoothingEnabled-and-deprecate-mozImageSmoothingEnabled.diff

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

This all looks fine to me, someone who's more into standards and such should probably sign off on this as well though.
Attachment #8782931 - Flags: review?(bas) → review+
Let's do an intent to implement?
(Assignee)

Comment 15

2 years ago
I don't mind sending a fresh one, if the one already sent in November 2015 is considered too old by now.
Flags: needinfo?(Ms2ger)
Thanks. If nobody objects by 2016-09-01, I think we should go ahead and land. The WebIDL change will need a review from a DOM peer, though.
(Assignee)

Comment 18

2 years ago
Comment on attachment 8782931 [details] [diff] [review]
768072-implement-imageSmoothingEnabled-and-deprecate-mozImageSmoothingEnabled.diff

Henri, were you alright with doing a WebIDL review here?
Attachment #8782931 - Flags: review?(hsivonen)
Comment on attachment 8782931 [details] [diff] [review]
768072-implement-imageSmoothingEnabled-and-deprecate-mozImageSmoothingEnabled.diff

(In reply to Thomas Wisniewski from comment #18)
> Henri, were you alright with doing a WebIDL review here?

Sorry, but I think I should defer to someone more familiar with Canvas WebIDL. Specifically, it's not clear to me if there's supposed to be some systematic pattern to when we do the same thing as the spec WebIDL in terms of grouping stuff to NoInterfaceObject interfaces that CanvasRenderingContext2D implements vs. when we differ from the spec by flattening stuff into CanvasRenderingContext2D directly.

Redirecting review to bz.
Attachment #8782931 - Flags: review?(hsivonen) → review?(bzbarsky)
Comment on attachment 8782931 [details] [diff] [review]
768072-implement-imageSmoothingEnabled-and-deprecate-mozImageSmoothingEnabled.diff

This is fine for now.

The spec's split into lots of interfaces is somewhat recent as legwork for things like canvas on workers.  We will in fact want to do that sort of split as we go forward, but it should happen in a bug dedicated to just that.

r=me
Attachment #8782931 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 21

2 years ago
Alright, since I don't see any objections raised in the intent-to-implement thread or here, and the patch still applies cleanly to inbound and passed the WebIDL review, I guess we might as well land it. Requesting check-in.
Keywords: checkin-needed
This patch needs review from a DOM peer, else it will get rejected by a commit hook.
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
(Assignee)

Comment 23

2 years ago
Ah, ok, I thought bz's review would count as such.

bz, mind rubber-stamping this as a DOM peer as well?
Flags: needinfo?(wisniewskit) → needinfo?(bzbarsky)
Yes, I did that already.  ;)  That was comment 20.

But the commit hook wouldn't know that, because the commit message doesn't say "r=bz" or "r=bzbarsky" anywhere in it, and the commit message is all the commit hook looks at.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 25

2 years ago
Created attachment 8787827 [details] [diff] [review]
768072-implement-imageSmoothingEnabled-and-deprecate-mozImageSmoothingEnabled.diff

Ah, I see. Ok, here's a version of the same patch with the commit message tweaked to include you and bas.

aryx, is that sufficient or is anything else (or a different commit message format) required?
Attachment #8782931 - Attachment is obsolete: true
Flags: needinfo?(aryx.bugmail)

Comment 26

2 years ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3ddee78f33
Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled. r=pbro,adw,bas,bz

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b3ddee78f33
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
We are no longer adding this kind of changes to the main release notes. We are using the (amazing) work of the MDN folks.
relnote-firefox: ? → ---
Depends on: 1418904
You need to log in before you can comment on or make changes to this bug.