1515187 - Rename nsSVGElement to SVGElement and put it in the mozilla:dom namespace

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Comment 1

[Robert Longson [:longsonr]]

Attachment: Move nsSVGElement to SVGElement namespaced under mozilla::dom

Comment 2

[Robert Longson [:longsonr]]

Most of this was done mechanically as a sed replacement followed by ./mach clang-format then I fixed the compilation errors.

Comment 3

[Daniel Holbert [:dholbert]]

Would you mind posting the sed command(s), if you've got them handy in your history?  For largely-automated patches, it's helpful to see what the automated command was.  (Then if the command looks good, I can skim the changes a bit more briskly, and e.g. generate a patch locally and simply diff it against your attachment, to reduce the scope of what I need to review manually.)

Comment 4

[Robert Longson [:longsonr]]

sed -i '.bak' 's/nsSVGElement/SVGElement/g' *.cpp
sed -i '.bak' 's/nsSVGElement/SVGElement/g' *.h
rm *.bak

in each affected directory.

Comment 5

[Daniel Holbert [:dholbert]]

One thing to change:
 - This patch has tons of
>#include "SVGElement.h"
...which should now be:
> #include "mozilla/dom/SVGElement.h"
...now that the .h file is in EXPORTS.mozilla.dom rather than in plain old EXPORTS.

Perhaps that might be easier to do in a second patch, rather than as part of the main largely-mechanical patch?

Comment 6

[Daniel Holbert [:dholbert]]

(Thank you for doing this cleanup, by the way! It's great to see nsFoo/mozilla::dom::foo inconsistencies being resolved.)

Comment 7

[Daniel Holbert [:dholbert]]

Note that the #include change suggested in comment 5 might want to be semi-manual, to keep include lists roughly sorted (or at least grouped into "mozilla/" includes vs. non-"mozilla/" includes).

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 9032271 [details] [diff] [review]
Move nsSVGElement to SVGElement namespaced under mozilla::dom

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

(not done yet, just posting a few notes as I go):

::: dom/svg/SVGContentUtils.cpp
@@ +36,5 @@
>  
> +namespace mozilla {
> +using namespace dom;
> +using namespace dom::SVGPreserveAspectRatio_Binding;
> +using namespace gfx;

This ^^ chunk should be reverted.

Two concerns:
 (1) RE "using":
Per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces :
 # using namespace ...; should always specify
 # the fully qualified namespace.
...which is what the old code was doing here.

 (2) RE "namespace mozilla {": this change seems to be putting this whole file (the function-impls after this chunk) inside of "namespace mozilla { ... }" wrapper -- which I think is fine in spirit, but I don't think it's a necessary part of this huge patch.  (We're already "using" the correct namespace to be able to reference SVGElement without having to fully-qualify it.)

So: to avoid unforseen side effects & to keep this megapatch as surgical/focused as possible, could you revert this chunk? (The namespace{} wrapper is probably good fodder for a followup if you want to do that.)

::: dom/svg/SVGContentUtils.h
@@ -36,5 @@
>  class SVGViewportElement;
>  }  // namespace dom
>  
> -}  // namespace mozilla
> -

As noted for SVGContentUtils.cpp: we seem to be adjusting the namespacing here -- putting SVGContentUtils inside of the mozilla namespace, I guess (?)  That seems like a fine thing to do, but is probably best to do separately, not as part of this nsSVGElement-rename patch.

(And if it's necessary for this patch for some reason [I don't think it is?], then perhaps do it as a "part 0" or something, since it seems like a change we could make on its own?)

@@ -81,5 @@
>    typedef mozilla::gfx::Rect Rect;
>    typedef mozilla::gfx::StrokeOptions StrokeOptions;
> -  typedef mozilla::SVGAnimatedPreserveAspectRatio
> -      SVGAnimatedPreserveAspectRatio;
> -  typedef mozilla::SVGPreserveAspectRatio SVGPreserveAspectRatio;

(These typedef removals probably hinge on the SVGContentUtils namespacing, I think, so they should move along with that namespacing)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 9032271 [details] [diff] [review]
Move nsSVGElement to SVGElement namespaced under mozilla::dom

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

OK, I got through it all :)  (diffing a patch that I generated automatically vs. your patch, to make sure all the manual stuff makes sense)

r- for the moment since I'd like to look at the changes, but this is probably r+ with feedback addressed.

::: dom/svg/SVGElement.cpp
@@ -64,5 @@
> -// vararg-list methods in this file:
> -//   nsSVGElement::GetAnimated{Length,Number,Integer}Values
> -// See bug 547964 for details:
> -static_assert(sizeof(void*) == sizeof(nullptr),
> -              "nullptr should be the correct size");

Right now the patch is moving this chunk (the static_assert and its comment), but I don't think there's any reason it needs to do that... This doesn't use any namespaced stuff, beyond the "nsSVGElement" (now SVGElement) in the comment itself.

So: probably best to leave this where it is (with s/ns//), to avoid moving code around for no reason?

@@ +71,5 @@
>  }
>  
> +namespace mozilla {
> +namespace dom {
> +using namespace SVGUnitTypes_Binding;

Per coding style nit above, this 'using' decl should be outside the namespace{} declarations, and should just be:

using namespace mozilla::dom::SVGUnitTypes_Binding;

(i.e. just restore this line from the original file -- it existed up at the top, after all the includes.)

::: dom/svg/SVGPolyElement.cpp
@@ +10,5 @@
>  #include "SVGContentUtils.h"
>  
> +namespace mozilla {
> +using namespace gfx;
> +namespace dom {

Similar to my comments above for SVGContentUtils:
 (1) "using namespace mozilla::gfx;" should remain as a standalone decl, outside of any namespace{} block.

 (2) To avoid side effects & to keep this patch as targeted as possible, it'd be best to avoid any unnecessary "namespace { ... }" wrappers around function-implementations in this patch (except for in SVGElement.* which are the files that we're explicitly moving into a namespace)

Really, I think you can revert all changes to this file besides the comment, right? ("// SVGElement methods")  That's the only mention/usage of the type that we're renaming here.  The namespace{{}} wrapping may be useful but it'd be best to do it separately.

::: dom/svg/nsSVGAnimatedTransformList.cpp
@@ +19,4 @@
>  namespace mozilla {
>  
>  using namespace dom;
> +using namespace dom::SVGTransform_Binding;

Technically, both of these using decls (the existing "using namespace dom;" and the new Binding line) should be declared outside of the "namespace{...}" block and should include the full path with mozilla:: at the start.

But I guess this is also a fine surgical tweak for the moment, given the state of the current code that you're inserting into here; and we could fix the 'using' style afterwards.

(Also, this change (adding SVGTransform_Binding) doesn't really makes sense as part of this commit; it's not a SVGElement-class-rename-related thing.   I'm guessing this was unified-reshuffling-build-bustage? I guess that's reasonable to include as part of the same patch, since this seems to be the only bit of that.)

Comment 10

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> One thing to change:
>  - This patch has tons of
> >#include "SVGElement.h"
> ...which should now be:
> > #include "mozilla/dom/SVGElement.h"
> ...now that the .h file is in EXPORTS.mozilla.dom rather than in plain old
> EXPORTS.
> 
> Perhaps that might be easier to do in a second patch, rather than as part of
> the main largely-mechanical patch?

Binding is bust without that and I couldn't figure out how to fix it unless you do this all together. I.e. you just get a whole load of compilation errors.

Comment 11

[Robert Longson [:longsonr]]

Attachment: Part 1 - fix nsSVGAnimatedTransform to compile once part 4 lands

Comment 12

[Robert Longson [:longsonr]]

Attachment: Part 2 - Move SVGPolyElement's implementation into the mozilla::dom namespace

Needed to prevent bustage when part 4 lands.

Comment 13

[Robert Longson [:longsonr]]

Attachment: Part 3 - move SVGContentUtils into mozilla namespace

Simplifies part 4 a little.

Comment 14

[Robert Longson [:longsonr]]

I've broken out everything you wanted except the EXPORTS.mozilla.dom part. I think its more trouble than its worth trying to split that out.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson [:longsonr] from comment #14)
> I've broken out everything you wanted

Thanks!

> except the EXPORTS.mozilla.dom part. I
> think its more trouble than its worth trying to split that out.

Sorry, I didn't mean to suggest that you should break out that part -- sorry if I gave that impression.  (In comment 5, I was just saying that I noticed a lot of #include "SVGElement.h" that technically needed a mozilla/dom path added to them -- and that those additional changes could happen together or separately, whatever was easier.)

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 9032393 [details] [diff] [review]
Part 1 - fix nsSVGAnimatedTransform to compile once part 4 lands

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

r=me

(Note for eventual-commit-message purposes: right now the attachment title mentions "part 4" but there doesn't seem to be a part 4.  Maybe a good commit message would be "Preemptively fix unified bustage in nsSVGAnimatedTransformList.cpp", or something along those lines?)

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #16)
> (Note for eventual-commit-message purposes: right now the attachment title
> mentions "part 4" but there doesn't seem to be a part 4.

(Oh, sorry, I see -- part 4 is still coming & is the main patch. Cool. I initially misread "part 3" as being the main patch.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 9032394 [details] [diff] [review]
Part 2 - Move SVGPolyElement's implementation into the mozilla::dom namespace

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

r=me

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 9032396 [details] [diff] [review]
Part 3 - move SVGContentUtils into mozilla namespace

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

r=me. Thanks for splitting this out.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c4061c4ca3
Part 1 nsSVGAnimatedTransform uses SVGTransform_Binding, preemptively fix it so later patches in this bug can land r=dholbert

Comment 21

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/f9c4061c4ca3

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf26e38d816
Part 2 move SVGPolyElement's implementation into the mozilla::dom namespace r=dholbert

Comment 23

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/eaf26e38d816

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8008cff3ac9
Part 3 move SVGContentUtils into the mozilla namespace r=dholbert

Comment 25

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/d8008cff3ac9

Comment 26

[Robert Longson [:longsonr]]

Attachment: Part 4 - move nsSVGElement into the mozilla:dom namespace

Comment 27

[Daniel Holbert [:dholbert]]

Just to be sure you're aware before landing stuff: it looks like part 4 here applies cleanly on inbound, but conflicts if you happen to apply bug 1515607's patch first, I think.

(Each applies cleanly on inbound, and they each make changes to some of the same lines, e.g. the typedef in nsSVGFilters.h, each expecting the current mozilla-inbound code to be present there.)

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 9032704 [details] [diff] [review]
Part 4 - move nsSVGElement into the mozilla:dom namespace

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

Looks good to me. I reviewed by diffing it against your original patch, and the changes made sense in light of the splitting out, with the exception of one chunk that did not exist in the original patch:

::: layout/svg/nsSVGSymbolFrame.cpp
@@ +31,4 @@
>  
>    nsSVGViewportFrame::Init(aContent, aParent, aPrevInFlow);
>  }
> +#endif /* DEBUG */

Consider reverting changes to this file -- it's not actually getting code changes.  The patch's change to it is just adding a newline at the end, which is good, but off-topic for the patch (which kinda matters, for a patch of this substantial size & automated-ness).

Comment 29

[Robert Longson [:longsonr]]

That's an artifact of clang-format, it keeps adding that file and I keep removing it. I'll remove it yet again before landing.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d778b55d6be
Part 4 Rename nsSVGElement to SVGElement and put it in the mozilla:dom namespace r=dholbert

Comment 31

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2d778b55d6be