1259861 - Remove the ns* prefix and move everything into the mozilla:: namespace in layout/svg

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Seth Fowler [:seth] [:s2h]]

See bug 1259860 for context.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Remove ns* prefix and move everything into mozilla:: in layout/svg.

Here's the patch. It's a monster, I'm afraid.

Comment 2

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd66d60901a0

Comment 3

[Daniel Holbert [:dholbert]]

So, I want to review this ASAP for you, to minimize bitrot. But before I dive into that, I have these requests:
 (1) Please rebase on top of a relatively recent mozilla-central commit. (It doesn't apply cleanly right now, and from looking at the Try run, it seems that's becasue it's based on a week-old mozilla-central commit, mozilla-central changeset b2dbee5ca727)

 (2) Consider generating the patch with 3 lines of context, instead of 8, to hedge against bitrot.  That's probably best-practice for a patch of this magnitude.

This will increase the likelihood that the patch that I review will be pretty similar to the patch that actually lands. (fewer last-minute bitrot hackarounds between review & landing)

Also:
 (3) the Try run has some build failures that need investigating, but that part probably doesn't need to block me from reviewing.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8737040 [details] [diff] [review]
Remove ns* prefix and move everything into mozilla:: in layout/svg.

From a first-pass skim, I have two nits, and two more sizeable review comments

NITS:
=====

>+++ b/layout/svg/ISVGChildFrame.h
>+class ISVGChildFrame : public nsQueryFrame
> {
> public:
>-  typedef mozilla::SVGAnimatedNumberList SVGAnimatedNumberList;
>-  typedef mozilla::SVGNumberList SVGNumberList;
>-  typedef mozilla::SVGAnimatedLengthList SVGAnimatedLengthList;
>-  typedef mozilla::SVGLengthList SVGLengthList;
>-  typedef mozilla::SVGUserUnitList SVGUserUnitList;
>+  typedef SVGAnimatedNumberList SVGAnimatedNumberList;
>+  typedef SVGNumberList SVGNumberList;
>+  typedef SVGAnimatedLengthList SVGAnimatedLengthList;
>+  typedef SVGLengthList SVGLengthList;
>+  typedef SVGUserUnitList SVGUserUnitList;

These typedefs are now tautologies. Just remove them.

>+++ b/layout/svg/SVGPatternFrame.h
>@@ -5,59 +5,58 @@
> public:
>-  typedef mozilla::SVGAnimatedPreserveAspectRatio SVGAnimatedPreserveAspectRatio;
>+  typedef SVGAnimatedPreserveAspectRatio SVGAnimatedPreserveAspectRatio;

Same here.

MORE-SIZEABLE REVIEW COMMENTS:
==============================

>+++ b/accessible/base/nsAccessibilityService.cpp
>@@ -56,27 +56,27 @@
> #ifdef MOZ_CRASHREPORTER
> #include "nsExceptionHandler.h"
> #endif
> 
> #include "nsImageFrame.h"
> #include "nsIObserverService.h"
> #include "nsLayoutUtils.h"
> #include "nsPluginFrame.h"
>-#include "nsSVGPathGeometryFrame.h"
[...]
> #include "nsXBLBinding.h"
> #include "mozilla/ArrayUtils.h"
> #include "mozilla/dom/DOMStringList.h"
> #include "mozilla/Preferences.h"
> #include "mozilla/Services.h"
> #include "nsDeckFrame.h"
>+#include "SVGPathGeometryFrame.h"

As part of this mass-conversion, I suspect we should be exporting & #including header files as "mozilla/Foo.h", not as "Foo.h"...  (Because it matches our current [not-entirely-followed] guidelines AFAIK, and also because there could be ambiguity if someone just has #include "Foo.h" for sufficiently-generic values of Foo. The "mozilla/" path helps disambiguate Foo.h just in the same way that "ns" prefix used to, from a header-file-name-collision perspective.)

I'm not sure if/where the rules for #include-header paths are documented -- I recall seeing language in the Coding Style Guide saying that they should match their namespace path, but I don't see any such language there right now.  Still: probably worth making sure we're following best practices for these, before proceeding with massive code-rewriting patches like this.  (Existing code isn't consistent about include paths for mozilla-namespaced things, but for a mass-change like this, I tend to think we should be careful to get this right...  We could fix it in a followup, too, but I suspect the followup would be similarly-massive, and it's probably best to just get it right up-front.)

>+++ b/layout/svg/CSSClipPathInstance.h
>@@ -1,39 +1,39 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>-#ifndef __NS_CSSCLIPPATHINSTANCE_H__
>-#define __NS_CSSCLIPPATHINSTANCE_H__
>+#ifndef __CSSCLIPPATHINSTANCE_H__
>+#define __CSSCLIPPATHINSTANCE_H__

Somewhat similar to the previous review comment: this #include guard doesn't match the modern mozilla-namespaced include guard pattern.

This is kind of a trivial nitpick, but it's somewhat important for disambiguation, in the abstract at least.  __NS_FOO_H__ was likely a unique guard, but I'm much less sure about __FOO_H__ being unique (for sufficiently-generic values of FOO).  So, we should be a bit hesitant to feel safe about simply dropping the NS prefix in the include guards here.

Ideally, I'd like to see us either:
 (a) leave these guards completely unmodified, as archeological cruft, and file a followup on bringing them up to date. (And in the meantime we can rely on the stale __NS prefix for uniqueness.)
or, better:
 (b) just go straight to the correct up-to-date form* as part of this patch.

I'd be more comfortable with either of those options as compared to simply dropping NS_ from the include-guards (what this patch currently does).

*See #include-guard guidelines here:
 https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> As part of this mass-conversion, I suspect we should be exporting &
> #including header files as "mozilla/Foo.h", not as "Foo.h"...

Assuming I'm right about this (and I'm fairly confident, after a sanity-check from mccr8): on the bright side, I think this could be partially fixed up in an automated way, using search-and-replace on the patch file itself. I'm thinking you could just open the patch in a text editor, and replace all instances of |+#include "| with |+#include "mozilla/|.

(This isn't quite sufficient, because these #includes would end up at a now-arbitrary sorted position in the list of #includes. That could be fixed manually with a merge tool, though, and I'm imagining that process would be easier once the lines themselves are correct and it's just the ordering that's wrong.)

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
>  (1) Please rebase on top of a relatively recent mozilla-central commit. (It
> doesn't apply cleanly right now, and from looking at the Try run, it seems
> that's becasue it's based on a week-old mozilla-central commit,
> mozilla-central changeset b2dbee5ca727)

Yeah, I can do that.

>  (2) Consider generating the patch with 3 lines of context, instead of 8, to
> hedge against bitrot.  That's probably best-practice for a patch of this
> magnitude.

I'm doing this in git and not an hg patch queue. =) Do you want me to do this just for reviewing purposes, though?

>  (3) the Try run has some build failures that need investigating, but that
> part probably doesn't need to block me from reviewing.

Yeah, taking a look.

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (In reply to Daniel Holbert [:dholbert] from comment #4)
> > As part of this mass-conversion, I suspect we should be exporting &
> > #including header files as "mozilla/Foo.h", not as "Foo.h"...
> 
> Assuming I'm right about this (and I'm fairly confident, after a
> sanity-check from mccr8): on the bright side, I think this could be
> partially fixed up in an automated way, using search-and-replace on the
> patch file itself. I'm thinking you could just open the patch in a text
> editor, and replace all instances of |+#include "| with |+#include
> "mozilla/|.
> 
> (This isn't quite sufficient, because these #includes would end up at a
> now-arbitrary sorted position in the list of #includes. That could be fixed
> manually with a merge tool, though, and I'm imagining that process would be
> easier once the lines themselves are correct and it's just the ordering
> that's wrong.)

Yeah, it's not too tricky to fix up. I need to figure out how to export as "mozilla/Foo.h" instead of "Foo.h", though. (I assume it's trivial, I just don't know offhand, but I'll poke around.)

I'll fix up the include guards, as well. It's probably worth just moving directly to the right thing.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #6)
> >  (2) Consider generating the patch with 3 lines of context, instead of 8, to
> > hedge against bitrot.  That's probably best-practice for a patch of this
> > magnitude.
> 
> I'm doing this in git and not an hg patch queue. =) Do you want me to do
> this just for reviewing purposes, though?

Ah, I see.

Yeah -- for reviewing purposes, please either: export the patch-file with 3 lines of context, or just let me know what cset ID that you're basing it on top of.  Either of those will help me import it into a local tree with minimal pain, so that I can inspect it in meld. (which is the easiest way to review giant patches with tiny tweaks to a zillion lines IMO)

(In reply to Seth Fowler [:seth] [:s2h] from comment #7)
> I need to figure out how to export as
> "mozilla/Foo.h" instead of "Foo.h", though. (I assume it's trivial, I just
> don't know offhand, but I'll poke around.)

I believe you just use "EXPORTS.mozilla" instead of "EXPORTS" in moz.build, or something like that.

> I'll fix up the include guards, as well. It's probably worth just moving
> directly to the right thing.

Great, thanks!

Comment 9

[Robert Longson [:longsonr]]

Attachment: Bug 1259861 - Move everything else into the mozilla namespace in layout/svg

Comment 10

[Robert Longson [:longsonr]]

Note that I ran

sed -i '.bak' 's/nsSVG/SVG/g' *

on layout/svg and dom/svg to convert them before manually editing the results.

Comment 11

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0db0834fff7c55b8278b3cac7599cec3fd3153

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78c7874b0ed9
Move everything else into the mozilla namespace in layout/svg r=dholbert

Comment 13

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/78c7874b0ed9