Depends on: 696630
Created attachment 8691870 [details] [diff] [review] Unprefix mozImageSmoothingEnabled
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
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!
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 #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+
Assignee: nobody → 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
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.
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?
I don't mind sending a fresh one, if the one already sent in November 2015 is considered too old by now.
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
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] 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+
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.
This patch needs review from a DOM peer, else it will get rejected by a commit hook.
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.
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
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b3ddee78f33 Implement imageSmoothingEnabled and deprecate mozImageSmoothingEnabled. r=pbro,adw,bas,bz
Thanks, this worked.
2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/canvas-2d-imagesmoothingenabled-has-been-unprefixed/
Updated: https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/imageSmoothingEnabled and https://developer.mozilla.org/en-US/Firefox/Releases/51#Canvas
Keywords: dev-doc-needed → dev-doc-complete
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: ? → ---
You need to log in before you can comment on or make changes to this bug.