Closed Bug 773204 Opened 12 years ago Closed 12 years ago

Various SIPCC Cleanup and bug fix

Categories

(Core :: WebRTC: Signaling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: emannion, Assigned: emannion)

Details

(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])

Attachments

(2 files, 4 obsolete files)

INCLUDED IN THIS PATCH

Added destroy method to PeerConnectionCtx.
Better state handling in PeerConnectionImpl in OnCallEvent
Fixed Warnings
Removed video pref (CC_SDP_DIRECTION_SENDRECV) from PC and API layer, needs completion when we implement mediaStreams.
Prevent audio and video media providers starting
Removed and ROAP code
Attached patch SIPCC Fixes and Cleanup (obsolete) — Splinter Review
This patch builds and runs on Mac and Linux

Is also has I believe a fix for shutdown.  To do this I added a destroy static method onto PeerConnectionCtx that is called when the tests finish.
Attachment #641616 - Flags: feedback?(ethanhugg)
Attachment #641616 - Flags: feedback?(ekr)
Comment on attachment 641616 [details] [diff] [review]
SIPCC Fixes and Cleanup

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

I see several places with "printf", "cout <<" or "cerr <<"  I think those should all be changed to CSFLogDebug or CSFLogDebugS which I believe are accessible from anywhere in signaling code now.  The only part of this code that should be going straight out is the unittest code.

You may want to be more consistent on how code is commented out.  I've found using '/* */' on multiple lines to be a bit sketchy when an auto-merge could put a line in between with a comment.  I don't know if there's a Mozilla standard, but using #if 0 like you do other places might be better, or '//' if it's only a few lines.

   	if (sdpmode == TRUE) {

This is a nit, but if you're in C without a bool type and using an int for a bool you could treat it like one with if (sdpmode) and if (!sdpmode).  This is one of the few things listed in the style guide - https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

#ifndef NO_WEBRTC_VIDEO
  // <emannion> return pVideo ? pVideo->getMediaTermination() : NULL;
  return NULL;
#else

I know this code will probably be removed soon, but this comment should probably say why you commented this out.  Also we should take a note to get rid of the def NO_WEBRTC_VIDEO
Attachment #641616 - Flags: feedback?(ethanhugg) → feedback+
Comment on attachment 641616 [details] [diff] [review]
SIPCC Fixes and Cleanup

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

::: media/webrtc/signaling/include/CC_CallInfo.h
@@ +80,5 @@
> +           print Call state
> +           @param [in] handle - call info handle
> +           @return call state as string
> +         */
> +        virtual std::string printCallState (cc_call_state_t state) = 0;

This doesn't print, so maybe a new name is needed.

@@ +87,5 @@
> +           print Call event
> +           @param [in] call event
> +           @return call event as string
> +         */
> +        virtual std::string printCallEvent (ccapi_call_event_e callEvent) = 0;

Same.

::: media/webrtc/signaling/include/PeerConnection.h
@@ +171,5 @@
>    // ICE state
>    virtual IceState ice_state() = 0;
>  
>    // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc.
> +  virtual void Close() = 0;

I actually think i like "Shutdown()" better.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +196,5 @@
>    // one for video
>    mIceStreams.push_back(mIceCtx->CreateStream("stream1", 2));
>    mIceStreams.push_back(mIceCtx->CreateStream("stream1", 2));
>  
> +  for (int i=0; i<(int)mIceStreams.size(); i++) {

size() is size_t()? 

Regardless, change i to the appropriate type.

Also, please don't use C-style casts in C++ code.

::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
@@ +257,5 @@
>  	static const char fname[] = "CC_CallFeature_CreateOffer";
>  	CCAPP_DEBUG(DEB_L_C_F_PREFIX, DEB_L_C_F_PREFIX_ARGS(SIP_CC_PROV, GET_CALL_ID(call_handle),
>  			GET_LINE_ID(call_handle), fname));
>  
> +	return cc_invokeFeature(call_handle, CC_FEATURE_CREATEOFFER, CC_SDP_DIRECTION_SENDRECV, JSEP_NO_ACTION, NULL);

Why not remove the numbers argument entirely.

@@ +262,3 @@
>  }
>  
> +cc_return_t CC_CallFeature_CreateAnswer(cc_call_handle_t call_handle, const char* sdp) {

Why is sdp not string_t?

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call.c
@@ +125,4 @@
>  	return CC_CallFeature_dial(handle, video_pref, digits);
>  }
>  
> +cc_return_t CCAPI_CreateOffer(cc_call_handle_t handle, int audioPort, int videoPort) {

Why do we still need the ports here?

@@ +130,3 @@
>  }
>  
> +cc_return_t CCAPI_CreateAnswer(cc_call_handle_t handle, cc_string_t offersdp, int audioPort, int videoPort) {

And here?

@@ +134,4 @@
>  }
>  
> +cc_return_t CCAPI_SetLocalDescription(cc_call_handle_t handle, cc_jsep_action_t action, cc_string_t sdp) {
> +	return CC_CallFeature_SetLocalDescription(handle, action, sdp);

Aren't these const strings?

@@ +138,3 @@
>  }
>  
> +cc_return_t CCAPI_SetRemoteDescription(cc_call_handle_t handle, cc_jsep_action_t action, cc_string_t sdp) {

And herE?

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3260,3 @@
>        }    
>  
>        fsmdef_init_dcb(dcb, call_id, FSMDEF_CALL_TYPE_NONE, NULL, line, fcb);

Dont' we still need error checks here?

@@ +3264,4 @@
>        fsm_set_fcb_dcbs(dcb);
>      }
>  
> +    /* cpr_assert(strlen(msg->data.pc.pc_handle) < PC_HANDLE_SIZE); */

Can you put this assert in, actually? I think it's just an include issue to get it to compile. Or maybe w need a different assert funciton.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1363,5 @@
>      		GSM_ERR_MSG("Failed to retrieve ICE attribute\n");
>      		cpr_free(ice_attribs);
>      		return FALSE;
>      	}
> +        (*ice_attribs)[i] = (char *) cpr_calloc(1, strlen(ice_attrib) + 1);

This need not be calloc, btw.

@@ +4028,5 @@
>          sip_config_get_nat_ipaddr(&ipaddr);
>      }
>  
> +
> +	ipaddr2dotted(addr_str, &ipaddr);

We're going to want to remove all this IP code eventually.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +429,5 @@
>  
>      char peerconnection[PC_HANDLE_SIZE];  /* A handle to the peerconnection */
>      boolean peerconnection_set;
>  
> +    char ice_ufrag[ICE_STRING_SIZE];

This still feels like it should be dynamically allocated.

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +611,5 @@
>      static const char fname[] = "lsm_open_rx";
>      int           port_allocated = 0;
>      cc_rcs_t      rc = CC_RC_ERROR;
>      fsmdef_dcb_t *dcb;
> +    int           sdpmode = 0;

If you are going to compare sdpmode against TRUE you should have it be boolean.

@@ +643,5 @@
>                "requested port", data->port);
>  
>      if (data->keep == TRUE) {
>  
> +    	//Todo IPv6: Add interface call for IPv6

Actually doesn't this need replacement later if we are in SDPmode.

@@ +745,5 @@
>  {
>      static const char fname[] = "lsm_close_rx";
>      fsmdef_media_t *start_media, *end_media;
>      fsmdef_dcb_t   *dcb;
> +    int             sdpmode = 0;

Same comment about boolean here.

@@ +820,5 @@
>  {
>      fsmdef_media_t *start_media, *end_media;
>      fsmdef_dcb_t   *dcb;
>      static const char fname[] = "lsm_close_tx";
> +    int             sdpmode = 0;

And here.

@@ +905,5 @@
>      fsmdef_media_t *start_media, *end_media;
>      boolean has_checked_conference = FALSE;
>      fsmdef_dcb_t   *dcb, *grp_id_dcb;
>      vcm_mediaAttrs_t attrs;
> +    int              sdpmode;

And here.

@@ +1850,5 @@
>      static const char fname[] = "lsm_release_port";
>      fsmdef_media_t *start_media, *end_media;
>      fsmdef_dcb_t   *dcb;
>      fsmdef_media_t *media;
> +    int            sdpmode = 0;

And here.

@@ +6425,5 @@
>  static void lsm_util_start_tone(vcm_tones_t tone, short alert_info,
>          cc_call_handle_t call_handle, groupid_t group_id,
>          streamid_t stream_id, uint16_t direction) {
>  
> +	int sdpmode;

and her.e..

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +5150,5 @@
>  
>      ptr = sdp_getnextstrtok(ptr, tmp, "\r\n", &result);
>      if (result != SDP_SUCCESS){
>   	/* return success just means attribute not found */
>        /* TODO(emannion): is this really right? how can this happen? */

Can you address thsi comment? When would we be called if there was no attr?

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_threads.c
@@ +122,5 @@
>           * such as a show command or walking the list to ensure
>           * that an application does not attempt to create
>           * the same thread twice.
>           */
> +        threadPtr->u.handleInt = (uint32_t)threadId;

Are we sure that thread ids are only 4 bytes long?

::: media/webrtc/signaling/src/sipcc/include/cc_call_feature.h
@@ +190,2 @@
>  
> +cc_return_t CC_CallFeature_CreateAnswer(cc_call_handle_t call_handle, const char* sdp);

string_t here and below?

::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
@@ +561,5 @@
>  /*
>   * This method works asynchronously, there will be an onCallEvent with the resulting SDP
>   * When Constraints are implemented the Audio and Video port will not be a parameter to CCAPI_CreateAnswer
>   */
> +int CC_SIPCCCall::createOffer (const std::string& hints) {

We don't seemt o need ports here.

@@ +568,5 @@
>  
>  /*
>   * This method works asynchronously, there will be an onCallEvent with the resulting SDP
>   */
> +int CC_SIPCCCall::createAnswer (const std::string & hints, const std::string & offersdp) {

Or here.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +379,5 @@
>  
>  class SignalingTest : public ::testing::Test {
>   public:
> +
> +  ~SignalingTest() {

Put this in TearDown()?
(In reply to Ethan Hugg [:ehugg] from comment #2)
> Comment on attachment 641616 [details] [diff] [review]
> SIPCC Fixes and Cleanup
> 
> Review of attachment 641616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see several places with "printf", "cout <<" or "cerr <<"  I think those
> should all be changed to CSFLogDebug or CSFLogDebugS which I believe are
> accessible from anywhere in signaling code now.  The only part of this code
> that should be going straight out is the unittest code.

done now

> 
> You may want to be more consistent on how code is commented out.  I've found
> using '/* */' on multiple lines to be a bit sketchy when an auto-merge could
> put a line in between with a comment.  I don't know if there's a Mozilla
> standard, but using #if 0 like you do other places might be better, or '//'
> if it's only a few lines.
> 

agreed and implemented

>    	if (sdpmode == TRUE) {
> 
> This is a nit, but if you're in C without a bool type and using an int for a
> bool you could treat it like one with if (sdpmode) and if (!sdpmode).  This
> is one of the few things listed in the style guide -
> https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
> 
> #ifndef NO_WEBRTC_VIDEO
>   // <emannion> return pVideo ? pVideo->getMediaTermination() : NULL;
>   return NULL;
> #else
> 
> I know this code will probably be removed soon, but this comment should
> probably say why you commented this out.  Also we should take a note to get
> rid of the def NO_WEBRTC_VIDEO

note taken and comments improved
(In reply to Eric Rescorla from comment #3)
> Comment on attachment 641616 [details] [diff] [review]
> SIPCC Fixes and Cleanup
> 
> Review of attachment 641616 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/include/CC_CallInfo.h
> @@ +80,5 @@
> > +           print Call state
> > +           @param [in] handle - call info handle
> > +           @return call state as string
> > +         */
> > +        virtual std::string printCallState (cc_call_state_t state) = 0;
> 
> This doesn't print, so maybe a new name is needed.
> 
> @@ +87,5 @@
> > +           print Call event
> > +           @param [in] call event
> > +           @return call event as string
> > +         */
> > +        virtual std::string printCallEvent (ccapi_call_event_e callEvent) = 0;
> 
> Same.
> 
> ::: media/webrtc/signaling/include/PeerConnection.h
> @@ +171,5 @@
> >    // ICE state
> >    virtual IceState ice_state() = 0;
> >  
> >    // puts the SIPCC engine back to 'kIdle', shuts down threads, deletes state, etc.
> > +  virtual void Close() = 0;
> 
> I actually think i like "Shutdown()" better.

Shutdown sounds too terminal and I am using that to shutdown to shutdown SIPCC, but if you insist I will re-name Close.

> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +196,5 @@
> >    // one for video
> >    mIceStreams.push_back(mIceCtx->CreateStream("stream1", 2));
> >    mIceStreams.push_back(mIceCtx->CreateStream("stream1", 2));
> >  
> > +  for (int i=0; i<(int)mIceStreams.size(); i++) {
> 
> size() is size_t()? 
> 
> Regardless, change i to the appropriate type.
> 
> Also, please don't use C-style casts in C++ code.
> 
> ::: media/webrtc/signaling/src/sipcc/core/ccapp/cc_call_feature.c
> @@ +257,5 @@
> >  	static const char fname[] = "CC_CallFeature_CreateOffer";
> >  	CCAPP_DEBUG(DEB_L_C_F_PREFIX, DEB_L_C_F_PREFIX_ARGS(SIP_CC_PROV, GET_CALL_ID(call_handle),
> >  			GET_LINE_ID(call_handle), fname));
> >  
> > +	return cc_invokeFeature(call_handle, CC_FEATURE_CREATEOFFER, CC_SDP_DIRECTION_SENDRECV, JSEP_NO_ACTION, NULL);
> 
> Why not remove the numbers argument entirely.

That argument is also used to pass other data such as sdp in createanswer.

> 
> @@ +262,3 @@
> >  }
> >  
> > +cc_return_t CC_CallFeature_CreateAnswer(cc_call_handle_t call_handle, const char* sdp) {
> 
> Why is sdp not string_t?

done now

> 
> ::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_call.c
> @@ +125,4 @@
> >  	return CC_CallFeature_dial(handle, video_pref, digits);
> >  }
> >  
> > +cc_return_t CCAPI_CreateOffer(cc_call_handle_t handle, int audioPort, int videoPort) {
> 
> Why do we still need the ports here?

Thanks for spotting this, cleaned up now

> 
> @@ +130,3 @@
> >  }
> >  
> > +cc_return_t CCAPI_CreateAnswer(cc_call_handle_t handle, cc_string_t offersdp, int audioPort, int videoPort) {
> 
> And here?

done now.

> 
> @@ +134,4 @@
> >  }
> >  
> > +cc_return_t CCAPI_SetLocalDescription(cc_call_handle_t handle, cc_jsep_action_t action, cc_string_t sdp) {
> > +	return CC_CallFeature_SetLocalDescription(handle, action, sdp);
> 
> Aren't these const strings?
> 
> @@ +138,3 @@
> >  }
> >  
> > +cc_return_t CCAPI_SetRemoteDescription(cc_call_handle_t handle, cc_jsep_action_t action, cc_string_t sdp) {
> 
> And herE?
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +3260,3 @@
> >        }    
> >  
> >        fsmdef_init_dcb(dcb, call_id, FSMDEF_CALL_TYPE_NONE, NULL, line, fcb);
> 
> Dont' we still need error checks here?

fsmdef_init_dcb  returns void, not sure I want to change that function as I have not yet touched it and it just sets values in the dcb.


> 
> @@ +3264,4 @@
> >        fsm_set_fcb_dcbs(dcb);
> >      }
> >  
> > +    /* cpr_assert(strlen(msg->data.pc.pc_handle) < PC_HANDLE_SIZE); */
> 
> Can you put this assert in, actually? I think it's just an include issue to
> get it to compile. Or maybe w need a different assert funciton.

Our revision of sipcc assert does not do much.  If you feel its useful I can update it accross platforms to actually do something, like abort, increase an assert count or log to syslog for example. 

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +1363,5 @@
> >      		GSM_ERR_MSG("Failed to retrieve ICE attribute\n");
> >      		cpr_free(ice_attribs);
> >      		return FALSE;
> >      	}
> > +        (*ice_attribs)[i] = (char *) cpr_calloc(1, strlen(ice_attrib) + 1);
> 
> This need not be calloc, btw.
> 
> @@ +4028,5 @@
> >          sip_config_get_nat_ipaddr(&ipaddr);
> >      }
> >  
> > +
> > +	ipaddr2dotted(addr_str, &ipaddr);
> 
> We're going to want to remove all this IP code eventually.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
> @@ +429,5 @@
> >  
> >      char peerconnection[PC_HANDLE_SIZE];  /* A handle to the peerconnection */
> >      boolean peerconnection_set;
> >  
> > +    char ice_ufrag[ICE_STRING_SIZE];
> 
> This still feels like it should be dynamically allocated.

Allocating dynamically now, are you ok with me using strlen in the malloc?
If we do a re-negotiation will the memory get re-allocated without being freed.

I am freeing the two pointers in fsmdef_ev_onhook  called when pc.close is called.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
> @@ +611,5 @@
> >      static const char fname[] = "lsm_open_rx";
> >      int           port_allocated = 0;
> >      cc_rcs_t      rc = CC_RC_ERROR;
> >      fsmdef_dcb_t *dcb;
> > +    int           sdpmode = 0;
> 
> If you are going to compare sdpmode against TRUE you should have it be
> boolean.
> 
> @@ +643,5 @@
> >                "requested port", data->port);
> >  
> >      if (data->keep == TRUE) {
> >  
> > +    	//Todo IPv6: Add interface call for IPv6
> 
> Actually doesn't this need replacement later if we are in SDPmode.
> 
> @@ +745,5 @@
> >  {
> >      static const char fname[] = "lsm_close_rx";
> >      fsmdef_media_t *start_media, *end_media;
> >      fsmdef_dcb_t   *dcb;
> > +    int             sdpmode = 0;
> 
> Same comment about boolean here.
> 
> @@ +820,5 @@
> >  {
> >      fsmdef_media_t *start_media, *end_media;
> >      fsmdef_dcb_t   *dcb;
> >      static const char fname[] = "lsm_close_tx";
> > +    int             sdpmode = 0;
> 
> And here.
> 
> @@ +905,5 @@
> >      fsmdef_media_t *start_media, *end_media;
> >      boolean has_checked_conference = FALSE;
> >      fsmdef_dcb_t   *dcb, *grp_id_dcb;
> >      vcm_mediaAttrs_t attrs;
> > +    int              sdpmode;
> 
> And here.
> 
> @@ +1850,5 @@
> >      static const char fname[] = "lsm_release_port";
> >      fsmdef_media_t *start_media, *end_media;
> >      fsmdef_dcb_t   *dcb;
> >      fsmdef_media_t *media;
> > +    int            sdpmode = 0;
> 
> And here.
> 
> @@ +6425,5 @@
> >  static void lsm_util_start_tone(vcm_tones_t tone, short alert_info,
> >          cc_call_handle_t call_handle, groupid_t group_id,
> >          streamid_t stream_id, uint16_t direction) {
> >  
> > +	int sdpmode;
> 
> and her.e..
> 
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
> @@ +5150,5 @@
> >  
> >      ptr = sdp_getnextstrtok(ptr, tmp, "\r\n", &result);
> >      if (result != SDP_SUCCESS){
> >   	/* return success just means attribute not found */
> >        /* TODO(emannion): is this really right? how can this happen? */
> 
> Can you address thsi comment? When would we be called if there was no attr?

I looked at other similar code and yes we should return an error here, SDP_INVALID_PARAMETER

> 
> ::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_threads.c
> @@ +122,5 @@
> >           * such as a show command or walking the list to ensure
> >           * that an application does not attempt to create
> >           * the same thread twice.
> >           */
> > +        threadPtr->u.handleInt = (uint32_t)threadId;
> 
> Are we sure that thread ids are only 4 bytes long?

I added a new type uint64_t, my testing was good, threads are still stopping.
I must ensure I also test this on Linux.


> 
> ::: media/webrtc/signaling/src/sipcc/include/cc_call_feature.h
> @@ +190,2 @@
> >  
> > +cc_return_t CC_CallFeature_CreateAnswer(cc_call_handle_t call_handle, const char* sdp);
> 
> string_t here and below?
> 
> ::: media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp
> @@ +561,5 @@
> >  /*
> >   * This method works asynchronously, there will be an onCallEvent with the resulting SDP
> >   * When Constraints are implemented the Audio and Video port will not be a parameter to CCAPI_CreateAnswer
> >   */
> > +int CC_SIPCCCall::createOffer (const std::string& hints) {
> 
> We don't seemt o need ports here.

done now.

> 
> @@ +568,5 @@
> >  
> >  /*
> >   * This method works asynchronously, there will be an onCallEvent with the resulting SDP
> >   */
> > +int CC_SIPCCCall::createAnswer (const std::string & hints, const std::string & offersdp) {
> 
> Or here.
> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +379,5 @@
> >  
> >  class SignalingTest : public ::testing::Test {
> >   public:
> > +
> > +  ~SignalingTest() {
> 
> Put this in TearDown()?

I added a global Environment class for TearDown. It gets called at the end of all tests.


4 tests seems to be still running well after all these changes.
Attached patch SIPCC Fixes and Cleanup (obsolete) — Splinter Review
Attachment #641616 - Attachment is obsolete: true
Attachment #641616 - Flags: feedback?(ekr)
Attachment #641927 - Flags: feedback?(ethanhugg)
Attachment #641927 - Flags: feedback?(ekr)
This patch has a few small changes to work on the default tip after the m-c merge.
Attachment #642143 - Flags: feedback?(ethanhugg)
Attachment #642143 - Flags: feedback?(ekr)
This patch has a few minor changes that allow it to merge with alder after m-c was merged.
Attachment #641927 - Attachment is obsolete: true
Attachment #642143 - Attachment is obsolete: true
Attachment #641927 - Flags: feedback?(ethanhugg)
Attachment #641927 - Flags: feedback?(ekr)
Attachment #642143 - Flags: feedback?(ethanhugg)
Attachment #642143 - Flags: feedback?(ekr)
Attachment #642144 - Flags: feedback?(ethanhugg)
Attachment #642144 - Flags: feedback?(ekr)
Comment on attachment 642144 [details] [diff] [review]
SIPCC cleanup and fixes, works with alder after m-c merge

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +29,5 @@
>  
>  #include <string>
>  #include <iostream>
>  
>  #include "CSFLog.h"

You can remove CSFLog.h if you are including CSFLogStream.h

::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_service.c
@@ +134,4 @@
>   */
>  cc_return_t CCAPI_Service_stop() {
>  
>  	int  sdpmode = 0;

We have a type named 'boolean' in this code can we just use that, or are there more than two options for sdpmode?  gSDPMode in config_parser is boolean type.

@@ +138,5 @@
>  
>      CCAPP_ERROR("CCAPI_Service_stop - calling registration stop \n");
>  
>      config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
> +    if (sdpmode == 0) {

I still think the moz style guide wants if (!sdpmode) here
Attachment #642144 - Flags: feedback?(ethanhugg) → feedback+
(In reply to Ethan Hugg [:ehugg] from comment #9)
> Comment on attachment 642144 [details] [diff] [review]
> SIPCC cleanup and fixes, works with alder after m-c merge
> 
> Review of attachment 642144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +29,5 @@
> >  
> >  #include <string>
> >  #include <iostream>
> >  
> >  #include "CSFLog.h"
> 
> You can remove CSFLog.h if you are including CSFLogStream.h

Done.

> 
> ::: media/webrtc/signaling/src/sipcc/core/ccapp/ccapi_service.c
> @@ +134,4 @@
> >   */
> >  cc_return_t CCAPI_Service_stop() {
> >  
> >  	int  sdpmode = 0;
> 
> We have a type named 'boolean' in this code can we just use that, or are
> there more than two options for sdpmode?  gSDPMode in config_parser is
> boolean type.
> 

I am implementing sdpmode as an int because other config booleans are done like that, other examples are   gRegisterWithProxy and gNatEnabled.  When I made the variable a booleaan the comparison fails.  Basic reason is the cfg table is not set up to take booleans so its are used.



> @@ +138,5 @@
> >  
> >      CCAPP_ERROR("CCAPI_Service_stop - calling registration stop \n");
> >  
> >      config_get_value(CFGID_SDPMODE, &sdpmode, sizeof(sdpmode));
> > +    if (sdpmode == 0) {
> 
> I still think the moz style guide wants if (!sdpmode) here

This is the approach I took and it works the tests still work.
Incorporated Ethan's review comments
Attachment #642144 - Attachment is obsolete: true
Attachment #642144 - Flags: feedback?(ekr)
Attachment #642939 - Flags: feedback?(ethanhugg)
Comment on attachment 642939 [details] [diff] [review]
SIPCC Fixes and Cleanup


Pushed to Alder - https://hg.mozilla.org/projects/alder/rev/6fd97e86de6e
Attachment #642939 - Flags: feedback?(ethanhugg) → feedback+
fix for signaling tests global shutdown
Attachment #650025 - Flags: feedback+
QA Contact: jsmith
Whiteboard: [WebRTC], [blocking-webrtc+]
These patches landed on M-C with the Alder merge rollup - Bug 792188
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla18
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: