Summary: | Add support for the imageSmoothingQuality property for CanvasRenderingContext2D | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kat Graff <kgraff> | ||||||||||||||
Component: | Canvas | Assignee: | Kat Graff <kgraff> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Enhancement | CC: | cdumez, commit-queue, darin, dino, eoconnor, esprehn+autocc, gyuyoung.kim, rniwa, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Kat Graff
2015-09-24 14:18:58 PDT
Created attachment 262129 [details]
Patch
Comment on attachment 262129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262129&action=review > Source/WebCore/ChangeLog:3 > + Added support for the new imageSmoothingQuality attribute for CanvasRenderingContext2D, which exposes the smooothing quality of algorithms used for scaling images. Valid input values are low, medium, and high: associated algorithms are expected to vary for differing hardware. setImageSmoothingQuality provides a handle into CGInterpolationQuality. This title line should be: "Add support for the imageSmoothingQuality property for CanvasRenderingContext2D". Move the description below, after the reviewed by line. > Source/WebCore/ChangeLog:4 > + No blank line here. > Source/WebCore/ChangeLog:8 > + Description should come here. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2551 > + return WTF::ASCIILiteral("low"); No need for WTF:: (same remark below) > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2559 > + return emptyString(); Hmm, I would WTF::ASCIILiteral("low") here so that it is a valid value. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:387 > + static inline SmoothingQuality parseSmoothingQuality(const String& smoothingQualityString) Move the whole thing to the cpp. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:399 > + static inline InterpolationQuality smoothingToInterpolationQuality(SmoothingQuality quality) Move the whole thing to the cpp. > LayoutTests/ChangeLog:9 > + You're missing the corresponding -expected.txt file. > .gitignore:1 > +*.png What's this? Comment on attachment 262129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262129&action=review > Source/WebCore/ChangeLog:3 > + Added support for the new imageSmoothingQuality attribute for CanvasRenderingContext2D, which exposes the smooothing quality of algorithms used for scaling images. Valid input values are low, medium, and high: associated algorithms are expected to vary for differing hardware. setImageSmoothingQuality provides a handle into CGInterpolationQuality. This should appear below the bug URL with line breaks. > Source/WebCore/ChangeLog:5 > + https://bugs.webkit.org/show_bug.cgi?id=149541 This line should be preceded by the bug title line. See other entries. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:387 > + static inline SmoothingQuality parseSmoothingQuality(const String& smoothingQualityString) This shouldn't be in the class declaration. Move it to .cpp file. Also, there's no need to have it as a separate function. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:399 > + static inline InterpolationQuality smoothingToInterpolationQuality(SmoothingQuality quality) Ditto. This should be moved to the cpp file. > LayoutTests/ChangeLog:8 > + * fast/canvas/canvas-imageSmoothingQuality.html: Added. You're missing -expected.txt. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:45 > + return scaleImageData(canvas, ''); Wrong indentation with a tab. Always indent by 4 spaces. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:47 > + return scaleImageData(canvas, quality); Ditto. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:54 > + context.imageSmoothingQuality = quality; Ditto. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:72 > +shouldBe("JSON.stringify(defaultData)", "JSON.stringify(lowData)"); Instead of calling scaleImageData outside shouldBe, call it inside so that the expected result contains the code that got executed: e.g. shouldBe("JSON.stringify(scaleTestResults('default'))", "JSON.stringify(lowData)"); Comment on attachment 262129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262129&action=review >> Source/WebCore/ChangeLog:8 >> + > > Description should come here. Also, please provide a link to the specification of this new property. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2575 > + if (GraphicsContext* context = drawingContext()) auto* > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:15 > +var source = document.getElementById('source'); You sometimes use single quotes, sometimes double ones, please be consistent. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:30 > + y = 1; What is this? > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:45 > + return scaleImageData(canvas, ''); I would use undefined instead of the empty string. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:54 > + context.imageSmoothingQuality = quality; too many spaces > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:65 > +shouldNotBe("JSON.stringify(lowData)", "JSON.stringify(mediumData)"); It seems like you might want to write a utility function for this. This would make your checks a lot more readable. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:75 > +debug("Testing imageSmoothingQuality value persistent after imageSmoothingEnabled changed.\n"); debug() should add the trailing \n for you. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:81 > +debug("Testing various invalid input.\n"); debug() should add the trailing \n for you. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:82 > +persistData = scaleImageData(high, '3223'); shouldNotThrow("persistData = scaleImageData(high, '3223')"); Ditto for the ones below. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:90 > + You should also check that imageSmoothingQuality gets updated even if image smoothing is not enabled. Best spec link is https://html.spec.whatwg.org/multipage/scripting.html#image-smoothing Created attachment 262217 [details]
Patch
Comment on attachment 262217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262217&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2544 > +static inline InterpolationQuality smoothingToInterpolationQuality(CanvasRenderingContext2D::SmoothingQuality quality) "inline" keyword is not necessary here. Since the function definition appears in the same translation unit, modern C++ compiler will inline it anyway. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality-expected.txt:20 > +TEST: invalid input does not throw or change attribute. Can we add a blank line before this by calling debug('')? Also this "TEST:" prefix worsens the readability of the output. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:38 > + canvas.id = quality+"Canvas"; Nit: space around +. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:62 > +debug("TEST: imageSmoothingQuality 'low', 'medium, 'high' values supported."); I don't think we need this label. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:67 > +debug("TEST: 'low', 'medium', and 'high' produce distinct results."); I don't think this comment is necessary. The output is quite self explanatory. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:73 > +debug("TEST: 'low' alpha > 'medium' alpha > 'high' alpha values."); > +shouldBeGreaterThanOrEqual("lowData[3]", "mediumData[3]"); Instead of adding a comment, we should just add a helper function like "function alpha(data) { return [3] }" and do shouldBeGreaterThanOrEqual("alpha(lowData)", "alpha(mediumData)"); so that the output is self explanatory. > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:82 > +debug("TEST: imageSmoothingQuality value persistent after imageSmoothingEnabled value changed."); > +var highCanvas = document.getElementById("highCanvas"); > +highCanvas.getContext("2d").imageSmoothingEnabled = false; > +shouldBe("highCanvas.getContext('2d').imageSmoothingQuality", "'high'"); Instead of adding a comment, I suggest running the code inside shouldBe so that it's included in the output. Also, we should verify that the initial value of imageSmoothingQuality was "high" on highCanvas. e.g. shouldBe("highCanvas.getContext('2d').imageSmoothingQuality", "'high'"); shouldBe("highCanvas.getContext("2d").imageSmoothingEnabled = false; highCanvas.getContext('2d').imageSmoothingQuality", "'high'"); > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:88 > +highCanvas.getContext("2d").imageSmoothingQuality = 'medium'; > +shouldBe("highCanvas.getContext('2d').imageSmoothingQuality", "'medium'"); > +highCanvas.getContext("2d").imageSmoothingQuality = 'high'; > +highCanvas.getContext("2d").imageSmoothingEnabled = true; Ditto about executing it inside shouldBe and adding more ShouldBe's > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:90 > +debug("TEST: invalid input does not throw or change attribute."); I don't think this label is necessary since the test code and output is pretty self explanatory. Created attachment 262222 [details]
Patch
Comment on attachment 262222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262222&action=review > LayoutTests/fast/canvas/canvas-imageSmoothingQuality.html:104 > + You should also test that context.save() and context.restore(). See LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js Created attachment 262224 [details]
Patch
Comment on attachment 262224 [details] Patch Clearing flags on attachment: 262224 Committed r190383: <http://trac.webkit.org/changeset/190383> All reviewed patches have been landed. Closing bug. Created attachment 262292 [details]
Demo case for eyeballs.
Visual check to see if imageSmoothingQuality looks as though it is behaving properly, possibly useful because there is no good way to pixel test against every hardware's afforded scaling algorithms.
Comment on attachment 262224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262224&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2584 > + ASSERT_NOT_REACHED(); This assertion is incorrect. Webpage content can set imageSmoothingQuality to an invalid value, and it's not appropriate to assert in that case. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2594 > + if (!modifiableState().imageSmoothingEnabled) This should probably be state(), not modifiableState(). Comment on attachment 262224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262224&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2584 >> + ASSERT_NOT_REACHED(); > > This assertion is incorrect. Webpage content can set imageSmoothingQuality to an invalid value, and it's not appropriate to assert in that case. Actually the JS bindings will not call the implementation method if the input value is not a valid enum value, which is why I suggested the assertion. Also note that this is covered by the layout test in this patch. Comment on attachment 262224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=262224&action=review >>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2584 >>> + ASSERT_NOT_REACHED(); >> >> This assertion is incorrect. Webpage content can set imageSmoothingQuality to an invalid value, and it's not appropriate to assert in that case. > > Actually the JS bindings will not call the implementation method if the input value is not a valid enum value, which is why I suggested the assertion. Also note that this is covered by the layout test in this patch. OK, sounds good. Sorry, my mistake. Seems like a waste that the bindings check that the enum is a valid value, but then we have to repeat checking against all the valid values in the C++ DOM code. We should change how the bindings work so the strings don’t have to appear at all in the C++ DOM code! (In reply to comment #19) > Comment on attachment 262224 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262224&action=review > > >>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2584 > >>> + ASSERT_NOT_REACHED(); > >> > >> This assertion is incorrect. Webpage content can set imageSmoothingQuality to an invalid value, and it's not appropriate to assert in that case. > > > > Actually the JS bindings will not call the implementation method if the input value is not a valid enum value, which is why I suggested the assertion. Also note that this is covered by the layout test in this patch. > > OK, sounds good. Sorry, my mistake. > > Seems like a waste that the bindings check that the enum is a valid value, > but then we have to repeat checking against all the valid values in the C++ > DOM code. We should change how the bindings work so the strings don’t have > to appear at all in the C++ DOM code! Agreed, I actually started working on a patch where the bindings code would convert the string to a enum value before handing it over to the implementation. Our C++ implementation really shouldn't have to deal with string enumerations. However, I got side-tracked and haven't had a chance to finish this patch yet. I'll try to pick it up again soon. (In reply to comment #17) > Comment on attachment 262224 [details] > Patch > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2594 > > + if (!modifiableState().imageSmoothingEnabled) > > This should probably be state(), not modifiableState(). Thanks, Darin! Patching now. Reopening to attach new patch. Created attachment 262447 [details]
Patch
Comment on attachment 262447 [details]
Patch
r=me
Comment on attachment 262447 [details] Patch Clearing flags on attachment: 262447 Committed r190567: <http://trac.webkit.org/changeset/190567> All reviewed patches have been landed. Closing bug. |