Closed Bug 1259861 Opened 8 years ago Closed 4 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

See bug 1259860 for context.
Component: General → SVG
Here's the patch. It's a monster, I'm afraid.
Attachment #8737040 - Flags: review?(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 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
Attachment #8737040 - Flags: review?(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.)
(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.
(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.
(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!

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.

Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/78c7874b0ed9
Move everything else into the mozilla namespace in layout/svg r=dholbert
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Attachment #8737040 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: