Closed Bug 719288 Opened 12 years ago Closed 12 years ago

Draw filled and stroked SVG text in a single Draw call

Categories

(Core :: Layout: Text and Fonts, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: eflores, Assigned: eflores)

References

Details

Attachments

(1 file, 2 obsolete files)

Makes more sense in the context of bug 719286
Attached patch Patch (obsolete) — Splinter Review
Attachment #589710 - Flags: review?(roc)
Comment on attachment 589710 [details] [diff] [review]
Patch

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

This looks pretty good. Just fix those nits.

::: layout/svg/base/src/nsSVGGlyphFrame.cpp
@@ +878,5 @@
>    return baselineAppUnits * aMetricsScale;
>  }
>  
> +gfxFont::DrawMode
> +nsSVGGlyphFrame::SetupCairoState(gfxContext* context, nsRefPtr<gfxPattern> &strokePattern) {

make this nsRefPtr<gfxPattern>* to make it clearer in the caller that this is an out-parameter.

@@ +886,5 @@
> +  if (HasStroke()) {
> +    gfxContextMatrixAutoSaveRestore matrixRestore(context);
> +    context->IdentityMatrix();
> +
> +    toDraw = (gfxFont::DrawMode)(toDraw | gfxFont::GLYPH_STROKE);

Use a constructor style cast here and elsewhere. It's a little more readable. DrawMode(toDraw | gfxFont::GLYPH_STROKE).

Introduce "typedef gfxFont::DrawMode DrawMode;" on nsSVGGlyphFrame to avoid having to prefix stuff.

@@ +889,5 @@
> +
> +    toDraw = (gfxFont::DrawMode)(toDraw | gfxFont::GLYPH_STROKE);
> +
> +    SetupCairoStrokeHitGeometry(context);
> +    float opacity = MaybeOptimizeOpacity(style->mStrokeOpacity);

nsSVGUtils::CanOptimizeOpacity always returns false for glyph frames, so you can just use mStrokeOpacity without calling MaybeOptimizeOpacity.

::: layout/svg/base/src/nsSVGUtils.cpp
@@ +1525,5 @@
> +
> +/* static */ void
> +nsSVGUtils::GetFallbackOrPaintColor(gfxContext *aContext, nsStyleContext *aStyleContext,
> +                                    nsStyleSVGPaint nsStyleSVG::*aFillOrStroke,
> +                                    float &aOpacity, nscolor &color)

Make these out-parameters pointers so we can see in the caller that they are.
Attached patch Patch rev 2 (obsolete) — Splinter Review
Attachment #589710 - Attachment is obsolete: true
Attachment #589710 - Flags: review?(roc)
Attachment #592866 - Flags: review?(roc)
Which patch should I review? :-)
Attachment #592866 - Attachment is obsolete: true
Attachment #592866 - Flags: review?(roc)
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
Autoland Patchset:
	Patches: 592867
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/319f852447b5
Try run started, revision 319f852447b5. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=319f852447b5
Try run for 319f852447b5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=319f852447b5
Results (out of 207 total builds):
    exception: 1
    success: 183
    warnings: 23
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-319f852447b5
 Timed out after 06 hours without completing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dec46c6439c
Keywords: checkin-needed
Whiteboard: [autoland-in-queue]
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/8dec46c6439c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Are you planning to use GetFallbackOrPaintColor outside of nsSVGGeometryFrame or its subclasses in some other bug? Is that why you moved the code into nsSVGUtils?
I suppose we could have added it as a static method to nsSVGGeometryFrame, but it doesn't matter much, does it?
Not really, no. It's just that nsSVGUtils is rather a dumping ground.
All the Utils classes are dumping grounds. That's what they're for :-).

(IMHO a "dumping ground" is better than the alternatives of a) duplicating code or b) putting helper functions into some place that happens to be accessible to the N current users, but probably won't make sense for the N+1'th user.)
Blocks: 725918
You need to log in before you can comment on or make changes to this bug.