Last Comment Bug 719288 - Draw filled and stroked SVG text in a single Draw call
: Draw filled and stroked SVG text in a single Draw call
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla13
Assigned To: Edwin Flores [:eflores] [:edwin]
:
Mentors:
Depends on: 710521
Blocks: 719286 725918
  Show dependency treegraph
 
Reported: 2012-01-18 17:10 PST by Edwin Flores [:eflores] [:edwin]
Modified: 2012-02-10 04:52 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (35.36 KB, patch)
2012-01-18 17:12 PST, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Splinter Review
Patch rev 2 (35.77 KB, patch)
2012-01-30 14:32 PST, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Splinter Review
Patch rev 2 (35.74 KB, patch)
2012-01-30 14:38 PST, Edwin Flores [:eflores] [:edwin]
roc: review+
Details | Diff | Splinter Review

Description Edwin Flores [:eflores] [:edwin] 2012-01-18 17:10:12 PST
Makes more sense in the context of bug 719286
Comment 1 Edwin Flores [:eflores] [:edwin] 2012-01-18 17:12:01 PST
Created attachment 589710 [details] [diff] [review]
Patch
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-18 19:34:21 PST
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 [:eflores] [:edwin] 2012-01-30 14:32:00 PST
Created attachment 592866 [details] [diff] [review]
Patch rev 2
Comment 4 Edwin Flores [:eflores] [:edwin] 2012-01-30 14:38:00 PST
Created attachment 592867 [details] [diff] [review]
Patch rev 2
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-30 14:41:00 PST
Which patch should I review? :-)
Comment 6 Mozilla RelEng Bot 2012-02-03 15:06:35 PST
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 2012-02-03 21:45:19 PST
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 9 Ed Morley [:emorley] 2012-02-05 04:11:54 PST
https://hg.mozilla.org/mozilla-central/rev/8dec46c6439c
Comment 10 Robert Longson 2012-02-05 08:23:37 PST
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) (Exited; email my personal email if necessary) 2012-02-05 13:47:39 PST
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 2012-02-05 15:14:20 PST
Not really, no. It's just that nsSVGUtils is rather a dumping ground.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-05 15:25:47 PST
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.)

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