1185435 - [IMS] HD Voice Indicator

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

This bug is to provide an HD indicator of the telephony call for user to identify the ongoing call is a HD resolution audio call.

Comment 1

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

Hi Ben, please help this! Bevis and I could provide assistance and solution discussion.

Comment 2

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

We will land this on v2.2r only, so set status-b2g-master:wontfix.

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add |quality| to TelephonyCall (WebIDL)

Hi Hsin-Yi

Though the other patches need further tested, I think you may want to review this patch first :P

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 2: Add |quality| to TeleponyCallInfo and related functions (IDL)

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 3: Add |quality| to related DOM obejects (DOM)

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 4: Set call |quality| default to |Normal| (TelephonyService)

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 5: Extend IPC channel to handle |quality| (IPC)

Comment 8

[Ben Hsu [:HoPang]]

Attachment: Part 6: Check |quality| (testcases)

Since we fail to replace |TeleponyService| during runtime, we cannot have a better examination with mochitest. Now, we can only use this marionette testcase to prove that things are working as expected with quality preseted to |Normal| or |HD|. In this patch, the quality is preset to |Normal|, while |HD| is tested locally.

Comment 9

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

Comment on attachment 8653265 [details] [diff] [review]
Part 1: Add |quality| to TelephonyCall (WebIDL)

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

Thanks Ben!

::: dom/webidl/TelephonyCall.webidl
@@ +32,5 @@
>    readonly attribute DOMError? error;
>  
>    readonly attribute TelephonyCallGroup? group;
>  
> +  // Indicate whether the call quality is normal or HD

Though HD may sound trivial, it's always good to explain what an abbreviation stands for. Please comment that.

nit: place a period at the end of a line.

Comment 10

[Ben Hsu [:HoPang]]

A short update: these patches work as expected on Flame-kk verified with WebIDE.

Comment 11

[Ben Hsu [:HoPang]]

Attachment: Part 1: Add |quality| to TelephonyCall (WebIDL). r=htsai

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8653266 [details] [diff] [review]
Part 2: Add |quality| to TeleponyCallInfo and related functions (IDL)

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

r=me if the comments below are addressed.

::: dom/telephony/nsITelephonyCallInfo.idl
@@ +22,5 @@
>     */
>    readonly attribute unsigned short callState;
>  
>    /**
> +   * Indicate whether the call quality is normal or HD, and its value should be

/**
 * One of the nsITelephonyService::CALL_QUALITY_* values.
 *
 * Set to CALL_QUALITY_HD to indicate the voice quality is high definition.
 * e.g. AMR-WB is adopted.
 */

@@ +25,5 @@
>    /**
> +   * Indicate whether the call quality is normal or HD, and its value should be
> +   * one of the nsITelephonyService::CALL_QUALITY_* values.
> +   */
> +  readonly attribute unsigned short quality;

I prefer to rename |quality| as voiceQuality to have more clear understanding of what HD indicates to.

How do you think?

::: dom/telephony/nsITelephonyService.idl
@@ +139,5 @@
>     */
>    void notifyDialCallSuccess(in unsigned long cliendId,
>                               in unsigned long callIndex,
> +                             in AString number,
> +                             in unsigned short quality);

no uuid update for nsITelephonyDialCallback.

Comment 13

[Ben Hsu [:HoPang]]

Hi Bevis, thanks for your review.

> I prefer to rename |quality| as voiceQuality to have more clear
> understanding of what HD indicates to.
> 
> How do you think?

I think it's fine. However, if we want to modify the WebIDL as well, I think we should NI Hsin-Yi to get her approval.

Comment 14

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

(In reply to Ben Hsu [:bhsu] from comment #13)
> Hi Bevis, thanks for your review.
> 
> > I prefer to rename |quality| as voiceQuality to have more clear
> > understanding of what HD indicates to.
> > 
> > How do you think?
> 
> I think it's fine. However, if we want to modify the WebIDL as well, I think
> we should NI Hsin-Yi to get her approval.

Even better! r+++++

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8653267 [details] [diff] [review]
Part 3: Add |quality| to related DOM obejects (DOM)

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

r=me if nits and the rename of |quality| to |voiceQuality| are done.

Thanks!

::: dom/telephony/TelephonyCall.cpp
@@ +19,5 @@
>  
>  // static
>  already_AddRefed<TelephonyCall>
> +TelephonyCall::Create(Telephony* aTelephony,
> +

nit: we don't have to format the parameters in this way with empty lines.

@@ +25,5 @@
> +                      uint32_t aServiceId,
> +                      uint32_t aCallIndex,
> +                      uint16_t aCallState,
> +                      TelephonyCallQuality aQuality,
> +

ditto

::: dom/telephony/TelephonyCall.h
@@ +125,5 @@
>    IMPL_EVENT_HANDLER(groupchange)
>  
>    static already_AddRefed<TelephonyCall>
> +  Create(Telephony* aTelephony,
> +

nit: we don't have to format the parameters in this way with empty lines.

@@ +131,5 @@
> +         uint32_t aServiceId,
> +         uint32_t aCallIndex,
> +         uint16_t aCallState,
> +         TelephonyCallQuality aQuality,
> +

ditto

::: dom/telephony/TelephonyCallInfo.cpp
@@ +13,5 @@
>  TelephonyCallInfo::TelephonyCallInfo(uint32_t aClientId,
>                                       uint32_t aCallIndex,
>                                       uint16_t aCallState,
> +                                     uint32_t aQuality,
> +

nit: we don't have to format the parameters in this way with empty lines.

@@ +18,5 @@
>                                       const nsAString& aNumber,
>                                       uint16_t aNumberPresentation,
>                                       const nsAString& aName,
>                                       uint16_t aNamePresentation,
> +

ditto

::: dom/telephony/TelephonyCallInfo.h
@@ +22,5 @@
> +  TelephonyCallInfo(uint32_t aClientId,
> +                    uint32_t aCallIndex,
> +                    uint16_t aCallState,
> +                    uint32_t aQuality,
> +

nit: we don't have to format the parameters in this way with empty lines.

@@ +27,5 @@
> +                    const nsAString& aNumber,
> +                    uint16_t aNumberPresentation,
> +                    const nsAString& aName,
> +                    uint16_t aNamePresentation,
> +

ditto.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8653268 [details] [diff] [review]
Part 4: Set call |quality| default to |Normal| (TelephonyService)

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

r=me after nits and the rename of |quality| to |voiceQuality| are done.

::: dom/telephony/gonk/TelephonyService.js
@@ +61,5 @@
>  const MMI_MATCH_GROUP_SIC = 6;
>  const MMI_MATCH_GROUP_PWD_CONFIRM = 7;
>  const MMI_MATCH_GROUP_DIALING_NUMBER = 8;
>  
> +const TELEPHONY_CALL_QUALITY = nsITelephonyService.CALL_QUALITY_NORMAL;

nit: no need to an extra const here. 
nsITelephonyService.CALL_QUALITY_NORMAL is good enough for reference and
is more readable that this const are defined in nsITelephonyService.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8653269 [details] [diff] [review]
Part 5: Extend IPC channel to handle |quality| (IPC)

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

r=me after the rename of |quality| to |voiceQuality| is done.

Thanks!

Comment 18

[Ben Hsu [:HoPang]]

Attachment: Part 1 (V3): Add |quality| to TelephonyCall (WebIDL)

Comment 19

[Ben Hsu [:HoPang]]

Attachment: Part 2 (V2): Add |quality| to TeleponyCallInfo and related functions (IDL). r=btseng

Comment 20

[Ben Hsu [:HoPang]]

Attachment: Part 3 (V2): Add |quality| to related DOM obejects (DOM). r=btseng

Comment 21

[Ben Hsu [:HoPang]]

Attachment: Part 4 (V2): Set call |quality| default to |Normal| (Service ). r=btseng

Comment 22

[Ben Hsu [:HoPang]]

Attachment: Part 5 (V2): Extend IPC channel to handle |quality| (IPC). r=btseng

Comment 23

[Ben Hsu [:HoPang]]

Attachment: Part 6 (V2): Check |quality| (testcases)

Comment 24

[Ben Hsu [:HoPang]]

Comment on attachment 8654742 [details] [diff] [review]
Part 1 (V3): Add |quality| to TelephonyCall (WebIDL)

Hi Hsin-Yi,

Since the name of the attribute is change to |voiceQuality| and the related comment is also updated, I think it's better to ask for another round of review. Do you mind reviewing this patch again?

Comment 25

[Ben Hsu [:HoPang]]

Attachment: Part 1 (V3): Add |quality| to TelephonyCall (WebIDL). r=htsai

Sorry, I uploaded the wrong part :P

Comment 26

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

Comment on attachment 8654752 [details] [diff] [review]
Part 1 (V3): Add |quality| to TelephonyCall (WebIDL). r=htsai

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

Thanks!

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8654749 [details] [diff] [review]
Part 6 (V2): Check |quality| (testcases)

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

r=me with nits addressed.

Thanks!

::: dom/telephony/test/marionette/head.js
@@ +424,5 @@
>     *        An array of call info with the format of "callStrPool()[state]".
>     * @return Promise
>     */
>    function checkAll(active, calls, conferenceState, conferenceCalls, callList) {
> +    // Check quality

nit: Check voice quality

@@ +425,5 @@
>     * @return Promise
>     */
>    function checkAll(active, calls, conferenceState, conferenceCalls, callList) {
> +    // Check quality
> +    telephony.calls.forEach(call => is(call.voiceQuality, "Normal", "check quality"));

nit: "check voice quality"

@@ +426,5 @@
>     */
>    function checkAll(active, calls, conferenceState, conferenceCalls, callList) {
> +    // Check quality
> +    telephony.calls.forEach(call => is(call.voiceQuality, "Normal", "check quality"));
> +    conference.calls.forEach(call => is(call.voiceQuality, "Normal", "check quality"));

ditto

@@ +459,5 @@
>          ok(outCall instanceof TelephonyCall, "check instance");
>          is(outCall.id.number, number);
>          is(outCall.state, "dialing");
>          is(outCall.serviceId, serviceId);
> +        is(outCall.voiceQuality, "Normal", "check quality");

ditto

@@ +625,5 @@
>          let call = event.call;
>  
>          ok(call);
>          is(call.state, "incoming");
> +        is(call.voiceQuality, "Normal","check quality");

ditto

Comment 28

[Ben Hsu [:HoPang]]

Attachment: Part 6 (V3): Check |quality| (testcases). r=btseng

Comment 29

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14d237081061

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/7de3c213b801
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/30f837fb01dd
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/c1eb180dffff
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/0f7e9c2f194c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/4d6d91c16cc4
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/f147d0e75242