[Telephony] Refactoring logics between TelephonyService and ril_worker

RESOLVED FIXED in 2.2 S5 (6feb)

Status

Firefox OS
RIL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aknow, Assigned: aknow)

Tracking

unspecified
2.2 S5 (6feb)
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 1 obsolete attachment)

1.13 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.52 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
2.76 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.54 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
1.73 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
4.27 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
3.07 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
9.01 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
4.80 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
18.38 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
6.41 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
4.06 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
My idea is to make ril_worker simple and move most of telephony-specific logics and cdma handling to TelephonyService.
(Assignee)

Comment 1

3 years ago
Created attachment 8552882 [details] [diff] [review]
Part 01: Remove unused code
Attachment #8552882 - Flags: review?(htsai)
(Assignee)

Comment 2

3 years ago
Created attachment 8552883 [details] [diff] [review]
Part 02: ril_worker: expose API cdmaFlash
Attachment #8552883 - Flags: review?(htsai)
(Assignee)

Comment 3

3 years ago
Created attachment 8552884 [details] [diff] [review]
Part 03: TelephonyService: add method to detect cdma
Attachment #8552884 - Flags: review?(htsai)
(Assignee)

Comment 4

3 years ago
Created attachment 8552885 [details] [diff] [review]
Part 04: Use CDMA_FIRST_CALL_INDEX
Attachment #8552885 - Flags: review?(htsai)
(Assignee)

Comment 5

3 years ago
Created attachment 8552886 [details] [diff] [review]
Part 05: Refactoring sendUSSD, cancelUSSD
Attachment #8552886 - Flags: review?(htsai)
(Assignee)

Comment 6

3 years ago
Created attachment 8552887 [details] [diff] [review]
Part 06: Refactoring hangupCall
Attachment #8552887 - Flags: review?(htsai)
(Assignee)

Comment 7

3 years ago
Created attachment 8552888 [details] [diff] [review]
Part 07: Refactoring hangupConference
Attachment #8552888 - Flags: review?(htsai)
(Assignee)

Comment 8

3 years ago
Created attachment 8552889 [details] [diff] [review]
Part 08: Refactoring holdConference, resumeConference
Attachment #8552889 - Flags: review?(htsai)
(Assignee)

Comment 9

3 years ago
Created attachment 8552890 [details] [diff] [review]
Part 09: Refactoring conferenceCall, separateCall
Attachment #8552890 - Flags: review?(htsai)
(Assignee)

Comment 10

3 years ago
Created attachment 8552891 [details] [diff] [review]
Part 10: Refactoring holdCall, resumeCall
Attachment #8552891 - Flags: review?(htsai)
(Assignee)

Comment 11

3 years ago
Created attachment 8552892 [details] [diff] [review]
Part 11: Refactoring dial
Attachment #8552892 - Flags: review?(htsai)
(Assignee)

Comment 12

3 years ago
Created attachment 8552893 [details] [diff] [review]
Part 12: Provide a default response function
Attachment #8552893 - Flags: review?(htsai)
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.
Attachment #8552882 - Flags: review?(htsai)

Updated

3 years ago
Attachment #8552883 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552884 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552885 - Flags: review?(htsai) → review+
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.
Attachment #8552886 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552887 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552888 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552889 - Flags: review?(htsai) → review+
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.
Attachment #8552890 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552891 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552892 - Flags: review?(htsai) → review+

Updated

3 years ago
Attachment #8552893 - Flags: review?(htsai) → review+

Updated

3 years ago
Duplicate of this bug: 1086268
(Assignee)

Comment 17

3 years ago
Created attachment 8555658 [details] [diff] [review]
Part 01#2: Remove unused code
Attachment #8552882 - Attachment is obsolete: true
Attachment #8555658 - Flags: review?(htsai)

Updated

3 years ago
Attachment #8555658 - Flags: review?(htsai) → review+
(Assignee)

Comment 18

3 years ago
try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=92097bebe587
(Assignee)

Comment 19

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=f65d3ccbe8a2
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
status-b2g-v2.2: --- → fixed
Target Milestone: --- → 2.2 S5 (6feb)

Comment 21

3 years ago
This bug doesn't be included in 2.2 actually.
status-b2g-v2.2: fixed → ---
You need to log in before you can comment on or make changes to this bug.