Closed Bug 928150 Opened 11 years ago Closed 5 years ago

Implement canvas getTransform() and setTransform(DOMMatrixInit)

Categories

(Core :: Graphics: Canvas2D, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: tschneider, Assigned: saschanaz)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, parity-chrome, Whiteboard: [DocArea=Canvas])

Attachments

(4 files, 5 obsolete files)

29.72 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
bzbarsky
: review-
Details
47 bytes, text/x-phabricator-request
Details | Review
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).
Blocks: 927828
Attached patch WIP (obsolete) — Splinter Review
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?
Attachment #820802 - Flags: feedback?(bjacob)
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.
Flags: needinfo?(jmuizelaar)
Attachment #820802 - Flags: feedback?(bjacob) → feedback?(jmuizelaar)
Tobias: do we still want this for Shumway?
Flags: needinfo?(schneider)
Chris: Nope, Shumway doesn't depend on it anymore.
Flags: needinfo?(schneider)
No longer blocks: 927828
Summary: Implement currentTransform → Implement canvas currentTransform
Keywords: dev-doc-needed
Whiteboard: [parity-blink][DocArea=Canvas]
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.
Flags: needinfo?(jmuizelaar) → needinfo?(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.
Flags: needinfo?(longsonr)
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.
Summary: Implement canvas currentTransform → Implement canvas getTransform() and setTransform(DOMMatrixInit)
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
Assignee: nobody → wisniewskit
Attachment #820802 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #820802 - Flags: feedback?(jmuizelaar)
Attachment #8789092 - Flags: review?(bas)
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.
Attachment #8789092 - Flags: review?(bas)
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
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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!
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
Attachment #8789092 - Attachment is obsolete: true
Attachment #8791363 - Flags: review?(vladimir)
> 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 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.
Attachment #8791363 - Flags: review-
>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).
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 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.
Attachment #8791363 - Flags: review?(vladimir)
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
Attachment #8791363 - Attachment is obsolete: true
Attachment #8791450 - Flags: review?(gwright)
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 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.
Attachment #8791450 - Flags: review?(gwright)
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
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?
Flags: needinfo?(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).
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(zcorpan)
Just attaching my work-in-progress patch.
Attachment #8791450 - Attachment is obsolete: true
Thomas, please see https://github.com/whatwg/html/pull/2975
Flags: needinfo?(zcorpan)
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;
>};
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
Attachment #8907893 - Attachment is obsolete: true
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 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/
Attachment #8920466 - Flags: review?(bzbarsky) → review+
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.
Attachment #8920467 - Flags: review?(bzbarsky) → review-
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 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 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).
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [parity-blink][DocArea=Canvas] → [DocArea=Canvas]

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

Flags: needinfo?(wisniewskit)
Type: defect → task

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.

Status: ASSIGNED → NEW
Flags: needinfo?(wisniewskit)

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?

Flags: needinfo?(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.

Flags: needinfo?(bzbarsky)

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

Flags: needinfo?(bzbarsky)

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

Flags: needinfo?(bzbarsky)

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

Depends on: 1020975

Adds getTransform() and setTransform() to CanvasRenderingContext2D.

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.

Should the benchmark thing be filed as another bug?

Assignee: wisniewskit → saschanaz
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0aea246d01bb
Implement canvas getTransform() and setTransform() r=bzbarsky

Keywords: checkin-needed
Flags: needinfo?(saschanaz)
Keywords: checkin-needed

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/3795b8a53b2b
Implement canvas getTransform() and setTransform() r=bzbarsky

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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!

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.

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

Thank you!

Blocks: 683051
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: