Telephony dom refactoring: extract CreateNewCall

RESOLVED FIXED in 2.0 S5 (4july)

Status

Firefox OS
RIL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aknow, Assigned: aknow)

Tracking

unspecified
2.0 S5 (4july)
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8442657 [details] [diff] [review]
Extract CreateNewCall
Attachment #8442657 - Flags: review?(htsai)
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()?
Attachment #8442657 - Flags: review?(htsai) → review+
(Assignee)

Comment 3

3 years ago
(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.
(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.
(Assignee)

Comment 5

3 years ago
Created attachment 8444279 [details] [diff] [review]
Extract CreateCall and CreateCallId

Add some changes after rebasing.
Attachment #8442657 - Attachment is obsolete: true
Attachment #8444279 - Flags: review?(htsai)
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.
Attachment #8444279 - Flags: review?(htsai) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8445160 [details] [diff] [review]
[final] Extract CreateCall and CreateCallId. r=hsinyi
Attachment #8444279 - Attachment is obsolete: true
Attachment #8445160 - Flags: review+
(Assignee)

Comment 8

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=07a25baea699
Keywords: checkin-needed
Target Milestone: --- → 2.0 S5 (4july)
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 :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26cb9dad566c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Depends on: 1074377
You need to log in before you can comment on or make changes to this bug.