Closed Bug 580902 Opened 10 years ago Closed 10 years ago

Remove "using namespace mozilla;" from nsSVGElement.h

Categories

(Core :: SVG, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: craig.topper, Assigned: craig.topper)

References

Details

Attachments

(1 file, 1 obsolete file)

It's bad practice to put a using statement in a header file because it will apply to all files that include that header.

There is currently one nsSVGElement.h which is hiding improper namespace uses in other files. I'm working on a patch to remove and fix the fallout and will post soon.
Attached patch Patch (obsolete) — Splinter Review
Attachment #459719 - Flags: superreview?
Attachment #459719 - Flags: review?(jwatt)
Attachment #459719 - Flags: superreview?
Comment on attachment 459719 [details] [diff] [review]
Patch

Good catch, thanks!


Please add a 'using' statement to the .cpp files nsSVGElement.cpp and nsSVGSVGElement.cpp rather than prefixing names.


>+static nsSVGAttrTearoffTable<mozilla::SVGAnimatedLengthList, mozilla::DOMSVGAnimatedLengthList>
>+  sSVGAnimatedLengthListTearoffTable;
>+
> namespace mozilla {
> 
>-static nsSVGAttrTearoffTable<SVGAnimatedLengthList, DOMSVGAnimatedLengthList>
>-  sSVGAnimatedLengthListTearoffTable;
>-

Why is this change necessary?


>-  NS_INTERFACE_MAP_ENTRY(DOMSVGLength) // pseudo-interface
>+  NS_INTERFACE_MAP_ENTRY(mozilla::DOMSVGLength) // pseudo-interface

Is this change necessary?


>+namespace css = mozilla::css;
>+

This change seems unrelated to the patch at hand.

Otherwise looks good!
> >+static nsSVGAttrTearoffTable<mozilla::SVGAnimatedLengthList, mozilla::DOMSVGAnimatedLengthList>
> >+  sSVGAnimatedLengthListTearoffTable;
> >+
> > namespace mozilla {
> > 
> >-static nsSVGAttrTearoffTable<SVGAnimatedLengthList, DOMSVGAnimatedLengthList>
> >-  sSVGAnimatedLengthListTearoffTable;
> >-
> 
> Why is this change necessary?

No I only did it cause it was being the static in the namespace and still had 'ns' on the front. I'll change it back

> 
> 
> >-  NS_INTERFACE_MAP_ENTRY(DOMSVGLength) // pseudo-interface
> >+  NS_INTERFACE_MAP_ENTRY(mozilla::DOMSVGLength) // pseudo-interface
> 
> Is this change necessary?
> 
>

You'll find that the macro that expands to puts adds '::' in front of the argument so if I didn't put mozilla:: there it expands to ::DOMSVGLength which doesn't work.
 
> >+namespace css = mozilla::css;
> >+
> 
> This change seems unrelated to the patch at hand.

There were quite a few places that used 'css::' in that file which worked before because of the 'using mozilla'. So I aliased it rather than fixing every line. There are other files that do a the same 'namespace css = mozilla::css;' to avoid typing mozilla everywhere.
Attachment #459719 - Attachment is obsolete: true
Attachment #459840 - Flags: review?(jwatt)
Attachment #459719 - Flags: review?(jwatt)
Comment on attachment 459840 [details] [diff] [review]
Address reviews comments

Cool, thanks for the patch!
Attachment #459840 - Flags: review?(jwatt) → review+
Blocks: 515116
Comment on attachment 459840 [details] [diff] [review]
Address reviews comments

a2.0=dbaron
Attachment #459840 - Flags: approval2.0+
Pushed http://hg.mozilla.org/mozilla-central/rev/14221d34775f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.