Closed Bug 1073888 Opened 5 years ago Closed 5 years ago

Stop setting state on the gfxContext under SVGTextFrame::SetupCairoState

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jwatt, Assigned: jwatt)

Details

(Keywords: perf)

Attachments

(1 file)

The conversion to Moz2D requires that we set state at the points in the code where we paint (call gfxContext::Fill(), etc.) since DrawTargets require the state to be passed to their painting functions. The place that is causing me the most headaches in making headway on this is SVGTextFrame.

Turns out that SVGTextFrame::SetupCairoState and the functions that it calls (the functions defined below it in the .cpp file) do not need to be setting anything on gfxContext. They are simply setting up the SVGTextContextPaint that SetupCairoState creates and returns via its out-param.

The upcoming patch inlines a bunch of code that is called by these functions, dropping the parts that do things like gfxContext::SetPattern(). These functions could do with a lot more work, but for now this lets me progress with the Moz2D conversion which is all I have time for.
Attached patch patchSplinter Review
Attachment #8496500 - Flags: review?(cam)
Comment on attachment 8496500 [details] [diff] [review]
patch

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

::: layout/svg/SVGTextFrame.cpp
@@ -5651,5 @@
>    const nsStyleSVG *style = aFrame->StyleSVG();
>    nsSVGPaintServerFrame *ps =
>      nsSVGEffects::GetPaintServer(aFrame, &(style->*aFillOrStroke), aProperty);
>  
> -  if (ps && ps->SetupPaintServer(aContext, aFrame, aFillOrStroke, aOpacity)) {

We're losing the CacheColorStops call from inside SetupPaintServer.  Does this matter?

r=me if this doesn't matter, or if it does and you add a call to it up in PaintSVG.
Attachment #8496500 - Flags: review?(cam) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/800de79a433a

(In reply to Cameron McCormack (:heycam) (away October) from comment #2)
> We're losing the CacheColorStops call from inside SetupPaintServer.  Does
> this matter?

Well spotted. That wasn't intentional, but I think that's fine. Bug 1073524 should decide when we cache them.

(In reply to Jonathan Watt [:jwatt] from comment #0)
> These functions could do with a lot more work, but for now this lets me
> progress with the Moz2D conversion which is all I have time for.

Actually I felt that I should at least rename SetupCairoState to something much closer to what it does, so I renamed it to SetupContextPaint. Rather than also renaming SetupCairoFill/Stroke I simply inlined them, and SetupInheritablePaint I made a static function in the .cpp file rather than a method.

I also made a functional change - since I noticed there's no point in allocating the SVGTextContextPaint and passing it out of SetupCairoState I changed the code to create it on the stack and pass it in.

I assume these changes are all fine by you, but let me know if not.

Thanks for the review!
(In reply to Jonathan Watt [:jwatt] from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/800de79a433a
> 
> (In reply to Cameron McCormack (:heycam) (away October) from comment #2)
> > We're losing the CacheColorStops call from inside SetupPaintServer.  Does
> > this matter?
> 
> Well spotted. That wasn't intentional, but I think that's fine. Bug 1073524
> should decide when we cache them.

OK cool.

> (In reply to Jonathan Watt [:jwatt] from comment #0)
> > These functions could do with a lot more work, but for now this lets me
> > progress with the Moz2D conversion which is all I have time for.
> 
> Actually I felt that I should at least rename SetupCairoState to something
> much closer to what it does, so I renamed it to SetupContextPaint. Rather
> than also renaming SetupCairoFill/Stroke I simply inlined them, and
> SetupInheritablePaint I made a static function in the .cpp file rather than
> a method.

OK, great.  I was going to suggest renaming SetupCairoState, but then saw the other similarly named SetupCairoFill/Stroke and decided not to mention it. :-)

> I also made a functional change - since I noticed there's no point in
> allocating the SVGTextContextPaint and passing it out of SetupCairoState I
> changed the code to create it on the stack and pass it in.
> 
> I assume these changes are all fine by you, but let me know if not.

Sounds fine.
Adding perf keyword since this makes us do less work too.
Keywords: perf
https://hg.mozilla.org/mozilla-central/rev/800de79a433a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.