This issue was originally from [1]. We can't query GET_CURRENT_CALLS until there's no ongoing telephony operation. Otherwise, the response of GET_CURRENT_CALLS may not be the result we are waiting for and that could lead to surprising and weird call state behaviour. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1004152#c45

Remove the delay to make sure the patches in this bug could indeed resolve the issue in Bug 1004152.

Comment on attachment 8423729 [details] [diff] [review] Part 1#2: Remove delay of telephony diai in test case Review of attachment 8423729 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this nice workaround again :P

Comment on attachment 8423724 [details] [diff] [review] Part 2: Add telephony request queue Review of attachment 8423724 [details] [diff] [review]: ----------------------------------------------------------------- The overall structure looks good to me. One major concern is the execution of queryQueue, please refer to below for details. Thank you! ::: dom/system/gonk/ril_worker.js @@ +204,5 @@ > BufObject.prototype[p] = base[p]; > } > })(); > > +const TELEPHONY_REQUEST = [ s/_REQUEST/_REQUESTS/ @@ +207,5 @@ > > +const TELEPHONY_REQUEST = [ > + REQUEST_GET_CURRENT_CALLS, > + REQUEST_ANSWER, > + REQUEST_CDMA_FLASH, CDMA_FLASH doesn't trigger CLCC/call state changes. We don't really need it here. @@ +258,5 @@ > + return; > + } > + > + this.currentQueue = queue; > + for (let entry of queue) { Every time when _startQueue is running and if queue is queryQueue, we should just execute REQUEST_GET_CURRENT_CALLS once irrespective of the number of entries in queue. Otherwise, we are just querying redundant requests. @@ +277,5 @@ > + debug: function(msg) { > + this.ril.context.debug("[TeleQ] " + msg); > + }, > + > + isTelephonyRequest: function(request) { How about "isValidRequest"? The word "Telephony" is a mouthful since it's already a property of TelephonyRequestQueue. @@ +315,5 @@ > + > + if (DEBUG) this.debug("pop " + this._getRequestName(request)); > + let queue = this._getQueue(request); > + let index = this._find(queue, request); > + queue.splice(index, 1); Do we still want to call "splice" when index === -1? @@ +1780,5 @@ > + sendRilRequestAnswer: function() { > + this.context.Buf.simpleRequest(REQUEST_ANSWER); > + }, > + > + sendSwitchWaitingRequest: function() { Please have 'options' argument. This is for the protocol b/w ril_worker and RadioInterfaceLayer. Otherwise, error in [1] will be met. [1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#2201 @@ +1785,5 @@ > + this.telephonyRequestQueue.push(REQUEST_SWITCH_WAITING_OR_HOLDING_AND_ACTIVE, > + this.sendRilRequestSwitch, null); > + }, > + > + sendRilRequestSwitch: function() { ditto. @@ +5610,5 @@ > } > > this.getCurrentCalls(); > }; > +// TODO: This one is not used. Please go file a new bug and paste the bug number following TODO label, thanks. @@ +5641,5 @@ > } > > this.sendChromeMessage(options); > }; > +RilObject.prototype[REQUEST_UDUB] = function REQUEST_USUB(length, options) { The reason of this change is for printing out the debug message with function name? s/USUB/UDUB/ @@ +6048,5 @@ > } > > this.IMEISV = this.context.Buf.readString(); > }; > +RilObject.prototype[REQUEST_ANSWER] = function REQUEST_ANSWER(length, options) { ditto.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5) > @@ +5641,5 @@ > > } > > > > this.sendChromeMessage(options); > > }; > > +RilObject.prototype[REQUEST_UDUB] = function REQUEST_USUB(length, options) { > > The reason of this change is for printing out the debug message with > function name? Yes. > s/USUB/UDUB/ Thanks. > @@ +6048,5 @@ > > } > > > > this.IMEISV = this.context.Buf.readString(); > > }; > > +RilObject.prototype[REQUEST_ANSWER] = function REQUEST_ANSWER(length, options) { > > ditto.

Comment on attachment 8424714 [details] [diff] [review] Part 2#2: Add telephony request queue Review of attachment 8424714 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thank you! With this patch, some known issues such as bug 936982 should be automatically resolved. In addition to automation tests, please also verify on buri/tarako devices as we know that the emulator behaviour isn't 100% the same as real devices. Thank you! ::: dom/system/gonk/ril_worker.js @@ +5605,5 @@ > } > > this.getCurrentCalls(); > }; > +// TODO 1012599: This one is not used. nit: s/1012599/Bug 1012599

https://tbpl.mozilla.org/?tree=Try&rev=587d906595ec jb emulater build failures are not related to the patch. They fail at (1) hg pull, (2) config.sh Further more, only one .js is modified in the patch. It's impossible to cause the build fail.

(In reply to Szu-Yu Chen [:aknow] from comment #11) > https://tbpl.mozilla.org/?tree=Try&rev=587d906595ec > > jb emulater build failures are not related to the patch. > They fail at (1) hg pull, (2) config.sh > Further more, only one .js is modified in the patch. It's impossible to > cause the build fail. https://hg.mozilla.org/integration/b2g-inbound/rev/27bf2f94e994 https://hg.mozilla.org/integration/b2g-inbound/rev/e0a5498d4742

https://hg.mozilla.org/mozilla-central/rev/27bf2f94e994 https://hg.mozilla.org/mozilla-central/rev/e0a5498d4742