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)
Tracking
(tracking-b2g:backlog)
People
(Reporter: hsinyi, Assigned: aknow)
References
Details
(Whiteboard: [p=2])
Attachments
(3 files, 6 obsolete files)
1.87 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
4.38 KB,
patch
|
aknow
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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
Reporter | ||
Comment 2•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → szchen
Reporter | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8402449 -
Flags: review?(htsai)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8402450 -
Flags: review?(htsai)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8402451 -
Flags: review?(htsai)
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → 1.4 S6 (25apr)
Reporter | ||
Comment 7•10 years ago
|
||
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+
Reporter | ||
Comment 8•10 years ago
|
||
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+
Reporter | ||
Comment 9•10 years ago
|
||
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)
Updated•10 years ago
|
blocking-b2g: --- → backlog
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=3]
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=3] → [p=2]
Assignee | ||
Comment 10•10 years ago
|
||
rebase
Attachment #8402451 -
Attachment is obsolete: true
Attachment #8406703 -
Flags: review?(htsai)
Assignee | ||
Comment 11•10 years ago
|
||
make sure the 3rd call is rejected
Attachment #8406703 -
Attachment is obsolete: true
Attachment #8406703 -
Flags: review?(htsai)
Attachment #8406707 -
Flags: review?(htsai)
Reporter | ||
Comment 12•10 years ago
|
||
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)
Reporter | ||
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8402449 -
Attachment is obsolete: true
Attachment #8407295 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8402450 -
Attachment is obsolete: true
Attachment #8407296 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
add into manifest
Attachment #8407299 -
Attachment is obsolete: true
Attachment #8407300 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=11b98ebd6371
Keywords: checkin-needed
Comment 19•10 years ago
|
||
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
Flags: in-testsuite+
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c36ef908c673 https://hg.mozilla.org/mozilla-central/rev/7d26e5a537d8 https://hg.mozilla.org/mozilla-central/rev/f9157f73f5fc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•