1027513 - Telephony dom refactoring: extract CreateNewCall

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Szu-Yu Chen [:aknow] (inactive)]

Extract CreateNewCall, which will be the only place to generate the call instance. The we could put all the assertion check inside this function.

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Extract CreateNewCall

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8442657 [details] [diff] [review]
Extract CreateNewCall

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

::: dom/telephony/Telephony.cpp
@@ +273,5 @@
>  
>  already_AddRefed<TelephonyCall>
> +Telephony::CreateNewCall(uint32_t aServiceId, const nsAString& aNumber,
> +                         uint32_t aCallIndex, uint16_t aCallState,
> +                         bool aEmergency, bool aConference,

Have the naming the same as those used in CallStateChanged() and EnumerateCallState()?

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> Comment on attachment 8442657 [details] [diff] [review]
> Extract CreateNewCall
> 
> Review of attachment 8442657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/Telephony.cpp
> @@ +273,5 @@
> >  
> >  already_AddRefed<TelephonyCall>
> > +Telephony::CreateNewCall(uint32_t aServiceId, const nsAString& aNumber,
> > +                         uint32_t aCallIndex, uint16_t aCallState,
> > +                         bool aEmergency, bool aConference,
> 
> Have the naming the same as those used in CallStateChanged() and
> EnumerateCallState()?

Did you mean the name like a'Is'XXX?

Actually, I really want to remove 'Is' from them.
The following is the current naming used in TelephonyCall.

   static already_AddRefed<TelephonyCall>
   Create(Telephony* aTelephony, uint32_t aServiceId,
          const nsAString& aNumber, uint32_t aCallIndex, uint16_t aCallState,
          bool aEmergency = false, bool aIsConference = false,
          bool aSwitchable = true, bool aMergeable = true);

It is even not consistent within function itself. We got a'Is'Conference and others without 'Is'.
We should choose one style and stick with it. In my opinion, I prefer no 'Is' since the name Switch'able' already indicates the boolean type.

If you agree with me, I'd also like to correct the above function prototype in this bug. For CallStateChanged() and EnumerateCallState(), we need to modify the interface. Maybe leave them in another bug is better.

Comment 4

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #3)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> > Comment on attachment 8442657 [details] [diff] [review]
> > Extract CreateNewCall
> > 
> > Review of attachment 8442657 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/telephony/Telephony.cpp
> > @@ +273,5 @@
> > >  
> > >  already_AddRefed<TelephonyCall>
> > > +Telephony::CreateNewCall(uint32_t aServiceId, const nsAString& aNumber,
> > > +                         uint32_t aCallIndex, uint16_t aCallState,
> > > +                         bool aEmergency, bool aConference,
> > 
> > Have the naming the same as those used in CallStateChanged() and
> > EnumerateCallState()?
> 
> Did you mean the name like a'Is'XXX?

Yup.

> 
> Actually, I really want to remove 'Is' from them.
> The following is the current naming used in TelephonyCall.
> 
>    static already_AddRefed<TelephonyCall>
>    Create(Telephony* aTelephony, uint32_t aServiceId,
>           const nsAString& aNumber, uint32_t aCallIndex, uint16_t aCallState,
>           bool aEmergency = false, bool aIsConference = false,
>           bool aSwitchable = true, bool aMergeable = true);
> 
> It is even not consistent within function itself. We got a'Is'Conference and
> others without 'Is'.
> We should choose one style and stick with it. 

This is what I am looking forward, too!

> In my opinion, I prefer no
> 'Is' since the name Switch'able' already indicates the boolean type.
> 

I don't have strong opinions if we make them consistent.

> If you agree with me, I'd also like to correct the above function prototype
> in this bug.

That would  be better.

> For CallStateChanged() and EnumerateCallState(), we need to
> modify the interface. Maybe leave them in another bug is better.

Fair enough. Or we may not need this if you are going to do the streamline nsITelephonyService work soon.

Comment 5

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Extract CreateCall and CreateCallId

Add some changes after rebasing.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8444279 [details] [diff] [review]
Extract CreateCall and CreateCallId

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

Thank you!

::: dom/telephony/TelephonyCall.cpp
@@ +25,2 @@
>  {
>    NS_ASSERTION(aTelephony, "Null pointer!");

Please assert aId, too.

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Extract CreateCall and CreateCallId. r=hsinyi

Comment 8

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=07a25baea699

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/26cb9dad566c

Please try to avoid running builds/tests across all platforms on Try pushes for code that's B2G-only. Thanks :)

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/26cb9dad566c