Closed Bug 619516 Opened 9 years ago Closed 9 years ago

Rename nsSVGPreserveAspectRatio to SVGAnimatedPreserveAspectRatio & split out its inner helper-class

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(5 files, 3 obsolete files)

Per discussion with jwatt earlier today, we want the following class renames:
 nsSVGPreserveAspectRatio  --> SVGAnimatedPreserveAspectRatio
 nsSVGPreserveAspectRatio::PreserveAspectRatio --> SVGPreserveAspectRatio

This will simplify the patch for bug 272288.
(There'll be less need to pack PRUint64 values -- we can just pass around "SVGPreserveAspectRatio" values instead.  We can't easily do the equivalent of that right now, because C++ prevents us from forward-declaring an inner class like "nsSVGPreserveAspectRatio::PreserveAspectRatio" in header files.)
Attached patch patch 1: Rename classes (obsolete) — Splinter Review
Attachment #497949 - Flags: review?(jwatt)
(Note: patch 1 includes a nsSVGPreserveAspectRatio typedef in a few places so that it'll still compile. That gets removed in the next patch.)
Attachment #497951 - Flags: review?(jwatt)
This updates nsSVGUtils::GetViewBoxTransform take an SVGPreserveAspectRatio value instead of the animated version.  (This is useful for bug 272288.)
Attachment #497953 - Flags: review?(jwatt)
Attachment #497952 - Flags: review?(jwatt)
Comment on attachment 497949 [details] [diff] [review]
patch 1: Rename classes

(ah, forgot to update some indentation in patch 1 - canceling review on that one for now)
Attachment #497949 - Flags: review?(jwatt)
Arright, here's patch 1 again with function signatures lined up better & wrapped to be less than 80 chars where necessary.
Attachment #497966 - Flags: review?(jwatt)
Attachment #497949 - Attachment is obsolete: true
Attachment #497966 - Flags: review?(jwatt) → review+
Here's the "diff -w" version of patch 1 (might be more readable for the "nsSVGPreserveAspectRatio.h" changes, which shifts nsSVGPreserveAspectRatio::PreserveAspectRatio (aka SVGPreserveAspectRatio) away of being an inner class & tweaks the indentation of that entire class definition as a result.)
Comment on attachment 497951 [details] [diff] [review]
patch 2: propagate class-rename to clients

>-  virtual nsSVGPreserveAspectRatio *GetPreserveAspectRatio();
>+  virtual mozilla::SVGAnimatedPreserveAspectRatio *GetPreserveAspectRatio();

Can you typedef SVGAnimatedPreserveAspectRatio in the declaration for nsSVGElement so that you can avoid the 'mozilla::' prefix here and in all other element code, please?


>diff --git a/content/svg/content/src/nsSVGImageElement.cpp b/content/svg/content/src/nsSVGImageElement.cpp
...
>+using namespace mozilla;
...
>-nsSVGPreserveAspectRatio *
>+mozilla::SVGAnimatedPreserveAspectRatio *

No need for the 'mozilla::' here.

>diff --git a/content/svg/content/src/nsSVGImageElement.h b/content/svg/content/src/nsSVGImageElement.h
...
>-  virtual nsSVGPreserveAspectRatio *GetPreserveAspectRatio();
>+  virtual mozilla::SVGAnimatedPreserveAspectRatio *GetPreserveAspectRatio();

'mozilla::' here. In fact can you search your patches to make sure these die after addressing the first comment above. I'll not mention these again in these reviews.
Attachment #497951 - Flags: review?(jwatt) → review+
Attachment #497952 - Flags: review?(jwatt) → review+
Comment on attachment 497953 [details] [diff] [review]
patch 4: tweak nsSVGUtils::GetViewBoxTransform to take un-animated type

r- as discussed. r+ on the next patch to add a second overloaded method.
Attachment #497953 - Flags: review?(jwatt) → review-
Attachment #497974 - Attachment description: patch 1 v2, diff -w version → (patch 1 v2, diff -w version)
(In reply to comment #9)
> >+  virtual mozilla::SVGAnimatedPreserveAspectRatio *GetPreserveAspectRatio();
> 
> Can you typedef SVGAnimatedPreserveAspectRatio in the declaration for
> nsSVGElement so that you can avoid the 'mozilla::' prefix here and in all other
> element code

Yup, done.

> In fact can you search your patches to make sure these die

Done -- the only remaining mozilla::'s are in typedefs.

Thanks!
Attachment #497951 - Attachment is obsolete: true
Attachment #498024 - Flags: review+
(In reply to comment #10)
> r- as discussed. r+ on the next patch to add a second overloaded method.

Turns out I can't use an *inline* overloaded method (or rather -- if I do make it inline, then it means every includer of nsSVGUtils.h complains unless I #include SVGAnimatedPreserveAspectRatio.h, since all those includers see a  block of code containing "aPreserveAspectRatio.GetAnimValue()" -- a SVGAnimatedPreserveAspectRatio method.)

So, this patch just uses an overloaded method, with both impls in the .cpp file.  Re-requesting review in case you have any issues with the not-inline aspect.
Attachment #497953 - Attachment is obsolete: true
Attachment #498026 - Flags: review?(jwatt)
Attachment #498026 - Flags: review?(jwatt) → review+
Comment on attachment 497966 [details] [diff] [review]
patch 1 v2: Rename classes (with indentation fixes)

Requesting approval.  No functional change - just reorganizing things to help bug 272288's patch be simpler (and bringing nsSVGPreserveAspectRatio into the our New Style svg code organization) -- in particular, this lets me just use a single GetViewBoxTransform() instead of two, call per the beginning of bug 272288 comment 66.
Attachment #497966 - Flags: approval2.0?
Attachment #498024 - Flags: approval2.0?
Attachment #497952 - Flags: approval2.0?
Attachment #498026 - Flags: approval2.0?
You're in the path of a blocker, so I don't think you need approval.
(In reply to comment #14)
> You're in the path of a blocker
(Not anymore -- as of today's layout retriage meeting, bug 272288 is not a blocker anymore (but is expected-to-be-approved)).
Ah, right.
Summary: Rename nsSVGPreserveAspectRatio to SVGAnimatedPreserveAspectRatio → Rename nsSVGPreserveAspectRatio to SVGAnimatedPreserveAspectRatio & split out its inner helper-class
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.