Closed Bug 997003 Opened 6 years ago Closed 6 years ago

Refactor: setupConference in telephony test code

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(feature-b2g:2.0, tracking-b2g:backlog)

RESOLVED FIXED
2.0 S1 (9may)
feature-b2g 2.0
tracking-b2g backlog

People

(Reporter: aknow, Assigned: aknow)

Details

(Whiteboard: [p=2])

Attachments

(1 file, 3 obsolete files)

No description provided.
Attachment #8407289 - Flags: review?(htsai)
Attachment #8407289 - Attachment is obsolete: true
Attachment #8407289 - Flags: review?(htsai)
Attachment #8407291 - Flags: review?(htsai)
Whiteboard: [p=2]
Target Milestone: --- → 1.4 S6 (25apr)
blocking-b2g: --- → backlog
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()?
Attachment #8407291 - Flags: review?(htsai)
(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
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
Attachment #8407291 - Attachment is obsolete: true
Attachment #8410761 - Flags: review?(htsai)
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
Attachment #8410761 - Flags: review?(htsai) → review+
Attachment #8410761 - Attachment is obsolete: true
Attachment #8418665 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=a7e4ba6124d3
Keywords: checkin-needed
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
feature-b2g: --- → 2.0
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.