Closed Bug 1010067 Opened 10 years ago Closed 10 years ago

Rename animation classes so they are easier to understand

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(8 files)

101.72 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.56 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.40 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.87 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.49 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
45.19 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
47.72 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
18.02 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Based on discussion in bug 1004383, once the other refactoring for bug 999922 is complete we should do the following renaming:

  * nsStyleAnimation::Value -> mozilla::StyleAnimationValue including folding in static methods in nsStyleAnimation
  * nsAnimation -> mozilla::StyleAnimation
  * nsTransition -> mozilla::StyleTransition
  * ElementPropertyTransition -> mozilla::ElementTransition

There are a few other animation-related classes in layout/style that are in the global namespace and have no 'ns' prefix so we should probably move them to the mozilla namespace at the same time.
(In reply to Brian Birtles (:birtles) from comment #0)
>   * nsStyleAnimation::Value -> mozilla::StyleAnimationValue including
> folding in static methods in nsStyleAnimation
>   * nsAnimation -> mozilla::StyleAnimation
>   * nsTransition -> mozilla::StyleTransition
>   * ElementPropertyTransition -> mozilla::ElementTransition

And probably the following too:
  * CommonElementAnimationData -> mozilla::ElementAnimationCollection
  * Move ElementAnimationCollection to a separate file
  * Move ElementAnimation to a separate file
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Summary: Rename animation classes so they make sense → Rename animation classes so they are easier to understand
This patch also moves the static methods defined on nsStyleAnimation so that
they are part of StyleAnimationValue class.

Renaming nsStyleAnimation.h to StyleAnimationValue.h is performed in a separate
patch to ease reviewing (build changes requiring separate review) and simplify
the diff (since some tools may not handle file renames elegantly).
Attachment #8441088 - Flags: review?(dbaron)
Just the first part for now. I'll wait until bug 1026302 lands before doing the rest.
Comment on attachment 8441089 [details] [diff] [review]
part 2 - Rename nsStyleAnimation.{h,cpp} to StyleAnimationValue.{h,cpp}

Switching review request to glandium since this includes moz.build changes
Attachment #8441089 - Flags: review?(dbaron) → review?(mh+mozilla)
Comment on attachment 8441089 [details] [diff] [review]
part 2 - Rename nsStyleAnimation.{h,cpp} to StyleAnimationValue.{h,cpp}

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

This kind of simple change doesn't need build peer review.
Attachment #8441089 - Flags: review?(mh+mozilla) → review?(dbaron)
Comment on attachment 8441088 [details] [diff] [review]
part 1 - Rename nsStyleAnimation::Value to mozilla::StyleAnimationValue

>     NS_ABORT_IF_FALSE(aStyleAnimValue.GetUnit() ==
>-                        nsStyleAnimation::eUnit_Coord,
>+                      StyleAnimationValue::eUnit_Coord,
>                       "'font-size' value with unexpected style unit");

I prefer leaving the indentation as it was; it makes more sense to
indent that way once && or || are mixed in.

>-  private:
>-    Unit mUnit;
>-    union {
>+private:
>+  Unit mUnit;
>+  union {
>       int32_t mInt;
>       nscoord mCoord;
>       float mFloat;
>       nscolor mColor;
>       nsCSSValue* mCSSValue;
>       nsCSSValuePair* mCSSValuePair;
>       nsCSSValueTriplet* mCSSValueTriplet;
>       nsCSSRect* mCSSRect;
>       nsCSSValueList* mCSSValueList;
>       nsCSSValueSharedList* mCSSValueSharedList;
>       nsCSSValuePairList* mCSSValuePairList;
>       nsStringBuffer* mString;
>-    } mValue;
...
>+  } mValue;

Please reindent the contents of the union too.


(I wonder whether some of the transform stuff should be separated or moved
to nsStyleTransformMatrix, but I think it's fine to keep the way it is, at
least for now.)

r=dbaron
Attachment #8441088 - Flags: review?(dbaron) → review+
Comment on attachment 8441089 [details] [diff] [review]
part 2 - Rename nsStyleAnimation.{h,cpp} to StyleAnimationValue.{h,cpp}

r=dbaron
Attachment #8441089 - Flags: review?(dbaron) → review+
David, I plan to do the rest of this work soon (now if possible) but since it will bitrot quickly and possibly bitrot anything you are working on I'd like to get your OK first.

Specifically, the remaining work is:

* nsAnimation -> mozilla::StyleAnimation
* nsTransition -> mozilla::StyleTransition
* ElementPropertyTransition -> mozilla::ElementTransition
* CommonElementAnimationData -> mozilla::ElementAnimationCollection
* Move ElementAnimationCollection and ElementAnimation to a separate files (i.e. out of AnimationCommon)

Does this look ok to you? And is it ok to do it now or would you prefer I hold off?
Flags: needinfo?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #11)
> David, I plan to do the rest of this work soon (now if possible) but since
> it will bitrot quickly and possibly bitrot anything you are working on I'd
> like to get your OK first.
> 
> Specifically, the remaining work is:
> 
> * nsAnimation -> mozilla::StyleAnimation
> * nsTransition -> mozilla::StyleTransition

These are fine.

> * ElementPropertyTransition -> mozilla::ElementTransition

I sort of like this one as it is, although putting it in the mozilla namespace is fine.  I guess I'd be ok with changing it if you really want to, though.

> * CommonElementAnimationData -> mozilla::ElementAnimationCollection

This is fine.

> * Move ElementAnimationCollection and ElementAnimation to a separate files
> (i.e. out of AnimationCommon)

I'd prefer to leave it in the same file; I'd rather not scatter animations stuff across too many files, given that animations is only a small part of what's in the directory.  (It's also harder to merge with.)

> Does this look ok to you? And is it ok to do it now or would you prefer I
> hold off?

Now is fine.
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #12)
> (In reply to Brian Birtles (:birtles) from comment #11)
> > * ElementPropertyTransition -> mozilla::ElementTransition
> 
> I sort of like this one as it is, although putting it in the mozilla
> namespace is fine.  I guess I'd be ok with changing it if you really want
> to, though.

I'll just change the namespace for now.

> > * Move ElementAnimationCollection and ElementAnimation to a separate files
> > (i.e. out of AnimationCommon)
> 
> I'd prefer to leave it in the same file; I'd rather not scatter animations
> stuff across too many files, given that animations is only a small part of
> what's in the directory.  (It's also harder to merge with.)

Ok. I'd find the separate files easier to work with (particularly for ElementAnimation) but that can certainly wait until later (when I imagine ElementAnimation might move to dom/animation anyway hopefully alleviate the concern of confusing the contents of layout/style).
Comment on attachment 8446345 [details] [diff] [review]
part 3 - Rename nsAnimation to mozilla::StyleAnimation

>+} // end namespace mozilla

I think our normal convention is to omit the "end ".

r=dbaron
Attachment #8446345 - Flags: review?(dbaron) → review+
Comment on attachment 8446347 [details] [diff] [review]
part 5 - Move ElementPropertyTransition to mozilla namespace

 
>+} // end mozilla namespace

"} // namespace mozilla"

r=dbaron
Attachment #8446347 - Flags: review?(dbaron) → review+
Comment on attachment 8446349 [details] [diff] [review]
part 7 - Rename instances of ElementAnimationCollection

> void
>-nsAnimationManager::UpdateStyleAndEvents(ElementAnimationCollection* aEA,
>-                                         TimeStamp aRefreshTime,
>-                                         EnsureStyleRuleFlags aFlags)
>+nsAnimationManager::UpdateStyleAndEvents(
>+  ElementAnimationCollection* aCollection,
>+  TimeStamp aRefreshTime,
>+  EnsureStyleRuleFlags aFlags)

...

>-      NS_ASSERTION(et->mElementProperty == nsGkAtoms::transitionsProperty ||
>-                   et->mElementProperty == nsGkAtoms::transitionsOfBeforeProperty ||
>-                   et->mElementProperty == nsGkAtoms::transitionsOfAfterProperty,
>-                   "Unexpected element property; might restyle too much");
>+      MOZ_ASSERT(
>+        collection->mElementProperty == nsGkAtoms::transitionsProperty ||
>+        collection->mElementProperty ==
>+          nsGkAtoms::transitionsOfBeforeProperty ||
>+        collection->mElementProperty == nsGkAtoms::transitionsOfAfterProperty,
>+        "Unexpected element property; might restyle too much");

I'm not crazy about this indentation style, but I guess it's ok.

r=dbaron
Attachment #8446349 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #22)
> Comment on attachment 8446349 [details] [diff] [review]
> part 7 - Rename instances of ElementAnimationCollection
...
> I'm not crazy about this indentation style, but I guess it's ok.

Neither am I but I think it's preferable to going over 80 chars per line. I'm definitely open to other suggestions.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: