Last Comment Bug 768067 - Implement canvasRenderingContext2D.get/setLineDash
: Implement canvasRenderingContext2D.get/setLineDash
Status: RESOLVED FIXED
[parity with Safari, Chrome and IE]
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla27
Assigned To: Rik Cabanier
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
: 809586 (view as bug list)
Depends on:
Blocks: html5test 931643 1119470 918589
  Show dependency treegraph
 
Reported: 2012-06-25 10:11 PDT by Peter Van der Beken [:peterv]
Modified: 2015-01-08 12:35 PST (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
27+


Attachments
canvaslinedash.patch (8.64 KB, patch)
2013-10-02 15:32 PDT, Rik Cabanier
no flags Details | Diff | Review
Add support for set/getLineDash and dash offset (8.56 KB, patch)
2013-10-02 15:35 PDT, Rik Cabanier
no flags Details | Diff | Review
Add support for set/getLineDash and dash offset (8.56 KB, patch)
2013-10-02 15:37 PDT, Rik Cabanier
no flags Details | Diff | Review
Add support for set/getLineDash and dash offset (8.54 KB, patch)
2013-10-09 17:25 PDT, Rik Cabanier
roc: review+
Details | Diff | Review
Add support for set/getLineDash and dash offset (8.54 KB, patch)
2013-10-15 13:30 PDT, Rik Cabanier
no flags Details | Diff | Review
Fix style (2.13 KB, patch)
2013-10-27 14:13 PDT, Rik Cabanier
no flags Details | Diff | Review
Fix incorrect style (2.13 KB, patch)
2013-10-27 14:15 PDT, Rik Cabanier
no flags Details | Diff | Review

Description Peter Van der Beken [:peterv] 2012-06-25 10:11:08 PDT

    
Comment 1 Rik Cabanier 2013-10-02 15:32:26 PDT
Created attachment 813352 [details] [diff] [review]
canvaslinedash.patch
Comment 2 Rik Cabanier 2013-10-02 15:35:06 PDT
Created attachment 813353 [details] [diff] [review]
Add support for set/getLineDash and dash offset
Comment 3 Rik Cabanier 2013-10-02 15:37:11 PDT
Created attachment 813354 [details] [diff] [review]
Add support for set/getLineDash and dash offset
Comment 4 Rik Cabanier 2013-10-02 15:53:51 PDT
Not sure about removing the moz methods. That would break current sites...
Comment 5 Rik Cabanier 2013-10-02 16:24:39 PDT
try build: https://tbpl.mozilla.org/?tree=Try&rev=92fa748e253b
Comment 6 Rik Cabanier 2013-10-09 17:25:15 PDT
Created attachment 815180 [details] [diff] [review]
Add support for set/getLineDash and dash offset
Comment 7 Florian Bender 2013-10-11 11:29:10 PDT
This Bug 768067, Bug 740284, Bug 683051 and Bug 809586 seem to overlap at least in part. Can somebody with the necessary knowledge (and privs) please clean up/merge and clarify the individual bugs & summaries?
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-10-13 19:14:50 PDT
Comment on attachment 815180 [details] [diff] [review]
Add support for set/getLineDash and dash offset

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2856,5 @@
> +CanvasRenderingContext2D::SetLineDash(const mozilla::dom::AutoSequence<double>& mSegments) {
> +  FallibleTArray<mozilla::gfx::Float>& dash = CurrentState().dash;
> +  dash.Clear();
> +
> +  for(mozilla::dom::AutoSequence<double>::index_type x = 0; x < mSegments.Length(); x++) {

uint32_t type for 'x'.

@@ +2859,5 @@
> +
> +  for(mozilla::dom::AutoSequence<double>::index_type x = 0; x < mSegments.Length(); x++) {
> +    dash.AppendElement(mSegments[x]);
> +  }
> +  if(mSegments.Length()%2) { // If the number of elements is odd, concatenate again

Sapace after 'if' and 'for'.

@@ +2867,5 @@
> +  }
> +}
> +
> +void
> +CanvasRenderingContext2D::GetLineDash(nsTArray<double>& mSegments) const {

This parameter should be called aSegments

@@ +2871,5 @@
> +CanvasRenderingContext2D::GetLineDash(nsTArray<double>& mSegments) const {
> +  const FallibleTArray<mozilla::gfx::Float>& dash = CurrentState().dash;
> +  mSegments.Clear();
> +
> +  for(FallibleTArray<mozilla::gfx::Float>::index_type x = 0; x < dash.Length(); x++) {

Space after 'for'.

Also, you can make x 'uint32_t'. Easier to read.
Comment 9 Rik Cabanier 2013-10-15 13:30:44 PDT
Created attachment 817399 [details] [diff] [review]
Add support for set/getLineDash and dash offset
Comment 10 Rik Cabanier 2013-10-15 13:31:19 PDT
Updated patch per roc's comments.
Try server: https://tbpl.mozilla.org/?tree=Try&rev=e6dd13184378
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-10-16 05:29:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b16182c733bb
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-10-16 14:22:43 PDT
https://hg.mozilla.org/mozilla-central/rev/b16182c733bb
Comment 13 :Ms2ger 2013-10-26 10:10:11 PDT
*** Bug 809586 has been marked as a duplicate of this bug. ***
Comment 14 :Ms2ger 2013-10-26 10:12:07 PDT
(In reply to Rik Cabanier from comment #10)
> Updated patch per roc's comments.

The comments were not addressed.
Comment 15 Rik Cabanier 2013-10-27 13:27:57 PDT
(In reply to :Ms2ger from comment #14)
> (In reply to Rik Cabanier from comment #10)
> > Updated patch per roc's comments.
> 
> The comments were not addressed.

I must have copied the wrong patch :-(
Will fix this asap
Comment 16 Rik Cabanier 2013-10-27 14:13:28 PDT
Created attachment 823033 [details] [diff] [review]
Fix style
Comment 17 Rik Cabanier 2013-10-27 14:15:02 PDT
Created attachment 823037 [details] [diff] [review]
Fix incorrect style
Comment 18 :Ms2ger 2013-10-27 14:17:48 PDT
Comment on attachment 823037 [details] [diff] [review]
Fix incorrect style

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

This is r=roc in comment 8.
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-10-27 17:54:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/222175ec725f
Comment 20 Phil Ringnalda (:philor) 2013-10-27 21:48:48 PDT
https://hg.mozilla.org/mozilla-central/rev/222175ec725f

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