859318 - [b2g-ril] Handle call waiting settings through mozMobilleConnection/mozIccManager API

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

In order to implement a better UX for call waiting settings we need to implement RIL_REQUEST_QUERY_CALL_WAITING request. The approach followed in call forwarding settings seems to work fine so the idea is to add a couple of functions like `setCallWaitingOption()` and `getCallWaitingOption()` to mozMobilleConnection/mozIccManager API.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v1

Jonas, this WIP code is about adding to mozMobileConnection API a couple of functions for handling call waiting settings. Could you take a look please? Thx!

Comment 2

[Daniel Coloma:dcoloma]

blocks a blocker

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: IDL changes for call waiting functions. v1.

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 2: Changes in MobileConnection.cpp for call waiting functions. v1.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 3: RIL changes for call waiting functions. v1.

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 735185 [details] [diff] [review]
Part 1: IDL changes for call waiting functions. v1.

Jonas, this WIP code is about adding to mozMobileConnection API a couple of functions for handling call waiting settings. Could you take a look please? Thx!

Comment 7

[José Antonio Olivera Ortega [:jaoo]]

It still need test. Working on it.

Comment 8

[Anshul]

Jose, does the settings app query for call waiting every time user enters the "Call Settings" or only when the settings app is started? The reason I am asking is that call waiting can also be set through MMI codes and if the settings app doesn't requery everytime the call settings screen is open the call waiting setting might be out of syn if user changed the call waiting setting through MMI code.

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Anshul from comment #8)
> Jose, does the settings app query for call waiting every time user enters
> the "Call Settings" or only when the settings app is started? The reason I
> am asking is that call waiting can also be set through MMI codes and if the
> settings app doesn't requery everytime the call settings screen is open the
> call waiting setting might be out of syn if user changed the call waiting
> setting through MMI code.

Anshul, that's a very good question. We run into this issue in the past with call forwarding settings so this time we will do it properly in the gaia side of this development (bug 857691). BTW, AFAIK Moz RIL implementation doesn't support handling call waiting settings through MMI codes, does it? (See [1] please).

[1] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2313

Comment 10

[Anshul]

Jose, I know Moz RIL doesn't support call waiting through MMI codes but if it did in future, there would be a problem.

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 4: Add test for call waiting functions.

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 735187 [details] [diff] [review]
Part 2: Changes in MobileConnection.cpp for call waiting functions. v1.

This patch is about adding the boilerplate cpp code for adding a couple of function for setting call waiting options to mozMobileConnection API. Could you take a look please? Thanks.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 735188 [details] [diff] [review]
Part 3: RIL changes for call waiting functions. v1.

Vicamo, this patch is about adding and changing the code in the RIL for adding a couple of functions for setting call waiting options to mozMobileConnection. Existing approach through mozSetting API didn't implement the function for querying call waiting status and it cause some weird states even being the phone able to receive the call waiting settings from the network. This was raised by the certification team. Could you take a look please? Thx!

Comment 14

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

Comment on attachment 735185 [details] [diff] [review]
Part 1: IDL changes for call waiting functions. v1.

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +303,5 @@
> +   * Queries current call waiting options.
> +   *
> +   * If successful, the request's onsuccess will be called, and the request's
> +   * result will be an object containing information about the operation
> +   * e.g. {enabled: true}.

Why does it have to be wrapped by an object?

Comment 15

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

Comment on attachment 735188 [details] [diff] [review]
Part 3: RIL changes for call waiting functions. v1.

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

This means those interested in call waiting setting will not be notified unless the request is made himself. Doesn't that cause UI inconsistency?

::: dom/system/gonk/RILContentHelper.js
@@ +112,5 @@
> +  this.enabled = options.enabled;
> +};
> +CallWaitingOption.prototype = {
> +  __exposedProps__ : {enabled: 'r'}
> +};

Can't we just fireSuccess(boolean)?

@@ +753,5 @@
> +    let request = Services.DOMRequest.createRequest(window);
> +    let requestId = this.getRequestId(request);
> +
> +    cpmm.sendAsyncMessage("RIL:GetCallWaitingOption", {
> +      requestId: requestId    

nit: trailing ws

::: dom/system/gonk/ril_worker.js
@@ +4748,5 @@
> +    return;
> +  }
> +  options.length = Buf.readUint32();
> +  options.enabled = ((Buf.readUint32() == 1) && 
> +                     ((Buf.readUint32() & ICC_SERVICE_CLASS_VOICE) == 0x01)); 

nit: trailing ws

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> Comment on attachment 735185 [details] [diff] [review]
> Part 1: IDL changes for call waiting functions. v1.
> 
> Review of attachment 735185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> @@ +303,5 @@
> > +   * Queries current call waiting options.
> > +   *
> > +   * If successful, the request's onsuccess will be called, and the request's
> > +   * result will be an object containing information about the operation
> > +   * e.g. {enabled: true}.
> 
> Why does it have to be wrapped by an object?

Because I was wondering if it'd be useful to return the service class bit vector of services for which call waiting is enabled and add it another property for that. Thoughts?

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> Comment on attachment 735188 [details] [diff] [review]
> Part 3: RIL changes for call waiting functions. v1.
> 
> Review of attachment 735188 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This means those interested in call waiting setting will not be notified
> unless the request is made himself. Doesn't that cause UI inconsistency?

Why? AFAIK call waiting settings are set under user action and there is no any kind of unsolicited message coming from the network letting know the device that call waiting settings have changed so IMHO there is no need of notifications here. If you look into the code of bug 857691 the user sets the call waiting preferences but right after setting them we query those settings before showing the real status reported from the network. That way the UI always maintains consistency with the real status. That's what we did for call forwarding settings and how Android works too.

Comment 18

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #17)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> > Comment on attachment 735188 [details] [diff] [review]
> > Part 3: RIL changes for call waiting functions. v1.
> > 
> > Review of attachment 735188 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This means those interested in call waiting setting will not be notified
> > unless the request is made himself. Doesn't that cause UI inconsistency?
> 
> Why? AFAIK call waiting settings are set under user action and there is no
> any kind of unsolicited message coming from the network letting know the
> device that call waiting settings have changed so IMHO there is no need of
> notifications here. 

Not exactly. The current call waiting settings notification isn't based on some event but on settings change. The current implementation is that, settings value is not just the user settings but the real call waiting status as well. Once the settings value change (means the call waiting status changes as well), every application listening will get the notification. That makes the all the applications interested in call waiting status consistant.

> If you look into the code of bug 857691 the user sets
> the call waiting preferences but right after setting them we query those
> settings before showing the real status reported from the network. That way
> the UI always maintains consistency with the real status. That's what we did
> for call forwarding settings and how Android works too.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> Not exactly. The current call waiting settings notification isn't based on
> some event but on settings change. The current implementation is that,
> settings value is not just the user settings but the real call waiting
> status as well. 

Well, I know current implementation notifies other applications by listening for/observing changes in the 'ril.callwaiting.enabled' setting and its value reflects which the real status is always.

> Once the settings value change (means the call waiting
> status changes as well), every application listening will get the
> notification. That makes the all the applications interested in call waiting
> status consistant.

IIRC there are no other applications observing changes for the 'ril.callwaiting.enabled' settings, aren't there? If so we could keep setting the call waiting status requested from the network in the 'ril.callwaiting.enabled' settings every time the `getCallWaitingOption()` function get called from the call setting UI. Thoughts?

Comment 20

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #19)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> > Not exactly. The current call waiting settings notification isn't based on
> > some event but on settings change. The current implementation is that,
> > settings value is not just the user settings but the real call waiting
> > status as well. 
> 
> Well, I know current implementation notifies other applications by listening
> for/observing changes in the 'ril.callwaiting.enabled' setting and its value
> reflects which the real status is always.
> 
> > Once the settings value change (means the call waiting
> > status changes as well), every application listening will get the
> > notification. That makes the all the applications interested in call waiting
> > status consistant.
> 
> IIRC there are no other applications observing changes for the
> 'ril.callwaiting.enabled' settings, aren't there? If so we could keep
> setting the call waiting status requested from the network in the
> 'ril.callwaiting.enabled' settings every time the `getCallWaitingOption()`
> function get called from the call setting UI. Thoughts?

I do support that we have a separate API for set/get call waiting because the current implementation is sort of misusing the real concept of 'setttings.' We should keep settings value simply what it should be, instead of letting the value indicating both the user's expectation and real state. It's indeed appropriate to correct the misbeaviour in this issue. So I am afraid that I incline to not supporting changing 'ril.callwaiting.enabled.' 

As I know, Gaia team/Settings App owners have discussed the misusing of settings API, and planned to corret that.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #20)
> I do support that we have a separate API for set/get call waiting because
> the current implementation is sort of misusing the real concept of
> 'setttings.' We should keep settings value simply what it should be, instead
> of letting the value indicating both the user's expectation and real state.
> It's indeed appropriate to correct the misbeaviour in this issue. So I am
> afraid that I incline to not supporting changing 'ril.callwaiting.enabled.'

I'm sorry but I'm getting myself lost in the discussion. Are you saying that we should keep current implementation and keep handling call waiting settings through mozSettings API? If so IMHO I don't agree because in current implementation there is no way to be sure about which the real status is. It just simply sets the desired value and reflects that status in the UI but doesn't query/check if that desired stated was set by the network successfully because the function needed for requesting real status is not implemented in current implementation.

This work was started as a need for bug 857691 found for the certification process. Take a look at it please.

> As I know, Gaia team/Settings App owners have discussed the misusing of
> settings API, and planned to corret that.

Comment 22

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #21)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #20)
> > I do support that we have a separate API for set/get call waiting because
> > the current implementation is sort of misusing the real concept of
> > 'setttings.' We should keep settings value simply what it should be, instead
> > of letting the value indicating both the user's expectation and real state.
> > It's indeed appropriate to correct the misbeaviour in this issue. So I am
> > afraid that I incline to not supporting changing 'ril.callwaiting.enabled.'
> 
> I'm sorry but I'm getting myself lost in the discussion. Are you saying that
> we should keep current implementation and keep handling call waiting
> settings through mozSettings API? If so IMHO I don't agree because in
> current implementation there is no way to be sure about which the real
> status is. It just simply sets the desired value and reflects that status in
> the UI but doesn't query/check if that desired stated was set by the network
> successfully because the function needed for requesting real status is not
> implemented in current implementation.
> 
> This work was started as a need for bug 857691 found for the certification
> process. Take a look at it please.
> 

No, I wasn't saying that we should keep the current implementation. As I said, I do support that we have a separate API for set/get call waiting as what you're trying to do in this bug, because the current implementation is sort of misusing the real concept of 'setttings.' 

I just said that, Not update ril.callwaiting.enabled even when getCallWaitingOption() got called. We should keep the value of ril.callwaiting.enabled simply as what user requests instead what the exact call waiting state. Hope it's clear now.
> > As I know, Gaia team/Settings App owners have discussed the misusing of
> > settings API, and planned to corret that.

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

If mozSetings is used to keep user preference, it should only be changed at the time of users set the setting value explicitly. On the other hand, if mozSettins is used for UI (i.e. icons on the status bar), we should update the corresponding setting value in mozSettings after an API got called. All of this really depend on how we use mozSettings. Now we use it in both two ways. 

In this case, if we are going to make it consistent with the implementation of call forwarding, I would suggest update ril.callwaiting.enabled when getCallWaitingOption() got called.

> I just said that, Not update ril.callwaiting.enabled even when
> getCallWaitingOption() got called. We should keep the value of
> ril.callwaiting.enabled simply as what user requests instead what the exact
> call waiting state. Hope it's clear now.
> > > As I know, Gaia team/Settings App owners have discussed the misusing of
> > > settings API, and planned to corret that.

Comment 24

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

(In reply to Arthur Chen [:arthurcc] from comment #23)
> If mozSetings is used to keep user preference, it should only be changed at
> the time of users set the setting value explicitly. On the other hand, if
> mozSettins is used for UI (i.e. icons on the status bar), we should update
> the corresponding setting value in mozSettings after an API got called. All
> of this really depend on how we use mozSettings. Now we use it in both two
> ways. 
> 

Yes, I know that mozSettings are used in both 2 ways, but I also heard that Gaia/Settings owners have a plan to correct the inconsistant behaviours, so that mozSettings express user preference only. That'w why I think it's better to not update ril.callwating.enabled in this issue. If you think having the same behaviour with call forwarding, then I'll be fine that we file another bug to correct the mozSetting's inconsistancy. 


> In this case, if we are going to make it consistent with the implementation
> of call forwarding, I would suggest update ril.callwaiting.enabled when
> getCallWaitingOption() got called.
> 
> > I just said that, Not update ril.callwaiting.enabled even when
> > getCallWaitingOption() got called. We should keep the value of
> > ril.callwaiting.enabled simply as what user requests instead what the exact
> > call waiting state. Hope it's clear now.
> > > > As I know, Gaia team/Settings App owners have discussed the misusing of
> > > > settings API, and planned to corret that.

Comment 25

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> (In reply to Arthur Chen [:arthurcc] from comment #23)
> > If mozSetings is used to keep user preference, it should only be changed at
> > the time of users set the setting value explicitly. On the other hand, if
> > mozSettins is used for UI (i.e. icons on the status bar), we should update
> > the corresponding setting value in mozSettings after an API got called. All
> > of this really depend on how we use mozSettings. Now we use it in both two
> > ways. 
> > 
> 
> Yes, I know that mozSettings are used in both 2 ways, but I also heard that
> Gaia/Settings owners have a plan to correct the inconsistant behaviours, so
> that mozSettings express user preference only. That'w why I think it's
> better to not update ril.callwating.enabled in this issue. If you think
> having the same behaviour with call forwarding, then I'll be fine that we

Oops, missing some keywords, though you might be able to infer my complete thoughts from the conext. 
"If you think having the same behaviour with call forwarding is better, then I'll be fine .... "

> file another bug to correct the mozSetting's inconsistancy. 
> 
> 
> > In this case, if we are going to make it consistent with the implementation
> > of call forwarding, I would suggest update ril.callwaiting.enabled when
> > getCallWaitingOption() got called.

Comment 26

[José Antonio Olivera Ortega [:jaoo]]

Hsin-Yi, Arthur, we could avoid updating `ril.callwating.enabled` in the RIL plumbing and let the Setting App doing it to make it consistent with the implementation of call forwarding. That way the Setting could update it in bug 857691 and remove it depending on how the mozSetting API changes in the future.

Comment 27

[Arthur Chen [:arthurcc][inactive after 6/30]]

Jose, Hsin-Yi, I just found that what my suggestion in comment 23 is not very clear. What I really meant was updating ril.callwaiting.enabled from *gaia* after getCallWaitingOption() got called.

Comment 28

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

(In reply to Arthur Chen [:arthurcc] from comment #27)
> Jose, Hsin-Yi, I just found that what my suggestion in comment 23 is not
> very clear. What I really meant was updating ril.callwaiting.enabled from
> *gaia* after getCallWaitingOption() got called.

Cool, that sounds good to me as well as jaoo's comment 26.

Comment 29

[José Antonio Olivera Ortega [:jaoo]]

https://tbpl.mozilla.org/?tree=Try&rev=7a6be47bcdc0

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 735185 [details] [diff] [review]
Part 1: IDL changes for call waiting functions. v1.

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +303,5 @@
> +   * Queries current call waiting options.
> +   *
> +   * If successful, the request's onsuccess will be called, and the request's
> +   * result will be an object containing information about the operation
> +   * e.g. {enabled: true}.

I agree with Vicamo, this shouldn't need to be wrapped in an object.

Comment 31

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 4: Add test for call waiting functions. v2.

Vicamo, these are the tests. I've already checked that they pass. Could you take a look please?

Comment 32

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

Comment on attachment 735188 [details] [diff] [review]
Part 3: RIL changes for call waiting functions. v1.

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

Please fire request results with a boolean value instead.

Comment 33

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

Comment on attachment 738551 [details] [diff] [review]
Part 4: Add test for call waiting functions. v2.

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

::: dom/system/gonk/tests/test_ril_worker_cw.js
@@ +37,5 @@
> +  };
> +
> +  worker.RIL.setCallWaiting({
> +    enabled: true
> +  });

Boolean along is sufficient. Is there any reason we must wrap a simple boolean value with an object?

Comment 34

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 1: IDL changes for call waiting functions. v2. r=sicking

Sicking's comments addressed.

Comment 35

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Part 3: RIL changes for call waiting functions. v2.

Vicamo's comments addressed. Vicamo, could you take a look please? Thanks!

Comment 36

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Vicamo Yang (@Telefonica 6F) [:vicamo][:vyang] from comment #33)
> Comment on attachment 738551 [details] [diff] [review]
> Part 4: Add test for call waiting functions. v2.
> 
> Review of attachment 738551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/tests/test_ril_worker_cw.js
> @@ +37,5 @@
> > +  };
> > +
> > +  worker.RIL.setCallWaiting({
> > +    enabled: true
> > +  });
> 
> Boolean along is sufficient. Is there any reason we must wrap a simple
> boolean value with an object?

The setCallWaiting() function in ril_worker receives an object as parameter.

Comment 37

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

Comment on attachment 738950 [details] [diff] [review]
Part 3: RIL changes for call waiting functions. v2.

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

Thank you ;)

Comment 38

[José Antonio Olivera Ortega [:jaoo]]

https://hg.mozilla.org/projects/birch/rev/143ca81b0e11
https://hg.mozilla.org/projects/birch/rev/cfa6e89e9d4f
https://hg.mozilla.org/projects/birch/rev/311d5a0b4384
https://hg.mozilla.org/projects/birch/rev/8e4d4e5495ee

Comment 39

[José Antonio Olivera Ortega [:jaoo]]

Ooops! I resolved the bug by accident. Sorry for that. Reopening.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/143ca81b0e11
https://hg.mozilla.org/mozilla-central/rev/cfa6e89e9d4f
https://hg.mozilla.org/mozilla-central/rev/311d5a0b4384
https://hg.mozilla.org/mozilla-central/rev/8e4d4e5495ee

Comment 41

[Anshul]

Dietrich, if this bug is a NO_UPIFT because of us would like you to know that this can be uplifted as the corresponding change in commercial RIL is ready.

Comment 42

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Anshul from comment #41)
> Dietrich, if this bug is a NO_UPIFT because of us would like you to know
> that this can be uplifted as the corresponding change in commercial RIL is
> ready.

Anshul, we need you to review if this bug could be uplifted to mozilla-b2g18 and mozilla-b2g18_v1_0_1 release repos. This bug is blocking other upliftings. Take a look please. Thanks.

Comment 43

[Anshul]

Antonio, yes we already have the corresponding change in commercial RIL. We are testing the change by manually pulling in the dependencies. In fact getting this landed will help us test it quickly.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/1052393b84cc
https://hg.mozilla.org/releases/mozilla-b2g18/rev/f656eaa5b024
https://hg.mozilla.org/releases/mozilla-b2g18/rev/168391893e4f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0eb9b2d7b60c

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c88a0fe74d7b
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/95b432a4f94e
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/6dfdb0ddd58a
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5d454864b668