955917 - Remove unused functions from nsSVGUtils

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: utils.txt

Comment 1

[Daniel Holbert [:dholbert]]

It looks like nsSVGUtils::SetupContextPaint (made into a private-to-nsSVGUtils.cpp function in this patch) is actually near-identical to SVGTextFrame::SetupContextPaint().

Seems like maybe we should keep the nsSVGUtils method public, and (perhaps in a separate bug) make SVGTextFrame share its code rather than reimplementing it.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8355071 [details] [diff] [review]
utils.txt

Definitely r=me on the rest, though (the non-SetupContextPaint-related, strictly-code-removal parts.

Let's hold off on the SetupContextPaint tweak for now, since per above, I think there may be reasons to keep that function public.

Comment 3

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2343e90583b0

Comment 4

[Robert Longson [:longsonr]]

Attachment: part 2

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8355196 [details] [diff] [review]
part 2

>diff --git a/layout/svg/SVGTextFrame.cpp b/layout/svg/SVGTextFrame.cpp
>--- a/layout/svg/SVGTextFrame.cpp
>+++ b/layout/svg/SVGTextFrame.cpp
>@@ -5582,56 +5582,26 @@ SVGTextFrame::SetupInheritablePaint(gfxC
>                                     const FramePropertyDescriptor* aProperty)
> {
>   const nsStyleSVG *style = aFrame->StyleSVG();
>   nsSVGPaintServerFrame *ps =
>     nsSVGEffects::GetPaintServer(aFrame, &(style->*aFillOrStroke), aProperty);
> 
>   if (ps && ps->SetupPaintServer(aContext, aFrame, aFillOrStroke, aOpacity)) {
>     aTargetPaint.SetPaintServer(aFrame, aContext->CurrentMatrix(), ps);
>-  } else if (SetupContextPaint(aContext, aFrame, aFillOrStroke, aOpacity, aOuterContextPaint)) {
>+  } else if (nsSVGUtils::SetupContextPaint(aContext, aOuterContextPaint,
>+                                           aFrame->StyleSVG()->*aFillOrStroke,

We already looked up StyleSVG() above, so this last line can just be:
 style->*aFillOrStroke

r=me with that

Optional: I'd marginally prefer to have this part split this out from the second half of this patch (dropping PathExtentsToMaxStrokeExtents), because a function-swap is functionally different from unused-code-removal. (It's also conceivable that we'd want to back out the first piece, e.g. if the SetupContextPaint functions were subtly different in a way that we didn't notice; in that case, we wouldn't want to back out the second piece).

If you do that, r=me on both pieces.

Comment 6

[Robert Longson [:longsonr]]

Split out into another bug or split out into another patch in this bug?

Comment 7

[Daniel Holbert [:dholbert]]

I just meant a separate patch (in this bug is fine).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2343e90583b0

Comment 9

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/247d083cf4dd

Comment 10

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=32f4505415be

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/247d083cf4dd

Comment 12

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df7e0f8fa993

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df7e0f8fa993