1337294 - PeerConnectionMedia.cpp: Unnecessary call to std::string::c_str()

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P4)

Description

[Sylvestre Ledru [:Sylvestre]]

A trivial bug for beginner, mostly to understand our workflows:

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?q=media%2Fwebrtc%2Fsignaling%2Fsrc%2Fpeerconnection%2FPeerConnectionMedia.cpp&redirect_type=direct#455
c_str() should be removed

Please use mozreview and not bugzilla for patch submission.

Comment 1

[Eric Rescorla (:ekr)]

Actually for stuff this trivial, I would prefer you just send patches through Bugzlla/Splinter.

Comment 2

[Shashwat Mishra]

I want to work on this bug.Can someone help me get started?

Comment 3

[Byron Campen [:bwc]]

It looks like the line numbers aren't right anymore on this. Do we mean to remove the c_str() here?

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?q=media%2Fwebrtc%2Fsignaling%2Fsrc%2Fpeerconnection%2FPeerConnectionMedia.cpp&redirect_type=direct#431

Comment 4

[Byron Campen [:bwc]]

(In reply to Shashwat Mishra from comment #2)
> I want to work on this bug.Can someone help me get started?

Sure. Where are you starting from? (dev environment, etc)

Comment 5

[Nils Ohlmeier [:drno]]

Yep I think the old link used to point at the c_str() call in line 431 https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#431

Comment 6

[pkssg2006]

How do i accept this bug ?

Comment 7

[Nils Ohlmeier [:drno]]

I think the ability to assign a bug requires to have a track record of work with Mozilla.

Since Shashwat asked here first in comment #2 I'm inclined to assign the bug to him.

Comment 8

[Shashwat Mishra]

(In reply to Byron Campen [:bwc] from comment #4)
> (In reply to Shashwat Mishra from comment #2)
> > I want to work on this bug.Can someone help me get started?
> 
> Sure. Where are you starting from? (dev environment, etc)

Well I am a newbie.Can you please guide me so as to what do I exactly have to do? Just remove the c_str() function and I am done?

Comment 9

[Shashwat Mishra]

(In reply to Nils Ohlmeier [:drno] from comment #7)
> I think the ability to assign a bug requires to have a track record of work
> with Mozilla.
> 
> Since Shashwat asked here first in comment #2 I'm inclined to assign the bug
> to him.

Thanks a lot sir.Can you tell me how should I proceed? I mean what do I have to do exactly? Just remove c_str() and I am done?

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Yes, just remove it.

Comment 11

[Shashwat Mishra]

(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Yes, just remove it.

Well I installed mercurial on ubuntu.When I typed the following command in the terminal to extract the source code in order to make changes it shows:- "abort: error: Name or service not known".
I typed in the following command in the terminal:-"
hg clone https://dxr.mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp"

Comment 12

[Nils Ohlmeier [:drno]]

DXR is only a code source browsing solution, not where we actually host our code.

Try this instead:
  hg clone https://hg.mozilla.org/mozilla-central

Note: cloning the full repository might take some time, as it is pretty big.

I think you actually have to clone the whole repository, and can not just clone/download a single file.

You should then be able to locate the code file at the following location:
  mozilla-central/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp

Probably you want to step into the mozilla-central directory and run:
  ./mach bootstrap

Once you are done with the bootstrapping, you can do:
  ./mach build

Once you have successfully locally build Firefox I would start with modifying the code.

Comment 13

[Vineet Reddy]

Attachment: Bug 1337294 - Removed the unnecessary call to c_str()

Review commit: https://reviewboard.mozilla.org/r/116510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116510/

Comment 14

[Vineet Reddy]

Comment on attachment 8842747 [details]
Bug 1337294 - Removed the unnecessary call to c_str()

https://reviewboard.mozilla.org/r/116510/#review118116

Removed the call to c.str()

Comment 15

[Shashwat Mishra]

(In reply to Nils Ohlmeier [:drno] from comment #12)
> DXR is only a code source browsing solution, not where we actually host our
> code.
> 
> Try this instead:
>   hg clone https://hg.mozilla.org/mozilla-central
> 
> Note: cloning the full repository might take some time, as it is pretty big.
> 
> I think you actually have to clone the whole repository, and can not just
> clone/download a single file.
> 
> You should then be able to locate the code file at the following location:
>  
> mozilla-central/media/webrtc/signaling/src/peerconnection/
> PeerConnectionMedia.cpp
> 
> Probably you want to step into the mozilla-central directory and run:
>   ./mach bootstrap
> 
> Once you are done with the bootstrapping, you can do:
>   ./mach build
> 
> Once you have successfully locally build Firefox I would start with
> modifying the code.

Thank you so much for your valuable guidance sir.

Comment 16

[Andi [:andi]]

(In reply to Shashwat Mishra from comment #15)
> (In reply to Nils Ohlmeier [:drno] from comment #12)
> > DXR is only a code source browsing solution, not where we actually host our
> > code.
> > 
> > Try this instead:
> >   hg clone https://hg.mozilla.org/mozilla-central
> > 
> > Note: cloning the full repository might take some time, as it is pretty big.
> > 
> > I think you actually have to clone the whole repository, and can not just
> > clone/download a single file.
> > 
> > You should then be able to locate the code file at the following location:
> >  
> > mozilla-central/media/webrtc/signaling/src/peerconnection/
> > PeerConnectionMedia.cpp
> > 
> > Probably you want to step into the mozilla-central directory and run:
> >   ./mach bootstrap
> > 
> > Once you are done with the bootstrapping, you can do:
> >   ./mach build
> > 
> > Once you have successfully locally build Firefox I would start with
> > modifying the code.
> 
> Thank you so much for your valuable guidance sir.

Hello,

I highly suggest to follow the basic steps provided here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions
It has detailed instructions on how you can build across all of our supported platforms.

Comment 17

[Shashwat Mishra]

Sorry for the de(In reply to Andi-Bogdan Postelnicu from comment #16)
> (In reply to Shashwat Mishra from comment #15)
> > (In reply to Nils Ohlmeier [:drno] from comment #12)
> > > DXR is only a code source browsing solution, not where we actually host our
> > > code.
> > > 
> > > Try this instead:
> > >   hg clone https://hg.mozilla.org/mozilla-central
> > > 
> > > Note: cloning the full repository might take some time, as it is pretty big.
> > > 
> > > I think you actually have to clone the whole repository, and can not just
> > > clone/download a single file.
> > > 
> > > You should then be able to locate the code file at the following location:
> > >  
> > > mozilla-central/media/webrtc/signaling/src/peerconnection/
> > > PeerConnectionMedia.cpp
> > > 
> > > Probably you want to step into the mozilla-central directory and run:
> > >   ./mach bootstrap
> > > 
> > > Once you are done with the bootstrapping, you can do:
> > >   ./mach build
> > > 
> > > Once you have successfully locally build Firefox I would start with
> > > modifying the code.
> > 
> > Thank you so much for your valuable guidance sir.
> 
> Hello,
> 
> I highly suggest to follow the basic steps provided here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions
> It has detailed instructions on how you can build across all of our
> supported platforms.

Sorry for the delay.Our college allows us to access only 2 GB of data per day and my data transfer limit exceeded.I'll do it as soon as I get the required amount of data.

Comment 18

[Hemant Singh Patwal]

Attachment: removed c_str() bug 1337294

Review commit: https://reviewboard.mozilla.org/r/128048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128048/

Comment 19

[Hemant Singh Patwal]

(In reply to Sylvestre Ledru [:sylvestre] from comment #10)
> Yes, just remove it.


I have fixed it https://reviewboard.mozilla.org/r/128046.Please review.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

I reassigned it to Vineet Reddy who proposed a patch for a while.

please send me an email if you want me to give you an easy bug.

Comment 21

[Hemant Singh Patwal]

(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> I reassigned it to Vineet Reddy who proposed a patch for a while.
> 
> please send me an email if you want me to give you an easy bug.

Glad to get this oppotunity.
[e-mail]

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8842747 [details]
Bug 1337294 - Removed the unnecessary call to c_str()

https://reviewboard.mozilla.org/r/116510/#review130616

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5a10d34a6ce
Removed the unnecessary call to c_str() r=jesup

Comment 24

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

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

Comment 25

[Hemant Singh Patwal]

Attachment: removed duplicate rules

Review commit: https://reviewboard.mozilla.org/r/132728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132728/

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 8860769 [details]
removed duplicate rules

https://reviewboard.mozilla.org/r/132728/#review135644

Hi Hemant,

You need to make sure that you push from a clean branch without any other of your own commits, and include the bug number and reviewer in the commit message, then mozreview will attach it to the right bug.

Comment 27

[Gerry Chang [:gchang]]

Mark 54 won't fix as this is late for Beta54.