Closed Bug 990472 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
1.4 S6 (25apr)
tracking-b2g backlog

People

(Reporter: hsinyi, Assigned: aknow)

References

Details

(Whiteboard: [p=2])

Attachments

(3 files, 6 obsolete files)

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
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
(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 ...?
Assignee: nobody → szchen
(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.
Attachment #8402449 - Flags: review?(htsai)
Attachment #8402450 - Flags: review?(htsai)
Attached patch Part 3: Test case (obsolete) — Splinter Review
Attachment #8402451 - Flags: review?(htsai)
Target Milestone: --- → 1.4 S6 (25apr)
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'
Attachment #8402449 - Flags: review?(htsai) → review+
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.
Attachment #8402450 - Flags: review?(htsai) → review+
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.
Attachment #8402451 - Flags: review?(htsai)
blocking-b2g: --- → backlog
Whiteboard: [p=3]
Whiteboard: [p=3] → [p=2]
Attached patch Part 3#2: Test case (obsolete) — Splinter Review
rebase
Attachment #8402451 - Attachment is obsolete: true
Attachment #8406703 - Flags: review?(htsai)
Attached patch Part 3#3: Test case (obsolete) — Splinter Review
make sure the 3rd call is rejected
Attachment #8406703 - Attachment is obsolete: true
Attachment #8406703 - Flags: review?(htsai)
Attachment #8406707 - Flags: review?(htsai)
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.
Attachment #8406707 - Flags: review?(htsai)
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.
Attachment #8406707 - Flags: review+
I have add the handler for promise rejection.
The one in head.js only catch the reject from setUp()
Attachment #8406707 - Attachment is obsolete: true
Attachment #8407299 - Flags: review+
add into manifest
Attachment #8407299 - Attachment is obsolete: true
Attachment #8407300 - Flags: review+
Depends on: 1005732
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.