Tobias Schneider [:tobytailor] Reporter
	Description • 10 years ago

Having setTransform for CanvasPattern is a quite an useful new addition to the Canvas spec. When using the traditional Canvas API, the same effect could be achieved by changing the current transform right before calling fill(). This is not longer possible when using Shape2D objects since setting a transformation before calling fill(path) transforms the entire path drawn.

	Till Schneidereit [:till]
	Comment 1 • 10 years ago

This blocks landing of the Shumway "nat" development branch with its huge amount of correctness and performance fixes, so it's very high priority for us.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 2 • 10 years ago

Can you attach a (simple) example and what the result would look like?  Also, we have this issue with repeat-* and fill (though we're ok with repeat-* and fillRect), like in bug 468358, would that need to be fixed as well, or is repeat-* not an issue?

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 3 • 10 years ago

For example, if you were to apply 45-ish degree rotation in the setTransform, would you get the result on the left?

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 4 • 10 years ago

Let me know if this is the right direction for what the result would look like.  The setTransform is like the canvas setTransform, taking six doubles.

	Tobias Schneider [:tobytailor] Reporter
	Comment 5 • 10 years ago

Yeah, that's what the result using the tranform-before-fill workaround would look like and therefore exactly what we need. Don't think the standard had anything else in mind.

	Till Schneidereit [:till]
	Comment 6 • 10 years ago

Comment on attachment 8433608 [details] [diff] [review]
WIP: No error handling, etc., but is this what you/standard had in mind?

Review of attachment 8433608 [details] [diff] [review]:
-----------------------------------------------------------------

Result-wise, this looks very good - and I'm delighted to see how simple it is to implement.

Looking at the spec more in-depth, however, I have a couple of additional questions:

- should setTransform really take an SVGMatrix? I don't think that makes sense, and your implementation is much better. (See also inline)
- regarding error handling, the spec only says "If a radial gradient or repeated pattern is used when the transformation matrix is singular, the resulting style must be transparent black". I know too little about WebIDL to know if everything else is implicitly covered by specifying the argument types. The spec, however, probably doesn't do a more thorough job here because it's not required if the matrix is supplied as an SVGMatrix instance

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4051,5 @@
>                                                  bool hasDirtyRect, int32_t dirtyX, int32_t dirtyY,
>                                                  int32_t dirtyWidth, int32_t dirtyHeight)
>  {
>    if (w == 0 || h == 0) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

This and the change below are unrelated cleanup, right?

::: dom/webidl/CanvasRenderingContext2D.webidl
@@ +286,5 @@
>  
>  interface CanvasPattern {
>    // opaque object
> +  [Throws, LenientFloat]
> +  void setTransform(double a, double b, double c, double d, double e, double f);

Per spec, setTransform takes an SVGMatrix. Given that CanvasRenderingContext2D#setTransform takes a 6-tuple of floats, and that SVGMatrix is supposed to be replaced by DOMMatrix, I think it makes a lot of sense to use the 6-tuple here, too. That should probably be brought up with spec people, though.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 7 • 10 years ago

Thanks for the quick feedback!

(In reply to Till Schneidereit [:till] from comment #6)
> Comment on attachment 8433608 [details] [diff] [review]
> ...
> 
> - should setTransform really take an SVGMatrix? I don't think that makes
> sense, and your implementation is much better. (See also inline)

I think the SVGMatrix is defined as a vector of six floats, but I'll definitely check that with people that know better.

> - regarding error handling, the spec only says "If a radial gradient or
> repeated pattern is used when the transformation matrix is singular, the
> resulting style must be transparent black".

Right, I'll cover those, and I was also thinking of the "here's 1.0e347, let's see if you start overwriting memory and cause a security issue" type of things.  There are also the tests that need to be added.


> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> > +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> 
> This and the change below are unrelated cleanup, right?

Yes, sorry about that, another bug.

Anyway, I'll keep this moving.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 9 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=e10f39e18e44

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 10 • 10 years ago

:gw280 - I still have the NS_ERROR_DOM_INVALID_STATE_ERR changes in the patch I attached, but that change wouldn't go in - I'm just anticipating seeing bug 1011574 land before this :)

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 11 • 10 years ago

Review ping

	George Wright (:gw280) (needinfo me!)
	Comment 12 • 10 years ago

Comment on attachment 8435213 [details] [diff] [review]
Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL.

Review of attachment 8435213 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +4053,5 @@
>                                                  bool hasDirtyRect, int32_t dirtyX, int32_t dirtyY,
>                                                  int32_t dirtyWidth, int32_t dirtyHeight)
>  {
>    if (w == 0 || h == 0) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

Looks unrelated?

@@ +4107,5 @@
>    uint32_t dataLen = aArray->Length();
>  
>    uint32_t len = w * h * 4;
>    if (dataLen != len) {
> +    return NS_ERROR_DOM_INVALID_STATE_ERR;

Same - unrelated?

::: content/canvas/test/test_canvas.html
@@ +21551,5 @@
>    isPixel(ctx, 15, 5, 128, 0, 0, 255, 0);
>  }
>  </script>
>  
> +<p>Canvas test: 2d.composite.canvaspattern.setTransform</p>

Where did this test come from? I think test_canvas.html is supposed to be mostly taken from the canvas testsuite (http://philip.html5.org/tests/canvas/suite/tests/). I could be wrong about that though.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 13 • 10 years ago

(In reply to George Wright (:gw280) from comment #12)
> ...
> > +    return NS_ERROR_DOM_INVALID_STATE_ERR;
> 
> Same - unrelated?

Right - see comment 10 - part of bug 1011574 - I'll produce a patch without it.

> 
> ::: content/canvas/test/test_canvas.html
> @@ +21551,5 @@
> >    isPixel(ctx, 15, 5, 128, 0, 0, 255, 0);
> >  }
> >  </script>
> >  
> > +<p>Canvas test: 2d.composite.canvaspattern.setTransform</p>
> 
> Where did this test come from? I think test_canvas.html is supposed to be
> mostly taken from the canvas testsuite
> (http://philip.html5.org/tests/canvas/suite/tests/). I could be wrong about
> that though.

I just wrote the test (it at least passes the sanity check in that it fails if setTransform is a stub), I don't think it's part of the canvas testsuite yet.  Should I take it out and forget about it, move it somewhere else or just leave it in and start some process to get the "real test" in the canvas testsuite somehow created?

	George Wright (:gw280) (needinfo me!)
	Comment 14 • 10 years ago

(In reply to Milan Sreckovic [:milan] from comment #13)

> I just wrote the test (it at least passes the sanity check in that it fails
> if setTransform is a stub), I don't think it's part of the canvas testsuite
> yet.  Should I take it out and forget about it, move it somewhere else or
> just leave it in and start some process to get the "real test" in the canvas
> testsuite somehow created?

Let's move it into test_2d.composite.canvaspattern.setTransform.html in content/canvas/test. Not a fan of lumping everything into test_canvas.html anyway. :)

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 15 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=f1d8dd4df1cd

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 17 • 10 years ago

Remove checkin-needed, I should get a DOM peer review first.

	Milan Sreckovic [:milan] (needinfo for best results) Assignee
	Comment 18 • 10 years ago

Comment on attachment 8439236 [details] [diff] [review]
Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL.  Test in a new file, rather than test_canvas.html.  Carry r=gw280.

Forgot about the DOM peer review (already have the canvas one.)  Boris won, because he knows the context of bug 1020975 mentioned in the webidl file.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 years ago

Comment on attachment 8439236 [details] [diff] [review]
Expose CanvasPattern.setTransform(SVGMatrix) in WebIDL.  Test in a new file, rather than test_canvas.html.  Carry r=gw280.

r=me

	Till Schneidereit [:till]
	Comment 20 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbb41bd1d7a

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 21 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/1fbb41bd1d7a

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 22 • 10 years ago

Is there a need for manual testing on this feature?

	Till Schneidereit [:till]
	Comment 23 • 10 years ago

I changed the availability information in [1], but this could probably do with some explanation/examples.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/CanvasPattern

	Mihaela Velimiroviciu (:mihaelav)
	Comment 24 • 10 years ago

Setting in-testsuite+ since an automated test landed for this bug. Please change back if you think other tests are needed.

	Florian Scholz (Open Web Docs)
	Comment 25 • 10 years ago

https://developer.mozilla.org/en-US/docs/Web/API/CanvasPattern.setTransform
https://developer.mozilla.org/en-US/Firefox/Releases/33#Interfaces.2FAPIs.2FDOM