1103731 - Fail a dial request when failing to automatically hold the existing call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ben Hsu [:HoPang]]

When dialing a new call if there is already an existing call, current design of gecko automatically hold the existing call. Since automatically holding a call fails in some cases (e.g., bug 944302), we should fail the new dial request.

Comment 1

[Ben Hsu [:HoPang]]

Attachment: Part 1: Internal architecture changes (TelephonySerive.js)

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Part 1: Internal architecture (TelephonySerive.js)

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 2: Internal architecture changes (ril_worker.js)

To make the log easier to trace, I extended one debug message.

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 3: Update the related testcase

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 1: Internal architecture (TelephonySerive.js)

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 2: Internal architecture (ril_worker.js)

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 3: Update the related testcase

Comment 8

[Ben Hsu [:HoPang]]

Attachment: [hardware/ril] pull request 42

Comment 9

[Ben Hsu [:HoPang]]

Attachment: [external/qemu] pull request 127

Comment 10

[Ben Hsu [:HoPang]]

Hi Edgar,

To enable/disable the hold feature of a modem, I have to modify the emulator. Can you review those patches? The details are written in the comments of the pull requests.

Comment 11

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

Comment on attachment 8551064 [details] [diff] [review]
Part 1: Internal architecture (TelephonySerive.js)

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

I'm fine with original code but I'd like to know your opinion of my comment.

::: dom/telephony/gonk/TelephonyService.js
@@ +645,1 @@
>        }

How about just define a callback object explicitly and use it?

let callback = {
  QueryInterface: XPCOMUtils.generateQI([Ci.nsITelephonyCallback]),

  notifySuccess: () => {
    this._cachedDialRequest = {
      ...
    };
  },

  notifyError: error => {
    ...
  }
};

this.holdConference(aClientId, callback);
this.holdCall(aClientId, activeCall.callIndex, callback);

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 1: Internal architecture (TelephonySerive.js) (V2)

Hi aknow,

Thanks for your advice. I think explicitly defining a callback object is better, because it make the code easier to understand, while both |holdCall(...)| and |holdConference(...)| are designed to be used that way. Besides, I also add a new check before auto-holding. Can you review this patch?

Comment 13

[Edgar Chen [:edgar]]

Comment on attachment 8551099 [details] [review]
[external/qemu] pull request 127

Please check the comments I left on github, thank you.

Comment 14

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

Comment on attachment 8551610 [details] [diff] [review]
Part 1: Internal architecture (TelephonySerive.js) (V2)

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

Thank you.

Comment 15

[Edgar Chen [:edgar]]

Comment on attachment 8551095 [details] [review]
[hardware/ril] pull request 42

r=me with the comments on github addressed or answered. Thank you.

Comment 16

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

Comment on attachment 8551066 [details] [diff] [review]
Part 3: Update the related testcase

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

r=me with comment addressed.

::: dom/telephony/test/marionette/test_outgoing_auto_hold.js
@@ +10,3 @@
>    let outCall1;
>    let outCall2;
> +  let callNumber1="0900000001";

use const and add spaces before and after '='
const callNumber1 = "..."

@@ +32,5 @@
> +  log('= testAutoHoldCallFailed =');
> +
> +  let outCall1;
> +  let outCall2;
> +  let callNumber1="0900000011";

use const and add spaces before and after '='
const callNumber1 = "..."

@@ +36,5 @@
> +  let callNumber1="0900000011";
> +  let callNumber2="0900000012";
> +
> +  return Promise.resolve()
> +    .then(() => emulator.runCmd("gsm enable_hold N"))

remember to update the command based on the conclusion of emulator review

@@ +58,4 @@
>    let subCall1;
>    let subCall2;
>    let outCall;
> +  let subNumber1="0900000021";

use const and add spaces before and after '='
const callNumber1 = "..."

Comment 17

[Edgar Chen [:edgar]]

Comment on attachment 8551099 [details] [review]
[external/qemu] pull request 127

r=me with comments on github addressed.

And please provide the try link for hardware_ril and qemu changes. Thank you.

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 3: Update the related testcase (V2)

Hi aknow,

I make a little enhencement to handle failing to reject the dial requst in the second test. Can you take a look at that? Thank you.

Comment 19

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

Comment on attachment 8552854 [details] [diff] [review]
Part 3: Update the related testcase (V2)

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

r=me with comments addressed.

::: dom/telephony/test/marionette/test_outgoing_auto_hold.js
@@ +44,5 @@
> +    .then(() => {
> +      is(outCall1.state, "connected");
> +    })
> +    .then(() => gDial(callNumber2))
> +    .then(call => { ok(false, "The second |dial()| should be rejected.");

Put the code after '{' in a new line.

@@ +49,5 @@
> +                    outCall2 = call;
> +                    return gRemoteHangUpCalls([outCall2]); },
> +          () => log("The second |dial()| is rejected as expected."))
> +    .then(() => gRemoteHangUpCalls([outCall1]))
> +    .then(() => emulator.runCmd("gsm enable hold"));

Please add a |.catch(...)| before this line. Otherwise, "gsm enable hold" might be skipped if its previous promise is rejected.

Comment 20

[Ben Hsu [:HoPang]]

Attachment: Part 3: Update the related testcase (V3)

Hi aknow,

Thanks for your review, I think every undesired reject will be caught by the |catch(...)| in the end of this file, and the hold feature will be re-enbled there if needed.

Comment 21

[Ben Hsu [:HoPang]]

Attachment: Part 3: Update the related testcase (V4)

Comment 22

[Ben Hsu [:HoPang]]

Hi Edgar,

I addressed the advice came of vicamo, so the patches of |external/qemu| are updated. Do you mind reviewing the pull request again?

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8551099 [details] [review]
[external/qemu] pull request 127

(In reply to Ben Hsu [:benhsu] from comment #22)
> Hi Edgar,
> 
> I addressed the advice came of vicamo, so the patches of |external/qemu| are
> updated. Do you mind reviewing the pull request again?

Sure, I will take a look again.

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 8551099 [details] [review]
[external/qemu] pull request 127

r=me with comments on github addressed. Thank you.

Comment 25

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7da6d07432

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4ff3dd299e6a
https://hg.mozilla.org/integration/b2g-inbound/rev/422bed2705ee
https://hg.mozilla.org/integration/b2g-inbound/rev/12ac667d1532

Master: https://github.com/mozilla-b2g/platform_hardware_ril/commit/eb1795a9002eb142ac58c8d68f8f4ba094af07ca
Master: https://github.com/mozilla-b2g/platform_external_qemu/commit/2c31ac3a31a340b40ecd9c291df9b9613d3afa72

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ff3dd299e6a
https://hg.mozilla.org/mozilla-central/rev/422bed2705ee
https://hg.mozilla.org/mozilla-central/rev/12ac667d1532