Move nsSMILCSSProperty, nsSMILKeySpline and nsSMILParserUtils to the mozilla namespace

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

No description provided.
Posted patch patchSplinter Review
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.[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.
Attachment #9034382 - Flags: review?(bbirtles) → review+
Priority: -- → P3

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

Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0db2741d6eda
Move nsSMILCSSProperty, nsSMILKeySpline and nsSMILParserUtils to the mozilla namespace r=bbirtles
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.