Closed Bug 567848 Opened 14 years ago Closed 12 years ago

Split up nsSVGUtils into more granular utility classes

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: dholbert, Assigned: longsonr)

Details

Attachments

(1 file)

Currently nsSVGUtils strikes me as a grab-bag of SVG utility code that's been shoehorned into one file in /layout/, when a lot of it isn't layout-dependent at all.  This is messy, & can cause problems.  For example, in bug 276431 comment 85, longsonr suggests I call an nsSVGUtils method from inside imglib -- but I can't do that right now, because nsSVGUtils lives in gklayout, and imglib doesn't (and shouldn't) depend on gklayout.

I'm filing this bug on spinning off chunks of nsSVGUtils into library-specific chunks.  Perhaps something like:
    layout/svg/base/src/nsSVGLayoutUtils.[cpp|h]
    content/svg/content/src/nsSVGContentUtils.[cpp|h]
    gfx/src/thebes/utils/gfxSVGUtils.[cpp|h]
(not sure about that last one -- we don't have any SVG-specific directories inside of gfx, AFAIK)
Summary: Split up nsSVGUtils into more granular util-classes → Split up nsSVGUtils into more granular utility classes
Blocks: 276431
Hold on.  Why is imgContainerSVG in imglib?  Wouldn't it be simpler to put it in gklayout to start with?  imglib just instantiates it by contract, no?  I thought I'd mentioned that at some point....
In particular, messing with document viewers directly from outside gklayout is probably counterindicated.
(In reply to comment #1)
> Hold on.  Why is imgContainerSVG in imglib?  Wouldn't it be simpler to put it
> in gklayout to start with?

Ah -- yes, that's doable.  You probably did mention it, and I just missed that / misunderstood.

So, that use case for this bug is invalid (and so this is lower-priority). Nonetheless, I think the bug itself is still valid -- nsSVGUtils could be stand to be refactored into more sensible utils classes. (at least into /layout/ vs. /content/ bits -- maybe the /gfx/ chunks don't need to get split out.)
Yeah, splitting stuff out might still be a good idea.
No longer blocks: 276431
Attached patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #661589 - Flags: review?(dholbert)
Flags: in-testsuite-
OS: Linux → All
Hardware: x86 → All
Myabe ObjectSpace and UserSpace could go to SVGContentUtils. Their implementation is content but their callers are layout. I left them where they were in the end so as not to have to change all the layout callers.
Comment on attachment 661589 [details] [diff] [review]
patch

Firstly: it looks like nsSVGUtils.cpp was tweaked yesterday, so you'll need a (minor) bitrot-fix to account for that.  (That's my fault from review-lag -- sorry for that)   The tweak was: https://hg.mozilla.org/mozilla-central/diff/e8e05a209476/layout/svg/base/src/nsSVGUtils.cpp

Secondly: Could you include a general comment in both XYZUtils header-files (SVGContentUtils.h and nsSVGUtils.h) briefly explaining their purpose?  (in particular, what differentiates them?)  When someone adds a new SVG utility method in the future, it'd be helpful to have a documented rule-of-thumb to help determine which SVG*Utils class it should go in. (It looks like ContentUtils mostly deals with Element, nsIContent, etc. and nsSVGUtils mostly deals with nsIFrames. And, it looks like nsSVGUtils is allowed to call SVGContentUtils methods, but not vice-versa.  (I might be misreading that)  Assuming those are the design principals, it'd be nice to document them.)

>+++ b/content/svg/content/src/DOMSVGMatrix.cpp
>-const double radPerDegree = 2.0*3.1415926535 / 360.0;
>+const double radPerDegree = 2.0 * M_PI / 360.0;
[...]
>+++ b/content/svg/content/src/SVGTransform.cpp
> namespace {
>-  const double radPerDegree = 2.0*3.1415926535 / 360.0;
>+  const double radPerDegree = 2.0 * M_PI / 360.0;

We shouldn't declare/define two copies of radPerDegree -- we should just consolidate these into a single variable.  I think it could be a public static class-variable on SVGTransform, since DOMSVGMatrix (indirectly) includes SVGTransform.h and would still be able to see it.  Or it could live in SVGContentUtils.

That could happen here, or I'm also happy for that consolidation to be a followup-bug.

>+++ b/content/svg/content/src/nsSVGLength2.cpp
>@@ -178,20 +178,20 @@ nsSVGLength2::GetAxisLength(nsSVGSVGElem
> 
> float
> nsSVGLength2::GetAxisLength(nsIFrame *aNonSVGFrame) const
> {
[...]
>   switch (mCtxType) {
>-  case nsSVGUtils::X: length = size.width; break;
>-  case nsSVGUtils::Y: length = size.height; break;
>-  case nsSVGUtils::XY:
>-    length = nsSVGUtils::ComputeNormalizedHypotenuse(size.width, size.height);
>+  case SVGContentUtils::X: length = size.width; break;
>+  case SVGContentUtils::Y: length = size.height; break;
>+  case SVGContentUtils::XY:
>+    length = SVGContentUtils::ComputeNormalizedHypotenuse(size.width, size.height);
>     break;
>

Hmm... The formatting for the "X" and "Y" cases there is a bit ugly.  Could you fix those up while you're touching this? (just add some newlines/whitespace, to match the "XY" case's formatting)

>--- a/layout/svg/base/src/nsSVGUtils.h
>+++ b/content/svg/content/src/SVGContentUtils.h
>@@ -1,233 +1,63 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>-#ifndef NS_SVGUTILS_H
>-#define NS_SVGUTILS_H
>+#ifndef SVGCONTENTUTILS_H
>+#define SVGCONTENTUTILS_H

I think most of our "SVGWhatever.h" headers use a MOZILLA_ prefix in their ifndef/define -- e.g. "#define MOZILLA_SVGWHATEVER_H".  Probably better to stick with that pattern, for consistency & higher-likelihood-of-being-unique.

> // include math.h to pick up definition of M_SQRT1_2 if the platform defines it
> #define _USE_MATH_DEFINES
> #include <math.h>

Looks like SVGContentUtils.h doesn't actually need these lines ^.  (nsSVGUtils.cpp still has a M_SQRT1_2 usage, but SVGContentUtils does not.)

> #define SVG_WSP_DELIM       "\x20\x9\xD\xA"
> #define SVG_COMMA_WSP_DELIM "," SVG_WSP_DELIM

These #defines exist in both nsSVGUtils.h and SVGContentUtils.h, but it looks like they're unused & hence should be removed entirely (from both Utils files)
MXR reference:
http://mxr.mozilla.org/mozilla-central/search?string=SVG_.*WSP_DELIM&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

>   /*
>    * Get the parent element of an nsIContent
>    */
>   static mozilla::dom::Element *GetParentElement(nsIContent *aContent);

This also appears to be unused and undefined in current m-c.  The current patch moves this decl to SVGContentUtils.h, but we should just delete it.

>diff --git a/layout/svg/base/src/nsSVGUtils.cpp b/content/svg/content/src/SVGContentUtils.cpp
>copy from layout/svg/base/src/nsSVGUtils.cpp
>copy to content/svg/content/src/SVGContentUtils.cpp
>--- a/layout/svg/base/src/nsSVGUtils.cpp
>+++ b/content/svg/content/src/SVGContentUtils.cpp
[...]
> // Main header first:
> // This is also necessary to ensure our definition of M_SQRT1_2 is picked up
>-#include "nsSVGUtils.h"
>+#include "SVGContentUtils.h"

Drop the second line of that comment, about M_SQRT1_2. (It's not used in SVGContentUtils, as noted above.)

>-nsSVGUtils::ReportToConsole(nsIDocument* doc,
>+SVGContentUtils::ReportToConsole(nsIDocument* doc,
>                             const char* aWarning,
>                             const PRUnichar **aParams,
>                             uint32_t aParamsLength)

Looks like this method-definition needs some indentation-fixup love. It's still indented as if it were a nsSVGUtils method.
(This applies to both of the GetViewBoxTransform() implementations, too, at the end of this file.)

>diff --git a/layout/svg/base/src/nsSVGUtils.h b/layout/svg/base/src/nsSVGUtils.h
>--- a/layout/svg/base/src/nsSVGUtils.h
>+++ b/layout/svg/base/src/nsSVGUtils.h
[...]
>   static gfxMatrix GetCTM(nsSVGElement *aElement, bool aScreenCTM);

This method strikes me as something "content-ish" that should live in SVGContentUtils, since
 (a) it takes an element, not a frame
 (b) Its callers are essentially all in /content/svg

There sole non-content caller is another method in nsSVGUtils.cpp itself, and that caller should still be able to call this method just fine, since nsSVGUtils.cpp has an #include for SVGContentUtils.h.

So -- I think we should move GetCTM() from nsSVGUtils to SVGContentUtils.

>   /* Generate a viewbox to viewport tranformation matrix */
> 
>-  static gfxMatrix
>-  GetViewBoxTransform(const nsSVGElement* aElement,
>-                      float aViewportWidth, float aViewportHeight,

Looks like this method is moving out of nsSVGUtils, but its documentation-comment was left behind ("Generate a viewbox...") in nsSVGUtils.h.  Nix the orphaned comment.

ALSO: Your patch doesn't touch the method "nsSVGUtils::WritePPM" right now, but that method appears to be completely unused:
  http://mxr.mozilla.org/mozilla-central/search?string=WritePPM
As long as we're refactoring this class, we might as well delete that method.

r=me with the above
Attachment #661589 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #7)
> 
> That could happen here, or I'm also happy for that consolidation to be a
> followup-bug.

Left for follow up.

> > // include math.h to pick up definition of M_SQRT1_2 if the platform defines it
> > #define _USE_MATH_DEFINES
> > #include <math.h>
> 
> Looks like SVGContentUtils.h doesn't actually need these lines ^. 
> (nsSVGUtils.cpp still has a M_SQRT1_2 usage, but SVGContentUtils does not.)

It's all M_ defines including M_PI which is everywhere. I changed the comment but left the include.
https://hg.mozilla.org/mozilla-central/rev/8d077e0638d4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: