997003 - Refactor: setupConference in telephony test code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Attachment: Refactor: setupConference in test code

Comment 2

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

Attachment: #2 Refactor: setupConference in test code

Comment 3

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

Comment on attachment 8407291 [details] [diff] [review]
#2 Refactor: setupConference in test code

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

::: dom/telephony/test/marionette/head.js
@@ +865,5 @@
>     * @param inNumber
>     *        Number of an incoming call.
>     * @return Promise<[outCall, inCall]>
>     */
>    function setupConferenceTwoCalls(outNumber, inNumber) {

Ideally, I think we should drop setupConferenceTwoCalls() but keep addCallsToConference(). And make use of the latter to compose a conference with N calls.

@@ +896,5 @@
>        });
>    }
>  
>    /**
> +   * Setup a conference by adding a new incoming call.

Isn't this the utility function "addCallsToConference()" for?

@@ +927,5 @@
>        })
> +      .then(() => checkAll(conference, [newCall], 'connected', conferenceCalls,
> +                           toState(allInfo, 'active').concat(newInfo.incoming)))
> +      .then(() => answer(newCall, function() {
> +        checkState(newCall, [newCall], 'held', conferenceCalls);

nit: can we remove checkState() here since we are going to check it again in checkAll()?

Comment 4

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #3)
> Comment on attachment 8407291 [details] [diff] [review]
> #2 Refactor: setupConference in test code
> 
> Review of attachment 8407291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/test/marionette/head.js
> @@ +865,5 @@
> >     * @param inNumber
> >     *        Number of an incoming call.
> >     * @return Promise<[outCall, inCall]>
> >     */
> >    function setupConferenceTwoCalls(outNumber, inNumber) {
> 
> Ideally, I think we should drop setupConferenceTwoCalls() but keep
> addCallsToConference(). And make use of the latter to compose a conference
> with N calls.
> @@ +896,5 @@
> >        });
> >    }
> >  
> >    /**
> > +   * Setup a conference by adding a new incoming call.
> 
> Isn't this the utility function "addCallsToConference()" for?
> @@ +927,5 @@
> >        })
> > +      .then(() => checkAll(conference, [newCall], 'connected', conferenceCalls,
> > +                           toState(allInfo, 'active').concat(newInfo.incoming)))
> > +      .then(() => answer(newCall, function() {
> > +        checkState(newCall, [newCall], 'held', conferenceCalls);
> 
> nit: can we remove checkState() here since we are going to check it again in
> checkAll()?

I didn't expose setupConferenceTwoCalls(), all the users should use setupConference().

We cannot drop setupConferenceTwoCalls(). The handler of two calls are the special case. In order to have an N-call conference. We should (1) create a conference with 2 calls first, and then (2) add the calls one by one.

(1) => setupConferenceTwoCalls()
(2) => setupConferenceAddCall()


addCallsToConference() only do conference.add(). You should have the calls first.
setupConferenceAddCall() is different. The utility will make the call first, answer, and then call addCallsToConference() to add it into the conference.

Although I don't like the names, but they are different. setupConferenceAddCall() is just the utility used in head.js. Also, I suggest to rename all the utility related to conference.

addCallsToConference;
holdConference;
resumeConference;
removeCallInConference;
hangUpCallInConference;

to 

conferenceAddCalls
conferenceHold
conferenceResume
conferenceRemoveCall
conferenceHangUpCall

Then the following name could be

setupConferenceTwoCalls() => createConferenceWithTwoCalls
setupConferenceAddCall() => createCallAndAddToConference

Again, they are only referred in head.js

Comment 5

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

Thanks for the clarification. I got your thoughts better. :)

(In reply to Szu-Yu Chen [:aknow] from comment #4)
> We cannot drop setupConferenceTwoCalls(). The handler of two calls are the
> special case. In order to have an N-call conference. We should (1) create a
> conference with 2 calls first, and then (2) add the calls one by one.
> 
> (1) => setupConferenceTwoCalls()
> (2) => setupConferenceAddCall()
> 
> 
> addCallsToConference() only do conference.add(). You should have the calls
> first.
> setupConferenceAddCall() is different. The utility will make the call first,
> answer, and then call addCallsToConference() to add it into the conference.
> 
> Although I don't like the names, but they are different.
> setupConferenceAddCall() is just the utility used in head.js. Also, I
> suggest to rename all the utility related to conference.
> 
> addCallsToConference;
> holdConference;
> resumeConference;
> removeCallInConference;
> hangUpCallInConference;
> 

I'd like to keep these as what they are due to the function naming convention, use a verb for function.

> to 
> 
> conferenceAddCalls
> conferenceHold
> conferenceResume
> conferenceRemoveCall
> conferenceHangUpCall
> 
> Then the following name could be
> 
> setupConferenceTwoCalls() => createConferenceWithTwoCalls
> setupConferenceAddCall() => createCallAndAddToConference
> 

And these two look really good to me.

> Again, they are only referred in head.js

Comment 6

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

Attachment: #3 Refactor: setupConference in test code

Comment 7

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

Comment on attachment 8410761 [details] [diff] [review]
#3 Refactor: setupConference in test code

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

Looks good, thank you.

::: dom/telephony/test/marionette/head.js
@@ +954,2 @@
>  
> +    function addInfoState(allInfo, state) {

nit: define the function before the above for-loop uses it

Comment 8

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

Attachment: [final] Refactor: setupConference in test code

Comment 9

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

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

Comment 10

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

https://hg.mozilla.org/integration/b2g-inbound/rev/96f4e62a2efb

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/96f4e62a2efb