Bug 179935 – Add support for CanvasPattern.setTransform()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Add support for CanvasPattern.setTransform()

[Simon Fraser (smfr)]

Created attachment 327442 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 327443 [details]
Patch

[EWS Watchlist]

Comment on attachment 327443 [details]
Patch

Attachment 327443 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5326720

New failing tests:
fast/canvas/canvas-pattern-transform.html

[EWS Watchlist]

Created attachment 327445 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Sam Weinig]

Comment on attachment 327443 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Unlike the spec I think this needs to throw exceptions just as CanvasTransform.setTransform()
> +        does.

Did you file a bug on the spec?

> Source/WebCore/html/canvas/CanvasPattern.h:28
> +#include "AffineTransform.h"

Why is this needed?

> LayoutTests/ChangeLog:29
> [phone]  Simon Fraser  <[e-mail]>
> +
> +        Add support for CanvasPattern.setTransform()
> +        https://bugs.webkit.org/show_bug.cgi?id=179935
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * fast/canvas/canvas-pattern-transform-expected.txt:
> +        * fast/canvas/canvas-pattern-transform.html:
> +        * fast/canvas/canvas-pattern-with-transform-expected.txt: Copied from LayoutTests/fast/canvas/canvas-pattern-transform-expected.txt.
> +        * fast/canvas/canvas-pattern-with-transform.html: Copied from LayoutTests/fast/canvas/canvas-pattern-transform.html.
> +        * fast/canvas/canvas-pattern-with-transform.js: Renamed from LayoutTests/fast/canvas/canvas-pattern-transform.js.
> +
> [phone]  Simon Fraser  <[e-mail]>
> +
> +        Add support for CanvasPattern.setTransform()
> +        https://bugs.webkit.org/show_bug.cgi?id=179935
> +
> +        Reviewed by NOBODY (OOPS!).
> +        
> +        Moved the test previously known as canvas-pattern-transform.html to canvas-pattern-with-transform.html
> +        and added a new test.
> +
> +        * fast/canvas/canvas-pattern-transform-expected.txt:
> +        * fast/canvas/canvas-pattern-transform.html:
> +        * fast/canvas/canvas-pattern-with-transform-expected.txt: Copied from LayoutTests/fast/canvas/canvas-pattern-transform-expected.txt.
> +        * fast/canvas/canvas-pattern-with-transform.html: Copied from LayoutTests/fast/canvas/canvas-pattern-transform.html.
> +        * fast/canvas/canvas-pattern-with-transform.js: Renamed from LayoutTests/fast/canvas/canvas-pattern-transform.js.
> +

Double changelog.

[Simon Fraser (smfr)]

(In reply to Sam Weinig from comment #5)
> Comment on attachment 327443 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327443&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Unlike the spec I think this needs to throw exceptions just as CanvasTransform.setTransform()
> > +        does.
> 
> Did you file a bug on the spec?

There are existing issues:

https://github.com/whatwg/html/issues/2845
https://github.com/whatwg/html/issues/3069

It seems like throwing should only happen for invalid combinations of arguments when initializing the matrix.

[Simon Fraser (smfr)]

Created attachment 327503 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 327504 [details]
Patch

[Sam Weinig]

Comment on attachment 327504 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasPattern.h:28
> +#include "AffineTransform.h"

You sure this is needed?

[Simon Fraser (smfr)]

Created attachment 327506 [details]
Patch

[Darin Adler]

Comment on attachment 327506 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasPattern.cpp:82
> +    auto checkValid = DOMMatrixReadOnly::validateAndFixup(matrixInit);
> +    if (checkValid.hasException())
> +        return checkValid.releaseException();
> +
> +    m_pattern->setPatternSpaceTransform({ matrixInit.a.value_or(1), matrixInit.b.value_or(0), matrixInit.c.value_or(0), matrixInit.d.value_or(1), matrixInit.e.value_or(0), matrixInit.f.value_or(0) });
> +    return { };

This looks wrong! DOMMatrixReadOnly::validateAndFixup moves values *from* a/b/c/d/e/f *to* m11/m12/m21/m22/m41/m42 but *not* in the other direction. So it’s critical that we use the latter names, not the former. We also need test cases that show using those instead of using a/b/c/d/e/f. This makes me think that DOMMatrixReadOnly::validateAndFixup should return a new structure without the redundancy so we can’t make this mistake in the future.

[Simon Fraser (smfr)]

Will fix via bug 179984.

[Simon Fraser (smfr)]

https://trac.webkit.org/r225121

[Radar WebKit Bug Importer]

<rdar://problem/35683926>