Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(968)

Issue 24233004: Support currentTransform in 2D Canvas. (Closed)

Created:
6 years, 9 months ago by dshwang
Modified:
6 years, 9 months ago
CC:
blink-reviews, Nils Barth (inactive), kojih, jsbell+bindings_chromium.org, abarth-chromium, aandrey+blink_chromium.org, marja+watch_chromium.org, dglazkov+blink, Rik, adamk+blink_chromium.org, Nate Chapin
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Support currentTransform in 2D Canvas. Canvas v5 has a currentTransform that is an SVGMatrix. http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-currenttransform [Immutable] extended attribute is used, because canvas must not change CTM in the following case. var matrix = context.currentTransform; matrix.a = 2; BUG=277107 TEST=fast/canvas/canvas-currentTransform.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159303

Patch Set 1 #

Total comments: 5

Patch Set 2 : Previous CL includes TB #

Patch Set 3 : Previous CL includes WIP file. #

Total comments: 1

Patch Set 4 : Consider Return value optimization. Check assignment operator correctly. #

Total comments: 1

Patch Set 5 : Remove custom binding based on ch.dumez's work #

Total comments: 1

Patch Set 6 : Clean up binding code using [Immutable]. #

Patch Set 7 : Make setCurrentTransform succinct #

Patch Set 8 : Rebase to upstream #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+907 lines, -2 lines) Patch
A + LayoutTests/fast/canvas/canvas-currentTransform.html View 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/fast/canvas/canvas-currentTransform-expected.txt View 1 chunk +587 lines, -0 lines 0 comments Download
A LayoutTests/fast/canvas/script-tests/canvas-currentTransform.js View 1 2 3 1 chunk +305 lines, -0 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.h View 1 2 3 4 5 6 7 2 chunks +7 lines, -0 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.cpp View 1 2 3 4 5 6 7 2 chunks +6 lines, -1 line 2 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.idl View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 34 (0 generated)
dshwang
6 years, 9 months ago (2013-09-19 17:44:16 UTC) #1
Justin Novosad
https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js File LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js (right): https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js#newcode25 LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js:25: matrix = ctx.currentTransform; Should use a different variable here ...
6 years, 9 months ago (2013-09-19 19:23:48 UTC) #2
Justin Novosad
https://codereview.chromium.org/24233004/diff/10001/Source/core/html/canvas/CanvasRenderingContext2D.h File Source/core/html/canvas/CanvasRenderingContext2D.h (right): https://codereview.chromium.org/24233004/diff/10001/Source/core/html/canvas/CanvasRenderingContext2D.h#newcode121 Source/core/html/canvas/CanvasRenderingContext2D.h:121: SVGMatrix currentTransform() const; Would you still need the custom ...
6 years, 9 months ago (2013-09-19 19:30:07 UTC) #3
dshwang
Thank you for review :) On 2013/09/19 19:23:48, junov wrote: > https://codereview.chromium.org/24233004/diff/1/LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js > File LayoutTests/fast/canvas/script-tests/canvas-currentTransform2.js ...
6 years, 9 months ago (2013-09-19 19:47:03 UTC) #4
Justin Novosad
Thanks for investigating deeper. I think it is very inconvenient that we would need custom ...
6 years, 9 months ago (2013-09-19 19:52:05 UTC) #5
dshwang
On 2013/09/19 19:47:03, dshwang wrote: > https://codereview.chromium.org/24233004/diff/1/Source/core/html/canvas/CanvasRenderingContext2D.h#newcode121 > > Source/core/html/canvas/CanvasRenderingContext2D.h:121: SVGMatrix > > currentTransform() const; ...
6 years, 9 months ago (2013-09-19 20:19:33 UTC) #6
dshwang
@ch.dumez Could you advise me how to avoid custom binding in this case?
6 years, 9 months ago (2013-09-19 20:30:59 UTC) #7
do-not-use
On 2013/09/19 20:30:59, dshwang wrote: > @ch.dumez Could you advise me how to avoid custom ...
6 years, 9 months ago (2013-09-20 05:01:22 UTC) #8
dshwang
On 2013/09/20 05:01:22, Christophe Dumez (OOO) wrote: > On 2013/09/19 20:30:59, dshwang wrote: > > ...
6 years, 9 months ago (2013-09-20 05:06:33 UTC) #9
haraken
https://codereview.chromium.org/24233004/diff/23001/Source/core/html/canvas/CanvasRenderingContext2D.idl File Source/core/html/canvas/CanvasRenderingContext2D.idl (right): https://codereview.chromium.org/24233004/diff/23001/Source/core/html/canvas/CanvasRenderingContext2D.idl#newcode33 Source/core/html/canvas/CanvasRenderingContext2D.idl:33: [EnabledAtRuntime=ExperimentalCanvasFeatures, Custom] attribute SVGMatrix currentTransform; What code is generated ...
6 years, 9 months ago (2013-09-20 07:07:19 UTC) #10
do-not-use
This should hopefully set you in the right direction: http://pastebin.com/Em6CcZ00 No custom code, it builds ...
6 years, 9 months ago (2013-09-20 07:31:40 UTC) #11
do-not-use
On 2013/09/20 07:31:40, Christophe Dumez (OOO) wrote: > This should hopefully set you in the ...
6 years, 9 months ago (2013-09-20 07:43:58 UTC) #12
do-not-use
On 2013/09/20 07:43:58, Christophe Dumez (OOO) wrote: > On 2013/09/20 07:31:40, Christophe Dumez (OOO) wrote: ...
6 years, 9 months ago (2013-09-20 08:25:37 UTC) #13
dshwang
> What code is generated if you omit [Custom]? How is the generated code different ...
6 years, 9 months ago (2013-09-20 10:29:03 UTC) #14
dshwang
https://codereview.chromium.org/24233004/diff/35001/Source/core/html/canvas/CanvasRenderingContext2D.cpp File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/24233004/diff/35001/Source/core/html/canvas/CanvasRenderingContext2D.cpp#newcode711 Source/core/html/canvas/CanvasRenderingContext2D.cpp:711: modifiableState().m_transform = newTransform; when setting non-invertible matrix, currentTransform should ...
6 years, 9 months ago (2013-09-20 11:38:51 UTC) #15
do-not-use
On 2013/09/20 10:29:03, dshwang wrote: > > What code is generated if you omit [Custom]? ...
6 years, 9 months ago (2013-09-20 14:39:15 UTC) #16
dshwang
On 2013/09/20 14:39:15, Christophe Dumez (OOO) wrote: > I believe you want to use the ...
6 years, 9 months ago (2013-09-20 14:46:04 UTC) #17
do-not-use
On 2013/09/20 14:46:04, dshwang wrote: > On 2013/09/20 14:39:15, Christophe Dumez (OOO) wrote: > > ...
6 years, 9 months ago (2013-09-20 14:50:03 UTC) #18
dshwang
On 2013/09/20 14:46:04, dshwang wrote: > On 2013/09/20 14:39:15, Christophe Dumez (OOO) wrote: > > ...
6 years, 9 months ago (2013-09-20 14:51:00 UTC) #19
haraken
> Immutable is fantastic. It generates the same code to the custom binding in > ...
6 years, 9 months ago (2013-09-20 14:53:32 UTC) #20
do-not-use
On 2013/09/20 14:51:00, dshwang wrote: > On 2013/09/20 14:46:04, dshwang wrote: > > On 2013/09/20 ...
6 years, 9 months ago (2013-09-20 14:55:02 UTC) #21
haraken
> The doc says it *might* be deprecated. At this point, it is not deprecated ...
6 years, 9 months ago (2013-09-20 14:58:11 UTC) #22
dshwang
On 2013/09/20 14:55:02, Christophe Dumez (OOO) wrote: > The doc says it *might* be deprecated. ...
6 years, 9 months ago (2013-09-20 15:03:26 UTC) #23
dshwang
http://crrev.com/25037006/ blocks this CL. I submitted CL about svg setter generation (http://crrev.com/25037006/ ) to make ...
6 years, 9 months ago (2013-09-27 16:36:15 UTC) #24
krit
On 2013/09/20 14:58:11, haraken wrote: > > The doc says it *might* be deprecated. At ...
6 years, 9 months ago (2013-09-27 19:08:12 UTC) #25
dshwang
On 2013/09/27 19:08:12, krit wrote: > The new DOMMatrix specification will specify two interfaces. One ...
6 years, 9 months ago (2013-09-30 14:51:24 UTC) #26
dshwang
now binding code is simple. junov@ , could you review? :)
6 years, 9 months ago (2013-10-09 16:11:42 UTC) #27
Justin Novosad
https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/CanvasRenderingContext2D.cpp File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/CanvasRenderingContext2D.cpp#newcode706 Source/core/html/canvas/CanvasRenderingContext2D.cpp:706: modifiableState().m_transform = newTransform; Interesting bug you fixed here, but ...
6 years, 9 months ago (2013-10-09 17:17:03 UTC) #28
dshwang
Thank you for review. :) https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/CanvasRenderingContext2D.cpp File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right): https://codereview.chromium.org/24233004/diff/54001/Source/core/html/canvas/CanvasRenderingContext2D.cpp#newcode706 Source/core/html/canvas/CanvasRenderingContext2D.cpp:706: modifiableState().m_transform = newTransform; On ...
6 years, 9 months ago (2013-10-09 17:48:19 UTC) #29
Justin Novosad
Ok, I take back my previous comment. This is fine. We'll worry about invertible matrices ...
6 years, 9 months ago (2013-10-09 17:52:25 UTC) #30
Stephen White
LGTM
6 years, 9 months ago (2013-10-09 17:53:56 UTC) #31
dshwang
Thank you for your review :)
6 years, 9 months ago (2013-10-10 04:22:40 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/24233004/54001
6 years, 9 months ago (2013-10-10 04:22:54 UTC) #33
commit-bot: I haz the power
6 years, 9 months ago (2013-10-10 06:17:34 UTC) #34
Message was sent while issue was closed.
Change committed as 159303

Powered by Google App Engine
This is Rietveld 408576698