Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(230)

Issue 14298022: Add support for new canvas ellipse method. (Closed)

Created:
7 years, 8 months ago by dshwang
Modified:
7 years, 3 months ago
CC:
blink-reviews, jamesr, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, danakj, aandrey+blink_chromium.org, jeez, pdr.
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

Add support for new canvas ellipse method. Canvas v5 API adds a new path segment type: ellipse. http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-context-2d-ellipse Add ellipse API into Path.h because Canvas v5 adds following API also. path.ellipse(x, y, radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise) When we support Path primitives, we can reuse Path::ellipse. BUG=130260 TEST=fast/canvas/canvas-ellipse-360-winding.html, fast/canvas/canvas-ellipse-circumference.html, fast/canvas/canvas-ellipse-connecting-line.html, fast/canvas/canvas-ellipse-zero-lineto.html, fast/canvas/canvas-ellipse.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156828

Patch Set 1 #

Total comments: 7

Patch Set 2 : WIP: Implementation is completed. I'll add various ellipse tests. #

Patch Set 3 : Match addEllipse impl to addArc style. Add 4 more layout tests. #

Total comments: 8

Patch Set 4 : Patch to discuss about degenerate ellipse. #

Total comments: 4

Patch Set 5 : Patch v2 to discuss about degenerateEllipse. Fix rotation problem. #

Total comments: 8

Patch Set 6 : Complete degenerateEllipse. Make canvas-ellipse-zero-lineto.html cover various degenerate edge case… #

Total comments: 5

Patch Set 7 : Rebase to upstream. Make canvas-ellipse-circumference cover more extensive cases. #

Total comments: 6

Patch Set 8 : Add comments for degenerateEllipse(). Remove redundant tests. #

Patch Set 9 : Make canvas-ellipse-360-winding.html for virtual/gpu pass. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+735 lines, -27 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/fast/canvas/arc360.html View 1 2 1 chunk +1 line, -4 lines 0 comments Download
A + LayoutTests/fast/canvas/canvas-ellipse.html View 1 2 1 chunk +1 line, -1 line 0 comments Download
A + LayoutTests/fast/canvas/canvas-ellipse-360-winding.html View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
A + LayoutTests/fast/canvas/canvas-ellipse-360-winding-expected.txt View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -6 lines 0 comments Download
A + LayoutTests/fast/canvas/canvas-ellipse-circumference.html View 1 2 3 4 5 6 2 chunks +1 line, -2 lines 0 comments Download
A + LayoutTests/fast/canvas/canvas-ellipse-circumference-expected.txt View 1 2 3 4 5 6 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/fast/canvas/canvas-ellipse-connecting-line.html View 1 2 1 chunk +20 lines, -0 lines 0 comments Download
A LayoutTests/fast/canvas/canvas-ellipse-connecting-line-expected.html View 1 2 1 chunk +21 lines, -0 lines 0 comments Download
A LayoutTests/fast/canvas/canvas-ellipse-expected.txt View 1 chunk +24 lines, -0 lines 0 comments Download
A LayoutTests/fast/canvas/canvas-ellipse-zero-lineto.html View 1 2 3 4 5 1 chunk +314 lines, -0 lines 0 comments Download
A LayoutTests/fast/canvas/canvas-ellipse-zero-lineto-expected.txt View 1 2 3 4 5 6 1 chunk +82 lines, -0 lines 0 comments Download
M LayoutTests/fast/canvas/script-tests/canvas-arc-connecting-line.js View 1 2 1 chunk +2 lines, -5 lines 0 comments Download
A LayoutTests/fast/canvas/script-tests/canvas-ellipse.js View 1 2 1 chunk +85 lines, -0 lines 0 comments Download
A + LayoutTests/fast/canvas/script-tests/canvas-ellipse-360-winding.js View 1 2 3 4 5 6 7 8 2 chunks +15 lines, -9 lines 0 comments Download
A LayoutTests/fast/canvas/script-tests/js-ellipse-implementation.js View 1 2 1 chunk +24 lines, -0 lines 0 comments Download
M Source/core/html/canvas/CanvasPathMethods.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/canvas/CanvasPathMethods.cpp View 1 2 3 4 5 6 7 2 chunks +109 lines, -0 lines 0 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
M Source/core/platform/graphics/Path.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/platform/graphics/Path.cpp View 1 2 3 4 5 6 1 chunk +26 lines, -0 lines 0 comments Download

Messages

Total messages: 42 (0 generated)
dshwang
7 years, 8 months ago (2013-04-26 13:56:13 UTC) #1
Stephen Chennney
While this is a relatively benign API change, it still needs to go through an ...
7 years, 8 months ago (2013-04-26 14:33:24 UTC) #2
Stephen Chennney
Link to the relevant spec please. Are there W3C tests for this new functionality? Testing ...
7 years, 8 months ago (2013-04-26 14:51:02 UTC) #3
Stephen Chennney
On 2013/04/26 14:51:02, Stephen Chenney wrote: > Link to the relevant spec please. Sorry, I ...
7 years, 8 months ago (2013-04-26 14:51:31 UTC) #4
Stephen Chennney
Corrected myself. https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/CanvasPathMethods.cpp#newcode137 Source/core/html/canvas/CanvasPathMethods.cpp:137: return sa - 2 * piFloat; I ...
7 years, 8 months ago (2013-04-26 14:57:56 UTC) #5
dshwang
https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/CanvasPathMethods.cpp#newcode188 Source/core/html/canvas/CanvasPathMethods.cpp:188: m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle, anticlockwise); On ...
7 years, 8 months ago (2013-04-26 15:23:33 UTC) #6
dshwang
On 2013/04/26 14:33:24, Stephen Chenney wrote: > While this is a relatively benign API change, ...
7 years, 8 months ago (2013-04-26 15:26:04 UTC) #7
Stephen Chennney
https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/1/Source/core/html/canvas/CanvasPathMethods.cpp#newcode188 Source/core/html/canvas/CanvasPathMethods.cpp:188: m_path.addEllipse(FloatPoint(x, y), rx, ry, rot, sa, adjustedEndAngle, anticlockwise); On ...
7 years, 8 months ago (2013-04-26 16:02:04 UTC) #8
dshwang
Hi, The second patch is WIP because I'll add various tests. I need to feedback ...
7 years, 7 months ago (2013-04-29 12:46:02 UTC) #9
dshwang
Hi, long time no see. I update this patch. I add 4 tests more. So ...
7 years, 5 months ago (2013-07-09 13:00:07 UTC) #10
aandrey
FYI https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode178 Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa == endAngle) ...
7 years, 5 months ago (2013-07-09 13:49:55 UTC) #11
Stephen White
https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/graphics/Path.cpp File Source/core/platform/graphics/Path.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/platform/graphics/Path.cpp#newcode313 Source/core/platform/graphics/Path.cpp:313: if (!rotation && !startAngle && SkScalarNearlyEqual(twoPiScalar, SkScalarAbs(endAngleScalar))) { If ...
7 years, 5 months ago (2013-07-09 13:58:07 UTC) #12
alph
https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode180 Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa + rot), y + ...
7 years, 5 months ago (2013-07-09 14:16:00 UTC) #13
dshwang
thx you for review :) https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode178 Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry ...
7 years, 5 months ago (2013-07-09 14:42:04 UTC) #14
alph
https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode178 Source/core/html/canvas/CanvasPathMethods.cpp:178: if (!rx || !ry || sa == endAngle) { ...
7 years, 5 months ago (2013-07-09 14:43:08 UTC) #15
alph
https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/15001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode180 Source/core/html/canvas/CanvasPathMethods.cpp:180: lineTo(x + rx * cosf(sa + rot), y + ...
7 years, 5 months ago (2013-07-09 15:16:35 UTC) #16
dshwang
https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode215 Source/core/html/canvas/CanvasPathMethods.cpp:215: degenerateEllipse(this, x, y, radiusX, radiusY, rotation, startAngle, endAngle, anticlockwise); ...
7 years, 5 months ago (2013-07-09 15:48:01 UTC) #17
dshwang
On 2013/07/09 15:48:01, dshwang wrote: You don't have to explain axis or something. I understand ...
7 years, 5 months ago (2013-07-09 15:57:35 UTC) #18
alph
https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/27001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode178 Source/core/html/canvas/CanvasPathMethods.cpp:178: path->lineTo(x + radiusX * cosf(endAngle + rotation), y + ...
7 years, 5 months ago (2013-07-09 16:21:23 UTC) #19
dshwang
Thx for kind explanation about rotation. As I mentioned comment #18, I understand :) And ...
7 years, 5 months ago (2013-07-09 16:37:21 UTC) #20
alph
https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode191 Source/core/html/canvas/CanvasPathMethods.cpp:191: sweep = std::fmod(-sweep, piFloat * 2); I'm not sure ...
7 years, 5 months ago (2013-07-09 17:18:17 UTC) #21
dshwang
Thx for review. I'll make degenerateEllipse more succinct, and make fast/canvas/canvas-ellipse-connecting-line.html cover more extensively. https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/CanvasPathMethods.cpp ...
7 years, 5 months ago (2013-07-09 17:40:52 UTC) #22
alph
https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/32001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode195 Source/core/html/canvas/CanvasPathMethods.cpp:195: lineTo(path, endPoint); On 2013/07/09 17:40:53, dshwang wrote: > On ...
7 years, 5 months ago (2013-07-09 18:16:10 UTC) #23
dshwang
On 2013/07/09 18:16:10, alph wrote: > I meant the line should go through points: > ...
7 years, 5 months ago (2013-07-10 05:29:54 UTC) #24
dshwang
On 2013/07/09 18:16:10, alph wrote: > I meant the line should go through points: > ...
7 years, 5 months ago (2013-07-10 13:03:20 UTC) #25
alph
Thanks for addressing my concerns. https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/canvas-ellipse-circumference.html File LayoutTests/fast/canvas/canvas-ellipse-circumference.html (right): https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/canvas-ellipse-circumference.html#newcode24 LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore(); Again, as for ...
7 years, 5 months ago (2013-07-10 13:38:03 UTC) #26
dshwang
Thx for review, alph :) https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/canvas-ellipse-circumference.html File LayoutTests/fast/canvas/canvas-ellipse-circumference.html (right): https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/canvas-ellipse-circumference.html#newcode24 LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore(); On 2013/07/10 13:38:04, ...
7 years, 5 months ago (2013-07-10 14:19:56 UTC) #27
dshwang
On 2013/07/10 14:19:56, dshwang wrote: > https://codereview.chromium.org/14298022/diff/43001/LayoutTests/fast/canvas/canvas-ellipse-circumference.html#newcode24 > LayoutTests/fast/canvas/canvas-ellipse-circumference.html:24: ctx.restore(); > On 2013/07/10 13:38:04, alph ...
7 years, 5 months ago (2013-07-10 14:28:43 UTC) #28
alph
> @alph, do you know good reference tests that I can follow. I don't want ...
7 years, 5 months ago (2013-07-10 15:22:29 UTC) #29
dshwang
@senorblanco Could you have a chance to look at this CL?
7 years, 4 months ago (2013-08-25 15:57:53 UTC) #30
Stephen White
I appreciate the extensive testing, but I wouldn't bother duplicating arc360.html (ellipse360) or arc-crash.html. The ...
7 years, 3 months ago (2013-08-27 14:07:36 UTC) #31
Stephen White
https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html File LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html (right): https://codereview.chromium.org/14298022/diff/53001/LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html#newcode49 LayoutTests/inspector/profiler/canvas2d/canvas2d-api-changes.html:49: "ellipse", Rather than just adding this, we should mark ...
7 years, 3 months ago (2013-08-27 14:23:49 UTC) #32
Justin Novosad
https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode231 Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle - fmodf(startAngle, halfPiFloat) + ...
7 years, 3 months ago (2013-08-27 17:51:01 UTC) #33
dshwang
Thanks for review! I submitted Patch Set 8 that applies your review! On 2013/08/27 14:07:36, ...
7 years, 3 months ago (2013-08-27 18:45:25 UTC) #34
dshwang
Thx for your review! https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/CanvasPathMethods.cpp File Source/core/html/canvas/CanvasPathMethods.cpp (right): https://codereview.chromium.org/14298022/diff/53001/Source/core/html/canvas/CanvasPathMethods.cpp#newcode231 Source/core/html/canvas/CanvasPathMethods.cpp:231: for (float angle = startAngle ...
7 years, 3 months ago (2013-08-27 18:54:16 UTC) #35
Justin Novosad
On 2013/08/27 18:54:16, dshwang wrote: > Good concern. > However, all primitives early exit as ...
7 years, 3 months ago (2013-08-27 22:45:19 UTC) #36
Stephen White
LGTM. Thanks!
7 years, 3 months ago (2013-08-28 01:05:32 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14298022/64001
7 years, 3 months ago (2013-08-28 05:40:16 UTC) #38
dshwang
Thx Stephen, junov and alph for your sincere review! :D
7 years, 3 months ago (2013-08-28 05:40:56 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dongseong.hwang@intel.com/14298022/78001
7 years, 3 months ago (2013-08-28 06:25:53 UTC) #40
dshwang
canvas-ellipse-360-winding.html failed in virtual/gpu test because the test is html ref test and some pixels ...
7 years, 3 months ago (2013-08-28 06:26:20 UTC) #41
commit-bot: I haz the power
7 years, 3 months ago (2013-08-28 07:59:55 UTC) #42
Message was sent while issue was closed.
Change committed as 156828

Powered by Google App Engine
This is Rietveld 408576698