There might be a few exceptions, but that sounds like a general rule that people agree on. It's also a concrete issue: today I had to back out a patch for compile errors on one platform because on all other platforms, some headers were doing "using namespace" for me so I relied accidentally on it. Problem is, we have tons of headers that do "using namespace": $ ack-grep 'using namespace' -G '.*gfx.*h$' gfx/layers/ipc/ShadowLayers.h:26:using namespace gl; gfx/2d/DrawTargetSkia.h:14:using namespace std; dom/alarm/AlarmHalService.h:22:using namespace hal; dom/indexedDB/IndexedDatabase.h:28: using namespace mozilla::dom::indexedDB; dom/camera/DOMCameraPreview.h:15:using namespace mozilla; dom/camera/DOMCameraPreview.h:16:using namespace mozilla::layers; dom/camera/ICameraControl.h:16:using namespace dom; dom/camera/CameraControlImpl.h:18:using namespace dom; dom/camera/DOMCameraControl.h:19:using namespace mozilla; dom/camera/DOMCameraControl.h:20:using namespace dom; dom/camera/GonkCameraHwMgr.h:35:using namespace mozilla; dom/camera/GonkCameraHwMgr.h:36:using namespace android; dom/file/FileCommon.h:23: using namespace mozilla::dom::file; dom/devicestorage/nsDeviceStorage.h:43:using namespace mozilla::dom; dom/workers/Workers.h:23: using namespace mozilla::dom::workers; dom/workers/DOMBindingInlines.h:34: using namespace mozilla::dom; \ dom/workers/DOMBindingInlines.h:41: using namespace mozilla::dom; \ dom/workers/EventTarget.h:17:using namespace mozilla::dom; dom/telephony/TelephonyCommon.h:23: using namespace mozilla::dom::telephony; dom/plugins/ipc/PluginInstanceChild.h:23:using namespace mozilla::plugins::PluginUtilsOSX; dom/plugins/ipc/PluginLibrary.h:32:using namespace mozilla::layers; dom/system/unix/QTMLocationProvider.h:13:using namespace QtMobility; dom/bluetooth/BluetoothCommon.h:19: using namespace mozilla::dom::bluetooth; netwerk/protocol/device/GonkCaptureProvider.h:31:using namespace android; js/src/yarr/wtfbridge.h:30:using namespace js::unicode; js/ipc/CPOWTypes.h:19:using namespace IPC; js/ipc/CPOWTypes.h:46:using namespace mozilla::jsipc; js/public/Utility.h:38:using namespace JS; js/public/Utility.h:39:using namespace mozilla; mfbt/RefPtr.h:259:using namespace mozilla; media/webrtc/trunk/src/video_engine/main/test/SimpleCocoaGUI/SimpleCocoaGUIAppDelegate.h:17:using namespace std; media/webrtc/trunk/src/video_engine/main/test/WindowsTest/WindowsTestMainDlg.h:24:using namespace webrtc; media/webrtc/trunk/src/video_engine/main/test/WindowsTest/WindowsTest.h:28:using namespace webrtc; media/webrtc/trunk/src/video_engine/main/test/WindowsTest/ChannelDlg.h:46:using namespace webrtc; media/webrtc/trunk/src/video_engine/main/test/WindowsTest/CaptureDevicePool.h:23:using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_external_media.h:19:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_call_report.h:21:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_audio_processing.h:23:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_base.h:23:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_hardware.h:19:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_file.h:19:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_volume_control.h:21:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_codec.h:19:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_video_sync.h:19:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_encryption.h:17:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_rtp_rtcp.h:25:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_dtmf.h:18:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/include/voe_network.h:23:// using namespace webrtc; media/webrtc/trunk/src/voice_engine/test/auto_test/voe_stress_test.h:20:using namespace webrtc; media/webrtc/trunk/src/voice_engine/test/auto_test/voe_test_interface.h:22:using namespace webrtc; media/webrtc/trunk/src/voice_engine/test/win_test/WinTestDlg.h:105:using namespace webrtc; media/webrtc/trunk/src/modules/rtp_rtcp/test/BWEStandAlone/TestSenderReceiver.h:26:using namespace webrtc; media/webrtc/trunk/testing/gtest/include/gtest/gtest-printers.h:230: using namespace ::testing::internal2; // NOLINT content/media/nsAudioAvailableEventManager.h:15:using namespace mozilla; content/media/gstreamer/nsGStreamerReader.h:14:using namespace mozilla; content/media/ogg/nsOggReader.h:20:using namespace mozilla; content/xul/document/src/nsXULPrototypeCache.h:24:using namespace mozilla::scache; content/canvas/src/CanvasUtils.h:24:using namespace gfx; other-licenses/snappy/src/snappy-stubs-internal.h:65:using namespace std; widget/qt/mozqorientationsensorfilter.h:15:using namespace QtMobility; obj-mobile-debug/dist/include/js/Utility.h:38:using namespace JS; obj-mobile-debug/dist/include/js/Utility.h:39:using namespace mozilla; toolkit/crashreporter/google-breakpad/src/third_party/glog/src/base/mutex.h:321:using namespace MUTEX_NAMESPACE; toolkit/crashreporter/google-breakpad/src/third_party/glog/src/utilities.h:224:using namespace GOOGLE_NAMESPACE::glog_internal_namespace_; toolkit/components/url-classifier/nsUrlClassifierProxies.h:15:using namespace mozilla::safebrowsing; toolkit/components/places/tests/cpp/places_test_harness.h:27:using namespace mozilla; xpcom/base/ClearOnShutdown.h:74: using namespace ClearOnShutdown_Internal; xpcom/base/ClearOnShutdown.h:87: using namespace ClearOnShutdown_Internal;

Comment on attachment 668167 [details] [diff] [review] content/canvas/ part It turns out, making patches here is not so simple: files that #include these bad headers come to depend on them doing "using namespace" for them; cancelling these patches, will ask for people to step up to help.

OK, let's try to make this a mentored bug! This is IMHO very useful to fix, but I don't have time. Anyone willing to help: - take a look at the list of bad header files in comment 0. Don't attempt to fix all of them at once; it makes most sense to process one directory at a time. - Fix the headers in your directory by removing "using namespace" and adapting the rest of the header - Try building. That will quite likely fail as some .cpp files that were #including these headers were relying on them doing "using namespace". So fix these .cpp files by adding "using namespace to them" -- AFTER the #includes. - Once you have a patch that builds, try finding a reviewer. I can't review patches in arbitrary directories. To find a reviewer, use `hg annotate` or `hg log` to find who had been a reviewer for past patches touching the same file or directory. The reviewer is given in the "r=Someone" part of commit message. You can also look at the bug itself.

Hi, I'm new to Firefox developing but I think this is probably within my abilities since it mostly just seems like busy work. Is the general idea that we want to get rid of "using namespace" and replace all the references with the fully qualified names?

(In reply to Matt Joras from comment #5) > Hi, I'm new to Firefox developing but I think this is probably within my > abilities since it mostly just seems like busy work. > > Is the general idea that we want to get rid of "using namespace" and replace > all the references with the fully qualified names? No, "using namespace" is fine *outside* of headers. In other words, this is what we currently have (bad): Foo.h: using namespace xyz; ...some code... Foo.cpp: #include "Foo.h" ...some code... And this is how I suggest that we change it to: Foo.h: // no "using namespace" in this file! ...some code... Foo.cpp: #include "Foo.h" using namespace xyz; ...some code...

Oh wait --- of course, *inside* of the headers, since we don't want "using namespace", then indeed using fully qualified names (xyz::foo()) is the only thing we can do, yes.

Ah I see. So essentially just moving the using directive out of the headers and into the files that include the headers?

Yes, basically. But we don't want to automatically add the 'using namespace' to all the files that include the headers -- we want to only add it to files that actually need it.

Sounds reasonable to me. Before I get started, is there a good way to find out which files need the particular namespace other than going through each one and analyzing it manually?

Manually analyzing is going to be very time-consuming. I would suggest letting the compiler do it for you: remove "using namespace from header", try to compile, see what error messages you get. Good luck :)

Sounds good! I'll take a crack at it and see how things go :).

So far I've removed it from 5 header files and modified the respective files that include them. How / when should I show what I'm doing so you can tell me how it looks?

Also, these are the headers I've done so far: xpcom/base/ClearOnShutdown.h toolkit/components/url-classifier/nsUrlClassifierProxies.h toolkit/components/places/tests/cpp/places_test_harness.h gfx/2d/DrawTargetSkia.h gfx/layers/ipc/ShadowLayers.h

Once you're done with a given toplevel (or otherwise large enough) directory (like gfx/) and you've checked that it still builds, please make a patch with just these changes, and find an appropriate reviewer. For gfx/, an appropriate reviewer is jmuizelaar. For xpcom/, I would ask khuey or bsmedberg. In general, hg log / hg annotate can help, see comment 4.

Comment on attachment 668832 [details] [diff] [review] Removes "using namespace" directive from gfx headers. I accidentally screwed up the whitespace a little bit. Sorry about that.

Do you want to upload a new version and mark it as obsoleting the first version? We actually are quite strict about whitespace :-) Also, no whitespace changes outside of the specific lines that you have to change, please. (I haven't actually looked at the patch).

Comment on attachment 668832 [details] [diff] [review] Removes "using namespace" directive from gfx headers. Review of attachment 668832 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicCanvasLayer.cpp @@ +13,5 @@ > #include "BasicLayersImpl.h" > #include "nsXULAppAPI.h" > > using namespace mozilla::gfx; > +using namespace mozilla::gl; OK I see now --- there was an empty line here, please preserve it.

Fixed whitespace.

Comment on attachment 668840 [details] [diff] [review] Removes "using namespace" directive from gfx headers. >diff -r f4c2a6f2b838 gfx/2d/DrawTargetSkia.h >--- a/gfx/2d/DrawTargetSkia.h Fri Oct 05 11:19:16 2012 -0700 >+++ b/gfx/2d/DrawTargetSkia.h Sat Oct 06 17:25:01 2012 -0500 >@@ -6,17 +6,16 @@ > #pragma once > > #include "skia/SkCanvas.h" > #include "2D.h" > #include "Rect.h" > #include "PathSkia.h" > #include <sstream> > #include <vector> >-using namespace std; > > namespace mozilla { > namespace gfx { > > class SourceSurfaceSkia; > > class DrawTargetSkia : public DrawTarget > { >@@ -100,13 +99,13 @@ private: > void RemoveSnapshot(SourceSurfaceSkia* aSnapshot); > > void MarkChanged(); > > IntSize mSize; > SkBitmap mBitmap; > SkRefPtr<SkCanvas> mCanvas; > SkRefPtr<SkDevice> mDevice; >- vector<SourceSurfaceSkia*> mSnapshots; >+ std::vector<SourceSurfaceSkia*> mSnapshots; > }; > > } > } >diff -r f4c2a6f2b838 gfx/layers/basic/BasicCanvasLayer.cpp >--- a/gfx/layers/basic/BasicCanvasLayer.cpp Fri Oct 05 11:19:16 2012 -0700 >+++ b/gfx/layers/basic/BasicCanvasLayer.cpp Sat Oct 06 17:25:01 2012 -0500 >@@ -9,16 +9,17 @@ > #include "gfxUtils.h" > #include "gfxPlatform.h" > #include "mozilla/Preferences.h" > > #include "BasicLayersImpl.h" > #include "nsXULAppAPI.h" > > using namespace mozilla::gfx; >+using namespace mozilla::gl; > > namespace mozilla { > namespace layers { > > class BasicCanvasLayer : public CanvasLayer, > public BasicImplData > { > public: >diff -r f4c2a6f2b838 gfx/layers/ipc/ShadowLayerUtilsX11.cpp >--- a/gfx/layers/ipc/ShadowLayerUtilsX11.cpp Fri Oct 05 11:19:16 2012 -0700 >+++ b/gfx/layers/ipc/ShadowLayerUtilsX11.cpp Sat Oct 06 17:25:01 2012 -0500 >@@ -8,16 +8,17 @@ > #include "mozilla/layers/PLayers.h" > #include "mozilla/layers/ShadowLayers.h" > > #include "gfxPlatform.h" > > #include "gfxXlibSurface.h" > #include "mozilla/X11Util.h" > #include "cairo-xlib.h" > >+using namespace mozilla::gl; > > namespace mozilla { > namespace layers { > > // Return true if we're likely compositing using X and so should use > // Xlib surfaces in shadow layers. > static bool > UsingXCompositing() >diff -r f4c2a6f2b838 gfx/layers/ipc/ShadowLayers.h >--- a/gfx/layers/ipc/ShadowLayers.h Fri Oct 05 11:19:16 2012 -0700 >+++ b/gfx/layers/ipc/ShadowLayers.h Sat Oct 06 17:25:01 2012 -0500 >@@ -18,17 +18,16 @@ > class gfxSharedImageSurface; > > namespace mozilla { > > namespace gl { > class GLContext; > class TextureImage; > } >-using namespace gl; > > namespace layers { > > class Edit; > class EditReply; > class OptionalThebesBuffer; > class PLayerChild; > class PLayersChild; >@@ -411,18 +410,18 @@ public: > > virtual void NotifyShadowTreeTransaction() {} > > /** > * Try to open |aDescriptor| for direct texturing. If the > * underlying surface supports direct texturing, a non-null > * TextureImage is returned. Otherwise null is returned. > */ >- static already_AddRefed<TextureImage> >- OpenDescriptorForDirectTexturing(GLContext* aContext, >+ static already_AddRefed<mozilla::gl::TextureImage> >+ OpenDescriptorForDirectTexturing(mozilla::gl::GLContext* aContext, > const SurfaceDescriptor& aDescriptor, > GLenum aWrapMode); > > static void PlatformSyncBeforeReplyUpdate(); > > void SetCompositorID(uint32_t aID) > { > NS_ASSERTION(mCompositorID==0, "The compositor ID must be set only once.");

Matt, thanks for helping out! Before you fix them all, though, note that these two: > js/public/Utility.h:38:using namespace JS; > js/public/Utility.h:39:using namespace mozilla; should be kept.

Duly noted! What's the rationale for keeping those directives?

It allows people in js/src/ to use, for example, js::Value instead of JS::Value. They seem to quite like that.

Comment on attachment 668988 [details] [diff] [review] Removes "using namespace" directive from xpcom headers. Review of attachment 668988 [details] [diff] [review]: ----------------------------------------------------------------- 'using namespace' inside functions is perfectly fine.

Oh, right. I didn't think of that use case when I filed the bug and grepped.

I was wondering about that but figured I'd just let the reviewers decide. I won't remove them function scope anymore.

Comment on attachment 668840 [details] [diff] [review] Removes "using namespace" directive from gfx headers. Review of attachment 668840 [details] [diff] [review]: ----------------------------------------------------------------- Other than the extra 'mozilla::' this looks good. ::: gfx/layers/ipc/ShadowLayers.h @@ +415,5 @@ > * underlying surface supports direct texturing, a non-null > * TextureImage is returned. Otherwise null is returned. > */ > + static already_AddRefed<mozilla::gl::TextureImage> > + OpenDescriptorForDirectTexturing(mozilla::gl::GLContext* aContext, The mozilla qualifier here is unnecessary because we're already in the mozilla namespace. gl::TextureImage and gl::GLContext should be sufficient.

Removed redundancies.

Matt, since Jeff has already granted the review, you don't need an additional review. Un-obsolete the reviewed patch, and mark yourself review+ on this updated patch with a comment like "carrying forward review+ from Jeff". Then find someone to land your patch --- I can do it.

Ok, thanks for letting me know. I think everything is alright now. Also, regarding header files: There's quite a few header files in the media directory with the "using namespace" directive currently commented out. Should I go ahead and remove these comments as well? E.g.: media/webrtc/trunk/src/voice_engine/include/voe_external_media.h 19:// using namespace webrtc

That sounds like a good idea -- as a separate patch. Otherwise they're going to create more such issues if they uncomment these statements. But it's less of a priority than fixing the bunch of remaining non-commented statements.

Comment on attachment 668840 [details] [diff] [review] Removes "using namespace" directive from gfx headers. carrying forward review+ from Jeff

Sounds good. I will do patches in media, one for getting rid of the comments and another for deleting the lingering using directives. Also, should I mark "checkin ?" on my review+ patch? Is that what you mean by finding someone to land the patch?

Isn't the webrtc code imported from upstream? You may want to ask Randell Jesup (who I just CCed) about whether there's any restrictions about modifying it.

(In reply to Matt Joras from comment #35) > Also, should I mark "checkin ?" on my review+ patch? Is that what you mean > by finding someone to land the patch? I think that's how you would do it yes. In the present case though, on this bug, I'll land your patches. Ping me if I forget.

Matt, please make patches that have Mercurial metadata -- your username and email address. The best way to do that is to configure your identity in Mercurial, and use Mercurial Queues to generate patches. Something like hg init --mq hg qnew my-patch hg qrefresh -m "Bug 798033 - Commit Message - r=jmuizelaar" hg export tip https://developer.mozilla.org/en-US/docs/Mercurial https://developer.mozilla.org/en-US/docs/Mercurial_Queues

This page has some instructions on how to set up Mercurial to produce patches in the proper format: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me

(In reply to Benoit Jacob [:bjacob] from comment #38) > Matt, please make patches that have Mercurial metadata -- your username and > email address. > > The best way to do that is to configure your identity in Mercurial, and use > Mercurial Queues to generate patches. > > Something like > > hg init --mq > hg qnew my-patch > hg qrefresh -m "Bug 798033 - Commit Message - r=jmuizelaar" > hg export tip > > https://developer.mozilla.org/en-US/docs/Mercurial > https://developer.mozilla.org/en-US/docs/Mercurial_Queues Ok, thanks. I will brush up my Mercurial know-how before attaching more patches. I was previously just following the basic instructions from https://developer.mozilla.org/en-US/docs/Creating_a_patch

(In reply to Matt Joras from comment #32) > Ok, thanks for letting me know. I think everything is alright now. > > Also, regarding header files: > There's quite a few header files in the media directory with the "using > namespace" directive currently commented out. Should I go ahead and remove > these comments as well? > E.g.: > media/webrtc/trunk/src/voice_engine/include/voe_external_media.h > 19:// using namespace webrtc Um, please don't touch that. That's in a comment telling you how to use the classes in this file. And, on top of that, it's imported code (media/webrtc/trunk), and spurious patches for style or non-functionality changes will make our (my) life a living hell trying to import updates, which I'll be doing regularly. In general, avoid mass changes to imported code. See current discussion in m.d.platform about how imported code is maintained. We have more ability to change code in media/webrtc/signaling (since we've largely taken over maintenance of that and have extensively modified it). media/mtransport/third_party is somewhere in-between, and media/mtransport is new code, but it's written in the same style as the third_party directories since it's deeply integrated with them. On any suggested change of this nature to media/webrtc or media/mtransport, please get a review from me or someone I delegate it to.

(In reply to Randell Jesup [:jesup] from comment #41) > (In reply to Matt Joras from comment #32) > > Ok, thanks for letting me know. I think everything is alright now. > > > > Also, regarding header files: > > There's quite a few header files in the media directory with the "using > > namespace" directive currently commented out. Should I go ahead and remove > > these comments as well? > > E.g.: > > media/webrtc/trunk/src/voice_engine/include/voe_external_media.h > > 19:// using namespace webrtc > > Um, please don't touch that. That's in a comment telling you how to use the > classes in this file. And, on top of that, it's imported code > (media/webrtc/trunk), and spurious patches for style or non-functionality > changes will make our (my) life a living hell trying to import updates, > which I'll be doing regularly. > > In general, avoid mass changes to imported code. See current discussion in > m.d.platform about how imported code is maintained. > > We have more ability to change code in media/webrtc/signaling (since we've > largely taken over maintenance of that and have extensively modified it). > media/mtransport/third_party is somewhere in-between, and media/mtransport > is new code, but it's written in the same style as the third_party > directories since it's deeply integrated with them. > > On any suggested change of this nature to media/webrtc or media/mtransport, > please get a review from me or someone I delegate it to. Duly noted. I didn't know that code was upstream and hadn't gotten to checking the context of the commented out directives. On the subject of upstream code, I figured I shouldn't be modifying any of that, however I don't really know how to tell what is and isn't upstream right now. Some stuff seems pretty obviously upstream, e.g. toolkit/crashreporter/google-breakpad/src/third_party/glog/src/utilities.h Is there any other upstream code in the original list I should take note of?

These probably are upstream code: other-licenses/snappy/src/snappy-stubs-internal.h toolkit/crashreporter/google-breakpad/src/third_party These are not real source files, they are from an object directory: obj-mobile-debug/dist/include/js/Utility.h:38:using namespace JS; obj-mobile-debug/dist/include/js/Utility.h:39:using namespace mozilla;

Bugzilla question -- After my initial review of a bug has been accepted, what do I generally do to submit fixes on the patch after that? I now know I DON'T obsolete the initially reviewed patch, and then my next one I mark as review+. However, after that, do I mark each subsequent one as review+ and obsolete the previous review+ patch?

I tried making this patch the more "proper" way, let me know if it's still not the right format.

(In reply to Matt Joras from comment #44) > Bugzilla question -- After my initial review of a bug has been accepted, > what do I generally do to submit fixes on the patch after that? I now know I > DON'T obsolete the initially reviewed patch, and then my next one I mark as > review+. However, after that, do I mark each subsequent one as review+ and > obsolete the previous review+ patch? If it turns out that the patch was really broken and hasn't landed yet, better obsolete it. If the patch was fine to land and/or is already landed, there is no point in obsoleting it. Does that help?

(In reply to Benoit Jacob [:bjacob] from comment #46) > (In reply to Matt Joras from comment #44) > > Bugzilla question -- After my initial review of a bug has been accepted, > > what do I generally do to submit fixes on the patch after that? I now know I > > DON'T obsolete the initially reviewed patch, and then my next one I mark as > > review+. However, after that, do I mark each subsequent one as review+ and > > obsolete the previous review+ patch? > > If it turns out that the patch was really broken and hasn't landed yet, > better obsolete it. If the patch was fine to land and/or is already landed, > there is no point in obsoleting it. Does that help? I think that clears things. So with the gfx-namespace patch, should I attach my version that has the correct metadata and obsolete the one I have up now? Also, what's the best way to know when the patch has landed? Seeing if the change is in my latest pull?

what's a reasonable patch size? I just did the dom/camera directory, and it's already pretty sizable. If I do the entire dom directory in one patch it might be a tad unruly in size.

(In reply to Matt Joras from comment #47) > Also, what's the best way to know when the patch has landed? Seeing if the > change is in my latest pull? Somebody will post a link to the commit in the bug. The patch will land on mozilla-inbound first, then later somebody else will merge it to mozilla-central, which is the main repository that is used to make Nightly builds. (In reply to Matt Joras from comment #48) > what's a reasonable patch size? I just did the dom/camera directory, and > it's already pretty sizable. If I do the entire dom directory in one patch > it might be a tad unruly in size. What matters more is how many kinds of changes you are doing. Presumably you are just doing the namespace thing and not random other changes, so it is fine if the patch is big, for the purposes of the reviewer. For this kind of mechanical change, the reviewer will mostly just be eyeballing what you did, relying on the compiler being able to detect any severe errors. If you'd like to break down your patches a bit more by subdirectory or whatnot, that's okay, too.

(In reply to Matt Joras from comment #47) > I think that clears things. So with the gfx-namespace patch, should I attach > my version that has the correct metadata and obsolete the one I have up now? Yup! Obsolete the previous one; do not obsolete the earlier one that had received the review.

Made patch with proper mercurial metadata.

Is putting the 'using namespace' directive in macros okay style? For example, the dom/workers directory uses the following macro (defined in Workers.h) extensively: #define USING_WORKERS_NAMESPACE \ using namespace mozilla::dom::workers; I'm not really sure why someone decided to make a macro for this, but it isn't really violating the style guideline. Should I just leave this one alone?

It looks like it is only used in .cpp files, so it seems reasonable to just leave it alone.

Backed out for build failures at least on Mac, Windows, and Android: https://hg.mozilla.org/integration/mozilla-inbound/rev/224fddb79a38 Next time I'll put it on tryserver first :-P Matt, do you have tryserver ( == level 1 ) access?

Also, the failures are here. Click the red Bs and then 'view log'.

Woops... I do not as of now. I will do that before proceeding further with testing that patch.

I completely forgot about other build environments... Definitely my fault. After I get try server access I'll be able to test the other build targets before attaching anymore patches.

Accounts for other builds, just added using namespace to more .cpp files. Can't test until I have try server access but here it is.

Pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=845170895de5 I suggest you apply for level 1 access: http://www.mozilla.org/hacking/commit-access-policy/ http://www.mozilla.org/hacking/committer/

Cool, so the gfx patch looks ok now? I'm definitely going to apply for level 1 access. I'm sure most of my patches are going to breaking platform-specific files.

https://bugzilla.mozilla.org/show_bug.cgi?id=802327 is my access request bug. Can you vouch for me, Benoit?

So I'm not sure what's holding up the tryserver access, but I have most of the patches as far as I can get them without testing them on other platforms. Once I can do that I'll attach them all here unless anyone thinks I should do otherwise. Also, is the gfx patch okay to go now?

Sorry, dropped that ball. Landing your gfx patch asap. Until you get L1 access, I can push to try any patch you like. Please just give me a combined patch so it's easier for me to just apply it and push to try.

Looks like the content-namespace builds. Should be good to go I think: https://tbpl.mozilla.org/?tree=Try&rev=3c8274feedbe

Yep. Note: you want -u none, not -u all, for tryserver pushes for this bug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c25bc74b7fd3 Note: I removed the double quotes around the commit message in this patch.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b9ec7a9582c9 - as https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c25bc74b7fd3 says, that wound up lacking a little bit in the "actually compiles" department. Somebody change things out from under you?

Hm. Looks like it. In file included from ../../../../../content/media/raw/nsRawDecoder.cpp:5: In file included from ../../../dist/include/nsBuiltinDecoderStateMachine.h:82: ../../../dist/include/nsAudioAvailableEventManager.h:32:30: error: unknown type name 'AudioDataValue'; did you mean 'js::AudioDataValue'? void QueueWrittenAudioData(AudioDataValue* aAudioData, ^~~~~~~~~~~~~~ js::AudioDataValue ../../../dist/include/AudioSampleFormat.h:45:54: note: 'js::AudioDataValue' declared here typedef AudioSampleTraits<AUDIO_OUTPUT_FORMAT>::Type AudioDataValue;

This one should work.

Could you do a little try push on just one of the platforms where the previous patch failed to build, with -u none?

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf52e5234e91 So what's this checkin+ thing --- should I put checkin+ on patches I've landed? I've never done that.

It is handy if you have a patch with a bunch of parts, and you have only landed some of them.

Woops, I had run a new try on that newer patch but forgot to link to it last night, my bad. Also, is it just me or is the Windows compile output from the tryserver really difficult to parse? It doesn't seem to do the nifty thing the Unix-ish platforms do, skipping the lines until the relevant error sections.

I haven't seen a Windows vs Unix difference there. But for each build job, there are two different links: "view full log" and "view brief log". Sounds like you wanted the brief one.

Well at least for the patches I've tried, Windows builds have never given me an actual brief log when they fail to build. E.g. https://tbpl.mozilla.org/?tree=Try&rev=fbbbec604dc6 When I hit "view brief log" it actually shows the entire log for Windows, as well as saying "No errors or warnings found. See below for the full log.", which seems rather odd.

Hm yes, that means that our error logs parser was confused by that output. Bad luck; I guess the most you could do about this is file a bug against TBPL (I don't know which bugzilla component that would be).

That would be "Component Where We File Duplicates Of Bug 803466" ;)

Comment on attachment 675920 [details] [diff] [review] Removes 'using namespace' from dom headers Kyle is a better reviewer for DOM.

Changed reviewer in patch message to khuey.

Try results for toolkit patch: https://tbpl.mozilla.org/?tree=Try&rev=0b7d88dd2f84

Recent try for widget patch: https://tbpl.mozilla.org/?tree=Try&rev=0b7d88dd2f84 Recent try for toolkit patch: https://tbpl.mozilla.org/?tree=Try&rev=3495c28ce55a Both should be okay to checkin.

Comment on attachment 675972 [details] [diff] [review] Removes 'using namespace' from js headers Review of attachment 675972 [details] [diff] [review]: ----------------------------------------------------------------- As long as this compiles, it looks good to me.

widget and toolkit: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb9c03d12542 https://hg.mozilla.org/integration/mozilla-inbound/rev/16772d0c770c

Recent try for js patch: https://tbpl.mozilla.org/?tree=Try&rev=4c93b1ee8dd5

https://hg.mozilla.org/mozilla-central/rev/bb9c03d12542 https://hg.mozilla.org/mozilla-central/rev/16772d0c770c

Refresh after conflicts.

Refresh to fix some conflicts.

Do any of these patches fix netwerk/protocol/device/GonkCaptureProvider.h? ("using namespace android").

That's the next (and last?) one on my todo list. Haven't gotten around to it quite yet.

Comment on attachment 678033 [details] [diff] [review] Removes 'using namespace' from dom headers Review of attachment 678033 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/CameraControlImpl.cpp @@ +8,5 @@ > #include "CameraControlImpl.h" > #include "CameraCommon.h" > > using namespace mozilla; > +using namespace dom; Please do 'using namespace mozilla::dom;' to make it clearer what's going on. ::: dom/camera/DOMCameraCapabilities.cpp @@ +12,5 @@ > #include "DOMCameraCapabilities.h" > #include "CameraCommon.h" > > using namespace mozilla; > +using namespace dom; Please do 'using namespace mozilla::dom'. ::: dom/camera/FallbackCameraControl.cpp @@ +5,5 @@ > #include "DOMCameraControl.h" > #include "CameraControlImpl.h" > > using namespace mozilla; > +using namespace dom; mozilla::dom

Carrying on review from khuey

Update. Someone committed a partial fix for this patch before this one was committed.

Fun: |using namespace| stuff is being discussed on dev-platform list, 'Proposed style guide modification: using declarations and nested namespaces' thread.

Can you make a try push with patches remaining to land?

Interesting! People have been "cleaning" up while I've been doing this. It's happened a few times that I've noticed. They've been running for a while! Try url (currently still building for windows): https://tbpl.mozilla.org/?tree=Try&rev=61bc588eba39

DOM and JS: https://hg.mozilla.org/integration/mozilla-inbound/rev/5054c2552a30 https://hg.mozilla.org/integration/mozilla-inbound/rev/a1418d7f9671 Is this the end or are more patches coming? Thanks a lot by the way!

Had to back out the DOM patch for b2g bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/91480ec2061e https://tbpl.mozilla.org/php/getParsedLog.php?id=16925373&tree=Mozilla-Inbound FileReaderSyncBinding.cpp In file included from /builds/slave/m-in-ics-armv7a-gecko/build/dom/camera/GonkCameraManager.cpp:19: /builds/slave/m-in-ics-armv7a-gecko/build/dom/camera/GonkCameraControl.h:49: error: 'CameraSize' was not declared in this scope /builds/slave/m-in-ics-armv7a-gecko/build/dom/camera/GonkCameraControl.h:49: error: template argument 1 is invalid /builds/slave/m-in-ics-armv7a-gecko/build/dom/camera/GonkCameraControl.h:55: error: 'CameraSize' was not declared in this scope /builds/slave/m-in-ics-armv7a-gecko/build/dom/camera/GonkCameraControl.h:55: error: template argument 1 is invalid

Benoit, I've got one more patch to finish up for the netwerk/protocol/device/GonkCaptureProvider.h file. It's a pesky one to do since I have to run a try to check the build. And no problem! It's been a nice experience. Ah, yeah I never ran a try with B2G. I'm trying that now. I'll post the updated patch when I've confirmed it builds.

Changing the dom patch to work with B2G is proving more difficult than I anticipated. It might be a while before I can dig into it.

I have a B2G build environment, I will take care of it then.

This fixes the remaining cases in dom/camera.

B2G try push: https://tbpl.mozilla.org/?tree=Try&rev=96367335f216

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb83b74c287 https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2ea9461eec https://hg.mozilla.org/integration/mozilla-inbound/rev/b2043b1dd93d

Waldo pointed out in IRC that this is empty: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2ea9461eec

Oh, that explains things. So it had already landed, and contrary to the DOM patch, it hadn't been backed out. When I re-applied it today, I got rejects... and didn't notice that the rejects were _all_ of the patch.

https://hg.mozilla.org/mozilla-central/rev/bcb83b74c287 https://hg.mozilla.org/mozilla-central/rev/6f2ea9461eec https://hg.mozilla.org/mozilla-central/rev/b2043b1dd93d

Can you confirm that you're still working on this bug?

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6317c0979e90

Comment on attachment 8337360 [details] [diff] [review] Remove 'using namespace' from dom/ headers Review of attachment 8337360 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Thanks.

Comment on attachment 8337362 [details] [diff] [review] Remove 'using namespace' from toolkit/ headers I'd prefer benjamin to look at this

Rebased patch.

Rebased patch and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5782993941f1

https://hg.mozilla.org/integration/mozilla-inbound/rev/0974fda4a154 https://hg.mozilla.org/integration/mozilla-inbound/rev/6f362df0b724 https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b06549f680

https://hg.mozilla.org/mozilla-central/rev/0974fda4a154 https://hg.mozilla.org/mozilla-central/rev/6f362df0b724 https://hg.mozilla.org/mozilla-central/rev/a8b06549f680

Comment on attachment 8363044 [details] [diff] [review] Remove 'using namespace' intl/uconv/ucvcn headers I should have pointed out this when I reviewed bug 935453... Thanks.

Comment on attachment 8363257 [details] [diff] [review] Removes using namespace statements from 2 header files I'm not a WebRTC peer. Could you ask someone else for review?

(In reply to snigdhaagarwal93 from comment #132) > Created attachment 8363257 [details] [diff] [review] > Removes using namespace statements from 2 header files To find a suitable person to review your patch, take a look at: https://wiki.mozilla.org/Modules/All Alternatively, you might want to look at the hg blame of the relevant files to find out who have reviewed previous changes. I'll change you to the assignee to take care of the last few uses.

Comment on attachment 8363041 [details] [diff] [review] Remove 'using namespace' from CompileInfo-inl.h https://hg.mozilla.org/integration/mozilla-inbound/rev/64fac1e050b4

Comment on attachment 8363044 [details] [diff] [review] Remove 'using namespace' intl/uconv/ucvcn headers https://hg.mozilla.org/integration/mozilla-inbound/rev/45166cf53552

https://hg.mozilla.org/mozilla-central/rev/64fac1e050b4 https://hg.mozilla.org/mozilla-central/rev/45166cf53552

Comment on attachment 8364211 [details] [diff] [review] Removes using namespace statement from 5 header files Review of attachment 8364211 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. Thanks for doing this. ::: media/webrtc/signaling/test/FakePCObserver.h @@ -30,5 @@ > class nsIDOMWindow; > class nsIDOMDataChannel; > > namespace test { > -using namespace mozilla::dom; Note that this using namespace statement is not at file scope, but rather inside another namespace named test, hence technically not within the mandate of this bug report. But an improvement nonetheless.

Comment on attachment 8364211 [details] [diff] [review] Removes using namespace statement from 5 header files Cancelling review request from me since you've already got Jim's approval for the Metro side of things.

Comment on attachment 8364211 [details] [diff] [review] Removes using namespace statement from 5 header files There are conflicts in media/webrtc and I'm not sure where they came from. What did you diff this against?

Comment on attachment 8364211 [details] [diff] [review] Removes using namespace statement from 5 header files Review of attachment 8364211 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/d3d10/ImageLayerD3D10.h @@ +14,1 @@ > Nit: Please remove one empty line here. ::: media/webrtc/signaling/src/media/CSFMediaProvider.h @@ -8,5 @@ > #if __cplusplus > > #include <string> > > -//using namespace std; This line was already modified in your previous patch. I think it would be best to update your previous patch instead of touching it again here. (Also remove one empty line here as well.) ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.h @@ -39,5 @@ > > namespace sipcc { > > -//using namespace mozilla; > - Remove this in the earlier patch. ::: widget/android/AndroidBridge.h @@ +51,1 @@ > Nit: Please remove one empty line here. ::: widget/windows/winrt/ToastNotificationHandler.h @@ +13,1 @@ > Nit: Please remove one empty line here. ::: widget/windows/winrt/UIABridgePrivate.h @@ +27,1 @@ > Nit: Please remove one empty line here.

Sorry for the multiple attachments. I have compiled the changes of the both the previous patches into this one. Hopefully, there should be no conflict problems in this one. This patch handles using namespace statements in all remaining header files under this bug.

Comment on attachment 8365376 [details] [diff] [review] Removes using namespace statements Review of attachment 8365376 [details] [diff] [review]: ----------------------------------------------------------------- r+ for what I care about

Comment on attachment 8365376 [details] [diff] [review] Removes using namespace statements Review of attachment 8365376 [details] [diff] [review]: ----------------------------------------------------------------- The WebRTC parts of this look fine.

Comment on attachment 8365376 [details] [diff] [review] Removes using namespace statements You already have r+ from me on the previous patch, and I see no substantive WebRTC difference, so I don't need to see it again. It is fine to set r+ yourself on a followup and say "Carrying forward r+ from jib" in the comment, provided there's nothing new for me to look at.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #147) > Comment on attachment 8365376 [details] [diff] [review] > Removes using namespace statements > > You already have r+ from me on the previous patch, and I see no substantive > WebRTC difference, so I don't need to see it again. > > It is fine to set r+ yourself on a followup and say "Carrying forward r+ > from jib" in the comment, provided there's nothing new for me to look at. Didn't know that was allowed. Will keep that in mind from next time onwards. :)

Requesting review for pending AndroidBridge.h (Carrying forward r+ from jib, rjesup,ekr,jmathies,ncameron for remaining files)

Comment on attachment 8367923 [details] [diff] [review] Updated Patch Redirecting review for AndroidBridge.h to someone who is more likely to have looked at it recently.

Snigdha, I pushed your patch to the try server: https://tbpl.mozilla.org/?tree=Try&rev=c4e68a616575 As you can see, it caused build failures on several platforms. If you need someone to repush an updated patch to try, feel free to ping me.

Comment on attachment 8384285 [details] [diff] [review] Build errors removed Build errors have been removed. Link to the tryserver push log: https://tbpl.mozilla.org/?tree=Try&rev=f177fbfc357e

Comment on attachment 8384285 [details] [diff] [review] Build errors removed Here's how 'checkin' works: 1. make sure that your patch has a proper commit message and your identity information. To do that, configure your user identity in your .hgrc file, and then, refresh/amend your patch with that and with a commit message of the form "Bug 123456 - What this patch is doing - r=your_reviewer". https://developer.mozilla.org/en/docs/Mercurial_FAQ should have some relevant information. 2. Once your patch has user information and a commit message, set the 'checkin' flag to '?' but do not set a requestee. Tree sheriffs search bugzilla for such flags every day and land these patches for us.

Oh, btw I can see that your patch already has user info, so all it needs now is a commit message (overwriting the one you put there for tryserver).

Sorry, forgot to pull earlier.

I just re-read https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch and it seems that we also need to set the checkin-needed keyword on the bug, which would explain why this didn't get landed yesterday.

Comment on attachment 8385705 [details] [diff] [review] Pull conflicts resolved Honestly, I ignored it because it was missing commit information and you (bjacob) were specifically on the checkin? request, so I assumed you would handle it. Also, you don't need to set both checkin? and checkin-needed. It's redundant.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f05267b4afc Is this still [leave open]?

Comment on attachment 8385705 [details] [diff] [review] Pull conflicts resolved Backed out for Android bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/a136d86479c5 https://tbpl.mozilla.org/php/getParsedLog.php?id=35658554&tree=Mozilla-Inbound

Tryserver log: https://tbpl.mozilla.org/?tree=Try&rev=fa7a27f7dc9e

Comment on attachment 8392369 [details] [diff] [review] Build errors removed Review of attachment 8392369 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp @@ +165,5 @@ > namespace sipcc { > > PeerConnectionCtx* PeerConnectionCtx::gInstance; > nsIThread* PeerConnectionCtx::gMainThread; > +mozilla::StaticRefPtr<mozilla::PeerConnectionCtxShutdown> PeerConnectionCtx::gPeerConnectionCtxShutdown; Could we instead add the "using namespace mozilla" to this cpp file (since we removed it from the equivalent header) instead of adding a bunch of mozilla::'s? This bug shouldn't (IMHO) morph into "remove using namespace mozilla from cpp files". Not a huge deal, but it also retains 'blame' status

Yeah, I will do that, and resubmit soon.

Required changes made in PeerConnectionCtx.cpp . Here, is the tryserver log: https://tbpl.mozilla.org/?tree=Try&rev=2056c8580d78

Comment on attachment 8399323 [details] [diff] [review] Changed Patch https://hg.mozilla.org/integration/mozilla-inbound/rev/e06713a76a41

The following changeset is now in Firefox Nightly: > e06713a76a41 Bug 798033 - Headers should generally not do "using namespace" at file scope. r=jib, r=jmathies, r=rjesup, r=ekr, r=ncameron, r=blassey Nightly Build Information: ID: 20140402030201 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central Download Links: > Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2 > Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2 > Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2 > Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg > Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe > Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe Previous Nightly Build Information: ID: 20140401030203 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4 Version: 31.0a1 TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8 URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

Comment on attachment 8424538 [details] [diff] [review] Remove 'using namespace' from remaining headers Review of attachment 8424538 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me for the file NetworkUtils.h. But I am not capable of reviewing nsDumpUtils.h. You probably need to ask dhylands for review again.