768072 - Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Comment 1

[Florian Scholz (Open Web Docs)]

Attachment: Unprefix mozImageSmoothingEnabled

Comment 2

[Florian Scholz (Open Web Docs)]

Attachment: Unprefix mozImageSmoothingEnabled in devtools/

Comment 3

[Florian Scholz (Open Web Docs)]

Attachment: Unprefix mozImageSmoothingEnabled in toolkit/

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[George Wright (:gw280) (needinfo me!)]

r=me for the test changes

Comment 6

[Florian Scholz (Open Web Docs)]

Attachment: Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled. r=bz,gwright

Comment 7

[Florian Scholz (Open Web Docs)]

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!

Comment 8

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8691871 [details] [diff] [review]
Unprefix mozImageSmoothingEnabled in devtools/

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

Looks good to me.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Status?

Comment 11

[Florian Scholz (Open Web Docs)]

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.

Comment 12

[Thomas Wisniewski]

Attachment: 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.

Comment 13

[Bas Schouten (:bas.schouten)]

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.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Let's do an intent to implement?

Comment 15

[Thomas Wisniewski]

I don't mind sending a fresh one, if the one already sent in November 2015 is considered too old by now.

Comment 16

[Thomas Wisniewski]

I went ahead and sent a fresh intent: https://groups.google.com/forum/?_escaped_fragment_=topic/mozilla.dev.platform/ozygu09pg_o#!topic/mozilla.dev.platform/ozygu09pg_o

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 18

[Thomas Wisniewski]

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

Henri, were you alright with doing a WebIDL review here?

Comment 19

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

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.

Comment 20

[Boris Zbarsky [: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

Comment 21

[Thomas Wisniewski]

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.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This patch needs review from a DOM peer, else it will get rejected by a commit hook.

Comment 23

[Thomas Wisniewski]

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

bz, mind rubber-stamping this as a DOM peer as well?

Comment 24

[Boris Zbarsky [: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.

Comment 25

[Thomas Wisniewski]

Attachment: 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?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3ddee78f33
Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled. r=pbro,adw,bas,bz

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Thanks, this worked.

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9b3ddee78f33

Comment 29

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/canvas-2d-imagesmoothingenabled-has-been-unprefixed/

Comment 30

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/imageSmoothingEnabled
and
https://developer.mozilla.org/en-US/Firefox/Releases/51#Canvas

Comment 31

[Sylvestre Ledru [:Sylvestre]]

We are no longer adding this kind of changes to the main release notes. We are using the (amazing) work of the MDN folks.