The enhancement idea comes from Bug 931697 when we do develop multi-sim telephony. However the change is not really about the multi-sim itself, so I separate the patch and create a bug (this bug) for it.

The attachment is the same with the one in Bug 814625.

Based on above patch, move some utilities to 'head.js'. Put all the functions under object '$H' ('H' could stands for 'header' or 'helper').

Comment on attachment 823747 [details] [diff] [review] #3: rewrite conference test case Review of attachment 823747 [details] [diff] [review]: ----------------------------------------------------------------- I left a note, almost what we've discussed, in case I forgot. ;) One thing I missed in our face-to-face discussion: The thing hangUpCalls() does is actually hanging up 'calls' remotely. Can't we simply extend 'remoteHangUp' to remoteHangUp(calls)? ::: dom/telephony/test/marionette/test_conference.js @@ +12,5 @@ > let inNumber = "5555552222"; > let inNumber2 = "5555553333"; > +let outCall; > +let inCall; > +let inCall2; As we are trying to make the test more architectural, more and more complicated scenarios are going to be composed with the utility functions. I am thinking maybe we should move these global variables to local ones in every single test. Otherwise, we might going to have several global variables used only by few cases. @@ +47,4 @@ > } > } > > +function checkCallList(expectedCallList) { Is checkEmulatorCallList better? @@ +100,1 @@ > let gotConnecting = false; We could simple get rid of this variable. We will clear |call.onconnecting| in the event handler so we can just make use of |ok(!call.onconnecting)| in call.onconnected. @@ +142,3 @@ > > +function remoteAnswer(call) { > + log("Answering the call."); How about "Remote answering the call"? Clearer? @@ +158,3 @@ > > +function remoteHangUp(call) { > + log("Hanging up the call from remote."); Remote hanging up the call. @@ +175,5 @@ > +// container might be telephony or conference. > +function addCheck_oncallschanged(container, containerName, expectedCalls, > + callback) { > + container.oncallschanged = function(event) { > + log("Received 'callschanged' event for the " + containerName + " call."); Should be "Received 'callschanged' event for the " + containerName." @@ +192,4 @@ > > +function addCheck_call_ongroupchange(call, callName, group, callback) { > + call.ongroupchange = function(event) { > + log("Received 'groupchange' event for the " + callName + " call."); ditto: the trailing "call." is redundant. @@ +202,4 @@ > > +function addCheck_call_onstatechange(call, callName, state, callback) { > + call.onstatechange = function(event) { > + log("Received 'statechange' event for the " + callName + " call"); ditto. @@ +212,5 @@ > > +function CallStateEventCheckFunc(state, event, previousEvent) { > + return function(call, callName, callback) { > + call[event] = function() { > + log("Received '" + state + "' event for the " + callName + " call."); ditto. @@ +230,2 @@ > > +let addCheck_call_onholding = CallStateEventCheckFunc('holding', 'onholding', null); How about simply name in "check_onholding"? @@ +230,3 @@ > > +let addCheck_call_onholding = CallStateEventCheckFunc('holding', 'onholding', null); > +let addCheck_call_onheld = CallStateEventCheckFunc('held', 'onheld', 'onholding'); ditto. @@ +230,4 @@ > > +let addCheck_call_onholding = CallStateEventCheckFunc('holding', 'onholding', null); > +let addCheck_call_onheld = CallStateEventCheckFunc('held', 'onheld', 'onholding'); > +let addCheck_call_onresuming = CallStateEventCheckFunc('resuming', 'onresuming', null); ditto. @@ +230,5 @@ > > +let addCheck_call_onholding = CallStateEventCheckFunc('holding', 'onholding', null); > +let addCheck_call_onheld = CallStateEventCheckFunc('held', 'onheld', 'onholding'); > +let addCheck_call_onresuming = CallStateEventCheckFunc('resuming', 'onresuming', null); > +let addCheck_call_onconnected = CallStateEventCheckFunc('connected', 'onconnected', 'onresuming'); ditto. @@ +235,3 @@ > > +// The length of calls should be 1 or 2. > +function conferenceAdd(calls, check) { s/calls/callsToAdd @@ +243,5 @@ > }; > > + let pending = ["conference.oncallschanged", "conference.onconnected"]; > + for (let i in calls) { > + let callName = "call" + i; Since callName is used for debug messages and the id for pending events, how about make it carry more information? @@ +421,5 @@ > } > > +// Release a call in conference. The only call left in conference will be > +// automatically moved from conference to the calls list of telephony. > +function conferenceHangUp(hangUpCall, anotherCall, check) { s/hangUpCall/callToHangUp @@ +487,4 @@ > > +let in2_incoming = inStrBuilder2('incoming'); > +let in2_active = inStrBuilder2('active'); > +let in2_held = inStrBuilder2('held'); Seems we'd figure out a more flexible way for building string. Imagine... if we would like to have a test for having 5 people in conference and 1 single call on hold, then how many combinations are we going to have just for one sub-test? @@ +502,2 @@ > > +function hangUpCalls(calls) { The thing hangUpCalls() does is actually hanging up 'calls' remotely. Can't we simply extend 'remoteHangUp' to remoteHangUp(calls)? @@ +631,5 @@ > } > > // Start the test > startTest(function() { > + test_conference_two_calls() I know, coding style is kinda annoying... Let's change to 'testConferenceTwoCalls' and use the coding style without " _ " for names of these test functions.

Comment on attachment 823766 [details] [diff] [review] Part 2: move some utilities to head.js Review of attachment 823766 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling the review for now until part1 is resolved.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #4) > Comment on attachment 823747 [details] [diff] [review] > #3: rewrite conference test case > > Review of attachment 823747 [details] [diff] [review]: > ----------------------------------------------------------------- > > I left a note, almost what we've discussed, in case I forgot. ;) > > One thing I missed in our face-to-face discussion: > The thing hangUpCalls() does is actually hanging up 'calls' remotely. Can't > we simply extend 'remoteHangUp' to remoteHangUp(calls)? You are right. I also found it and then update the code in part 2 patch. It is renamed to 'remoteHangUpCalls'. However, I would like to keep them as two functions. 'remoteHangUp' and 'remoteHangUpCalls'. All other utilities accept one 'call' as their parameters, e.g., answer, remoteAnswer. If we use an array 'calls' for hangUp, it might be weird.

(In reply to Szu-Yu Chen [:aknow] from comment #6) > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #4) > > Comment on attachment 823747 [details] [diff] [review] > > #3: rewrite conference test case > > > > Review of attachment 823747 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I left a note, almost what we've discussed, in case I forgot. ;) > > > > One thing I missed in our face-to-face discussion: > > The thing hangUpCalls() does is actually hanging up 'calls' remotely. Can't > > we simply extend 'remoteHangUp' to remoteHangUp(calls)? > > You are right. I also found it and then update the code in part 2 patch. It > is renamed to 'remoteHangUpCalls'. However, I would like to keep them as two > functions. 'remoteHangUp' and 'remoteHangUpCalls'. All other utilities > accept one 'call' as their parameters, e.g., answer, remoteAnswer. If we use > an array 'calls' for hangUp, it might be weird. Though I don't see big problems with keeping a single function, I don't have objection against to your proposal, either. However please move 'remoteHangUpCalls' right after remoteHangUp(). Also, I think we'd brain storm a little bit to have more distinctive names for CallStateEventCheckFunc and CheckEventCallState, for checkAll, checkState, ... ...

Comment on attachment 824502 [details] [diff] [review] #4: rewrite conference test case Review of attachment 824502 [details] [diff] [review]: ----------------------------------------------------------------- It looks really great. I can't wait to see it land and take advantage of the whole new architecture! Please address my minor comments below. I believe we will be ready then. Thank you thank you. ::: dom/telephony/test/marionette/test_conference.js @@ +8,2 @@ > > +// TODO(Aknow): better naming. I don't have more comment on the naming here, though I'd like to ask you to leave some comment saying this function is created due to the string format defined in emulator . @@ +35,5 @@ > + ok(telephony.calls, 'telephony.calls'); > + is(telephony.calls.length, 0, 'telephony.calls.length'); > + ok(conference, 'conference'); > + ok(conference.calls, 'conference.calls'); > + is(conference.calls.length, 0, 'conference.calls.length'); Maybe take advantage of checkState(). As ok(telephony) is examined in setUp() in head.js and ok(conference) is ensured in the beginning of 'startTest', we could remove those two. @@ +77,2 @@ > > +function checkState(active, calls, conferenceState, conferenceCalls) { nit: place this right after |checkConferenceStateAndCalls| @@ +244,2 @@ > > +// TODO(Aknow): Better naming. StateEventChecker is clear to me. @@ +270,3 @@ > > +// The length of callsToAdd should be 1 or 2. > +function conferenceAdd(callsToAdd, check) { s/check/callback ?! @@ +318,3 @@ > } > > +function conferenceHold(calls, check) { ditto. @@ +359,2 @@ > > +function conferenceResume(calls, check) { ditto. @@ +398,3 @@ > } > > +function conferenceRemove(callToRemove, autoRemovedCalls, remainedCalls, check) { ditto: s/check/callback ?! And we always have zero or one autoRemovedCall. @@ +425,2 @@ > > + // When removing from conference with 2 calls, another one will be auto When a call is removed from conference with 2 calls, another one will be automatically removed from group and be put on hold. @@ +425,3 @@ > > + // When removing from conference with 2 calls, another one will be auto > + // removed from group and held. s/held/be put on hold @@ +425,4 @@ > > + // When removing from conference with 2 calls, another one will be auto > + // removed from group and held. > + for (let call of autoRemovedCalls) { We always have zero or one autoRemovedCall. @@ +467,5 @@ > } > > +// Release a call in 2-call conference. The only call left in conference will be > +// automatically moved from conference to the calls list of telephony. > +function conferenceHangUp(callToHangUp, anotherCall, check) { s/conferenceHangUp/hangUpCallInConference because 'conferenceHangUp' makes me feel we are going to hang up the whole conference. And I would like to extend this function to make it more like 'conferenceRemove.' i.e. hangUpCallInConference(callToHangUp, autoRemovedCall, remainedCalls, check) @@ +543,5 @@ > + * testConferenceTwoCalls(). > + * > + * @return Promise<[outCall, inCall]> > + */ > +function setupConferenceTwoCalls(outNumber, inNumber) { Forgive picky me... but could we switch positions of setupConferenceTwoCalls() and testConferenceTwoCalls().

> @@ +270,3 @@ > > > > +// The length of callsToAdd should be 1 or 2. > > +function conferenceAdd(callsToAdd, check) { > > s/check/callback ?! Actually, it is not a usual callback for conferenceAdd(). Having callback and promise might be a little confusing. If you think |callback| is better, maybe we could name it |connectedCallback|. > @@ +398,3 @@ > > } > > > > +function conferenceRemove(callToRemove, autoRemovedCalls, remainedCalls, check) { > > ditto: s/check/callback ?! > > And we always have zero or one autoRemovedCall. Using array is easier to handle than an object|null parameter. I would like to keep it unchanged, but add a comment for "zero or one". > @@ +467,5 @@ > > } > > > > +// Release a call in 2-call conference. The only call left in conference will be > > +// automatically moved from conference to the calls list of telephony. > > +function conferenceHangUp(callToHangUp, anotherCall, check) { > > s/conferenceHangUp/hangUpCallInConference because 'conferenceHangUp' makes > me feel we are going to hang up the whole conference. > > And I would like to extend this function to make it more like > 'conferenceRemove.' > > i.e. hangUpCallInConference(callToHangUp, autoRemovedCall, remainedCalls, > check) OK. Then I have to add a test about hanging up a call in a 3-call conference. In this way, the function could be really used.

Comment on attachment 825097 [details] [diff] [review] #5: rewrite conference test case Review of attachment 825097 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!