Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Draw filled and stroked SVG text in a single Draw call

RESOLVED FIXED in mozilla13

Status

()

Core
Layout: Text
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: eflores, Assigned: eflores)

Tracking

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Makes more sense in the context of bug 719286
Created attachment 589710 [details] [diff] [review]
Patch
Attachment #589710 - Flags: review?(roc)
Blocks: 719286
Depends on: 710521
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.
Created attachment 592866 [details] [diff] [review]
Patch rev 2
Attachment #589710 - Attachment is obsolete: true
Attachment #589710 - Flags: review?(roc)
Attachment #592866 - Flags: review?(roc)
Created attachment 592867 [details] [diff] [review]
Patch rev 2
Attachment #592867 - Flags: review?(roc)
Which patch should I review? :-)
Attachment #592866 - Attachment is obsolete: true
Attachment #592866 - Flags: review?(roc)
Attachment #592867 - Flags: review?(roc) → review+
Keywords: checkin-needed

Updated

6 years ago
Whiteboard: [autoland-try]

Updated

6 years ago
Whiteboard: [autoland-try] → [autoland-in-queue]

Comment 6

6 years ago
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

6 years ago
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.

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dec46c6439c
Keywords: checkin-needed
Whiteboard: [autoland-in-queue]
Target Milestone: --- → mozilla13

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/8dec46c6439c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 10

6 years ago
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?

Comment 12

6 years ago
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.)

Updated

6 years ago
Blocks: 725918
You need to log in before you can comment on or make changes to this bug.