Last Comment Bug 662038 - Add support for dashed stroking
: Add support for dashed stroking
Status: VERIFIED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: x86_64 Linux
: -- normal with 1 vote (vote)
: mozilla7
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on: 668412 683051 740284 651858
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-04 04:26 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-06-30 07:57 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (2.62 KB, patch)
2011-06-04 04:26 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Working impl (4.41 KB, patch)
2011-06-09 18:12 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Add canvas.mozDash and .mozDashPhase (9.12 KB, patch)
2011-06-13 23:11 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
Add canvas.mozDash and .mozDashOffset (9.13 KB, patch)
2011-06-14 14:59 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
joe: review+
vladimir: superreview+
Details | Diff | Review
Very basic tests of mozDash/mozDashOffset (5.54 KB, patch)
2011-06-14 15:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
joe: review+
Details | Diff | Review
part 0: Add gfxContext::CurrentDash() (2.66 KB, patch)
2011-06-21 11:57 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
jmuizelaar: review+
Details | Diff | Review
part 2: Add dashing API to Azure (3.06 KB, patch)
2011-06-28 17:13 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
part 3: Implement dashing for d2d (3.53 KB, patch)
2011-06-28 17:14 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
part 4: Share some helpers for converting JSVal<->dash array (7.55 KB, patch)
2011-06-28 17:14 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
joe: review+
Details | Diff | Review
part 5: Implement .mozDash/mozDashOffset for Azure (9.39 KB, patch)
2011-06-28 17:15 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
part 2: Add dashing API to Azure, v2 (3.93 KB, patch)
2011-06-28 18:27 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
roc: superreview+
Details | Diff | Review
part 2: Add dashing API to Azure, v3 (2.11 KB, patch)
2011-06-28 19:56 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bas: review+
roc: superreview+
Details | Diff | Review
part 3: Implement dashing for d2d, v2 (3.51 KB, patch)
2011-06-28 19:56 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bas: review+
Details | Diff | Review
part 5: Implement .mozDash/mozDashOffset for Azure, v2 (9.18 KB, patch)
2011-06-28 19:57 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
joe: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-04 04:26:51 PDT
Created attachment 537336 [details] [diff] [review]
WIP

Canvas should support stroking with a dashed line.  Unless anyone knows of a better API than PDF's and cairo's, let's go with that.

The attached WIP patch is for my own archival.  The interface is easy to implement, but IDL makes it hard.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-08 16:31:17 PDT
Proposal:

  canvas2DContext.setDash([optional] float[] pattern, [optional] float phase); // [1]

|setDash()| specifies a dash style to use when stroking the current path.  More specifically

If |pattern| isn't specified, then strokes have no dash style, i.e. are continuous.

If |pattern| is a one-element array [x], then it specifies the dash pattern "on x units, off x units".

Otherwise, the |pattern| array [x, y, z, ...] specifies the pattern "on x units, off y units, on z units, ...".

If |phase| is specified, it represents an offset into the on/off pattern specified by |pattern| at which stroking should start.  If it's not specified, |phase| defaults to 0.0.

The elements of |pattern| and the value of |phase| are in user space at the time of stroking, i.e. are subject to the CTM.  Each "on" part is treated as a subpath, and are therefore subject to current line width/cap/miter style.

Thoughts?

[1] I would prefer "setLineDash()" for consistency with "lineWidth", "lineCap", etc., but going with "setDash()" for consistency with PostScript "setdash".  SVG's "stroke-dasharray" is different and rather ugly IMHO, so I don't think we should model the canvas API after it.
Comment 2 Philip Taylor 2011-06-09 03:58:49 PDT
I think the spec will need more detail about rendering. It sounds like the basic idea is that when you call stroke() (or strokeRect or strokeText, presumably?) it takes the current path's subpaths, then splits them into a new set of subpaths that include only the points whose path offset is in an 'on' section of the dash pattern (modulo the total pattern length), before joining and capping and rendering. That sounds alright to me, but the offset of a point needs defining.

Does each new subpath reset to offset 0 in the path (i.e. go back to the start of the dash pattern), or continue from the last subpath? Maybe dependent on how the subpath is created (moveTo/closePath/rect)?

> The elements of |pattern| and the value of |phase| are in user space at the time of stroking, i.e. are subject to the CTM.

If I do

  ctx.setDash([0.5, 0.5]);
  ctx.save();
  ctx.scale(10, 10);
  ctx.rect(0, 0, 1, 1);
  ctx.restore();
  ctx.stroke();

it'll draw a 10x10 rectangle. Does that mean it will give 10 dashes per side? If so, I suppose the path offsets can't just be determined when the path is constructed, and I'm not sure how to define what they actually are.

In that example, do the corners of the rect (which coincide with the starts of dashes) still draw line joins (as in the non-dashed case)? What direction do the dashes' line caps point in at those corners? (Maybe this would be a non-problem if any zero-length subsubpaths produced by the dashing process were pruned, as with zero-length subpaths in the current spec.)

The dash pattern should probably be affected by save/restore. setDash([]) and setDash([0]) etc should probably be defined to disable dashing. Maybe it'd be nice if setDash([1e-30]) wasn't a trivial DOS attack (assuming it'll draw countless line caps) or maybe that doesn't matter.

An interface like "ctx.dashPattern = [...]; ctx.dashPhase = 0.5" would seem more consistent with line properties and shadow properties - the current canvas API has no setter functions except for setTransform() (which goes for consistency with other transform methods instead).
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-09 18:12:51 PDT
Created attachment 538408 [details] [diff] [review]
Working impl

Pretty much the simplest possible impl for gecko.  Now down to the API wrangling ... ;)
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-10 15:50:17 PDT
(In reply to comment #2)
> I think the spec will need more detail about rendering. It sounds like the
> basic idea is that when you call stroke() (or strokeRect or strokeText,
> presumably?) it takes the current path's subpaths, then splits them into a
> new set of subpaths that include only the points whose path offset is in an
> 'on' section of the dash pattern (modulo the total pattern length), before
> joining and capping and rendering. 

Yes.  I'm not sure I would call the "on" segments "subpaths" in the spec (might be confusing), but that's the idea.

> That sounds alright to me, but the offset
> of a point needs defining.

The offset within the dash pattern of points in the subpath?

> 
> Does each new subpath reset to offset 0 in the path (i.e. go back to the
> start of the dash pattern)

Yes.

> > The elements of |pattern| and the value of |phase| are in user space at the time of stroking, i.e. are subject to the CTM.
> 
> If I do
> 
>   ctx.setDash([0.5, 0.5]);
>   ctx.save();
>   ctx.scale(10, 10);
>   ctx.rect(0, 0, 1, 1);
>   ctx.restore();
>   ctx.stroke();
> 
> it'll draw a 10x10 rectangle. Does that mean it will give 10 dashes per
> side?

Yes.  And if the stroke() were before the restore(), 1 dash per side would be drawn.

> If so, I suppose the path offsets can't just be determined when the
> path is constructed

Yes, that must happen when the path is stroked.

> In that example, do the corners of the rect (which coincide with the starts
> of dashes) still draw line joins (as in the non-dashed case)?

Yes.

> What direction
> do the dashes' line caps point in at those corners? (Maybe this would be a
> non-problem if any zero-length subsubpaths produced by the dashing process
> were pruned, as with zero-length subpaths in the current spec.)

I don't entirely understand this question.  Let me ask around and get back to you.

> The dash pattern should probably be affected by save/restore.

Agreed.  I meant to include that in comment 1 but forgot.

> setDash([]) and setDash([0]) etc should probably be defined to disable dashing. 

Agreed re: setDash([]).  I think setDash([0]) should be an error (if we allow that, do we also allow setDash([0,0,0]) to mean "turn of dashing"?), but I'm not zealously against "[0]" being a special case.

> Maybe
> it'd be nice if setDash([1e-30]) wasn't a trivial DOS attack (assuming it'll
> draw countless line caps) or maybe that doesn't matter.

I think the spec should leave that up to UAs.

> An interface like "ctx.dashPattern = [...]; ctx.dashPhase = 0.5" would seem
> more consistent with line properties and shadow properties - the current
> canvas API has no setter functions except for setTransform() (which goes for
> consistency with other transform methods instead).

At first I was against this, because it feels "righter" to set the pattern and phase atomically, but I've come over to your side.  The vastly common case ought to be setting just the pattern; I would expect setting the phase to be rarer.  This makes the common case simpler, and is a natural way to specify getters for these attributes too (although I can't think of any uses for that off-hand).  Sounds good.

Thanks for the great feedback.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-13 23:11:27 PDT
Created attachment 539128 [details] [diff] [review]
Add canvas.mozDash and .mozDashPhase
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-14 14:59:21 PDT
Created attachment 539331 [details] [diff] [review]
Add canvas.mozDash and .mozDashOffset

Includes s/dashPhase/dashOffset/ to match SVG.

This patch has some jsapi crud in it, please to be checking closely.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-14 15:01:47 PDT
Created attachment 539332 [details] [diff] [review]
Very basic tests of mozDash/mozDashOffset

We could easily add more tests that our SVG impl matches the canvas impl, but maybe it's better to wait on direct tests of dash behavior until we reach more of a consensus on how to spec it.

These tests made me realize we didn't define what dashOffset returns if it's set to a value greater or less than the total length of the dash pattern.  Cairo seems to return the value modulo dash-pattern length, which I guess makes sense.  Intentionally not testing that case here.
Comment 8 David Flanagan [:djf] 2011-06-15 22:45:11 PDT
This is awesome!  I asked for dashed lines in <canvas> 5 years ago, but Hixie wouldn't go for it: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2007-May/011224.html

I feel vindicated now!
Comment 9 Joe Drew (not getting mail) 2011-06-21 11:31:58 PDT
Comment on attachment 539331 [details] [diff] [review]
Add canvas.mozDash and .mozDashOffset

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

::: content/canvas/src/nsCanvasRenderingContext2D.cpp
@@ +3265,5 @@
>  
> +// Make a double out of |v|, treating undefined values as 0.0 (for the
> +// sake of sparse arrays).  Return true iff coercion succeeded.
> +static bool
> +CoerceDouble(jsval v, double* d)

Should this be a JS utility function in a more accessible place? (I honestly don't know.)

::: gfx/thebes/gfxContext.h
@@ +468,5 @@
>      void SetDash(gfxFloat *dashes, int ndash, gfxFloat offset);
> +    // Return true if dashing is set, false if it's not enabled or the
> +    // context is in an error state.  |offset| can be NULL to mean
> +    // "don't care".
> +    bool CurrentDash(FallibleTArray<gfxFloat>& dashes, gfxFloat* offset) const;

I'm not super-enthusiastic about this interface, but, more than that, I think it should be reviewed separately. Can you split out the gfxContext changes into a predecessor patch and get review from jrmuizel?
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-21 11:39:19 PDT
(In reply to comment #9)
> Comment on attachment 539331 [details] [diff] [review] [review]
> Add canvas.mozDash and .mozDashOffset
> 
> Review of attachment 539331 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2D.cpp
> @@ +3265,5 @@
> >  
> > +// Make a double out of |v|, treating undefined values as 0.0 (for the
> > +// sake of sparse arrays).  Return true iff coercion succeeded.
> > +static bool
> > +CoerceDouble(jsval v, double* d)
> 
> Should this be a JS utility function in a more accessible place? (I honestly
> don't know.)

I thought about that, but the reason I left this in content/canvas/ was that I want to retain control over coercion semantics.  But I too honestly don't know if that's a good idea.  Can we phone a friend?

> 
> ::: gfx/thebes/gfxContext.h
> @@ +468,5 @@
> >      void SetDash(gfxFloat *dashes, int ndash, gfxFloat offset);
> > +    // Return true if dashing is set, false if it's not enabled or the
> > +    // context is in an error state.  |offset| can be NULL to mean
> > +    // "don't care".
> > +    bool CurrentDash(FallibleTArray<gfxFloat>& dashes, gfxFloat* offset) const;
> 
> I'm not super-enthusiastic about this interface, but, more than that, I
> think it should be reviewed separately. Can you split out the gfxContext
> changes into a predecessor patch and get review from jrmuizel?

OK.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-21 11:57:15 PDT
Created attachment 540824 [details] [diff] [review]
part 0: Add gfxContext::CurrentDash()
Comment 12 Vladimir Vukicevic [:vlad] [:vladv] 2011-06-27 10:57:54 PDT
Comment on attachment 539331 [details] [diff] [review]
Add canvas.mozDash and .mozDashOffset

Looks great to me!
Comment 13 Jeff Muizelaar [:jrmuizel] 2011-06-28 05:30:27 PDT
Comment on attachment 540824 [details] [diff] [review]
part 0: Add gfxContext::CurrentDash()


> 
> void
> gfxContext::SetDash(gfxFloat *dashes, int ndash, gfxFloat offset)
> {
>     cairo_set_dash(mCairo, dashes, ndash, offset);
> }
>-//void getDash() const;
>+
>+bool
>+gfxContext::CurrentDash(FallibleTArray<gfxFloat>& dashes, gfxFloat* offset) const
>+{
>+    int count;
>+    if (0 >= (count = cairo_get_dash_count(mCairo)) ||

I don't see any reason the count initialization needs to be in the loop condition.

>+        !dashes.SetLength(count)) {
>+        return false;
>+    }
>+    cairo_get_dash(mCairo, dashes.Elements(), offset);
>+    return true;
>+}
>+
>+gfxFloat
>+gfxContext::CurrentDashOffset() const
>+{
>+    if (0 >= cairo_get_dash_count(mCairo)) {

The condition above is in a awkward order.

>+        return 0.0;
>+    }
>+    gfxFloat offset;
>+    cairo_get_dash(mCairo, nsnull, &offset);

Personally, I prefer NULL when calling cairo API.

>+    return offset;
>+}
>
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 16:33:46 PDT
(In reply to comment #13)
> >+gfxContext::CurrentDash(FallibleTArray<gfxFloat>& dashes, gfxFloat* offset) const
> >+{
> >+    int count;
> >+    if (0 >= (count = cairo_get_dash_count(mCairo)) ||
> 
> I don't see any reason the count initialization needs to be in the loop
> condition.

Oops, residue.  Fixed.

> 
> >+        !dashes.SetLength(count)) {
> >+        return false;
> >+    }
> >+    cairo_get_dash(mCairo, dashes.Elements(), offset);
> >+    return true;
> >+}
> >+
> >+gfxFloat
> >+gfxContext::CurrentDashOffset() const
> >+{
> >+    if (0 >= cairo_get_dash_count(mCairo)) {
> 
> The condition above is in a awkward order.

Flipped (and the previous one).

> 
> >+        return 0.0;
> >+    }
> >+    gfxFloat offset;
> >+    cairo_get_dash(mCairo, nsnull, &offset);
> 
> Personally, I prefer NULL when calling cairo API.
> 

Changed.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 17:13:58 PDT
Created attachment 542680 [details] [diff] [review]
part 2: Add dashing API to Azure

No one objected to implementing this as part of this bug, so here we are.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 17:14:22 PDT
Created attachment 542681 [details] [diff] [review]
part 3: Implement dashing for d2d
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 17:14:47 PDT
Created attachment 542682 [details] [diff] [review]
part 4: Share some helpers for converting JSVal<->dash array
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 17:15:23 PDT
Created attachment 542683 [details] [diff] [review]
part 5: Implement .mozDash/mozDashOffset for Azure

Not sure who should review this, but it's not a complicated patch.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-28 17:36:39 PDT
Comment on attachment 542680 [details] [diff] [review]
part 2: Add dashing API to Azure

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

::: gfx/2d/2D.h
@@ +84,5 @@
> +  }
> +
> +  ~CustomDash() { delete[] mPattern; }
> +
> +  Float* mPattern;

We need AutoPtr/AutoArrayPtr in mfbt!

@@ +91,5 @@
> +};
> +
> +struct Dash {
> +   Dash(size_t aPatternLength = 0, const Float* aPattern = 0,
> +        Float aOffset = 0.f) {

Can you document these parameters?
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 18:26:51 PDT
(In reply to comment #19)
> Comment on attachment 542680 [details] [diff] [review] [review]
> part 2: Add dashing API to Azure
> 
> Review of attachment 542680 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/2D.h
> @@ +84,5 @@
> > +  }
> > +
> > +  ~CustomDash() { delete[] mPattern; }
> > +
> > +  Float* mPattern;
> 
> We need AutoPtr/AutoArrayPtr in mfbt!

Maybe.  But here went with std::vector, per discussion.

> 
> @@ +91,5 @@
> > +};
> > +
> > +struct Dash {
> > +   Dash(size_t aPatternLength = 0, const Float* aPattern = 0,
> > +        Float aOffset = 0.f) {
> 
> Can you document these parameters?

Yes, I sort of totally neglected to document anything, sorry.
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 18:27:20 PDT
Created attachment 542704 [details] [diff] [review]
part 2: Add dashing API to Azure, v2
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-28 19:14:40 PDT
Comment on attachment 542704 [details] [diff] [review]
part 2: Add dashing API to Azure, v2

Review of attachment 542704 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 19:56:31 PDT
Created attachment 542709 [details] [diff] [review]
part 2: Add dashing API to Azure, v3

/me washes hands ;)
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 19:56:58 PDT
Created attachment 542710 [details] [diff] [review]
part 3: Implement dashing for d2d, v2
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-28 19:57:24 PDT
Created attachment 542711 [details] [diff] [review]
part 5: Implement .mozDash/mozDashOffset for Azure, v2
Comment 26 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-28 20:04:32 PDT
Comment on attachment 542709 [details] [diff] [review]
part 2: Add dashing API to Azure, v3

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

This isn't really any different from GlyphBuffer.
Comment 27 Bas Schouten (:bas.schouten) 2011-06-29 07:55:26 PDT
Comment on attachment 542709 [details] [diff] [review]
part 2: Add dashing API to Azure, v3

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

MOZ_ASSERT should be removed in my opinion. If we want to assert here an assertion helper should be put in Tools.h. For now we can just #define that to MOZ_ASSERT though, as far as I'm concerned. But if people want to do a stand-alone build keeping the link to the mozilla functions to a single file will help a lot. I've already had people mail me through my blog asking me if they could do Azure stand-alone builds.
Comment 28 Bas Schouten (:bas.schouten) 2011-06-29 08:05:26 PDT
Comment on attachment 542710 [details] [diff] [review]
part 3: Implement dashing for d2d, v2

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

::: gfx/2d/DrawTargetD2D.cpp
@@ +664,5 @@
>  {
> +  Rect srcRect(Float(aSourceRect.x), Float(aSourceRect.y),
> +               Float(aSourceRect.width), Float(aSourceRect.height));
> +  Rect dstRect(Float(aDestination.x), Float(aDestination.y),
> +               Float(aSourceRect.width), Float(aSourceRect.height));

I wonder if we should wrap this compiler warning fix into this patch, but since it's a good thing that it's being fixed I can live with it!

@@ +1632,5 @@
>  
>  
> +  HRESULT hr;
> +  if (aStrokeOptions.mDashPattern) {
> +    typedef vector<Float> FloatVector;

Really? To save two characters :)? I'm fine with leaving this in though.

@@ +1640,5 @@
> +    Float lineWidth = aStrokeOptions.mLineWidth;
> +    FloatVector dash(aStrokeOptions->mDashPattern,
> +                     aStrokeOptions->mDashPattern + aStrokeOptions->mDashLength);
> +    for (FloatVector::iterator it = dash.begin(); it != dash.end(); ++it)
> +      *it /= lineWidth;

nit: { }

@@ +1648,5 @@
> +                                  capStyle, joinStyle,
> +                                  aStrokeOptions.mMiterLimit,
> +                                  D2D1_DASH_STYLE_CUSTOM,
> +                                  aStrokeOptions.mDashOffset),
> +      &dash[0], // VC's STL doesn't have vector<T>::data() yet

Should probably update the comment to say that MSVC has had it for a while, but not VS2005 which we run on the build servers :). We should update these when we update to VS2010 since &dash[0] I believe crashes for 0-size vectors whereas dash.data() will properly return NULL.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-29 12:21:28 PDT
(In reply to comment #27)
> Comment on attachment 542709 [details] [diff] [review] [review]
> part 2: Add dashing API to Azure, v3
> 
> Review of attachment 542709 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> MOZ_ASSERT should be removed in my opinion. If we want to assert here an
> assertion helper should be put in Tools.h. For now we can just #define that
> to MOZ_ASSERT though, as far as I'm concerned. But if people want to do a
> stand-alone build keeping the link to the mozilla functions to a single file
> will help a lot. I've already had people mail me through my blog asking me
> if they could do Azure stand-alone builds.

I think that's an unhealthy approach that's going to end up with azure having implemented its own mfbt, exactly the problem that mfbt was intended to avoid (fragmented C++ low-level libs).

mfbt is totally intended to be a standalone library.  It's just that, there hasn't been enough motivation to set up the build/packaging gunk to do that yet.  I still don't think the motivation is there, but it will come eventually.

Would you rather spend time on packaging a standalone azure+mfbt for people who've contacted you through your blog to build, or spend time on say perf or cairo/quartz/GL backends?  I hope I know the answer to that ... ;)

(I also think a prerequisite to standalone azure builds would be testing infrastructure for standalone azure, which we don't have either.)
Comment 30 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-29 12:24:10 PDT
(In reply to comment #28)
> Comment on attachment 542710 [details] [diff] [review] [review]
> part 3: Implement dashing for d2d, v2
> 
> Review of attachment 542710 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawTargetD2D.cpp
> @@ +664,5 @@
> >  {
> > +  Rect srcRect(Float(aSourceRect.x), Float(aSourceRect.y),
> > +               Float(aSourceRect.width), Float(aSourceRect.height));
> > +  Rect dstRect(Float(aDestination.x), Float(aDestination.y),
> > +               Float(aSourceRect.width), Float(aSourceRect.height));
> 
> I wonder if we should wrap this compiler warning fix into this patch, but
> since it's a good thing that it's being fixed I can live with it!

Yeah, I thought about that.  We've got about 100 of these warnings from nsCanvasRenderingContextAzure.  Probably worth doing.

> 
> @@ +1632,5 @@
> >  
> >  
> > +  HRESULT hr;
> > +  if (aStrokeOptions.mDashPattern) {
> > +    typedef vector<Float> FloatVector;
> 
> Really? To save two characters :)? I'm fine with leaving this in though.

No, it's to save |for (typename vector<Float>::iterator it; ...)| ;).

> 
> @@ +1640,5 @@
> > +    Float lineWidth = aStrokeOptions.mLineWidth;
> > +    FloatVector dash(aStrokeOptions->mDashPattern,
> > +                     aStrokeOptions->mDashPattern + aStrokeOptions->mDashLength);
> > +    for (FloatVector::iterator it = dash.begin(); it != dash.end(); ++it)
> > +      *it /= lineWidth;
> 
> nit: { }

Fixed.

> 
> @@ +1648,5 @@
> > +                                  capStyle, joinStyle,
> > +                                  aStrokeOptions.mMiterLimit,
> > +                                  D2D1_DASH_STYLE_CUSTOM,
> > +                                  aStrokeOptions.mDashOffset),
> > +      &dash[0], // VC's STL doesn't have vector<T>::data() yet
> 
> Should probably update the comment to say that MSVC has had it for a while,
> but not VS2005 which we run on the build servers :). We should update these
> when we update to VS2010 since &dash[0] I believe crashes for 0-size vectors
> whereas dash.data() will properly return NULL.

Oh, I didn't intend this comment to be picking on VC --- data() is non-standard, and coming in through C++0x.  The comment should say that.  (FWIW it's not in VC9 either, as I recall.)
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-29 12:53:19 PDT
(In reply to comment #28)
> We should update these
> when we update to VS2010 since &dash[0] I believe crashes for 0-size vectors
> whereas dash.data() will properly return NULL.

BTW, this isn't a problem here because the code only runs for non-empty patterns (one or more elements).  We would crash earlier if we tried to use it for an empty pattern, because aStrokeOptions.mDashPattern would be null.
Comment 32 Joe Drew (not getting mail) 2011-06-29 13:50:57 PDT
Comment on attachment 542711 [details] [diff] [review]
part 5: Implement .mozDash/mozDashOffset for Azure, v2

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

::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
@@ +683,5 @@
>                         lineWidth(1.0f),
>                         miterLimit(10.0f),
>                         globalAlpha(1.0f),
>                         shadowBlur(0.0),
> +                       dashOffset(0.f),

0.0f plzkthx

@@ +3129,5 @@
>          RefPtr<Path> path = scaledFont->GetPathForGlyphs(buffer, mCtx->mTarget);
>              
>          Matrix oldTransform = mCtx->mTarget->GetTransform();
>  
> +        const ContextState& state = *mState;

is mState the same thing as mCtx->CurrentState()?

@@ +3573,5 @@
> +  if (NS_SUCCEEDED(rv)) {
> +    ContextState& state = CurrentState();
> +    state.dash = dash;
> +    if (state.dash.IsEmpty()) {
> +      state.dashOffset = 0;

I don't know enough about the dashing API you've set up, but it's sort of weird to me that you can set dashing to an empty array.
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-29 14:07:06 PDT
(In reply to comment #32)
> Comment on attachment 542711 [details] [diff] [review] [review]
> part 5: Implement .mozDash/mozDashOffset for Azure, v2
> 
> Review of attachment 542711 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/nsCanvasRenderingContext2DAzure.cpp
> @@ +683,5 @@
> >                         lineWidth(1.0f),
> >                         miterLimit(10.0f),
> >                         globalAlpha(1.0f),
> >                         shadowBlur(0.0),
> > +                       dashOffset(0.f),
> 
> 0.0f plzkthx
> 

Sure.

> @@ +3129,5 @@
> >          RefPtr<Path> path = scaledFont->GetPathForGlyphs(buffer, mCtx->mTarget);
> >              
> >          Matrix oldTransform = mCtx->mTarget->GetTransform();
> >  
> > +        const ContextState& state = *mState;
> 
> is mState the same thing as mCtx->CurrentState()?

Yes.  I don't know why the additional member exists.
 
> @@ +3573,5 @@
> > +  if (NS_SUCCEEDED(rv)) {
> > +    ContextState& state = CurrentState();
> > +    state.dash = dash;
> > +    if (state.dash.IsEmpty()) {
> > +      state.dashOffset = 0;
> 
> I don't know enough about the dashing API you've set up, but it's sort of
> weird to me that you can set dashing to an empty array.

Empty |dash| array in the canvas state means "dash disabled".  I don't know of a cleaner way to represent that.
Comment 35 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-29 14:37:48 PDT
Jeff/Joe/Philip/Vlad/et al.: How should we go about trying to spec this?
Comment 36 Jeff Muizelaar [:jrmuizel] 2011-06-29 19:08:26 PDT
(In reply to comment #35)
> Jeff/Joe/Philip/Vlad/et al.: How should we go about trying to spec this?

I'd suggest something similar to what SVG does.
Comment 38 Bas Schouten (:bas.schouten) 2011-07-01 04:37:20 PDT
(In reply to comment #30)
> (In reply to comment #28)
> > Comment on attachment 542710 [details] [diff] [review] [review] [review]
> > 
> > @@ +1632,5 @@
> > >  
> > >  
> > > +  HRESULT hr;
> > > +  if (aStrokeOptions.mDashPattern) {
> > > +    typedef vector<Float> FloatVector;
> > 
> > Really? To save two characters :)? I'm fine with leaving this in though.
> 
> No, it's to save |for (typename vector<Float>::iterator it; ...)| ;).

Hrm? |for (vector<Float>::iterator it; ...)| should work just fine right? (I still don't really care, just curious :))
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-01 13:16:51 PDT
Yes, that works in this case.
Comment 40 AndreiD[QA] 2011-07-07 07:08:38 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a2) Gecko/20110706 Firefox/7.0a2
Verified fixed; detailed by the changesets (comment 37)
Comment 41 Andreas Røsdal 2011-07-22 11:22:07 PDT
Thanks for implementing dashed lines. I would like to use this
dashed line implementation for www.freeciv.net, could I please
get an example about how to use this new dashed line function?
Also, have you talked to the Chrome developers about adding
this functionality there also?

Thanks!
Comment 42 Hixie (not reading bugmail) 2011-07-25 11:38:06 PDT
In case this hasn't been mentioned on the WHATWG list it would be good to post a mail briefly documenting these extensions so that I can make sure the spec doesn't deviate from it.
Comment 43 :Ms2ger 2011-10-07 05:56:53 PDT
Doesn't look like anyone ever bothered even acting like they tried to get this specced. Someone filed <http://www.w3.org/Bugs/Public/show_bug.cgi?id=14091>, though.
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-07 10:53:26 PDT
Oh, thanks for the reminder.
Comment 46 Hixie (not reading bugmail) 2012-03-28 14:48:36 PDT
HTML spec has been updated with an API for this.
Comment 47 j.j. 2012-03-28 15:27:00 PDT
Adding "dev-doc-needed" to get spec links into docs and to remove "Gecko specific attributes"
Comment 48 Hixie (not reading bugmail) 2012-03-28 16:34:38 PDT
Note that the API in the spec is (as requested :-) ) slightly different than what Moz does now, if I'm not mistaken.
Comment 49 Florian Scholz [:fscholz] (MDN) 2012-06-30 07:57:12 PDT
Standardized attributes/methods added to
https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D

I left "Gecko/WebKit-specific attributes" for now but added a note about unprefixing:
https://developer.mozilla.org/en/DOM/CanvasRenderingContext2D#Gecko-specific_attributes
(more doc updates will happen once bug 740284 is fixed)

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