928150 - Implement canvas getTransform() and setTransform(DOMMatrixInit)

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, task)

Description

[Tobias Schneider [:tobytailor]]

Canvas v5 introduces the currentTransform property which let us set and retrieve the current transformation matrix from a 2d context. It takes and returns a SVGMatrix object. See http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-currenttransform. Chromium has already implemented it (http://src.chromium.org/viewvc/blink?view=revision&revision=159303).

Comment 1

[Michael]

Attachment: WIP

WIP

Could you assign me to this bug?

Anyway, I've been working on this. The setter doesn't work (yet), and I'm not *quite* sure that I got the order of the Matrix parameters right. Any thoughts?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Sorry for not getting back to you earlier, I was travelling last week. I am the wrong person for canvas 2d things like this. Flagging Jeff Muizelaar instead.

Comment 3

[Chris Peterson [:cpeterson]]

Tobias: do we still want this for Shumway?

Comment 4

[Tobias Schneider [:tobytailor]]

Chris: Nope, Shumway doesn't depend on it anymore.

Comment 5

[Thomas Wisniewski]

I've rebased this patch, and it works in general, but there is an interop problem with the Chrome implementation that I ran into while trying to import their tests: Chrome allows SVGMatrix components (a,b,c,d,e,f) to be set to NaN and other non-finite values, but Firefox does not (it throws, unlike what's expected by the Chrome tests for Canvas v5 currentTransform).

Robert, unless I'm misinterpreting things the SVG 1.1 spec says that only "this value is read-only" exceptions ought to be thrown when trying to set a,b,c,d,e,f on SVGMatrices, which implies that we're possibly being too restrictive (though I don't see 1.0 declaring which exceptions could be thrown). Would you anticipate us running into serious web-compat (or other) problems by relaxing our exception-throwing for setting the values of SVGMatrix components to non-finite numbers? (Note that I'm not up-to-speed on SVG 1.2's updated SVGMatrix, though it does seem to be incompatible with the 1.1 version at-a-glance).

I suppose another (potentially better?) option would be to try to get the Canvas v5 API changed for currentTransform so that it deal with DOMMatrices, rather than SVGMatrices. DOMMatrix also isn't meant to throw in this case, and our implementation follows that expectation (though I haven't checked yet whether the APIs are sufficiently different as to be problematic in other ways).

PS: Because of this concern, I'm clearing the review ni? request until this is sorted out, despite the patch mostly working after rebase.

Comment 6

[Robert Longson [:longsonr]]

The webidl definitions for SVG 2 preclude setting NaN on an SVG Matrix (and more generally you're not allowed to set anything to NaN in SVG 2). You're right that SVG 1.1 was non-committal about this.

We don't want to allow NaNs, they screw up things when they end up in the graphics layer.

Comment 7

[Thomas Wisniewski]

Thanks for the quick reply, Robert!

Upon further investigation, it actually turns out that the canvas spec's latest draft is aiming to use DOMMatrix instead, and just have a getTransform method to complement setTransform, rather than having a currentTransform attribute.

Here's the (current) spec: https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-gettransform
Here's the related Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=637940

I'll try to update the patch accordingly.

Comment 8

[Thomas Wisniewski]

Attachment: 928150-implement-canvas-getTransform-and-setTransform(DOMMatrixInit).diff

Here's a patch which foregoes implementing currentTransform and just implements the draft spec's newer getTransform and setTransform(DOMMatrixInit) methods (the Chromium team seems to be moving to these new methods as well, as per the bug I linked to in the previous comment).

Note that I could not add a second setTransform method to the WebIDL without it also having [LenientFloat], which of course also meant adding an unused parameter of type double (or the compiler would reject it).

Also note that the new tests are essentially Chrome's tests for currentTransform, just modified to handle the revised spec (their shouldThrow tests seemed unnecessary, but I am testing the spec's DOMMatrixInit "validate and fix" procedure's exception cases).

I'm doing a try-run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=182b37f42c05

I've also sent an intent-to-implement: https://groups.google.com/forum/?_escaped_fragment_=topic/mozilla.dev.platform/dYn5Fel_p-M

Comment 9

[Thomas Wisniewski]

Comment on attachment 8789092 [details] [diff] [review]
928150-implement-canvas-getTransform-and-setTransform(DOMMatrixInit).diff

Hmm, it looks like there were some test failures, so I'm canceling the review for now.

Comment 10

[Thomas Wisniewski]

I've hit a WebIDL snag that I can't quite figure out. The current canvas draft spec declares these operations on the same interface (where DOMMatrixInit is a new WebIDL dictionary type in the draft spec):

  void setTransform(unrestricted double a, unrestricted double b, unrestricted double c, unrestricted double d, unrestricted double e, unrestricted double f);
  void setTransform(optional DOMMatrixInit transform);

We already have the first variant implemented on its own, and so we're expected to throw on calls like setTransform(null) or setTransform(undefined) by various tests, including the web platform test for WebIDL interfaces: testing/web-platform/tests/html/dom/interfaces.html

Unfortunately, adding the second variant breaks this expectation, because the null/undefined/etc parameters are now coerced into a DOMMatrixInit object with none of its values passed in/assigned. And looking over the WebIDL spec, I'm not sure if this is a flaw in our codegen (that it should actually be throwing a TypeError rather than making an empty dict), or maybe a possible flaw in the draft spec that will break previous expected behavior.

Here's a link to the draft spec, in case that helps: https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-gettransform

Comment 11

[Boris Zbarsky [:bzbarsky]]

It's not really a flaw in either, really.  In WebIDL, {} and null and undefined all do the same thing when converted to a dictionary: they create an empty dictionary.  So the draft spec is making various calls that used to not have enough arguments (because the old overload required 6 arguments) now be valid calls.

Comment 12

[Thomas Wisniewski]

Ah, I see. Then I'll just have to change any internal tests to not expect exceptions while these experimental new features are toggled on, and likewise mark any failing web platform test as such (while the option is toggled on). Thanks!

Comment 13

[Thomas Wisniewski]

Attachment: 928150-implement-canvas-getTransform-and-setTransform(DOMMatrixInit).diff

Here is a new version of the patch which hides these features behind the preference canvas.transform_extensions.enabled and fixes the tests that were failing.

Note that I changed the WebIDL parser so that it allows polymorphic operations to have varying LenientFloat or Pref extended attributes.

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ee64226826

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Note that I changed the WebIDL parser so that it allows polymorphic operations to have varying LenientFloat or Pref extended attributes.

Allows them, and then what?  The codegen assumes this situation does NOT occur, so I doubt you will get any sort of sane behavior out of it by just allowing it to occur in the parser.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8791363 [details] [diff] [review]
928150-implement-canvas-getTransform-and-setTransform(DOMMatrixInit).diff

r- on the IDL parser changes; I haven't looked at the rest.

Comment 16

[Thomas Wisniewski]

>Allows them, and then what?  The codegen assumes this situation does NOT occur, so I doubt you will get any sort of sane behavior out of it by just allowing it to occur in the parser.

I honestly don't know, as I'm new to that part of the codebase. I was hoping a review would help me figure out what else needs to be done, because all I can say right now is that the patch seems to work (it compiles, passes the tests, and does what's advertised).

Comment 17

[Boris Zbarsky [:bzbarsky]]

That's because you don't have two overloads with different Pref values in the patch, right?  And for the two overloads with different LenientFloat values, one of them doesn't have any floats to be lenient or not for (so you could just as easily add [LenientFloat] to it, with no observable behavior change).

Comment 18

[Thomas Wisniewski]

Comment on attachment 8791363 [details] [diff] [review]
928150-implement-canvas-getTransform-and-setTransform(DOMMatrixInit).diff

Ah, fair enough. I wasn't sure which approach was "better", but I'll gladly do that if it's deemed the better option. Canceling review until I've done so.

Comment 19

[Thomas Wisniewski]

Attachment: 928150-implement-canvas-getTransform-and-setTransform(DOMMatrixInit).diff

Alright, here's a version of the patch that just adds a double parameter to satisfy the LenientFloat requirement for polymorphism instead of hacking the IDL parser. I did another try run just in case, which seems okay: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9096ed987af

Comment 20

[Boris Zbarsky [:bzbarsky]]

No, that's no good either, because that has observably different behavior from not having the additional argument.

But OK, there is in fact a problem here: our enforcement that LenientFloat should apply to _something_ on a per-overload-signature basis doesn't play well with the need to have the same extended attributes across all overloads.

The simplest fix is probably to move the "some argument is a restricted float" check out of IDLMethod.addExtendedAttributes and into IDLMethod.validate.  At that point the IDLMethod will have all its signatures, and the check would be that some argument of some signature is a restricted float.

Comment 21

[Thomas Wisniewski]

Comment on attachment 8791450 [details] [diff] [review]
928150-implement-canvas-getTransform-and-setTransform(DOMMatrixInit).diff

Hmm, ok. I'm canceling the review again until I can investigate this new approach.

Comment 22

[Simon Pieters [:zcorpan]]

setTransform() has now been specified using a 2D variant dictionary where "validate and fixup" ignores 3D members.

https://github.com/whatwg/html/pull/2926
https://github.com/w3c/fxtf-drafts/pull/211
https://github.com/w3c/web-platform-tests/pull/7007

Comment 23

[Thomas Wisniewski]

Alright, I finally had time to take another look at this one. I'm still stuck getting this error:

>WebIDL.WebIDLError: error: Extended attributes differ on different overloads of <unresolved scope>::setTransform,
> /mozilla/central/dom/webidl/CanvasRenderingContext2D.webidl line 166:2
>   void setTransform(double a, double b, double c, double d, double e, double f);
>   ^
> /mozilla/central2/dom/webidl/CanvasRenderingContext2D.webidl line 168:2
>   void setTransform(optional DOMMatrix2DInit transform);

That's for the updated IDL:

>  [Throws, LenientFloat]
>  void setTransform(double a, double b, double c, double d, double e, double f);
> 
>  [Throws, LenientFloat, Pref="canvas.transform_extensions.enabled"]
>  void setTransform(optional DOMMatrix2DInit transform);

It seems I would either have to not hide the new method behind a Pref, or to relax the parser to allow overloads to have no Pref value while others do (all sharing the same one). But that seems to go against your concerns about the codegen not being prepared for this; would I have to modify it as well?

And that's assuming I should have that LenientFloat attribute on the new method as well. If I remove it, I get the same error as above regardless of whether I make the changes you suggest in comment 20. Mind shedding some light on what you think I should do here?

Comment 24

[Boris Zbarsky [:bzbarsky]]

The problem is that there is nothing to hide.  The function is exposed to JS.  Normally [Pref] would control whether that exposure happens or not, but in this case we _do_ want it exposed.  We just want it to possibly have different _behaviors_ depending on the pref.

Right now we have no support for this in codegen, as I said.  It's not quite trivial to do either, because https://heycam.github.io/webidl/#es-overloads is somewhat involved and making parts of it effectively pref-controlled is not trivial.  Especially because we have some optimizations in our implementation that depend on knowing the complete set of types distinguishing arguments might take on...  I've thought about trying to add pref support to overload resolution a few times, because it _does_ keep coming up, but haven't been able to justify taking the time to do it.

So in practice, what you could do is add it unconditionally and then throw from the implementation if the pref is off and the dictionary overload is called.  The good news about that is that the way overload resolution works calls with 2, 3, 4, 5 arguments will all throw and calls with 6 or more arguments will take the "6 doubles" overload.  So the only cases which will take the dictionary overload path are the ones that would have thrown anyway (due to not enough args) if the new functionality simply did not exist.

The "pref turned off" behavior won't quite match the spec for "no dictionary arg": the .length of the function will be 0 instead of 6, and calling it with 1 arg will observably do the dictionary conversion before throwing.  But it will be close enough, hopefully.  The length bit is actually the most worrying part, in case someone feature-detects this.

> And that's assuming I should have that LenientFloat attribute on the new method as well

I think that would be simplest.  We would then move the LenientFloat check from handleExtendedAttribute to someplace after we've done overload resolution (e.g. IDLMethod.validate, and check that it has at least one overload that has double arguments).

Comment 25

[Thomas Wisniewski]

Thanks, bz, that clears things up a great deal (I was getting caught up in the wrong details).

(In reply to Simon Pieters from comment #22)
> setTransform() has now been specified using a 2D variant dictionary where
> "validate and fixup" ignores 3D members.

zcorpan, was is intentional that this new setTransform(transform) method doesn't abort right away on Infinite/NaN parameters, contrary to the setTransform(a,b,c,d,e,f) method (and many others in the spec)? It just seems strange to me to allow them if they're not allowed via the older setTransform method.

Comment 26

[Thomas Wisniewski]

Attachment: 928150-implement-canvas-getTransform-and-setTransform(DOMMatrix2DInit).diff

Just attaching my work-in-progress patch.

Comment 27

[Simon Pieters [:zcorpan]]

Thomas, please see https://github.com/whatwg/html/pull/2975

Comment 28

[Thomas Wisniewski]

bz, I might just be forgetting what's going on here, but I'm running into another problem: according to a web platform test, setTransform() should throw (since there was no argument passed in), but it's just going through as though an implicit argument of {} was passed in.

The codegen expects me to provide this method:

>void SetTransform(const DOMMatrix2DInit& aTransform, ErrorResult& aError)

And sure enough, the generated binding falls through instead of throwing a TypeError:

>  switch (argcount) {
>    case 0: {
>      MOZ_FALLTHROUGH;
>    }

Was this expected behavior (implying that the test or IDL need to be changed), or something that needs to be addressed in the codegen?

For reference, the IDL in question is:

>[Throws, LenientFloat]
>void setTransform(optional DOMMatrix2DInit transform);
> 
>dictionary DOMMatrix2DInit {
>    unrestricted double a;
>    unrestricted double b;
>    unrestricted double c;
>    unrestricted double d;
>    unrestricted double e;
>    unrestricted double f;
>    unrestricted double m11;
>    unrestricted double m12;
>    unrestricted double m21;
>    unrestricted double m22;
>    unrestricted double m41;
>    unrestricted double m42;
>};

Comment 29

[Boris Zbarsky [:bzbarsky]]

> according to a web platform test, setTransform() should throw (since there was no argument passed in)

The idl is:

  void setTransform(optional DOMMatrix2DInit transform);

So the binding layer is certainly not going to throw on setTransform() with no args, because that argument is optional.  In particular, it will behave exactly as if {} or null were passed.  If the test expects something else, the test is broken.

Anyway, what you should get is an empty dictionary, which is then passed to https://drafts.fxtf.org/geometry/#create-a-dommatrix-from-the-dictionary where it's totally valid input at first glance.  So again the test is broken.

Comment 30

[Thomas Wisniewski]

Attachment: 928150-implement-canvas-getTransform-and-setTransform(DOMMatrix2DInit).diff

Alright, make sense. Thanks again, bz!

I filed a WPT issue with a pull request for that (#7357), and have submitted a PR for the also-related issue zcorpan mentioned in comment 27.

Once that stuff is sorted out I'll split this patch up for review, but for now here's an updated copy. Note that it now also includes the related updates for Path2D.addPath and CanvasPattern, since they're simple changes.

I think we'll still have to add analogous OffscreenCanvas get/set/reset transform methods at some point, but I'll file a bug for that after this is closer to landing.

Comment 31

[Thomas Wisniewski]

Attachment: Bug 928150 - Move the LenientFloat check from handleExtendedAttribute to IDLMethod.validate, confirming that it has at least one overload with double arguments;

Review commit: https://reviewboard.mozilla.org/r/191466/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191466/

Comment 32

[Thomas Wisniewski]

Attachment: Bug 928150 - Implement CanvasRenderingContext2D.{get|set}Transform behind the canvas.transform_extensions.enabled preference, and update CanvasPath.addPath and CanvasPattern.

Review commit: https://reviewboard.mozilla.org/r/191468/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191468/

Comment 33

[Thomas Wisniewski]

Okay, given that the relevant WPT changes have been merged for a while, I've rebased my patches, addressed the review comments, and submitted it to ReviewBoard.

Also, since CanvasPath::AddPath and CanvasPattern::SetTransform are in the same file, and they too are now specced to accept DOMMatrixInit2D, I also made the (simple) changes to let them do so. This lets us pass those related web platform tests as well.

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9124bca0b3eaa6e5f050221ba32745d8b34c6915

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8920466 [details]
Bug 928150 - Move the LenientFloat check from handleExtendedAttribute to IDLMethod.validate, confirming that it has at least one overload with double arguments;

https://reviewboard.mozilla.org/r/191466/#review196896

::: dom/bindings/parser/tests/test_float_types.py:131
(Diff revision 1)
> +            };
> +        """)
> +        parser.finish()
> +    except Exception, x:
> +        threw = True
> +    harness.ok(not threw, "[LenientFloat] only allowed if at least one overload has unrestricted float args (4)")

s/only allowed/should be allowed/

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8920467 [details]
Bug 928150 - Implement CanvasRenderingContext2D.{get|set}Transform behind the canvas.transform_extensions.enabled preference, and update CanvasPath.addPath and CanvasPattern.

https://reviewboard.mozilla.org/r/191468/#review196898

::: dom/base/DOMMatrix.h:266
(Diff revision 1)
>    void Ensure3DMatrix();
>  
>    virtual ~DOMMatrix() {}
>  };
>  
> +bool InitMatrixFromDOMMatrix2DInit(const DOMMatrix2DInit& aInit,

Please document what this method does and what the return value means.

::: dom/base/DOMMatrix.h:266
(Diff revision 1)
>    void Ensure3DMatrix();
>  
>    virtual ~DOMMatrix() {}
>  };
>  
> +bool InitMatrixFromDOMMatrix2DInit(const DOMMatrix2DInit& aInit,

Is there a reason this is a global method, not a DOMMatrix static, say?

::: dom/base/DOMMatrix.cpp:688
(Diff revision 1)
>  DOMMatrix::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto)
>  {
>    return DOMMatrixBinding::Wrap(aCx, this, aGivenProto);
>  }
>  
> +#define SAMEVALUEZERO(a, b)\

This is reinventing mozilla::NumbersAreIdentical, no?

::: dom/base/DOMMatrix.cpp:692
(Diff revision 1)
>  
> +#define SAMEVALUEZERO(a, b)\
> +  ((a == b) || (mozilla::IsNaN(a) && mozilla::IsNaN(b)) ||\
> +   (a == -0 && b == 0) || (a == 0 && b == -0))
> +
> +#define SET_DOMMATRIXINIT_DICTIONARY_MEMBER(dict, m, dflt, fallback)\

How about ENSURE_DICTIONARY_MEMBER?

::: dom/base/DOMMatrix.cpp:701
(Diff revision 1)
> +    } else {                                                        \
> +      dict.m.Construct(fallback);                                   \
> +    }                                                               \
> +  }
> +
> +#define THROW_ON_DOMMATRIXINIT_MEMBER_MISMATCH(dict, a, b, aStr, bStr, err)\

How about just THROW_ON_MEMBER_MISMATCH?

::: dom/base/DOMMatrix.cpp:702
(Diff revision 1)
> +      dict.m.Construct(fallback);                                   \
> +    }                                                               \
> +  }
> +
> +#define THROW_ON_DOMMATRIXINIT_MEMBER_MISMATCH(dict, a, b, aStr, bStr, err)\
> +  if (dict.a.WasPassed() && dict.b.WasPassed() &&                          \

This is moderately dangerous without parens around "dict"...

::: dom/base/DOMMatrix.cpp:709
(Diff revision 1)
> +    err.ThrowTypeError<MSG_MISMATCHED_MATRIX_MEMBERS>(                     \
> +      NS_LITERAL_STRING(aStr), NS_LITERAL_STRING(bStr));                   \
> +    return;                                                                \
> +  }
> +
> +void

Should this be static?

::: dom/base/DOMMatrix.cpp:710
(Diff revision 1)
> +      NS_LITERAL_STRING(aStr), NS_LITERAL_STRING(bStr));                   \
> +    return;                                                                \
> +  }
> +
> +void
> +ValidateAndFixupDOMMatrix2DInit(DOMMatrix2DInit& aInit, ErrorResult& aError)

So this basically assumes that ignore3D is true, right?  It's worth documenting that clearly; that this is https://drafts.fxtf.org/geometry/#matrix-validate-and-fixup for the specific case when ignore3D is true.

::: dom/base/DOMMatrix.cpp:730
(Diff revision 1)
> +  SET_DOMMATRIXINIT_DICTIONARY_MEMBER(aInit, mM41, mE, 0.0);
> +  SET_DOMMATRIXINIT_DICTIONARY_MEMBER(aInit, mM42, mF, 0.0);
> +}
> +
> +bool
> +InitMatrixFromDOMMatrix2DInit(const DOMMatrix2DInit& aInit,

Document (in the header) that this is basically https://drafts.fxtf.org/geometry/#create-a-dommatrixreadonly-from-the-dictionary but assuming ignore3D is true.

::: dom/base/DOMMatrix.cpp:737
(Diff revision 1)
> +                              ErrorResult& aError)
> +{
> +  DOMMatrix2DInit fixed(aInit);
> +  ValidateAndFixupDOMMatrix2DInit(fixed, aError);
> +  if (aError.Failed()) {
> +    return false;

Why do you need a return value that just duplicates information from aError?  Just have this function return void.

::: dom/canvas/BasicRenderingContext2D.h:53
(Diff revision 1)
>                              double aM22,
>                              double aDx,
>                              double aDy,
>                              mozilla::ErrorResult& aError) = 0;
> +  virtual void SetTransform(const DOMMatrix2DInit& aTransform,
> +                            mozilla::ErrorResult& aError) = 0;

Why do you need the "mozilla::" prefix here?

::: dom/canvas/CanvasRenderingContext2D.cpp:737
(Diff revision 1)
>    UniquePtr<AdjustedTargetForShadow> mShadowTarget;
>    UniquePtr<AdjustedTargetForFilter> mFilterTarget;
>  };
>  
>  void
> -CanvasPattern::SetTransform(SVGMatrix& aMatrix)
> +CanvasPattern::SetTransform(const DOMMatrix2DInit& aTransform,

So this is going to be a lot slower than the old code.  Worth watching out for performance regressions.

::: dom/canvas/CanvasRenderingContext2D.cpp:741
(Diff revision 1)
>  void
> -CanvasPattern::SetTransform(SVGMatrix& aMatrix)
> +CanvasPattern::SetTransform(const DOMMatrix2DInit& aTransform,
> +                            ErrorResult& aError)
>  {
> -  mTransform = ToMatrix(aMatrix.GetMatrix());
> +  Matrix transform;
> +  if (!InitMatrixFromDOMMatrix2DInit(aTransform, transform, aError)) {

Why can't you just pass mTransform to InitMatrixFromDOMMatrix2DInit, as long as that function promises to not touch the out param in error conditions?

::: dom/canvas/CanvasRenderingContext2D.cpp:2283
(Diff revision 1)
>  }
>  
> +already_AddRefed<DOMMatrix>
> +CanvasRenderingContext2D::GetTransform()
> +{
> +  TransformWillUpdate();

Why do you need that call?  You're not changing the transform.

::: dom/canvas/CanvasRenderingContext2D.cpp:2284
(Diff revision 1)
> +  RefPtr<DOMMatrix> matrix;
> +  matrix = new DOMMatrix(GetParentObject());

Why not one line here?

::: dom/canvas/CanvasRenderingContext2D.cpp:2287
(Diff revision 1)
> +{
> +  TransformWillUpdate();
> +  RefPtr<DOMMatrix> matrix;
> +  matrix = new DOMMatrix(GetParentObject());
> +  if (IsTargetValid()) {
> +    Matrix currentMatrix = mTarget->GetTransform();

How about "const Matrix&".  This way if they ever start returning a const ref you'll avoid copying...

::: dom/canvas/CanvasRenderingContext2D.cpp:2302
(Diff revision 1)
> +
> +void
> +CanvasRenderingContext2D::SetTransform(const DOMMatrix2DInit& aTransform,
> +                                       ErrorResult& aError)
> +{
> +  if (!Preferences::GetBool("canvas.transform_extensions.enabled")) {

Would be better to cache this pref on nsContentUtils or something instead of regetting it every time.

::: dom/canvas/CanvasRenderingContext2D.cpp:2312
(Diff revision 1)
> +
> +  Matrix transform;
> +  if (!InitMatrixFromDOMMatrix2DInit(aTransform, transform, aError)) {
> +    return;
> +  }
> +

The spec says:

> If one or more of matrix's m11 element, m12 element, m21 element, m22 element, m41 element, or m42 element are infinite or NaN, then return.

but I don't see that check in this code.  Where is it?  Do we not have tests for that?

::: dom/canvas/CanvasRenderingContext2D.cpp:6566
(Diff revision 1)
>  {
> +  Matrix transform;
> +  if (!InitMatrixFromDOMMatrix2DInit(aTransform, transform, aError)) {
> +    return;
> +  }
> +

Again, spec says:

> If one or more of matrix's m11 element, m12 element, m21 element, m22 element, m41 element, or m42 element are infinite or NaN, then return.

but I see no check like that happening.

::: testing/web-platform/tests/2dcontext/conformance-requirements/2d.missingargs.html
(Diff revision 1)
>      assert_throws(new TypeError(), function() { ctx.transform(1, 0, 0); });
>      assert_throws(new TypeError(), function() { ctx.transform(1, 0, 0, 1); });
>      assert_throws(new TypeError(), function() { ctx.transform(1, 0, 0, 1, 0); });
>  }
>  if (ctx.setTransform) {
> -    assert_throws(new TypeError(), function() { ctx.setTransform(); });

We should actually test that it does _NOT_ throw.

::: testing/web-platform/tests/css/geometry-1/DOMMatrix2DInit-validate-fixup.html:33
(Diff revision 1)
> +  if (transform === undefined) {
> +    transformedPath.addPath(path);
> +  } else {
> +    transformedPath.addPath(path, transform);
> +  }

That should be equivalent to:

    transformedPath.addPath(path, transform);
    
because undefined and missing is treated identically in this case by the callee.

Comment 36

[Thomas Wisniewski]

Comment on attachment 8920467 [details]
Bug 928150 - Implement CanvasRenderingContext2D.{get|set}Transform behind the canvas.transform_extensions.enabled preference, and update CanvasPath.addPath and CanvasPattern.

https://reviewboard.mozilla.org/r/191468/#review196898

I've finally had a chance to address your comments and will be updating the review request with a rebased patch shortly, but I figured I should ask you to take a brief look at the replies I've made on reviewboard just in case.

> This is reinventing mozilla::NumbersAreIdentical, no?

No, using NumbersAreIdentical causes these WPTs to fail:

>setTransform({c: 0, m21: -0})
>Expected PASS, got FAIL
>c and m21 must match if both are set.
> 
>setTransform({c: -0, m21: 0})
>Expected PASS, got FAIL
>c and m21 must match if both are set.
> 
>setTransform({f: -0, m42: null})
>Expected PASS, got FAIL
>f and m42 must match if both are set.

Which implies that negative values aren't treated the same way.

> Why do you need the "mozilla::" prefix here?

I was just copying what the surrounding code did, but it is indeed unnecessary.

> So this is going to be a lot slower than the old code.  Worth watching out for performance regressions.

Definitely. I guess we'll find out what it's sent through Talos, no?

> Why can't you just pass mTransform to InitMatrixFromDOMMatrix2DInit, as long as that function promises to not touch the out param in error conditions?

I would, but because of the NaN/Inf checks you mention in a comment below, I'll need to keep the intermediary "transform" variable so I can check if it's IsFinite() before setting mTransform.

> The spec says:
> 
> > If one or more of matrix's m11 element, m12 element, m21 element, m22 element, m41 element, or m42 element are infinite or NaN, then return.
> 
> but I don't see that check in this code.  Where is it?  Do we not have tests for that?

Good eye. CanvasRenderingContext2D::SetTransform does this when it calls SetTransformInternal, but CanvasPath::AddPath and CanvasPattern::SetTransform need to check if !transform->IsFinite(). I've made that adjustment.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8920467 [details]
Bug 928150 - Implement CanvasRenderingContext2D.{get|set}Transform behind the canvas.transform_extensions.enabled preference, and update CanvasPath.addPath and CanvasPattern.

https://reviewboard.mozilla.org/r/191468/#review196898

> No, using NumbersAreIdentical causes these WPTs to fail:
> 
> >setTransform({c: 0, m21: -0})
> >Expected PASS, got FAIL
> >c and m21 must match if both are set.
> > 
> >setTransform({c: -0, m21: 0})
> >Expected PASS, got FAIL
> >c and m21 must match if both are set.
> > 
> >setTransform({f: -0, m42: null})
> >Expected PASS, got FAIL
> >f and m42 must match if both are set.
> 
> Which implies that negative values aren't treated the same way.

Oh, you actually want -0 to test the same as 0?  I guess that's what your code is testing; it just confused me because if that's what it wants it's way too complicated.  Just do:

    a == b || (mozilla::IsNaN(a) && mozilla::IsNaN(b))
  
because -0 == 0 tests true.

> Definitely. I guess we'll find out what it's sent through Talos, no?

I will bet you $50 that this function is never called in our Talos tests.

What we should do is see whether we have testcases around for setTransform performance.  If not, it's worth writing a testcase to get some idea of what the before and after numbers look like, at least in a worst-case microbenchmark.

> Good eye. CanvasRenderingContext2D::SetTransform does this when it calls SetTransformInternal, but CanvasPath::AddPath and CanvasPattern::SetTransform need to check if !transform->IsFinite(). I've made that adjustment.

Great.  If that didn't cause more tests to pass, please either add tests or file a followup to do so?

Comment 38

[Thomas Wisniewski]

Comment on attachment 8920467 [details]
Bug 928150 - Implement CanvasRenderingContext2D.{get|set}Transform behind the canvas.transform_extensions.enabled preference, and update CanvasPath.addPath and CanvasPattern.

https://reviewboard.mozilla.org/r/191468/#review196898

> Oh, you actually want -0 to test the same as 0?  I guess that's what your code is testing; it just confused me because if that's what it wants it's way too complicated.  Just do:
> 
>     a == b || (mozilla::IsNaN(a) && mozilla::IsNaN(b))
>   
> because -0 == 0 tests true.

Done.

> I will bet you $50 that this function is never called in our Talos tests.
> 
> What we should do is see whether we have testcases around for setTransform performance.  If not, it's worth writing a testcase to get some idea of what the before and after numbers look like, at least in a worst-case microbenchmark.

Makes sense. I'll see if I can find the time to write such tests (or at least find them) before landing.

> Great.  If that didn't cause more tests to pass, please either add tests or file a followup to do so?

Will do (likely as part of these patches since I'll need to investigate adding perf tests anyhow).

Comment 39

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 40

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

What made this stale? Mozilla now requires patches be on Phabricator, do you mind try this again there?

Comment 41

[Thomas Wisniewski [:twisniewski]]

It's nothing mysterious, I've simply been too busy with other tasks to revisit this. I'll try to get it across the finish line as soon as possible, but if someone else wants to beat me to it, that's fine too.

Comment 42

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Hmm, so I just took a look and it seems LenientFloat makes things complicated. It seems only four methods in the whole IDL are using this, can we just remove them (and do things manually...) to make things simple?

Comment 43

[Boris Zbarsky [:bzbarsky]]

I'm not sure what the "four methods" are here; there's a bunch of stuff on canvas using [LenientFloat].

We _could just change setTransform to do things manually. Then we should probably also wontfix bug 1020975 and remove the comment mentioning it in CanvasPattern (especially because afaict the spec doesn't have an overload of setTransform using doubles there anyway).

If we end up having multiple things we need to remove [LenientFloat] from, we may want to just fix bug 1020975 instead.

Comment 44

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #43)

I'm not sure what the "four methods" are here; there's a bunch of stuff on canvas using [LenientFloat].

Ah... sorry, didn't made my search right.

remove the comment mentioning it in CanvasPattern

I think that comment was to be in CanvasTransform instead...

we may want to just fix bug 1020975 instead.

Is there another extended attribute that would be helped by the fix?

Comment 45

[Boris Zbarsky [:bzbarsky]]

The fix would be specific to LenientFloat; bug 1020975 as filed is very generic, but all the discussion in there is about LenientFloat.

Comment 46

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Hmm, not feeling very great about a fix only for a specific gecko-only attribute...

Comment 47

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Attachment: Bug 928150: Implement canvas getTransform() and setTransform()

Adds getTransform() and setTransform() to CanvasRenderingContext2D.

Comment 48

[Thomas Wisniewski [:twisniewski]]

Thanks for the assist! Note that we still also have to consider performance, as mentioned in comment #37:

What we should do is see whether we have testcases around for setTransform performance. If not, it's worth writing a testcase to get some idea of what the before and after numbers look like, at least in a worst-case microbenchmark.

Comment 49

[Kagami Rosylight [:saschanaz] (they/them) (OOO until Dec 11)]

Should the benchmark thing be filed as another bug?

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0aea246d01bb
Implement canvas getTransform() and setTransform() r=bzbarsky

Comment 51

[Arthur Iakab [arthur_iakab]]

Backed out changeset 0aea246d01bb (bug 928150) for causing build bustages on DOMMatrix.h CLOSED TREEE

Backout revision https://hg.mozilla.org/integration/autoland/rev/9a1a330266a857767b22c8146a3944eb220cdd43

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=255864377&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=0aea246d01bb64c3f87840a8ad6725ca48bce69c

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=255864377&repo=autoland

saschanaz can you please take a look?

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3795b8a53b2b
Implement canvas getTransform() and setTransform() r=bzbarsky

Comment 53

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/3795b8a53b2b

Comment 54

[Chris Mills [:cmills]]

I've documented the new method and new parameter type for the existing method. See https://github.com/mdn/sprints/issues/2118#issuecomment-531245964 for the full defailts of what I did.

Please can I have a review of the new docs? Thanks!

Comment 55

[Boris Zbarsky [:bzbarsky]]

Chris, thank you!

One nit: at https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/setTransform the text talks about the "matrix" argument "representing a DOMMatrix" object, but that argument is really a "DOMMatrixInit" object. It could be an actual DOMMatrix object, or any object that has the right property names.

The rest looks great.

Comment 56

[Chris Mills [:cmills]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #55)

Chris, thank you!

One nit: at https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/setTransform the text talks about the "matrix" argument "representing a DOMMatrix" object, but that argument is really a "DOMMatrixInit" object. It could be an actual DOMMatrix object, or any object that has the right property names.

The rest looks great.

Thanks Boris, good catch. I've updated the text to reflect this.

Comment 57

[Boris Zbarsky [:bzbarsky]]

Thank you!