Closed Bug 1005766 Opened 11 years ago Closed 11 years ago

[B2G] [Dialer] Use the new TelephonyCallId field alongside the existing call information

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(feature-b2g:2.0, tracking-b2g:backlog)

RESOLVED FIXED
2.0 S3 (6june)
feature-b2g 2.0
tracking-b2g backlog

People

(Reporter: hsinyi, Assigned: gsvelto)

References

Details

Attachments

(3 files, 3 obsolete files)

This is for the API backward compatibility. Bug 981519 is going to replace telephonyCall.secondNumber with telephonyCall.cdmaWaitingCall. We need corresponding modification on gaia to be able to land bug 981519 safely.
Taking this, I'll post a patch so that we can test right away with the changes in bug 981519.
Assignee: nobody → gsvelto
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Not asking for review just yet as we need the gecko part first.
Attachment #8417401 - Flags: feedback?(etienne)
Comment on attachment 8417401 [details] [diff] [review] [PATCH] Use the new CDMA call waiting API Review of attachment 8417401 [details] [diff] [review]: ----------------------------------------------------------------- looks good, sweet and simple :) and the name change does make the code more readable.
Attachment #8417401 - Flags: feedback?(etienne) → feedback+
As the TelephonyAPI changes are review granted, below is the modification needed: 1) replace telephonyCall.number with telephonyCall.id.number 2) replace telephonyCall.secondNumber with telephonyCall.secondId.number Thanks!
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5) > As the TelephonyAPI changes are review granted, below is the modification > needed: > 1) replace telephonyCall.number with telephonyCall.id.number > 2) replace telephonyCall.secondNumber with telephonyCall.secondId.number > > Thanks! Sorry I should be more accurate. This scope of the bug is to handle backward compatibility between the existing and new APIs, so that landing bug 981519 won't break anything.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6) > Sorry I should be more accurate. This scope of the bug is to handle backward > compatibility between the existing and new APIs, so that landing bug 981519 > won't break anything. OK sounds good. I'm currently stuck on a v1.3t issue though so this will have to wait until next week if it's not a problem.
set feature-b2g=2.0 for this blocks bug 981519.
blocking-b2g: --- → backlog
feature-b2g: --- → 2.0
I'm about to start working on this so I've changed the title to reflect the changed scope in this bug.
Summary: [B2G] [Dialer] replace telephonyCall.secondNumber with telephonyCall.cdmaWaitingCall → [B2G] [Dialer] Use the new TelephonyCallId field alongside the existing call information
Target Milestone: --- → 2.0 S3 (6june)
This patch tries to adjust all applications that make use of TelephonyCall.number to use TelephonyCall.id.number instead. The unit-tests have also been adjusted and the call mock has been updated to reflect this change. I did some testing on a device but this change requires more as we're touching stuff I have no idea how to test in practice (e.g. STK and CDMA).
Attachment #8417401 - Attachment is obsolete: true
Attachment #8432684 - Flags: review?(etienne)
Comment on attachment 8432684 [details] [diff] [review] [PATCH] Adjust applications to be forward-compatible with the TelephonyCall.id field Review of attachment 8432684 [details] [diff] [review]: ----------------------------------------------------------------- Hey Doug, since you're on a roll regarding the CallsHandler/HandledCalls stuff, mind reviewing this?
Attachment #8432684 - Flags: review?(etienne) → review?(drs+bugzilla)
Comment on attachment 8432684 [details] [diff] [review] [PATCH] Adjust applications to be forward-compatible with the TelephonyCall.id field Review of attachment 8432684 [details] [diff] [review]: ----------------------------------------------------------------- I'm not particularly confident in my review here since I have no idea how to test this. But if there are any problems, I think we can cross those bridges when we come to them. ::: apps/callscreen/test/unit/calls_handler_test.js @@ -729,5 @@ > test('should invoke hold to answer the second call', function() { > var holdSpy = this.sinon.spy(call, 'hold'); > CallsHandler.holdAndAnswer(); > assert.isTrue(holdSpy.calledOnce); > }); There will be more changes in this file when you rebase. ::: apps/communications/dialer/js/dialer.js @@ -239,5 @@ > } > > function callEnded(data) { > var highPriorityWakeLock = navigator.requestWakeLock('high-priority'); > - var number = data.number; I think you might have missed the "data.number" at line 159 in handleActivity. (though it might be a completely different class) ::: apps/communications/dialer/test/unit/dialer_test.js @@ +122,5 @@ > setup(function() { > this.sinon.spy(window, 'Notification'); > MockNavigatorMozIccManager.addIcc('12345', {'cardState': 'ready'}); > callEndedData = { > + id: { number: '123' }, I think you might have missed sysMsg at line 182.
Attachment #8432684 - Flags: review?(drs+bugzilla) → review+
Thanks for the review Doug. (In reply to Doug Sherk (:drs) from comment #13) > I'm not particularly confident in my review here since I have no idea how to > test this. But if there are any problems, I think we can cross those bridges > when we come to them. Yeah, that's the trickiest part. Note that I've modified the unit-tests and mocks to use the new field however I can run them successfully both with and w/o this change; this should prove that the covered parts should be working fine with both APIs and won't regress. > There will be more changes in this file when you rebase. OK, thanks. > I think you might have missed the "data.number" at line 159 in > handleActivity. (though it might be a completely different class) That's coming from an activity message and it's not a TelephonyCall object so it shouldn't be affected. > I think you might have missed sysMsg at line 182. Yes indeed, nice catch, thanks! Updated patch coming soon.
Status: NEW → ASSIGNED
Updated patch rebased on top of master.
Attachment #8432684 - Attachment is obsolete: true
Attachment #8434982 - Flags: review?(drs+bugzilla)
Since I used the linter to catch errors in my patch I've took the liberty to squash a few linter errors in the files I touched with the previous patch and removed them from the xfail.list file. This patch is not required for fixing this bug but it will help in keeping our code clean if you have some time to review it.
Attachment #8434983 - Flags: review?(drs+bugzilla)
Comment on attachment 8434982 [details] [diff] [review] [PATCH 1/2 v2] Adjust applications to be forward-compatible with the TelephonyCall.id field Review of attachment 8434982 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/callscreen/test/unit/calls_handler_test.js @@ +730,5 @@ > test('should invoke hold to answer the second call', function() { > var holdSpy = this.sinon.spy(call, 'hold'); > CallsHandler.holdAndAnswer(); > assert.isTrue(holdSpy.calledOnce); > }); There's a new suite, "> active calls getters", which you missed here. There will be a bunch of changes needed there.
Attachment #8434982 - Flags: review?(drs+bugzilla) → review+
Comment on attachment 8434983 [details] [diff] [review] [PATCH 2/2] Cleanup linter errors in the affected files Review of attachment 8434983 [details] [diff] [review]: ----------------------------------------------------------------- In bug 1007709, we're going to be fixing the linter errors in calls_handler.js and keypad.js. You can land this anyways and make Germán rebase, though. :p I prefer your fixes for keypad.js anyways.
Attachment #8434983 - Flags: review?(drs+bugzilla) → review+
Travis is finally green: https://travis-ci.org/mozilla-b2g/gaia/builds/27107128 I just had to fix a small conflict in build/jshint/xfail.list before merging. Both patches were merged in gaia/master 9811ebed202ad948366e25ed70e8eb794f362e8d https://github.com/mozilla-b2g/gaia/commit/9811ebed202ad948366e25ed70e8eb794f362e8d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 1037354
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: