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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Taking this, I'll post a patch so that we can test right away with the changes in bug 981519.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Use the new CDMA call waiting API

Not asking for review just yet as we need the gecko part first.

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Use the new CDMA call waiting API

Comment 4

[Etienne Segonzac (: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.

Comment 5

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

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!

Comment 6

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

(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.

Comment 7

[Gabriele Svelto [:gsvelto] (PTO)]

(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.

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

set feature-b2g=2.0 for this blocks bug 981519.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

I'm about to start working on this so I've changed the title to reflect the changed scope in this bug.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PULL REQUEST] Adjust applications to be forward-compatible with the TelephonyCall.id field

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH] Adjust applications to be forward-compatible with the TelephonyCall.id field

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).

Comment 12

[Etienne Segonzac (: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?

Comment 13

[Doug Sherk (:drs) (inactive)]

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.

Comment 14

[Gabriele Svelto [:gsvelto] (PTO)]

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.

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 1/2 v2] Adjust applications to be forward-compatible with the TelephonyCall.id field

Updated patch rebased on top of master.

Comment 16

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: [PATCH 2/2] Cleanup linter errors in the affected files

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.

Comment 17

[Doug Sherk (:drs) (inactive)]

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.

Comment 18

[Doug Sherk (:drs) (inactive)]

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.

Comment 19

[Gabriele Svelto [:gsvelto] (PTO)]

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