1378538 - DrawTargetCapture doesn't store StrokeOption's Dash Information

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Mason Chang [Inactive] [:mchang]]

Attachment: Copy Dash data when recording StrokeLines

With DrawTargetCapture, we were getting some random artifacts with StrokeLine(). This is because StrokeOptions has a dash pattern array that is owned by the caller [1]. DrawTargetCapture::StrokeLine was just copying the pointer, which was getting freed before we could replay the data. DrawTargetRecording doesn't have this problem because it copies all the data. Other methods such as DrawTargetCapture::StrokeRect, which also take in a StrokeOptions already copies the dash information[2]. Let's do the same thing for StrokeLines(). I already verified that all other moz 2d api calls that take a StrokeOption copy the dash information.

[1] http://searchfox.org/mozilla-central/source/gfx/2d/2D.h#160
[2] http://searchfox.org/mozilla-central/source/gfx/2d/DrawCommand.h#267

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8883731 [details] [diff] [review]
Copy Dash data when recording StrokeLines

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

::: gfx/2d/DrawCommand.h
@@ +304,5 @@
> +    // between now and replay.
> +    if (aStrokeOptions.mDashLength) {
> +      mDashes.resize(aStrokeOptions.mDashLength);
> +      mStrokeOptions.mDashPattern = &mDashes.front();
> +      memcpy(&mDashes.front(), aStrokeOptions.mDashPattern, mStrokeOptions.mDashLength * sizeof(Float));

You could also do this in one line with something like:
mDashes.insert(mDashes.end(), aStrokeOptions.mDashPattern, aStrokeOptions.mDashPattern + aStrokeOptions.mDashLength);

Comment 2

[Mason Chang [Inactive] [:mchang]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Comment on attachment 8883731 [details] [diff] [review]
> Copy Dash data when recording StrokeLines
> 
> Review of attachment 8883731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/DrawCommand.h
> @@ +304,5 @@
> > +    // between now and replay.
> > +    if (aStrokeOptions.mDashLength) {
> > +      mDashes.resize(aStrokeOptions.mDashLength);
> > +      mStrokeOptions.mDashPattern = &mDashes.front();
> > +      memcpy(&mDashes.front(), aStrokeOptions.mDashPattern, mStrokeOptions.mDashLength * sizeof(Float));
> 
> You could also do this in one line with something like:
> mDashes.insert(mDashes.end(), aStrokeOptions.mDashPattern,
> aStrokeOptions.mDashPattern + aStrokeOptions.mDashLength);

The rest of DrawCommand::Stroke* are like this, so I figured I'd copy the current style.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42a708048bf7
Copy Dash data when recording StrokeLines in DrawTargetCapture. r=jrmuizel

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42a708048bf7