Closed
Bug 619516
Opened 14 years ago
Closed 14 years ago
Rename nsSVGPreserveAspectRatio to SVGAnimatedPreserveAspectRatio & split out its inner helper-class
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla2.0b9
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
Attachments
(5 files, 3 obsolete files)
9.80 KB,
patch
|
jwatt
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
23.36 KB,
patch
|
jwatt
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
21.32 KB,
patch
|
Details | Diff | Splinter Review | |
29.06 KB,
patch
|
dholbert
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
4.09 KB,
patch
|
jwatt
:
review+
roc
:
approval2.0+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #497949 -
Flags: review?(jwatt)
Assignee | ||
Comment 2•14 years ago
|
||
(Note: patch 1 includes a nsSVGPreserveAspectRatio typedef in a few places so that it'll still compile. That gets removed in the next patch.)
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #497951 -
Flags: review?(jwatt)
Assignee | ||
Comment 4•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
This updates nsSVGUtils::GetViewBoxTransform take an SVGPreserveAspectRatio value instead of the animated version. (This is useful for bug 272288.)
Attachment #497953 -
Flags: review?(jwatt)
Assignee | ||
Updated•14 years ago
|
Attachment #497952 -
Flags: review?(jwatt)
Assignee | ||
Comment 6•14 years ago
|
||
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)
Assignee | ||
Comment 7•14 years ago
|
||
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)
Assignee | ||
Updated•14 years ago
|
Attachment #497949 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #497966 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 8•14 years ago
|
||
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 9•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #497952 -
Flags: review?(jwatt) → review+
Comment 10•14 years ago
|
||
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-
Assignee | ||
Updated•14 years ago
|
Attachment #497974 -
Attachment description: patch 1 v2, diff -w version → (patch 1 v2, diff -w version)
Assignee | ||
Comment 11•14 years ago
|
||
(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+
Assignee | ||
Comment 12•14 years ago
|
||
(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)
Updated•14 years ago
|
Attachment #498026 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 13•14 years ago
|
||
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?
Assignee | ||
Updated•14 years ago
|
Attachment #498024 -
Flags: approval2.0?
Assignee | ||
Updated•14 years ago
|
Attachment #497952 -
Flags: approval2.0?
Assignee | ||
Updated•14 years ago
|
Attachment #498026 -
Flags: approval2.0?
Comment 14•14 years ago
|
||
You're in the path of a blocker, so I don't think you need approval.
Attachment #498024 -
Flags: approval2.0? → approval2.0+
Attachment #498026 -
Flags: approval2.0? → approval2.0+
Attachment #497952 -
Flags: approval2.0? → approval2.0+
Attachment #497966 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 15•14 years ago
|
||
(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)).
Comment 16•14 years ago
|
||
Ah, right.
Assignee | ||
Updated•14 years ago
|
Summary: Rename nsSVGPreserveAspectRatio to SVGAnimatedPreserveAspectRatio → Rename nsSVGPreserveAspectRatio to SVGAnimatedPreserveAspectRatio & split out its inner helper-class
Assignee | ||
Comment 17•14 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/5ce6da519870 http://hg.mozilla.org/mozilla-central/rev/9235ac6b6053 http://hg.mozilla.org/mozilla-central/rev/3480ad2e571f http://hg.mozilla.org/mozilla-central/rev/be1f7ea06a5e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Target Milestone: --- → mozilla2.0b9
You need to log in
before you can comment on or make changes to this bug.
Description
•