940193 - rename nsSVGTextFrame2 to SVGTextFrame

Status: RESOLVED FIXED

(Core :: SVG, defect, P5)

Description

[Cameron McCormack (:heycam)]

Now that bug 889736 has removed the old SVG text frame classes, is there any downsize to renaming nsSVGTextFrame2 to nsSVGTextFrame?

Comment 1

[Cameron McCormack (:heycam)]

*downside

Comment 2

[Robert Longson [:longsonr]]

Attachment: renaming to SVGTextFrame

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8350103 [details] [diff] [review]
renaming to SVGTextFrame

>diff --git a/layout/svg/nsSVGTextFrame2.h b/layout/svg/SVGTextFrame.h
>rename from layout/svg/nsSVGTextFrame2.h
>rename to layout/svg/SVGTextFrame.h
>--- a/layout/svg/nsSVGTextFrame2.h
>+++ b/layout/svg/SVGTextFrame.h
> } // namespace mozilla
[...]
>-class nsSVGTextFrame2 : public nsSVGTextFrame2Base
>+class SVGTextFrame : public SVGTextFrameBase
> {

I believe this would be the first non-ns-prefixed frame header file.

If we're removing the "ns" prefixing, we probably need to replace it with some namespacing. (Note that it's currently not in any namespace -- it's outside of the "namespace mozilla{}" chunk of this file).  Whatever namespacing we use should also be consistent with any namespacing we're intending to use for other frame classes.

I'm not sure whether we want such classes to be mozilla::MyFrame, or mozilla::layout::MyFrame, or something else.

roc, any preference?

Comment 4

[Cameron McCormack (:heycam)]

roc is away, but I can give you my opinion. :-)  I prefer mozilla::MyFrame.  I'm not particularly fond of these deeply nested namespaces, and I know that dbaron regrets having classes in mozilla::css and wants them eventually moved down to mozilla and keeping a "CSS" prefix in the class name.

Comment 5

[Daniel Holbert [:dholbert]]

That's my recollection, too -- basically swapping in "mozilla::" for "ns".

(In any case, I think the namespacing should probably happen in a separate patch, to keep this one mechanical and simple, but we shouldn't land the first patch here until we have a namespacing patch to land in the same push.)

Comment 6

[Daniel Holbert [:dholbert]]

Ah, I suppose we have e.g. SVGFEContainerFrame.cpp and SVGViewFrame.cpp (which I missed earlier since they don't have .h files), and they don't have a namespace (yet).

So I guess it's not the end of the world if we add one more non-namespaced SVG*Frame class. So, nevermind about that.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8350103 [details] [diff] [review]
renaming to SVGTextFrame

This all looks good to me! Just one nit:

>diff --git a/layout/svg/nsSVGTextFrame2.h b/layout/svg/SVGTextFrame.h
>rename from layout/svg/nsSVGTextFrame2.h
>rename to layout/svg/SVGTextFrame.h
>--- a/layout/svg/nsSVGTextFrame2.h
>+++ b/layout/svg/SVGTextFrame.h
>-#ifndef NS_SVGTEXTFRAME2_H
>-#define NS_SVGTEXTFRAME2_H
>+#ifndef NS_SVGTEXTFRAME_H
>+#define NS_SVGTEXTFRAME_H

Let's drop the NS_ and instead use MOZILLA_SVGTEXTFRAME_H here -- for non-"nsFoo" classes, we're generally using MOZILLA_ as the header-guard-prefix nowadays.

r=me with that

Comment 8

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=e1dcd8797f10

Comment 9

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c9b80ab65f

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Cameron McCormack (:heycam) from comment #4)
> roc is away, but I can give you my opinion. :-)  I prefer mozilla::MyFrame. 
> I'm not particularly fond of these deeply nested namespaces, and I know that
> dbaron regrets having classes in mozilla::css and wants them eventually
> moved down to mozilla and keeping a "CSS" prefix in the class name.

Yes, I totally agree.