Bug 87853 – the imageSmoothingEnabled flag needs to be in the state object

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

[Keyar Hood]

Currently, the imageSmoothingEnabled is a property of the canvas. As per a recent update of the spec, this flag should be a property of a drawing state.

[Keyar Hood]

Created attachment 144903 [details]
Patch

[Stephen White]

Comment on attachment 144903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144903&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2283
> +    modifiableState().m_imageSmoothingEnabled = enabled;

Could you put this line right below the realizeSaves() call?  Most other state modifiers seem to do that.

> LayoutTests/fast/canvas/canvas-imageSmoothingEnabled-expected.txt:10
> +Test restore works. We save a false state; create, then save a true state; and then finally restore.

Nit:  "Test restore works" -> "Test that restore works".

> LayoutTests/fast/canvas/canvas-imageSmoothingEnabled-expected.txt:25
> +Test restoring actually changes smoothing and not just the attribute value. We are restoring to a point where imageSmoothingEnabled is set to false.

Nit:  "Test restoring" -> "Test that restoring"

> LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:11
> +debug("Test restore works. We save a false state; create, then save a true state; and then finally restore.");

Same as above.

> LayoutTests/fast/canvas/script-tests/canvas-imageSmoothingEnabled.js:68
> +debug("Test restoring actually changes smoothing and not just the attribute value. We are restoring to a point where imageSmoothingEnabled is set to false.");

Same as above.

[Keyar Hood]

Created attachment 144938 [details]
Patch

[Stephen White]

Comment on attachment 144938 [details]
Patch

Looks good (assuming the bots are ok with it).  r=me

[WebKit Review Bot]

Comment on attachment 144938 [details]
Patch

Clearing flags on attachment: 144938

Committed r119100: <http://trac.webkit.org/changeset/119100>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mario Sanchez Prada]

This is failing on the GTK bots for some reason, even if it looks like something that should work in a cross-platform basis.

Any idea?

The diff:
--- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/canvas/canvas-imageSmoothingEnabled-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/canvas/canvas-imageSmoothingEnabled-actual.txt 
@@ -23,9 +23,9 @@
 PASS left_of_center_pixel.data[1] !== 0 is true
 PASS left_of_center_pixel.data[2] !== 0 is true
 Test that restoring actually changes smoothing and not just the attribute value. We are restoring to a point where imageSmoothingEnabled is set to false.
-PASS left_of_center_pixel.data[0] is 0
-PASS left_of_center_pixel.data[1] is 0
-PASS left_of_center_pixel.data[2] is 0
+FAIL left_of_center_pixel.data[0] should be 0. Was 63.
+FAIL left_of_center_pixel.data[1] should be 0. Was 63.
+FAIL left_of_center_pixel.data[2] should be 0. Was 63.
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Stephen White]

(In reply to comment #7)
> This is failing on the GTK bots for some reason, even if it looks like something that should work in a cross-platform basis.
> 
> Any idea?
> 
> The diff:
> --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/canvas/canvas-imageSmoothingEnabled-expected.txt 
> +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/canvas/canvas-imageSmoothingEnabled-actual.txt 
> @@ -23,9 +23,9 @@
>  PASS left_of_center_pixel.data[1] !== 0 is true
>  PASS left_of_center_pixel.data[2] !== 0 is true
>  Test that restoring actually changes smoothing and not just the attribute value. We are restoring to a point where imageSmoothingEnabled is set to false.
> -PASS left_of_center_pixel.data[0] is 0
> -PASS left_of_center_pixel.data[1] is 0
> -PASS left_of_center_pixel.data[2] is 0
> +FAIL left_of_center_pixel.data[0] should be 0. Was 63.
> +FAIL left_of_center_pixel.data[1] should be 0. Was 63.
> +FAIL left_of_center_pixel.data[2] should be 0. Was 63.
>  PASS successfullyParsed is true
> 
>  TEST COMPLETE

Is it possible that GTK's (cairo's?) implementation of GraphicsContext does not save/restore the ImageInterpolation correctly?

[Stephen White]

(In reply to comment #8)
> (In reply to comment #7)
> > This is failing on the GTK bots for some reason, even if it looks like something that should work in a cross-platform basis.
> > 
> > Any idea?
> > 
> > The diff:
> > --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/canvas/canvas-imageSmoothingEnabled-expected.txt 
> > +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/canvas/canvas-imageSmoothingEnabled-actual.txt 
> > @@ -23,9 +23,9 @@
> >  PASS left_of_center_pixel.data[1] !== 0 is true
> >  PASS left_of_center_pixel.data[2] !== 0 is true
> >  Test that restoring actually changes smoothing and not just the attribute value. We are restoring to a point where imageSmoothingEnabled is set to false.
> > -PASS left_of_center_pixel.data[0] is 0
> > -PASS left_of_center_pixel.data[1] is 0
> > -PASS left_of_center_pixel.data[2] is 0
> > +FAIL left_of_center_pixel.data[0] should be 0. Was 63.
> > +FAIL left_of_center_pixel.data[1] should be 0. Was 63.
> > +FAIL left_of_center_pixel.data[2] should be 0. Was 63.
> >  PASS successfullyParsed is true
> > 
> >  TEST COMPLETE
> 
> Is it possible that GTK's (cairo's?) implementation of GraphicsContext does not save/restore the ImageInterpolation correctly?

Yes, it looks like PlatformContextCairo stores the interpolation quality directly, and it should be moved to PlatformContextCairo::State instead.  That would be my guess (assuming the GTK port uses Cairo).

[Mario Sanchez Prada]

Thanks for the feedback. Unfortunately, I've tried that suggestion locally and didn't seem to work, but it's mostly my fault, since I'm not an expert in the graphics part and I'm most likely doing it wrong.

Anyway, I will file a bug for this issue, add the test to test_expectations.txt and add the right people to CC, pointing back to this bug.

Thanks

[Mario Sanchez Prada]

Bug for tracking the issue down for GTK: bug 87985