B2G Emulator: support call forwarding

RESOLVED FIXED in 2.0 S4 (20june)

Status

Firefox OS
Emulator
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

(Blocks: 1 bug)

unspecified
2.0 S4 (20june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:ril][p=3])

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Enhance emulator to support call forwarding.
- RIL_REQUEST_QUERY_CALL_FORWARD_STATUS [1]
- RIL_REQUEST_SET_CALL_FORWARD [2]
- AT+CCFC [3]

[1] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L1676
[2] https://github.com/mozilla-b2g/platform_hardware_ril/blob/master/include/telephony/ril.h#L1692
[3] Please see TS 27.007 session 7.11

Thanks
(Assignee)

Comment 1

5 years ago
hardware/ril working branch:
https://github.com/EdgarChen/platform_hardware_ril/commits/bug_861725
Assignee: nobody → echen
(Assignee)

Comment 2

5 years ago
external/qemu working branch:
https://github.com/EdgarChen/platform_external_qemu/commits/bug_861725
Blocks: 905098
Component: General → Emulator
(Assignee)

Updated

4 years ago
Blocks: 1000014
(Assignee)

Updated

4 years ago
Whiteboard: [ft:ril]
(Assignee)

Comment 3

4 years ago
Created attachment 8424721 [details] [review]
platform_hardware_ril: PR #37
(Assignee)

Comment 4

4 years ago
Created attachment 8424722 [details] [review]
platform_external_qemu: PR #85
(Assignee)

Comment 5

4 years ago
Created attachment 8435472 [details] [diff] [review]
Part 1: Disable callforwarding test cases to wait emualtor changes, v1

The changes of reference-ril and qemu break the current callforwarding test cases, so we have to disable those test first.
(Assignee)

Comment 6

4 years ago
Created attachment 8435524 [details] [diff] [review]
Part 2: Revise the call forwarding test cases, v1
(Assignee)

Comment 7

4 years ago
Landing sequence:
1). Gecko part 1
2). qemu and hardware/ril
3). Gecko part 2
(Assignee)

Comment 8

4 years ago
Comment on attachment 8435472 [details] [diff] [review]
Part 1: Disable callforwarding test cases to wait emualtor changes, v1

Hi Vicamo, could you help to review this? Thank you
Attachment #8435472 - Flags: review?(vyang)
(Assignee)

Comment 9

4 years ago
Comment on attachment 8424721 [details] [review]
platform_hardware_ril: PR #37

Hi Vicamo, could you help to review this? Thank you
Attachment #8424721 - Flags: review?(vyang)
(Assignee)

Comment 10

4 years ago
Comment on attachment 8424722 [details] [review]
platform_external_qemu: PR #85

Hi Vicamo, could you help to review this? Thank you
Attachment #8424722 - Flags: review?(vyang)
(Assignee)

Comment 11

4 years ago
Comment on attachment 8435524 [details] [diff] [review]
Part 2: Revise the call forwarding test cases, v1

Hi Vicamo, could you help to review this? Thank you
Attachment #8435524 - Flags: review?(vyang)
(Assignee)

Updated

4 years ago
Whiteboard: [ft:ril] → [ft:ril][p=3]
Target Milestone: --- → 2.0 S3 (6june)
Attachment #8435472 - Flags: review?(vyang) → review+
Comment on attachment 8435524 [details] [diff] [review]
Part 2: Revise the call forwarding test cases, v1

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

::: dom/mobileconnection/tests/marionette/test_mobile_call_forwarding.js
@@ +36,5 @@
> +    is(aEvent.timeSeconds, aOptions.timeSeconds, "check timeSeconds");
> +    is(aEvent.serviceClass, aOptions.serviceClass, "check serviceClass");
> +  }));
> +  // Check DOMRequest's result.
> +  promises.push(setCallForwardingOption(aOptions).then(() => {}, (aError) => {

nit: simply place a null for the onresolve handler.

@@ +40,5 @@
> +  promises.push(setCallForwardingOption(aOptions).then(() => {}, (aError) => {
> +    ok(false, "got '" + aError.name + "' error");
> +  }));
> +
> +  return Promise.all(promises);

Please check validity after |Promise.all()| resolves.  Something like:

  return Promise.all()
                .then(function(aResults) {
    // aResults[0] should be an DOMEvent for "cfstatechange".
    ...
  });

@@ +84,5 @@
> +      reason: reason,
> +      action: MozMobileConnection.CALL_FORWARD_ACTION_ERASURE
> +    };
> +    promise =
> +      promise.then(setCallForwardingOption(options).then(() => {}, () => {}));

nit: to prevent rejects, you can simply assign onreject handler only, which makes |.then(null, true)| in this case.

@@ +99,5 @@
> +                     .then(() => testGetCallForwardingOption(data.reason, data));
> +  }
> +  // reset call forwarding settings.
> +  return promise.then(() => clearAllCallForwardingSettings(),
> +                      () => clearAllCallForwardingSettings());

nit: can be also rewritten as:

  return promise.then(null, true)
                .then(() => clearAllCallForwardingSettings());
Attachment #8435524 - Flags: review?(vyang) → review+
Attachment #8424721 - Flags: review?(vyang) → review+
(Assignee)

Comment 13

4 years ago
Comment on attachment 8424722 [details] [review]
platform_external_qemu: PR #85

Thank you for your reivew, Vicamo. Will send review request again once comments on github addressed. :)
Attachment #8424722 - Flags: review?(vyang)
(Assignee)

Comment 14

4 years ago
Try with qemu/reference-ril changes: https://tbpl.mozilla.org/?tree=Try&rev=830e50b8a448
(Assignee)

Updated

4 years ago
Attachment #8424722 - Flags: review?(vyang)
(Assignee)

Comment 15

4 years ago
Created attachment 8437469 [details] [diff] [review]
Part 2: Revise the call forwarding test cases, v2, r=vicamo

Address nit in comment #12.
Attachment #8435524 - Attachment is obsolete: true
Attachment #8437469 - Flags: review+
(Assignee)

Comment 16

4 years ago
Comment on attachment 8424722 [details] [review]
platform_external_qemu: PR #85

Need to address comments on github, cancel the review first.
Attachment #8424722 - Flags: review?(vyang)
(Assignee)

Comment 17

4 years ago
Comment on attachment 8424722 [details] [review]
platform_external_qemu: PR #85

Already updated patches on github, thank you.
Attachment #8424722 - Flags: review?(vyang)
Comment on attachment 8424722 [details] [review]
platform_external_qemu: PR #85

Thank you :)
Attachment #8424722 - Flags: review?(vyang) → review+
(Assignee)

Comment 19

4 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #7)
> Landing sequence:
> 1). Gecko part 1
> 2). qemu and hardware/ril
> 3). Gecko part 2

Gecko part1:
https://hg.mozilla.org/integration/b2g-inbound/rev/974b969d02c5
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/974b969d02c5
https://github.com/mozilla-b2g/platform_external_qemu/commit/eb0c93761bb9919567257d19bf25fa433cda3c00
https://github.com/mozilla-b2g/platform_hardware_ril/commit/cd88d860656c31c7da7bb310d6a160d0011b0961
(Assignee)

Comment 23

4 years ago
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> https://github.com/mozilla-b2g/platform_external_qemu/commit/
> eb0c93761bb9919567257d19bf25fa433cda3c00

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22)
> https://github.com/mozilla-b2g/platform_hardware_ril/commit/
> cd88d860656c31c7da7bb310d6a160d0011b0961

http://hg.mozilla.org/integration/b2g-inbound/rev/9ec91eeb5299
(In reply to Edgar Chen [:edgar][:echen] from comment #23)
> http://hg.mozilla.org/integration/b2g-inbound/rev/9ec91eeb5299

http://hg.mozilla.org/mozilla-central/rev/c482c28b35b6
(Assignee)

Comment 25

4 years ago
Ah, just found some typo in https://github.com/mozilla-b2g/platform_external_qemu/blob/eb0c93761bb9919567257d19bf25fa433cda3c00/telephony/android_modem.c#L2629-L2635. :(

It should be

if (reason == A_CALL_FORWARDING_REASON_ALL ||
    reason == A_CALL_FORWARDING_REASON_ALL_CONDITIONAL) {
 ....
}
(Assignee)

Comment 26

4 years ago
Created attachment 8441205 [details] [review]
(follow-up) platform_external_qemu: PR #99

(In reply to Edgar Chen [:edgar][:echen] from comment #25)
> Ah, just found some typo in
> https://github.com/mozilla-b2g/platform_external_qemu/blob/
> eb0c93761bb9919567257d19bf25fa433cda3c00/telephony/android_modem.c#L2629-
> L2635. :(
> 
> It should be
> 
> if (reason == A_CALL_FORWARDING_REASON_ALL ||
>     reason == A_CALL_FORWARDING_REASON_ALL_CONDITIONAL) {
>  ....
> }

PR for this.
(Assignee)

Updated

4 years ago
Attachment #8441205 - Flags: review?(vyang)
Comment on attachment 8441205 [details] [review]
(follow-up) platform_external_qemu: PR #99

https://github.com/mozilla-b2g/platform_external_qemu/commit/3326b51017252e09ccdd715dec6c5e12a7d1ecfe
Attachment #8441205 - Flags: review?(vyang) → review+
(Assignee)

Comment 28

4 years ago
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #27)
> Comment on attachment 8441205 [details] [review]
> (follow-up) platform_external_qemu: PR #99
> 
> https://github.com/mozilla-b2g/platform_external_qemu/commit/
> 3326b51017252e09ccdd715dec6c5e12a7d1ecfe

Thank you, Vicamo. :)

http://hg.mozilla.org/integration/b2g-inbound/rev/b5ec958db6d3
(Assignee)

Comment 29

4 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #28)
> http://hg.mozilla.org/integration/b2g-inbound/rev/b5ec958db6d3

http://hg.mozilla.org/mozilla-central/rev/b5ec958db6d3
(Assignee)

Comment 30

4 years ago
Try result for part2: https://tbpl.mozilla.org/?tree=Try&rev=5026f7446843
(Assignee)

Comment 31

4 years ago
Gecko part2:
https://hg.mozilla.org/integration/b2g-inbound/rev/fe0359fb57db
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/fe0359fb57db
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
(Assignee)

Updated

4 years ago
Blocks: 1027514
You need to log in before you can comment on or make changes to this bug.