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

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).

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

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.

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

Comment on attachment 8441089 [details] [diff] [review] part 2 - Rename nsStyleAnimation.{h,cpp} to StyleAnimationValue.{h,cpp} r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/01bb9424e4b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/f17fb925e26e

https://hg.mozilla.org/mozilla-central/rev/01bb9424e4b4 https://hg.mozilla.org/mozilla-central/rev/f17fb925e26e

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?

(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.

(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

Comment on attachment 8446347 [details] [diff] [review] part 5 - Move ElementPropertyTransition to mozilla namespace >+} // end mozilla namespace "} // namespace mozilla" r=dbaron

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

(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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0642ab2f8f0 https://hg.mozilla.org/integration/mozilla-inbound/rev/a30bed3807b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/848b4b5487ea https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e25ef0aa0d https://hg.mozilla.org/integration/mozilla-inbound/rev/d091d8da77a8 https://hg.mozilla.org/integration/mozilla-inbound/rev/b311a62db22b

https://hg.mozilla.org/mozilla-central/rev/e0642ab2f8f0 https://hg.mozilla.org/mozilla-central/rev/a30bed3807b5 https://hg.mozilla.org/mozilla-central/rev/848b4b5487ea https://hg.mozilla.org/mozilla-central/rev/c9e25ef0aa0d https://hg.mozilla.org/mozilla-central/rev/d091d8da77a8 https://hg.mozilla.org/mozilla-central/rev/b311a62db22b