Bug 149541 – Add support for the imageSmoothingQuality property for CanvasRenderingContext2D

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Kat Graff]

Adding support for the new imageSmoothingQuality property for CanvasRenderingContext2D as documented in the HTML Living Standard.

[Radar WebKit Bug Importer]

<rdar://problem/22845558>

[Radar WebKit Bug Importer]

<rdar://problem/22845570>

[Dean Jackson]

Was https://github.com/whatwg/html/pull/136

[Kat Graff]

Created attachment 262129 [details]
Patch

[Chris Dumez]

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?

[Ryosuke Niwa]

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)");

[Chris Dumez]

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.

[Theresa O'Connor]

Best spec link is https://html.spec.whatwg.org/multipage/scripting.html#image-smoothing

[Kat Graff]

Created attachment 262217 [details]
Patch

[Ryosuke Niwa]

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.

[Kat Graff]

Created attachment 262222 [details]
Patch

[Ryosuke Niwa]

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

[Kat Graff]

Created attachment 262224 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 262224 [details]
Patch

Clearing flags on attachment: 262224

Committed r190383: <http://trac.webkit.org/changeset/190383>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Kat Graff]

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.

[Darin Adler]

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().

[Chris Dumez]

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.

[Darin Adler]

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!

[Chris Dumez]

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

[Kat Graff]

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

[Kat Graff]

Reopening to attach new patch.

[Kat Graff]

Created attachment 262447 [details]
Patch

[Chris Dumez]

Comment on attachment 262447 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 262447 [details]
Patch

Clearing flags on attachment: 262447

Committed r190567: <http://trac.webkit.org/changeset/190567>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.