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 [inactive from 2016-12-01] [:eflores] [:edwin]
:
:
Mentors:
Depends on: 710521
Blocks: 719286 725918
  Show dependency treegraph
 
Reported: 2012-01-18 17:10 PST by Edwin Flores [inactive from 2016-12-01] [: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 [inactive from 2016-12-01] [:eflores] [:edwin]
no flags Details | Diff | Splinter Review
Patch rev 2 (35.77 KB, patch)
2012-01-30 14:32 PST, Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]
no flags Details | Diff | Splinter Review
Patch rev 2 (35.74 KB, patch)
2012-01-30 14:38 PST, Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]
roc: review+
Details | Diff | Splinter Review

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