1124550 - [Telephony] Refactoring logics between TelephonyService and ril_worker

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

My idea is to make ril_worker simple and move most of telephony-specific logics and cdma handling to TelephonyService.

Comment 1

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

Attachment: Part 01: Remove unused code

Comment 2

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

Attachment: Part 02: ril_worker: expose API cdmaFlash

Comment 3

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

Attachment: Part 03: TelephonyService: add method to detect cdma

Comment 4

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

Attachment: Part 04: Use CDMA_FIRST_CALL_INDEX

Comment 5

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

Attachment: Part 05: Refactoring sendUSSD, cancelUSSD

Comment 6

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

Attachment: Part 06: Refactoring hangupCall

Comment 7

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

Attachment: Part 07: Refactoring hangupConference

Comment 8

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

Attachment: Part 08: Refactoring holdConference, resumeConference

Comment 9

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

Attachment: Part 09: Refactoring conferenceCall, separateCall

Comment 10

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

Attachment: Part 10: Refactoring holdCall, resumeCall

Comment 11

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

Attachment: Part 11: Refactoring dial

Comment 12

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

Attachment: Part 12: Provide a default response function

Comment 13

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

Comment on attachment 8552882 [details] [diff] [review]
Part 01: Remove unused code

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

::: dom/system/gonk/ril_worker.js
@@ +2752,3 @@
>     */
>    sendUSSD: function(options) {
> +    if (!this._ussdSession) {

We need to stay with the original logic.

checkSession is used to indicate if ril_worker.sendUSSD is triggered by telephony.dial or ussdSession.send.

::: dom/telephony/gonk/TelephonyService.js
@@ +958,5 @@
>                            this._defaultCallbackHandler.bind(this, aCallback));
>    },
>  
>    sendUSSD: function(aClientId, aUssd, aCallback) {
> +    this._sendToRilWorker(aClientId, "sendUSSD", { ussd: aUssd },

ditto: checkSession is necessary.

Comment 14

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

Comment on attachment 8552886 [details] [diff] [review]
Part 05: Refactoring sendUSSD, cancelUSSD

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

Looks good but please remember to rebase on the revision of part01.

Comment 15

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

Comment on attachment 8552890 [details] [diff] [review]
Part 09: Refactoring conferenceCall, separateCall

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

r=me with comments addressed, thank you.

::: dom/telephony/gonk/TelephonyService.js
@@ +888,5 @@
> +  _conferenceCallGsm: function(aClientId, aCallback) {
> +    this._sendToRilWorker(aClientId, "conferenceCall", null, response => {
> +      if (!response.success) {
> +        aCallback.notifyError(RIL.GECKO_ERROR_GENERIC_FAILURE);
> +        // TODO: Deprecate it. Use callback response is enough.

Thanks for the comment. I just filed bug 1124993. Could you please note the bug number in the comment?

@@ +907,5 @@
>          return;
>        }
>      }
>  
> +    this._sendToRilWorker(aClientId, "cdmaFalsh", null, response => {

s/cdmaFalsh/cdmaFlash/

@@ +912,3 @@
>        if (!response.success) {
>          aCallback.notifyError(RIL.GECKO_ERROR_GENERIC_FAILURE);
> +        // TODO: Deprecate it. Use callback response is enough.

ditto.

@@ +946,5 @@
>      this._sendToRilWorker(aClientId, "separateCall", { callIndex: aCallIndex },
>                            response => {
>        if (!response.success) {
>          aCallback.notifyError(RIL.GECKO_ERROR_GENERIC_FAILURE);
> +        // TODO: Deprecate it. Use callback response is enough.

ditto.

@@ +963,5 @@
> +  _separateCallCdma: function(aClientId, aCallIndex, aCallback) {
> +    this._sendToRilWorker(aClientId, "cdmaFlash", null, response => {
> +      if (!response.success) {
> +        aCallback.notifyError(RIL.GECKO_ERROR_GENERIC_FAILURE);
> +        // TODO: Deprecate it. Use callback response is enough.

ditto.

Comment 17

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

Attachment: Part 01#2: Remove unused code

Comment 18

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

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92097bebe587

Comment 19

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

https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=f65d3ccbe8a2

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a002b6cd7d6
https://hg.mozilla.org/mozilla-central/rev/abd5fc07e1cd
https://hg.mozilla.org/mozilla-central/rev/c79a22f246c2
https://hg.mozilla.org/mozilla-central/rev/e48bc5e1b502
https://hg.mozilla.org/mozilla-central/rev/e2e01e70ed7b
https://hg.mozilla.org/mozilla-central/rev/887f0dfb67b3
https://hg.mozilla.org/mozilla-central/rev/ad9efb798e6e
https://hg.mozilla.org/mozilla-central/rev/c92af09c585c
https://hg.mozilla.org/mozilla-central/rev/36f22cd4b4a3
https://hg.mozilla.org/mozilla-central/rev/39bdc320a27f
https://hg.mozilla.org/mozilla-central/rev/234097791eaf
https://hg.mozilla.org/mozilla-central/rev/f65d3ccbe8a2

Comment 21

[Edgar Chen [:edgar]]

This bug doesn't be included in 2.2 actually.