Closed Bug 1517715 Opened 2 years ago Closed 2 years ago
SMILCSSProperty, ns SMILKey Spline and ns SMILParser Utils to the mozilla namespace
No description provided.
Assignee: nobody → longsonr
Attachment #9034382 - Flags: review?(bbirtles)
Comment on attachment 9034382 [details] [diff] [review] patch Review of attachment 9034382 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed ::: dom/animation/ComputedTimingFunction.h @@ +12,5 @@ > #include "nsTimingFunction.h" > > #include "mozilla/Assertions.h" > #include "mozilla/Maybe.h" > +#include "mozilla/SMILKeySpline.h" // SMILKeySpline (I'd probably drop the comment here. At least in dom/animation we've been taking the approach that if the thing you're importing matches the filename we don't add a comment.) ::: dom/smil/SMILAnimationFunction.h @@ +12,4 @@ > #include "nsGkAtoms.h" > #include "nsString.h" > #include "nsSMILTimeValue.h" > +#include "SMILKeySpline.h" We should probably sort this include list. Also, this patch is a little bit inconsistent about whether or not we import mozilla namespace headers using "mozilla/" or not (e.g. this file and SMILAnimationController.cpp don't using mozilla/ but most other files do). I don't have a strong preference about this but I tend to go with: 1. For importing the main header of a .cpp file, DON'T include the mozilla directory (e.g. "Class.cpp" just imports "Class.h"). 2. For all others, DO include the mozilla directory (e.g. "Class.cpp" imports "mozilla/OtherThing.h"). That seems to be what I mostly see (e.g. Element.cpp mostly seems to follow that, but not entirely) and lines up with my understanding of what the coding style suggests here. If you've gotten other advice elsewhere though, then feel free to follow that.  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices ::: dom/smil/SMILParserUtils.cpp @@ +32,5 @@ > #define REPEAT_PREFIX NS_LITERAL_STRING("repeat(") > #define WALLCLOCK_PREFIX NS_LITERAL_STRING("wallclock(") > > +typedef mozilla::SVGContentUtils SVGContentUtils; > +typedef mozilla::RangedPtr<const char16_t> RangedPtr; I think we should just add `using namespace mozilla' above the 'using namespace mozilla::dom' line earlier in this file. For the RangedPtr one I'd prefer without the typedef since I think it makes it harder to read.
Attachment #9034382 - Flags: review?(bbirtles) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/0db2741d6eda Move nsSMILCSSProperty, nsSMILKeySpline and nsSMILParserUtils to the mozilla namespace r=bbirtles
You need to log in before you can comment on or make changes to this bug.