[Presentation WebAPI] Remove "using namespace" from dom/presentation cpp files

NEW
Assigned to

Status

()

Core
DOM
2 years ago
9 months ago

People

(Reporter: kershaw, Assigned: Udayan Baidya, NeedInfo)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: [lang=c++][good first bug] btpp-backlog)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
As described in bug 1258602 comment #9, we should prevent importing all namespace.
(Reporter)

Updated

2 years ago
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [lang=c++][good first bug] btpp-backlog

Comment 1

2 years ago
Hello, Can I work on this bug? Can someone assign it to me and guide me to resolve it?
Flags: needinfo?(kechang)
(Reporter)

Comment 2

2 years ago
(In reply to Akshay Jain from comment #1)
> Hello, Can I work on this bug? Can someone assign it to me and guide me to
> resolve it?

Sure.

You just need to remove "using namespace XXX;" from cpp files in dom/presentation and replace it by "namespace XXX {" like [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/Crypto.cpp#20
Flags: needinfo?(kechang)
(Reporter)

Updated

2 years ago
Assignee: nobody → ajain2

Comment 3

2 years ago
Hello Kershaw, thanks for that. I got it. I am a newbie so could you guide me through the steps of how to get this code on my local system so that I can start working on it immediately. I will appreciate your guidance. Thanks.
(Reporter)

Comment 4

2 years ago
(In reply to Akshay Jain from comment #3)
> Hello Kershaw, thanks for that. I got it. I am a newbie so could you guide
> me through the steps of how to get this code on my local system so that I
> can start working on it immediately. I will appreciate your guidance. Thanks.

Here are some steps for you.
1. Build Firefox: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
2. Summit a patch: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
3. Become a committer: https://www.mozilla.org/en-US/about/governance/policies/commit/
4. Push your patch to try server for testing: https://wiki.mozilla.org/ReleaseEngineering/TryServer

Those instructions might be out-of-date, so don't hesitate to let me know if you have any problem.

Comment 5

2 years ago
Hello Kershaw, I am working on the bug and encountered that in the file dom/presentation/PresentationService.cpp, the namespace are redundant. Is it something wrong or I am misinterpreting ?
(Reporter)

Comment 6

2 years ago
(In reply to Akshay Jain from comment #5)
> Hello Kershaw, I am working on the bug and encountered that in the file
> dom/presentation/PresentationService.cpp, the namespace are redundant. Is it
> something wrong or I am misinterpreting ?

Yes, I think you are right. They are redundant.

Comment 7

2 years ago
Created attachment 8753668 [details] [diff] [review]
namespaces.patch

No QA Mentor assigned. So can someone please review the Patch. No changes made to ../dom/presentation/PresentationService.cpp , /PresentationSessionInfo.cpp and ../dom/presentation/ipc/PresentationIPCService.cpp due to conflict in redundancy of using namespace and removing the redundancy results in build failure.
Thanks.
Flags: needinfo?(kechang)
(Reporter)

Comment 8

2 years ago
(In reply to Akshay Jain from comment #7)
> Created attachment 8753668 [details] [diff] [review]
> namespaces.patch
> 
> No QA Mentor assigned. So can someone please review the Patch.

You can ask smaug (bugs@pettay.fi) to review your patch.

> No changes
> made to ../dom/presentation/PresentationService.cpp ,
> /PresentationSessionInfo.cpp and
> ../dom/presentation/ipc/PresentationIPCService.cpp due to conflict in
> redundancy of using namespace and removing the redundancy results in build
> failure.

For PresentationService.cpp and PresentationSessionInfo.cpp, I think you can just remove "using namespace mozilla;" and "using namespace mozilla::dom;".
Flags: needinfo?(kechang)

Comment 9

2 years ago
Hello
I want to work on it. This would be my first bug. Can somebody assign it to me
(Reporter)

Comment 10

2 years ago
(In reply to Vivek Jain from comment #9)
> Hello
> I want to work on it. This would be my first bug. Can somebody assign it to
> me

I think Akshay had been working on this for a while. Not sure if he is still working now.

Akshay, could you update the status, please?
Flags: needinfo?(ajain2)

Comment 11

2 years ago
Comment on attachment 8753668 [details] [diff] [review]
namespaces.patch

Request for review due to Comment 8
Attachment #8753668 - Flags: review?(bugs)
Comment on attachment 8753668 [details] [diff] [review]
namespaces.patch

Totally ok to me if kershaw reviews this :)
Attachment #8753668 - Flags: review?(bugs) → review?(kechang)
(Reporter)

Comment 13

2 years ago
Comment on attachment 8753668 [details] [diff] [review]
namespaces.patch

Review of attachment 8753668 [details] [diff] [review]:
-----------------------------------------------------------------

As comment #8, please also modify the files below. Thanks.

dom/presentation/PresentationService.cpp
dom/presentation/PresentationSessionInfo.cpp
dom/presentation/ipc/PresentationIPCService.cpp
Attachment #8753668 - Flags: review?(kechang)

Comment 14

a year ago
Created attachment 8771385 [details] [diff] [review]
diff of changes made to "/dom/presentation" .cpp files where "using namespace XXX;" redundancy has been changed to just "namespace XXX {"

Was looking for a first bug to tackle and this one only looked half fixed so I took a shot at fixing it.
Attachment #8771385 - Flags: review?(kechang)
(Reporter)

Comment 15

a year ago
Comment on attachment 8771385 [details] [diff] [review]
diff of changes made to "/dom/presentation" .cpp files where "using namespace XXX;" redundancy has been changed to just "namespace XXX {"

Review of attachment 8771385 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this. I have some comments below.

1. Please remove the *.orig in your patch.

2. We usually expect to see 8 lines of context in bugzilla.
Please add U8 option when creating your patch next time. Thanks.

::: dom/presentation/PresentationService.cpp
@@ +775,5 @@
>    return service.forget();
>  }
> +
> +} //namespace dom
> +} //namespace mozilla

We need to move the end of namespace declaration from http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/dom/presentation/PresentationService.cpp#60 to the end of PresentationService.cpp.

::: dom/presentation/PresentationSessionInfo.cpp
@@ +37,5 @@
>  #endif
>  
> +namespace mozilla {
> +namespace dom {
> +namespace services {

I think we just need to put everything inside namespace dom.
Please align the declaration in PresentationSessionInfo.h.

::: dom/presentation/ipc/PresentationIPCService.cpp
@@ +15,5 @@
>  #include "PresentationIPCService.h"
>  
> +namespace mozilla {
> +namespace dom {
> +namespace ipc {

Same here. Please align the declaration in PresentationIPCService.h.

::: dom/presentation/ipc/PresentationParent.cpp
@@ +13,4 @@
>  #include "PresentationService.h"
>  #include "PresentationSessionInfo.h"
>  
> +namespace dom {

We also need "namespace mozilla {".
Attachment #8771385 - Flags: review?(kechang)

Comment 16

a year ago
Created attachment 8810204 [details] [diff] [review]
Made changes to "/dom/presentation" cpp files. Changed "using namespace xxx" to "namespace xxx {"
(Reporter)

Comment 17

a year ago
Comment on attachment 8810204 [details] [diff] [review]
Made changes to "/dom/presentation" cpp files. Changed "using namespace xxx" to "namespace xxx {"

Review of attachment 8810204 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

Could you please also modify dom/presentation/PresentationCallbacks.cpp? Thanks.
Attachment #8810204 - Flags: review+

Comment 18

a year ago
Created attachment 8810762 [details] [diff] [review]
Changed "using namespace xxx" to "namespace xxx {" in dom/presentation/PresentationCallbacks.cpp

Updated

a year ago
Attachment #8810204 - Attachment is obsolete: true

Comment 19

a year ago
(In reply to Kershaw Chang [:kershaw] from comment #17)
> Comment on attachment 8810204 [details] [diff] [review]
> Made changes to "/dom/presentation" cpp files. Changed "using namespace xxx"
> to "namespace xxx {"
> 
> Review of attachment 8810204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this!
> 
> Could you please also modify dom/presentation/PresentationCallbacks.cpp?
> Thanks.

Sorry for missing one of the files in my first patch. 

I'm new to open source. Do you have any suggestions on what I should look at next, something with C++ preferably. Thanks.
(Assignee)

Comment 20

9 months ago
Is this bug still up? Can i take it?
(Assignee)

Comment 21

9 months ago
Created attachment 8848713 [details] [diff] [review]
New Patch for removing namsespace

Please check my patch and do the needfull.
Attachment #8848713 - Flags: review?(bugs)
Attachment #8848713 - Flags: review?(bugs) → review?(kechang)
(Reporter)

Comment 22

9 months ago
Comment on attachment 8848713 [details] [diff] [review]
New Patch for removing namsespace

Review of attachment 8848713 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/presentation/PresentationAvailability.cpp
@@ +14,5 @@
>  #include "nsServiceManagerUtils.h"
>  #include "PresentationLog.h"
>  
> +namespace mozilla{
> + namespace dom{

Please remove the unnecessary space.

@@ +186,5 @@
>    }
>  }
> +
> +}
> +

Extra new line.

::: dom/presentation/PresentationCallbacks.cpp
@@ +12,5 @@
>  #include "nsServiceManagerUtils.h"
>  #include "PresentationCallbacks.h"
>  #include "PresentationRequest.h"
>  #include "PresentationConnection.h"
> +#include "nsThreadUtils"

Missing ".h"

I believe this patch would not be compiled.

@@ +16,5 @@
> +#include "nsThreadUtils"
> +#include "PresentationCallbacks.h"
> +#include "PresentationRequest.h"
> +#include "PresentationConnection.h"
> +#include "PresentationTransportBuilderConstructor.h"

These files are already included in the original file.

::: dom/presentation/PresentationConnection.cpp
@@ +578,5 @@
>    return NS_OK;
>  }
> +
> +}
> +

Extra new line.
Please take a look at other files, such as [1].

http://searchfox.org/mozilla-central/source/dom/presentation/AvailabilityCollection.cpp
Attachment #8848713 - Flags: review?(kechang) → review-
(Reporter)

Updated

9 months ago
Attachment #8753668 - Attachment is obsolete: true
(Reporter)

Updated

9 months ago
Attachment #8771385 - Attachment is obsolete: true
(Reporter)

Updated

9 months ago
Attachment #8810762 - Attachment is obsolete: true
(Reporter)

Updated

9 months ago
Assignee: ajain2 → udayanbaidya
You need to log in before you can comment on or make changes to this bug.