Bug 149541

Summary: Add support for the imageSmoothingQuality property for CanvasRenderingContext2D
Product: WebKit Reporter: Kat Graff <kgraff>
Component: CanvasAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Demo case for eyeballs.
none
Patch none

Description Kat Graff 2015-09-24 14:18:58 PDT
Adding support for the new imageSmoothingQuality property for CanvasRenderingContext2D as documented in the HTML Living Standard.
Comment 1 Radar WebKit Bug Importer 2015-09-24 14:20:40 PDT
<rdar://problem/22845558>
Comment 2 Radar WebKit Bug Importer 2015-09-24 14:21:01 PDT
<rdar://problem/22845570>
Comment 3 Dean Jackson 2015-09-24 17:02:09 PDT
Was https://github.com/whatwg/html/pull/136
Comment 4 Kat Graff 2015-09-29 18:32:31 PDT
Created attachment 262129 [details]
Patch
Comment 5 Chris Dumez 2015-09-29 19:24:21 PDT
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 6 Ryosuke Niwa 2015-09-29 19:38:00 PDT
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 7 Chris Dumez 2015-09-29 19:39:16 PDT
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.
Comment 8 Theresa O'Connor 2015-09-30 16:17:57 PDT
Best spec link is https://html.spec.whatwg.org/multipage/scripting.html#image-smoothing
Comment 9 Kat Graff 2015-09-30 17:46:21 PDT
Created attachment 262217 [details]
Patch
Comment 10 Ryosuke Niwa 2015-09-30 18:01:11 PDT
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.
Comment 11 Kat Graff 2015-09-30 19:05:37 PDT
Created attachment 262222 [details]
Patch
Comment 12 Ryosuke Niwa 2015-09-30 19:13:20 PDT
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
Comment 13 Kat Graff 2015-09-30 19:26:15 PDT
Created attachment 262224 [details]
Patch
Comment 14 WebKit Commit Bot 2015-09-30 20:17:53 PDT
Comment on attachment 262224 [details]
Patch

Clearing flags on attachment: 262224

Committed r190383: <http://trac.webkit.org/changeset/190383>
Comment 15 WebKit Commit Bot 2015-09-30 20:17:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Kat Graff 2015-10-01 15:45:35 PDT
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 17 Darin Adler 2015-10-01 21:07:55 PDT
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 18 Chris Dumez 2015-10-01 21:09:32 PDT
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 19 Darin Adler 2015-10-01 21:12:18 PDT
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!
Comment 20 Chris Dumez 2015-10-01 21:15:35 PDT
(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.
Comment 21 Kat Graff 2015-10-05 09:29:23 PDT
(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.
Comment 22 Kat Graff 2015-10-05 10:43:32 PDT
Reopening to attach new patch.
Comment 23 Kat Graff 2015-10-05 10:43:35 PDT
Created attachment 262447 [details]
Patch
Comment 24 Chris Dumez 2015-10-05 10:46:26 PDT
Comment on attachment 262447 [details]
Patch

r=me
Comment 25 WebKit Commit Bot 2015-10-05 11:32:08 PDT
Comment on attachment 262447 [details]
Patch

Clearing flags on attachment: 262447

Committed r190567: <http://trac.webkit.org/changeset/190567>
Comment 26 WebKit Commit Bot 2015-10-05 11:32:14 PDT
All reviewed patches have been landed.  Closing bug.