rename nsSVGTextFrame2 to SVGTextFrame

RESOLVED FIXED in mozilla29

Status

()

Core
SVG
P5
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: heycam, Assigned: longsonr)

Tracking

Trunk
mozilla29
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Now that bug 889736 has removed the old SVG text frame classes, is there any downsize to renaming nsSVGTextFrame2 to nsSVGTextFrame?
*downside
Priority: -- → P5
(Assignee)

Comment 2

4 years ago
Created attachment 8350103 [details] [diff] [review]
renaming to SVGTextFrame
Assignee: nobody → longsonr
Attachment #8350103 - Flags: review?(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?
Attachment #8350103 - Attachment description: renaming → renaming to SVGTextFrame
Attachment #8350103 - Flags: feedback?(roc)
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.
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.)
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 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
Attachment #8350103 - Flags: review?(dholbert)
Attachment #8350103 - Flags: review+
Attachment #8350103 - Flags: feedback?(roc)
(Assignee)

Comment 8

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e1dcd8797f10
(Assignee)

Comment 9

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c9b80ab65f
(Assignee)

Updated

4 years ago
Flags: in-testsuite-
(Assignee)

Updated

4 years ago
Summary: rename nsSVGTextFrame2 to nsSVGTextFrame → rename nsSVGTextFrame2 to SVGTextFrame
https://hg.mozilla.org/mozilla-central/rev/d7c9b80ab65f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(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.
You need to log in before you can comment on or make changes to this bug.