990472 - [B2G][RIL] mozTelephony.dial() should reject when there are more than 2 calls

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Now Dialer checks if there are more then 2 calls (single or conference) before making a dial request [1]. But considering there could be multiple callers of this API, Gecko API should be responsible of checking the condition so that every App share the same behaviour.


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js#L33

Comment 1

[:gerard-majax]

I'm unsure, but I would say that the PHONE_STATE_IN_CALL can help there: http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyProvider.js#249

Comment 2

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

(In reply to Alexandre LISSY :gerard-majax [DND 03/22-03/29] from comment #1)
> I'm unsure, but I would say that the PHONE_STATE_IN_CALL can help there:
> http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/
> TelephonyProvider.js#249

Not sure I got your point. May you elaborate more on what and how PHONE_STATE_IN_CALL can help? To help gaia or ...?

Comment 3

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #0)
> Now Dialer checks if there are more then 2 calls (single or conference)
> before making a dial request [1]. But considering there could be multiple
> callers of this API, Gecko API should be responsible of checking the
> condition so that every App share the same behaviour.
> 
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/telephony_helper.js#L33

After my second thought, we need to be careful of the difference b/w gsm and cdma.
The above condition [1] definitely works for gsm but doesn't fit cdma quite well. However, the condition is laxer for cdma.

Comment 4

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

Attachment: Part 1: Refactor: check of call on other client

Comment 5

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

Attachment: Part 2: Add check for more than 2 calls on line

Comment 6

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

Attachment: Part 3: Test case

Comment 7

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

Comment on attachment 8402449 [details] [diff] [review]
Part 1: Refactor: check of call on other client

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

::: dom/telephony/gonk/TelephonyProvider.js
@@ +443,3 @@
>      }
> +
> +    // For DSDS, if there is aleady a call on SIM X, we cannot place any new

nit: s/on SIM X/on SIM 'aClientId'

Comment 8

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

Comment on attachment 8402450 [details] [diff] [review]
Part 2: Add check for more than 2 calls on line

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

r=me with comment addressed. Thank you!

::: dom/telephony/gonk/TelephonyProvider.js
@@ +464,5 @@
> +        }
> +      }
> +
> +      if (hasConference) {
> +        numCalls++;

We could simply use: numCall = hasConference ? numCalls + 1 : numCalls;

@@ +470,5 @@
> +
> +      return numCalls;
> +    }
> +
> +    if (numCallsOnLine.call(this, aClientId) >= 2) {

Currently TelephonyProvider doesn't maintain the state of a Cdma waiting call, so this condition doesn't count the situation in. I think maintaining Cdma waiting call and making all telephony instances (on a single process) share the same information is out of the scope of this bug. So, it is okay for me with the if condition.

Comment 9

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

Comment on attachment 8402451 [details] [diff] [review]
Part 3: Test case

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

Bug 989728 moved several helper functions to head.js. Please rebase on that.

Comment 10

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

Attachment: Part 3#2: Test case

rebase

Comment 11

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

Attachment: Part 3#3: Test case

make sure the 3rd call is rejected

Comment 12

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

Comment on attachment 8406707 [details] [diff] [review]
Part 3#3: Test case

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

::: dom/telephony/test/marionette/test_outgoing_when_two_calls_on_line.js
@@ +26,5 @@
> +}
> +
> +startTest(function() {
> +  testReject3rdCall()
> +    .then(finish);

A reject handler is missing at the end. For example, if "gClearCalls" rejects, there's no handler for that.

Comment 13

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

Comment on attachment 8406707 [details] [diff] [review]
Part 3#3: Test case

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

Please ignore my previous comment. It is no problem as there's a reject handler for Setup() in head.js. Thank you.

Comment 14

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

Attachment: [final] Part 1: Refactor: check of call on other client. r=hsinyi

Comment 15

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

Attachment: [final] Part 2: Add check for more than 2 calls on line. r=hsinyi

Comment 16

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

Attachment: [final] Part 3: Test case. r=hsinyi

I have add the handler for promise rejection.
The one in head.js only catch the reject from setUp()

Comment 17

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

Attachment: [final] Part 3#2: Test case. r=hsinyi

add into manifest

Comment 18

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

https://tbpl.mozilla.org/?tree=Try&rev=11b98ebd6371

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c36ef908c673
https://hg.mozilla.org/integration/b2g-inbound/rev/7d26e5a537d8
https://hg.mozilla.org/integration/b2g-inbound/rev/f9157f73f5fc

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c36ef908c673
https://hg.mozilla.org/mozilla-central/rev/7d26e5a537d8
https://hg.mozilla.org/mozilla-central/rev/f9157f73f5fc