Closed Bug 768072 Opened 11 years ago Closed 7 years ago

Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled


(Core :: Graphics: Canvas2D, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: peterv, Assigned: wisniewskit)


(Blocks 2 open bugs)


(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=Canvas])


(1 file, 5 obsolete files)

      No description provided.
Blocks: unprefix
Depends on: 696630
Keywords: dev-doc-needed
Whiteboard: [DocArea=Canvas]
Attachment #8691870 - Flags: superreview?
Attachment #8691870 - Flags: review?(gwright)
Attachment #8691870 - Flags: review?(bzbarsky)
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
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:!topic/

Follow-up bug to remove mozImageSmoothingEnabled filed: bug 1228850

Next step is sending the patch to try? Would anyone do that for me? Thanks!
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)]:

Blink and WebKit unprefixed awhile ago, too:
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+
Attachment #8691872 - Flags: review?(adw) → review+
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: (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)
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:

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
Attachment #8782931 - Flags: review?(bas)
Comment on attachment 8782931 [details] [diff] [review]

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?
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.
Comment on attachment 8782931 [details] [diff] [review]

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

(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]

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.

Attachment #8782931 - Flags: review?(bzbarsky) → review+
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
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)
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)
Pushed by
Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled. r=pbro,adw,bas,bz
Closed: 7 years ago
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.