1051715 - [Telephony] Show correct phone number in temporary mode clir

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Attachment: Part 1: Add temporary clir test

Comment 2

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

Attachment: Part 2: Also pass number in notifyDialSuccess

Comment 3

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

Attachment: Part 2-2: Also pass number in notifyDialSuccess

This part of code is rebased on Bug 1050696.
It will be squashed into the previous one in the final version (at that time, Bug 1050696 should be landed)

Comment 4

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

Attachment: Emulator: Handle temporary mode clir

When making a call with temporary mode clir, reference-ril adds 'i' or 'I' after the number. We should git rid of that in emulator modem. Otherwise, the number will not be a valid string because it contains the illegal character.

Comment 5

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

Comment on attachment 8470696 [details] [diff] [review]
Part 1: Add temporary clir test

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

Please don't forget to edit manifest.ini, thank you :)

::: dom/telephony/test/marionette/test_temporary_clir.js
@@ +27,5 @@
> +    };
> +
> +    return deferred.promise;
> +  })
> +  .then(() => gRemoteHangUpCalls([outCall]))

nit: we can simply use "gRemoteHangUp"

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Szu-Yu Chen [:aknow] from comment #4)
> Created attachment 8470699 [details] [review]
> Emulator: Handle temporary mode clir
> 
> When making a call with temporary mode clir, reference-ril adds 'i' or 'I'
> after the number. We should git rid of that in emulator modem. Otherwise,
> the number will not be a valid string because it contains the illegal
> character.

I wonder if that's a formal usage or just a dirty hack from AOSP?

Comment 7

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

Anshul,

This is a sub task of the work, unify sendMMI() and dial(). I am adding you into cc to keep you updated on the interface nsITelephonyService change.

Comment 8

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> (In reply to Szu-Yu Chen [:aknow] from comment #4)
> > Created attachment 8470699 [details] [review]
> > Emulator: Handle temporary mode clir
> > 
> > When making a call with temporary mode clir, reference-ril adds 'i' or 'I'
> > after the number. We should git rid of that in emulator modem. Otherwise,
> > the number will not be a valid string because it contains the illegal
> > character.
> 
> I wonder if that's a formal usage or just a dirty hack from AOSP?

It's formal usage defined in spec (27.007)

GSM/UMTS modifier characters
> (refer subclause "Direct dialling from phonebooks")
I or i (override the CLIR supplementary service subscription default value for this call; I = invocation (restrict CLI presentation) and i = suppression (allow CLI presentation); refer subclause "Calling line identification restriction +CLIR")

=> I and i are valid character in dial number.

An example where a voice call is originated:
ATD+1 812 555673I; (type of address defaults to 145, CLI presentation is restricted for this call)
OK					(call setup was successful)

=> Sample usage.

Comment 9

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

Vicamo,

I've tested it in local environment. Could you help to merge the qemu PR? Thank you.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Szu-Yu Chen [:aknow] from comment #9)
> Vicamo,
> 
> I've tested it in local environment. Could you help to merge the qemu PR?
> Thank you.

https://github.com/mozilla-b2g/platform_external_qemu/commit/f0592d4814d738e3f8d840915ef799c13601bdef

Comment 11

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

Attachment: [final] Part 1: Add temporary clir test. r=hsinyi

Comment 12

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

Attachment: [final] Part 2: Also pass number in notifyDialSuccess. r=hsinyi

Comment 13

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

check-in needed for part 1 and part 2. Thx

https://tbpl.mozilla.org/?tree=Try&rev=6cc280b361af

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/af8ce1cffd8c
https://hg.mozilla.org/integration/b2g-inbound/rev/77df300e7b10

Comment 15

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> Anshul,
> 
> This is a sub task of the work, unify sendMMI() and dial(). I am adding you
> into cc to keep you updated on the interface nsITelephonyService change.
Hsin-Yi, I don't see this issue with our RIL even without this interface change so am not sure if this change is really necessary. I couldn't even get temporary mode CLIR to be working with Moz RIL.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af8ce1cffd8c
https://hg.mozilla.org/mozilla-central/rev/77df300e7b10

Comment 17

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

(In reply to Anshul from comment #15)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> > Anshul,
> > 
> > This is a sub task of the work, unify sendMMI() and dial(). I am adding you
> > into cc to keep you updated on the interface nsITelephonyService change.
> Hsin-Yi, I don't see this issue with our RIL even without this interface
> change so am not sure if this change is really necessary. I couldn't even
> get temporary mode CLIR to be working with Moz RIL.

I could confirm temp CLIR works on mozRIL. The interface change isn't that necessary for letting temp clir mode working (because the original API works), but necessary for meeting our UX requirement. We'd correct the call number string exposed to a resolved promise.

Comment 18

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #17)
> (In reply to Anshul from comment #15)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> > > Anshul,
> > > 
> > > This is a sub task of the work, unify sendMMI() and dial(). I am adding you
> > > into cc to keep you updated on the interface nsITelephonyService change.
> > Hsin-Yi, I don't see this issue with our RIL even without this interface
> > change so am not sure if this change is really necessary. I couldn't even
> > get temporary mode CLIR to be working with Moz RIL.
> 
> I could confirm temp CLIR works on mozRIL. The interface change isn't that
> necessary for letting temp clir mode working (because the original API
> works), but necessary for meeting our UX requirement. We'd correct the call
> number string exposed to a resolved promise.
If by UX you mean to show only the number part without the *31# then we have it working without this change. Let me do some more analysis.

Comment 19

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

(In reply to Anshul from comment #18)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #17)
> > (In reply to Anshul from comment #15)
> > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #7)
> > > > Anshul,
> > > > 
> > > > This is a sub task of the work, unify sendMMI() and dial(). I am adding you
> > > > into cc to keep you updated on the interface nsITelephonyService change.
> > > Hsin-Yi, I don't see this issue with our RIL even without this interface
> > > change so am not sure if this change is really necessary. I couldn't even
> > > get temporary mode CLIR to be working with Moz RIL.
> > 
> > I could confirm temp CLIR works on mozRIL. The interface change isn't that
> > necessary for letting temp clir mode working (because the original API
> > works), but necessary for meeting our UX requirement. We'd correct the call
> > number string exposed to a resolved promise.
> If by UX you mean to show only the number part without the *31# then we have
> it working without this change. Let me do some more analysis.

Hey Anshul, that work might because of the current way Dialer implements, i.e. the number displayed is got from oncallschange event, not from the resolved promise. But from the platform point of view we can't reply on app implementation; instead, API should guarantee right data is exposed. Also, very soon the unify bug will land. At that time the information of the promise returned from telephony.dial() does matter to gaia. It's for sure gaia will need to change implementation, and we don't know how the implementation will be. So, correcting the API is the right and necessary way to go I would say.

Comment 20

[Anshul]

Thanks for the explanation Hsin-Yi, that does make sense.