1517715 - Move nsSMILCSSProperty, nsSMILKeySpline and nsSMILParserUtils to the mozilla namespace

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.[1]

If you've gotten other advice elsewhere though, then feel free to follow that.

[1] 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.

Comment 3

[Robert Longson [:longsonr]]

(In reply to Brian Birtles (:birtles) from comment #2)

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.[1]

Things that aren't exported from moz.build have to be included as just the file name. That's true of many SMIL headers.

Done the rest of the review comments.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0db2741d6eda
Move nsSMILCSSProperty, nsSMILKeySpline and nsSMILParserUtils to the mozilla namespace r=bbirtles

Comment 5

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/0db2741d6eda