719288 - Draw filled and stroked SVG text in a single Draw call

Status: RESOLVED FIXED

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

Description

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Makes more sense in the context of bug 719286

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch rev 2

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Patch rev 2

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Which patch should I review? :-)

Comment 6

[Mozilla RelEng Bot]

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

Comment 7

[Mozilla RelEng Bot]

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/[e-mail]-319f852447b5
 Timed out after 06 hours without completing.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dec46c6439c

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8dec46c6439c

Comment 10

[Robert Longson [:longsonr]]

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?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I suppose we could have added it as a static method to nsSVGGeometryFrame, but it doesn't matter much, does it?

Comment 12

[Robert Longson [:longsonr]]

Not really, no. It's just that nsSVGUtils is rather a dumping ground.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.)