856553 - [B2G] RIL: need an API to enable/disable radio

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Evelyn Hung]

Related Gecko issue for bug 851452. We need an API to achieve the goal, like my comment 4 on bug 851452.

Nominate leo+ because bug 851452 is leo+.

Comment 1

[leo.bugzilla.gecko]

I heard that it is already discussed with leo+.
They have a solution.
Please contact leo+.

Comment 2

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

Hi Anshul, in bug 851452 you confirmed that commertial RIL supports this feature, so I guess we should innominate this issue as well, right? 

Please renominate it if I misunderstood something. Thanks!

Comment 3

[Anshul]

Hsin-Yi, you are  right. Thanks.

Comment 4

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

The API will be implement in mobile connection.
currently, I have two proposals:

(1) provide two APIs
  setRadioPower(true|false) for 'normal' or 'airplane mode'
  and offer another one for 'phone power off'

setRadioPower() with a boolean argument makes it looks like a nature language (power on, power off).
The 'power off' API is intent to be called when and only when user power off the phone. It could also have more ability to do some clean up or anything should be handled at that situation.

(2) setRadioPower(on|off|phone power off)

Have three options may be a little weird for me.
However it may be more concise?

Any idea?

Comment 5

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

proposal 3
(3) setRadioPower(true|false [, phonePowerOff = true|false ])

ex:
setRadioPower(true)         // on
setRadioPower(false)        // airplane mode
setRadioPower(false, true)  // phone power off

Comment 6

[Mounir Lamouri (:mounir)]

Sorry in advance if that question is stupid but I'm lacking context here so I wonder why we can't simply have Gecko shutting down the radio when the device is in the process of shutting down?

Comment 7

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

Yes, it's also a good solution.

For the implementation:
Refer to https://developer.mozilla.org/en-US/docs/Observer_Notifications and my experimental results, we could observe the topic "quit-application" in RadioInterfaceLayer.js. Thus we could know the timing to shut down the radio.

In addition, RadioInterfaceLayer doesn't received the topic "xpcom-will-shutdown" or "xpcom-shutdown" when phone power off. So I think "quit-application" is the appropriate one.

Then, gecko could handle this automatically. No need for gaia to explicitly call the API when power off.

So, now the question is that do we have to expose an API for gaia to set the radio power. This could be used for switching the airplane mode.

Following is the current design for switching aireplane mde:
(1) gaia change setting => gecko observe the setting changed => gecko set radio power
(2) gecko found radio power change => gecko update the setting

AFAIK, we should loose the coupling between gecko and setting page. It's better to not directly alter the setting value from gecko. So we could provide the new API for set radio. 

Need HsinYi's further comment for this part.

Comment 8

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

(In reply to Szu-Yu Chen [:aknow] from comment #7)
> Yes, it's also a good solution.
> 
> For the implementation:
> Refer to https://developer.mozilla.org/en-US/docs/Observer_Notifications and
> my experimental results, we could observe the topic "quit-application" in
> RadioInterfaceLayer.js. Thus we could know the timing to shut down the radio.
> 
> In addition, RadioInterfaceLayer doesn't received the topic
> "xpcom-will-shutdown" or "xpcom-shutdown" when phone power off. So I think
> "quit-application" is the appropriate one.
> 

I might be wrong, but per my best understanding, 'quit-application' will be notified in any case that an application has being shutdown, and that doesn't necessarily imply to device powering off. So I am not sure whether 'quit-application' indicates the right hint. But yes, I definitely support that it's not necessary to expose an API for 'setting Radio in powering off.' It would be great to try to handle that in gecko automatically. :)

> Then, gecko could handle this automatically. No need for gaia to explicitly
> call the API when power off.
> 
> So, now the question is that do we have to expose an API for gaia to set the
> radio power. This could be used for switching the airplane mode.
> 
> Following is the current design for switching aireplane mde:
> (1) gaia change setting => gecko observe the setting changed => gecko set
> radio power
> (2) gecko found radio power change => gecko update the setting
> 
> AFAIK, we should loose the coupling between gecko and setting page. It's
> better to not directly alter the setting value from gecko. So we could
> provide the new API for set radio. 
> 
> Need HsinYi's further comment for this part.

Yes, I talked to settings app owner before and we thought settings API is overused or abused (see Bug 861794). Radio setting is an example that settings value not only presents user preference but also actual network status. It would be great to correct the ambiguous behaviour.

Comment 9

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

I try to send out the RIL request setRadioPower off in gecko when received `system-power-off`.
However, because it is during the phone power off, we could not ensure that rild could received this request in time and completed it.

... and I guess the result is not. Telling from log, I could see the outgoing parcel in main log, but nothing related in radio log. Therefore, it seems that rild has not yet received the request. It is also possible that log service is stopped first. Request is completed but not logged.

Comment 10

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

Got another idea.

How about oberve the topic "profile-change-net-teardown" => The network connection is going offline at this point.
(Ref: https://developer.mozilla.org/en-US/docs/Observer_Notifications)

See dom/power/nsIPowerManagerService.idl and PowerManagerService.cpp
The topic is notified when power off/reboot machine or restart gecko process.

From testing results, if we sent the radio off request at that time, the request could be completed by rild. However, it's still a timing issue. Just works on current environment.

Comment 11

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

We are now using 'ril.radio.disabled' not only to control radio power but also to reflect the actual radio state as well. The implementation leads to some problems 1) the value represents two meanings, user expectation & actual state 2) several components such as RIL，Wifi and BT are bound to this only one value, what if some operation fails? How would that value represent the situation? 

In the multi-sim scenario, the need for correcting this issue becomes more obvious, since there would be more components controlled by this single entry.

After discussing with gaia developers again, we think it would be great to have separate APIs to control each device/component. It would be also great to use separate APIs to get the state of a component, while the settings value could still indicate user preference referred by gaia.

Comment 12

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

Attachment: Part 1: idl

Comment 13

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

Attachment: Part 2: dom

Comment 14

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

Attachment: Part 4: ril

Comment 15

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

Attachment: Part 6: modify related tests

Comment 16

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

Comment on attachment 819562 [details] [diff] [review]
Part 1: idl

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +349,5 @@
> +   *
> +   * Otherwise, the request's onerror will be called, and the request's error
> +   * will be either 'RadioNotAvailable', or 'GenericFailure'.
> +   */
> +  nsIDOMDOMRequest setRadioPower(in boolean on);

Per our offline discussion, we still need an additional attribute 'radioState' and one more event fired when radioState changes.

Comment 17

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> Comment on attachment 819562 [details] [diff] [review]
> Part 1: idl
> 
> Review of attachment 819562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> @@ +349,5 @@
> > +   *
> > +   * Otherwise, the request's onerror will be called, and the request's error
> > +   * will be either 'RadioNotAvailable', or 'GenericFailure'.
> > +   */
> > +  nsIDOMDOMRequest setRadioPower(in boolean on);
> 
> Per our offline discussion, we still need an additional attribute
> 'radioState' and one more event fired when radioState changes.

Where is a better place for 'radioState'? nsIDOMMozMobileConnection?
If we add the attribute, we could just fire an empty radiostatechange event. Or it is better to carry the new state value in the event?

Comment 18

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

(In reply to Szu-Yu Chen [:aknow] from comment #17)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> > Comment on attachment 819562 [details] [diff] [review]
> > Part 1: idl
> > 
> > Review of attachment 819562 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> > @@ +349,5 @@
> > > +   *
> > > +   * Otherwise, the request's onerror will be called, and the request's error
> > > +   * will be either 'RadioNotAvailable', or 'GenericFailure'.
> > > +   */
> > > +  nsIDOMDOMRequest setRadioPower(in boolean on);
> > 
> > Per our offline discussion, we still need an additional attribute
> > 'radioState' and one more event fired when radioState changes.
> 
> Where is a better place for 'radioState'? nsIDOMMozMobileConnection?

Yup, nsIDOMMozMobileConnection. Gaia developers have asked about the information. Currently radioState is important to them.
 
I think we probably need to have these states, RADIO_STATE_UNKNOWN, STATE_CONNECTING, STATE_CONNECTED, STATE_DISCONNECTING, STATE_DISCONNECTED, exposing to gaia.

> If we add the attribute, we could just fire an empty radiostatechange event.

Sounds good enough to me.

> Or it is better to carry the new state value in the event?

Comment 19

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)
> (In reply to Szu-Yu Chen [:aknow] from comment #17)
> > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> > > Comment on attachment 819562 [details] [diff] [review]
> > > Part 1: idl
> > > 
> > > Review of attachment 819562 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> > > @@ +349,5 @@
> > > > +   *
> > > > +   * Otherwise, the request's onerror will be called, and the request's error
> > > > +   * will be either 'RadioNotAvailable', or 'GenericFailure'.
> > > > +   */
> > > > +  nsIDOMDOMRequest setRadioPower(in boolean on);
> > > 
> > > Per our offline discussion, we still need an additional attribute
> > > 'radioState' and one more event fired when radioState changes.
> > 
> > Where is a better place for 'radioState'? nsIDOMMozMobileConnection?
> 
> Yup, nsIDOMMozMobileConnection. Gaia developers have asked about the
> information. Currently radioState is important to them.
>  
> I think we probably need to have these states, RADIO_STATE_UNKNOWN,
> STATE_CONNECTING, STATE_CONNECTED, STATE_DISCONNECTING, STATE_DISCONNECTED,
> exposing to gaia.
> 
> > If we add the attribute, we could just fire an empty radiostatechange event.
> 
> Sounds good enough to me.
> 
> > Or it is better to carry the new state value in the event?

I have a question about the naming?
In my patch, I use setRadioPower(bool on) for the newly exposed api. For example, when setRadioPower(true), we are expecting the radioState becomes CONNECTING and then CONNECTED. But I don't think it is intuitive enough and has strong connection between setRadioPower and radioState (radio on == CONNECTED? radio off == DISCONNECTED?). Maybe we should use another API name, or property/state name.

Comment 20

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

(In reply to Szu-Yu Chen [:aknow] from comment #19)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)
> > (In reply to Szu-Yu Chen [:aknow] from comment #17)
> > > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> > > > Comment on attachment 819562 [details] [diff] [review]
> > > > Part 1: idl
> > > > 
> > > > Review of attachment 819562 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> > > > @@ +349,5 @@
> > > > > +   *
> > > > > +   * Otherwise, the request's onerror will be called, and the request's error
> > > > > +   * will be either 'RadioNotAvailable', or 'GenericFailure'.
> > > > > +   */
> > > > > +  nsIDOMDOMRequest setRadioPower(in boolean on);
> > > > 
> > > > Per our offline discussion, we still need an additional attribute
> > > > 'radioState' and one more event fired when radioState changes.
> > > 
> > > Where is a better place for 'radioState'? nsIDOMMozMobileConnection?
> > 
> > Yup, nsIDOMMozMobileConnection. Gaia developers have asked about the
> > information. Currently radioState is important to them.
> >  
> > I think we probably need to have these states, RADIO_STATE_UNKNOWN,
> > STATE_CONNECTING, STATE_CONNECTED, STATE_DISCONNECTING, STATE_DISCONNECTED,
> > exposing to gaia.
> > 
> > > If we add the attribute, we could just fire an empty radiostatechange event.
> > 
> > Sounds good enough to me.
> > 
> > > Or it is better to carry the new state value in the event?
> 
> I have a question about the naming?
> In my patch, I use setRadioPower(bool on) for the newly exposed api. For
> example, when setRadioPower(true), we are expecting the radioState becomes
> CONNECTING and then CONNECTED. But I don't think it is intuitive enough and
> has strong connection between setRadioPower and radioState (radio on ==
> CONNECTED? radio off == DISCONNECTED?). Maybe we should use another API
> name, or property/state name.

I am also thinking about the naming... ... Thank you for raising this :)

Per our offline discussion, we came to agreement that 

nsIDOMDOMRequest setRadio(in enabled);
RADIO_STATE_UNKNOWN, STATE_ENABLING, STATE_ENABLED, STATE_DISABLING, STATE_DISABLED.

Comment 21

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #20)
> (In reply to Szu-Yu Chen [:aknow] from comment #19)
> > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #18)
> > > (In reply to Szu-Yu Chen [:aknow] from comment #17)
> > > > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #16)
> > > > > Comment on attachment 819562 [details] [diff] [review]
> > > > > Part 1: idl
> > > > > 
> > > > > Review of attachment 819562 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> > > > > @@ +349,5 @@
> > > > > > +   *
> > > > > > +   * Otherwise, the request's onerror will be called, and the request's error
> > > > > > +   * will be either 'RadioNotAvailable', or 'GenericFailure'.
> > > > > > +   */
> > > > > > +  nsIDOMDOMRequest setRadioPower(in boolean on);
> > > > > 
> > > > > Per our offline discussion, we still need an additional attribute
> > > > > 'radioState' and one more event fired when radioState changes.
> > > > 
> > > > Where is a better place for 'radioState'? nsIDOMMozMobileConnection?
> > > 
> > > Yup, nsIDOMMozMobileConnection. Gaia developers have asked about the
> > > information. Currently radioState is important to them.
> > >  
> > > I think we probably need to have these states, RADIO_STATE_UNKNOWN,
> > > STATE_CONNECTING, STATE_CONNECTED, STATE_DISCONNECTING, STATE_DISCONNECTED,
> > > exposing to gaia.
> > > 
> > > > If we add the attribute, we could just fire an empty radiostatechange event.
> > > 
> > > Sounds good enough to me.
> > > 
> > > > Or it is better to carry the new state value in the event?
> > 
> > I have a question about the naming?
> > In my patch, I use setRadioPower(bool on) for the newly exposed api. For
> > example, when setRadioPower(true), we are expecting the radioState becomes
> > CONNECTING and then CONNECTED. But I don't think it is intuitive enough and
> > has strong connection between setRadioPower and radioState (radio on ==
> > CONNECTED? radio off == DISCONNECTED?). Maybe we should use another API
> > name, or property/state name.
> 
> I am also thinking about the naming... ... Thank you for raising this :)
> 
> Per our offline discussion, we came to agreement that 
> 
> nsIDOMDOMRequest setRadio(in enabled);
> RADIO_STATE_UNKNOWN, STATE_ENABLING, STATE_ENABLED, STATE_DISABLING,
> STATE_DISABLED.

readonly attribute DOMString radioState;
possible values: null (unknown), 'enabling', 'enabled', 'disabling', 'disabled'
-- using null to align with other api

Comment 22

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

Attachment: Part 1#2: idl

Comment 23

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

Attachment: Part 2#2: dom

Comment 24

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

Attachment: Part 3: bluetooth

Comment 25

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

Attachment: Part 5: add setRadio test

Comment 26

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

Attachment: Part 6#2: modify related tests

Comment 27

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

Attachment: (WIP) Part 4#2: ril

It is workable but need further revision.

Comment 28

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

Comment on attachment 821497 [details] [diff] [review]
(WIP) Part 4#2: ril

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +732,5 @@
>    };
>  
>    this.rilContext = {
>      radioState:     RIL.GECKO_RADIOSTATE_UNAVAILABLE,
> +    detailedRadioState: null,

We could get the detail simply from radioState. I wonder if we really need to keep detailedRadioState in rilContext.

@@ +1429,5 @@
>  
> +  _convertRadioState: function _converRadioState(state) {
> +    switch (state) {
> +      case RIL.GECKO_RADIOSTATE_OFF:
> +        return 'disabled';

Let's define it as const RADIO_STATE_DISABLED in nsIMobileConnectionProvider.

@@ +2328,5 @@
>  
>    handleError: function handleError(aErrorMessage) {
>      if (DEBUG) this.debug("There was an error while reading RIL settings.");
>  
> +    // TODO(Aknow): do we have to turn on radio here?

As we are not listening to |ril.radio.disabled|, I don't see a reason to turn on radio here.

@@ +2437,5 @@
> +      });
> +      return;
> +    }
> +
> +    let responseHandler = (function(response) {

How about simply name the handler 'callback'?

@@ +2450,5 @@
> +    this._setRadioRequest = (function() {
> +      this.setRadioInternal(message, responseHandler);
> +    }).bind(this);
> +
> +    if (message.on) {

Seem to be |message.enabled|?

@@ +2460,5 @@
> +
> +  setRadioInternal: function setRadioInternal(message, callback) {
> +    this._changingRadio = true;
> +
> +    let state = message.enabled ? 'enabling' : 'disabling';

Use constant for these states.

@@ +3381,5 @@
>        return;
>      }
>  
>      if (this.state == datacall.state) {
> +      if (datacall.state != RIL.GECKO_NETWORK_STATE_CONNECTED) {

Good catch.

::: dom/system/gonk/ril_worker.js
@@ +5126,5 @@
>  RIL[REQUEST_RADIO_POWER] = function REQUEST_RADIO_POWER(length, options) {
> +  if (options.rilMessageType == null) {
> +    // The request was made by ril_worker itself.
> +    if (options.rilRequestError) {
> +      if (this.cachedDialRequest && options.on) {

s/options.on/options.enabled ?!

Comment 29

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

Comment on attachment 821492 [details] [diff] [review]
Part 2#2: dom

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

Looks good!

Comment 30

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

Comment on attachment 821491 [details] [diff] [review]
Part 1#2: idl

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

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +38,1 @@
>  interface nsIMobileConnectionProvider : nsISupports

Let's define the constants for radio states.

Comment 31

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

Comment on attachment 821495 [details] [diff] [review]
Part 6#2: modify related tests

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

::: dom/telephony/test/marionette/test_outgoing_emergency_in_airplane_mode.js
@@ +21,3 @@
>      telephony = ifr.contentWindow.navigator.mozTelephony;
> +    connection = ifr.contentWindow.navigator.mozMobileConnection;
> +    icc = ifr.contentWindow.navigator.mozIccManager;

I don't think we need icc.

@@ +69,5 @@
> +    icc.addEventListener("cardstatechange", function handler() {
> +      // Wait until card state changes to "ready" after turning on radio.
> +      // Wait until card state changes to "not-ready" after turning off radio.
> +      if ((enabled && icc.cardState == "ready") ||
> +          (!enabled && icc.cardState != "ready")) {

We should check mobileConnection.radioState to see if it's time to turn on/off radio. We shouldn't depend on icc.cardState.

::: dom/telephony/test/marionette/test_outgoing_radio_off.js
@@ +37,4 @@
>      icc.addEventListener("cardstatechange", function handler() {
>        // Wait until card state changes to "ready" after turning on radio.
>        // Wait until card state changes to "not-ready" after turning off radio.
>        if ((enabled && icc.cardState == "ready") ||

We should check mobileConnection.radioState to see if it's time to turn on/off radio. We shouldn't depend on icc.cardState.

Comment 32

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #28)
> Comment on attachment 821497 [details] [diff] [review]
> (WIP) Part 4#2: ril
> 
> Review of attachment 821497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +732,5 @@
> >    };
> >  
> >    this.rilContext = {
> >      radioState:     RIL.GECKO_RADIOSTATE_UNAVAILABLE,
> > +    detailedRadioState: null,
> 
> We could get the detail simply from radioState. I wonder if we really need
> to keep detailedRadioState in rilContext.

Per offline discussion with Aknow, we need to have a place for maintaining transient radio state and that place might be good to be in chrome in consideration of timing issues b/w chrome and content. I think it's fine to keep additional 'detailedRadioState' here though ideally would be in MobileConnectionProvider.js after bug 843452.

Comment 33

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

Attachment: Part 6#3: modify related tests

Comment 34

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

Attachment: Part 1#3: idl

(w/ rebase)
We could not add string constant in interface, so the state constants are added to ril_consts.js.

Comment 35

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

Attachment: Part 2#3: dom

Hi Kyle,

Please help review the dom change. They are about:
 1. add readonly attribute 'radioState'
 2. add setRadio() api
 3. add 'radiostatechange' event

Thank you.

Comment 36

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

Attachment: Part 3#2: bluetooth

Hi Gina,

We have added a new function 'NotifyRadioStateChanged' into MobileConnectionListener interface. The patch is for the related modification in bluetooth.

Comment 37

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

Attachment: Part 4#3: ril

Comment 38

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

Attachment: Part 5#2: add setRadio test

Comment 39

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

Attachment: Part 6#4: modify related tests

Comment 40

[Michael Schwartz [:m4]]

Comment on attachment 823827 [details] [diff] [review]
Part 4#3: ril

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

::: dom/system/gonk/RILContentHelper.js
@@ +102,5 @@
>    "RIL:GetRoamingPreference",
>    "RIL:ExitEmergencyCbMode",
> +  "RIL:SetRadio",
> +  "RIL:RadioStateChanged",
> +  "RIL:SetVoicePrivacnyMode",

s/Privacny/Privacy/

@@ +1321,5 @@
>  
>      return request;
>    },
>  
> +  setRadio: function setRadio(window, enabled) {

Isn't this name too vague?  Sounds like you're trying to apply a radio object.  SetRadioEnabled?

Comment 41

[Michael Schwartz [:m4]]

Comment on attachment 823820 [details] [diff] [review]
Part 1#3: idl

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +78,5 @@
>  
>    /**
> +   * The current radio state.
> +   *
> +   * Possible values: null (unknown), 'enabling', 'enabled', 'disabling',

Out of curiosity, isn't it better to use named constants rather than magic strings?  If the API were to change, it would be nice to get an error where I'm using the constant.  With magic strings, changes to the API result in unexpected behaviour.

Comment 42

[Michael Schwartz [:m4]]

How does this address the power off issue?

Comment 43

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

(In reply to Michael Schwartz [:m4] from comment #40)
> Comment on attachment 823827 [details] [diff] [review]
> Part 4#3: ril
> 
> Review of attachment 823827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +102,5 @@
> >    "RIL:GetRoamingPreference",
> >    "RIL:ExitEmergencyCbMode",
> > +  "RIL:SetRadio",
> > +  "RIL:RadioStateChanged",
> > +  "RIL:SetVoicePrivacnyMode",
> 
> s/Privacny/Privacy/

Thank you for the catch. My mistake.

> @@ +1321,5 @@
> >  
> >      return request;
> >    },
> >  
> > +  setRadio: function setRadio(window, enabled) {
> 
> Isn't this name too vague?  Sounds like you're trying to apply a radio
> object.  SetRadioEnabled?

I am OK to change the name. So the API name on interface will also be modified.
Usage: MozMobileConnection.setRadioEnabled(true)

Comment 44

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

(In reply to Michael Schwartz [:m4] from comment #41)
> Comment on attachment 823820 [details] [diff] [review]
> Part 1#3: idl
> 
> Review of attachment 823820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> @@ +78,5 @@
> >  
> >    /**
> > +   * The current radio state.
> > +   *
> > +   * Possible values: null (unknown), 'enabling', 'enabled', 'disabling',
> 
> Out of curiosity, isn't it better to use named constants rather than magic
> strings?  If the API were to change, it would be nice to get an error where
> I'm using the constant.  With magic strings, changes to the API result in
> unexpected behaviour.

Yes. I know that constant is better. However it looks like we could not use string constant in idl. If it is possible, I am willing to change it.

Usually, we use string on interface. All the type of other attributes in MobileConnection are also DOMString. So I think keep the same style and convention is better for gaia side.

Comment 45

[Edgar Chen [:edgar]]

Hi Aknow:

Some test cases in icc will use "ril.radio.disabled", like

- http://dxr.mozilla.org/mozilla-central/source/dom/icc/tests/marionette/test_icc_card_state.js#l29
- http://dxr.mozilla.org/mozilla-central/source/dom/icc/tests/marionette/test_icc_info.js#l61

Could you help to address them as well? 

Thank you.

Comment 46

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

(In reply to Michael Schwartz [:m4] from comment #42)
> How does this address the power off issue?

Unfortunately, the patch does not address the phone power off issue. The main reason for changing setting-based operation to API-based one is that we need a way to configure the radio of a specified sim in multi-sim architecture.

Comment 47

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

(In reply to Szu-Yu Chen [:aknow] from comment #44)
> (In reply to Michael Schwartz [:m4] from comment #41)
> > Comment on attachment 823820 [details] [diff] [review]
> > Part 1#3: idl
> > 
> > Review of attachment 823820 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> > @@ +78,5 @@
> > >  
> > >    /**
> > > +   * The current radio state.
> > > +   *
> > > +   * Possible values: null (unknown), 'enabling', 'enabled', 'disabling',
> > 
> > Out of curiosity, isn't it better to use named constants rather than magic
> > strings?  If the API were to change, it would be nice to get an error where
> > I'm using the constant.  With magic strings, changes to the API result in
> > unexpected behaviour.
> 
> Yes. I know that constant is better. However it looks like we could not use
> string constant in idl. If it is possible, I am willing to change it.
> 
> Usually, we use string on interface. All the type of other attributes in
> MobileConnection are also DOMString. So I think keep the same style and
> convention is better for gaia side.

In terms of gaia convention, I agree with Aknow's comment. And I also learned that having 'strings' is a more webby way.

Comment 48

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

Comment on attachment 823820 [details] [diff] [review]
Part 1#3: idl

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

r=me with comments addressed.

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +362,5 @@
> +   * will be either 'InvalidStateError', 'RadioNotAvailable', or
> +   * 'GenericFailure'.
> +   *
> +   * Note: Request is not available when radioState is null, 'enabling', or
> +   *       'disabling'. Calling the api in above conditions will receive

s/api/function

@@ +365,5 @@
> +   * Note: Request is not available when radioState is null, 'enabling', or
> +   *       'disabling'. Calling the api in above conditions will receive
> +   *       'InvalidStateError' error.
> +   */
> +  nsIDOMDOMRequest setRadio(in boolean enabled);

Per comment 43, rename it setRadioEnabled.

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +26,5 @@
>                             in unsigned short serviceClass);
>    void notifyEmergencyCbModeChanged(in boolean active,
>                                      in unsigned long timeoutMs);
>    void notifyOtaStatusChanged(in DOMString status);
> +  void notifyRadioStateChanged();

uuid change for nsIMobileConnectionListener

@@ +86,5 @@
>    nsIDOMDOMRequest getCallingLineIdRestriction(in nsIDOMWindow   window);
>  
>    nsIDOMDOMRequest exitEmergencyCbMode(in nsIDOMWindow window);
> +
> +  nsIDOMDOMRequest setRadio(in nsIDOMWindow window, in bool enabled);

Rename!

Comment 49

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

Comment on attachment 823827 [details] [diff] [review]
Part 4#3: ril

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

::: dom/system/gonk/RILContentHelper.js
@@ +100,5 @@
>    "RIL:UpdateIccContact",
>    "RIL:SetRoamingPreference",
>    "RIL:GetRoamingPreference",
>    "RIL:ExitEmergencyCbMode",
> +  "RIL:SetRadio",

Remember to rename the string over the code.

@@ +102,5 @@
>    "RIL:GetRoamingPreference",
>    "RIL:ExitEmergencyCbMode",
> +  "RIL:SetRadio",
> +  "RIL:RadioStateChanged",
> +  "RIL:SetVoicePrivacnyMode",

Thanks for m4's catch :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1467,3 @@
>        return;
>      }
> +    this.detailedRadioState = state;

Should be this.rilContext.detailedRadioState.

@@ +2146,5 @@
>        case kSysMsgListenerReadyObserverTopic:
>          Services.obs.removeObserver(this, kSysMsgListenerReadyObserverTopic);
> +        // TODO(Aknow): The following code will not be included in the final
> +        // version. It is just a workaround to turn on the radio when power on
> +        // before we have done the proper modification on gaia side.

I understood this is for debug purpose. Be sure to remove it before landing.

@@ +2208,5 @@
>    _radioEnabled: null,
>  
>    // Flag to ignore any radio power change requests during We're changing
>    // the radio power.
> +  _changingRadio: false,

As we have |detailedRadioState| to memorize the transient state, which reflects radio being changing or not, we don't need to have an additional flag tracking that. In this way, we are freed from taking care of the synchronization b/w this flag and the transient state.

Having a helper function isRadioChanging() {return detailedRadioState == 'enabling' || detailedRadioState == 'disabling';} looks better to me.

@@ +2213,1 @@
>  

_setRadioRequest: null,

@@ +2418,5 @@
>        return false;
>      }).bind(this));
>    },
>  
> +  setRadio: function setRadio(target, message) {

s/setRadio/setRaioEnabled

@@ +2420,5 @@
>    },
>  
> +  setRadio: function setRadio(target, message) {
> +    if (DEBUG) {
> +      this.debug("setRadio: " + JSON.stringify(message));

ditto.

@@ +3402,5 @@
>        return;
>      }
>  
>      if (this.state == datacall.state) {
> +      if (datacall.state != RIL.GECKO_NETWORK_STATE_CONNECTED) {

Good catch.

::: dom/system/gonk/ril_worker.js
@@ +941,2 @@
>     */
> +  setRadio: function setRadio(options) {

s/setRadio/setRadioEnabled

Comment 50

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

Comment on attachment 823828 [details] [diff] [review]
Part 5#2: add setRadio test

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

::: dom/network/tests/marionette/manifest.ini
@@ +17,5 @@
>  [test_mobile_roaming_preference.js]
>  [test_call_barring_get_option.js]
>  [test_call_barring_set_error.js]
>  [test_call_barring_change_password.js]
> +[test_mobile_data_set_radio.js]

You name the file 'test_mobile_set_radio.js' which looks good to me. Be sure to make the manifest.ini consistent.

::: dom/network/tests/marionette/test_mobile_set_radio.js
@@ +66,5 @@
> +    receivedPending('onsuccess', pending, done);
> +  };
> +
> +  req.onerror = function() {
> +    ok(false, 'setRadio should not fail');

Shouldn't we have 'deferred.reject()' here?

Comment 51

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

Comment on attachment 823829 [details] [diff] [review]
Part 6#4: modify related tests

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

Thank to Edgar's reminding. 

The followings are going to be affected. Please help address them.
- http://dxr.mozilla.org/mozilla-central/source/dom/icc/tests/marionette/test_icc_card_state.js#l29
- http://dxr.mozilla.org/mozilla-central/source/dom/icc/tests/marionette/test_icc_info.js#l61

::: dom/telephony/test/marionette/test_outgoing_emergency_in_airplane_mode.js
@@ +17,5 @@
>  
> +  let desiredRadioState = enabled ? 'enabled' : 'disabled';
> +  request.onsuccess = function onsuccess() {
> +    log('setRadio to ' + desiredRadioState);
> +    is(connection.radioState, desiredRadioState);

request.onsuccess is fired before connection.onradiostatechange. I could imagine the statement is likely to fail.

::: dom/telephony/test/marionette/test_outgoing_radio_off.js
@@ +16,5 @@
>  
> +  let desiredRadioState = enabled ? 'enabled' : 'disabled';
> +  request.onsuccess = function onsuccess() {
> +    log('setRadio to ' + desiredRadioState);
> +    is(connection.radioState, desiredRadioState);

ditto.

Comment 52

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

Attachment: Part 1#4: Add setRadioEnabled API (idl). r=hsinyi

Comment 53

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

Attachment: Part 2#4: dom

Rename SetRadio => SetRadioEnabled.

Comment 54

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

Attachment: Part 4#4: ril

Comment 55

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

Attachment: Part 5#3: add setRadio test

Comment 56

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

Attachment: Part 6#5: modify related tests

Icc tests added.

Comment 57

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

Comment on attachment 824539 [details] [diff] [review]
Part 4#4: ril

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

Awesome, thank you! I guess you need to rebase due to bug 818353.

Comment 58

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

Comment on attachment 824540 [details] [diff] [review]
Part 5#3: add setRadio test

\o/

Comment 59

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

Comment on attachment 824542 [details] [diff] [review]
Part 6#5: modify related tests

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

Thank you.

Comment 60

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #57)
> Comment on attachment 824539 [details] [diff] [review]
> Part 4#4: ril
> 
> Review of attachment 824539 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome, thank you! I guess you need to rebase due to bug 818353.

... yes, but strange.
As I remember, when I write the first version of this patch, there are 'clientId' in mobile connection interface. However, recently, I found the 'clientId' is removed. So I have rebased to a version w/o 'clientId'. Now, 'clientId' is back again!!!

Comment 61

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 824538 [details] [diff] [review]
Part 2#4: dom

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

::: dom/network/src/MobileConnection.cpp
@@ +204,5 @@
> +  radioState.SetIsVoid(true);
> +
> +  if (!mProvider || !CheckPermission("mobileconnection")) {
> +     return NS_OK;
> +  }

Why is this check written in a different order than all the other functions in this class?  And why does it return NS_OK for !mProvider instead of NS_ERROR_FAILURE.

If this is intentional, it deserves a comment.

Comment 62

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #61)
> Comment on attachment 824538 [details] [diff] [review]
> Part 2#4: dom
> 
> Review of attachment 824538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/MobileConnection.cpp
> @@ +204,5 @@
> > +  radioState.SetIsVoid(true);
> > +
> > +  if (!mProvider || !CheckPermission("mobileconnection")) {
> > +     return NS_OK;
> > +  }
> 
> Why is this check written in a different order than all the other functions
> in this class?  And why does it return NS_OK for !mProvider instead of
> NS_ERROR_FAILURE.
> 
> If this is intentional, it deserves a comment.

Hi Kyle,

The function has the similar form as GetVoice, GetData, GetNetworkSelectionMode. All of them check the provider first and then permission. They also return NS_OK in this condition.

Looks like we always return NS_OK for readonly attribute in mobileconnection interface. Therefore, it is available for the users to access the property. But the value they get is null.

Comment 63

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 824538 [details] [diff] [review]
Part 2#4: dom

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

Ok.  I guess I should have looked at a few more functions.

r=me

Comment 64

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 823826 [details] [diff] [review]
Part 3#2: bluetooth

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

Thanks, aknow.

Comment 65

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

Comment on attachment 824537 [details] [diff] [review]
Part 1#4: Add setRadioEnabled API (idl). r=hsinyi

Hi Kyle,

The patch is r+ by hsinyi, but we need two reviewers for the interface change.
Would you help on this? Thank you.

Comment 66

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

Now reverse the dependency. We should land this patch after Bug 928851

Comment 67

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #47)
> (In reply to Szu-Yu Chen [:aknow] from comment #44)
> > (In reply to Michael Schwartz [:m4] from comment #41)
> > > Comment on attachment 823820 [details] [diff] [review]
> > > Part 1#3: idl
> > > 
> > > Review of attachment 823820 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> > > @@ +78,5 @@
> > > >  
> > > >    /**
> > > > +   * The current radio state.
> > > > +   *
> > > > +   * Possible values: null (unknown), 'enabling', 'enabled', 'disabling',
> > > 
> > > Out of curiosity, isn't it better to use named constants rather than magic
> > > strings?  If the API were to change, it would be nice to get an error where
> > > I'm using the constant.  With magic strings, changes to the API result in
> > > unexpected behaviour.
> > 
> > Yes. I know that constant is better. However it looks like we could not use
> > string constant in idl. If it is possible, I am willing to change it.
> > 
> > Usually, we use string on interface. All the type of other attributes in
> > MobileConnection are also DOMString. So I think keep the same style and
> > convention is better for gaia side.
> 
> In terms of gaia convention, I agree with Aknow's comment. And I also
> learned that having 'strings' is a more webby way.

I agree that consistency is good but it's important to make decisions that are more usable/maintainable.  Strings are indeed "webby" so we could continue to use them but with the benefit of identifying which strings using constants.  webidl supports constant strings but XPIDL appears not to at this time.  Not sure when/where in Moz we can use webidl and I'm willing to drop this rathole at this time ;)

Comment 68

[Michael Schwartz [:m4]]

(In reply to Szu-Yu Chen [:aknow] from comment #46)
> (In reply to Michael Schwartz [:m4] from comment #42)
> > How does this address the power off issue?
> 
> Unfortunately, the patch does not address the phone power off issue. The
> main reason for changing setting-based operation to API-based one is that we
> need a way to configure the radio of a specified sim in multi-sim
> architecture.

Hi Szu-Yu, I don't think that what you're talking about is possible.  Airplane mode is either on or off - that's it.  Subscriptions are activated/deactivated but that's completely different and shouldn't affect this at all.

Comment 69

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

(In reply to Michael Schwartz [:m4] from comment #67)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #47)
> > (In reply to Szu-Yu Chen [:aknow] from comment #44)
> > > (In reply to Michael Schwartz [:m4] from comment #41)
> > > > Comment on attachment 823820 [details] [diff] [review]
> > > > Part 1#3: idl
> > > > 
> > > > Review of attachment 823820 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > ::: dom/network/interfaces/nsIDOMMobileConnection.idl
> > > > @@ +78,5 @@
> > > > >  
> > > > >    /**
> > > > > +   * The current radio state.
> > > > > +   *
> > > > > +   * Possible values: null (unknown), 'enabling', 'enabled', 'disabling',
> > > > 
> > > > Out of curiosity, isn't it better to use named constants rather than magic
> > > > strings?  If the API were to change, it would be nice to get an error where
> > > > I'm using the constant.  With magic strings, changes to the API result in
> > > > unexpected behaviour.
> > > 
> > > Yes. I know that constant is better. However it looks like we could not use
> > > string constant in idl. If it is possible, I am willing to change it.
> > > 
> > > Usually, we use string on interface. All the type of other attributes in
> > > MobileConnection are also DOMString. So I think keep the same style and
> > > convention is better for gaia side.
> > 
> > In terms of gaia convention, I agree with Aknow's comment. And I also
> > learned that having 'strings' is a more webby way.
> 
> I agree that consistency is good but it's important to make decisions that
> are more usable/maintainable.  Strings are indeed "webby" so we could
> continue to use them but with the benefit of identifying which strings using
> constants.  webidl supports constant strings but XPIDL appears not to at
> this time.  Not sure when/where in Moz we can use webidl and I'm willing to
> drop this rathole at this time ;)

Here => Bug 898445
Once we have migrated to webidl, we could change them to constant strings.

Comment 70

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

(In reply to Michael Schwartz [:m4] from comment #68)
> (In reply to Szu-Yu Chen [:aknow] from comment #46)
> > (In reply to Michael Schwartz [:m4] from comment #42)
> > > How does this address the power off issue?
> > 
> > Unfortunately, the patch does not address the phone power off issue. The
> > main reason for changing setting-based operation to API-based one is that we
> > need a way to configure the radio of a specified sim in multi-sim
> > architecture.
> 
> Hi Szu-Yu, I don't think that what you're talking about is possible. 
> Airplane mode is either on or off - that's it.  Subscriptions are
> activated/deactivated but that's completely different and shouldn't affect
> this at all.

:m4,

Another reason for changing from settings to API: Bug 861794.
"""
We believe we're overusing the Settings API for some settings, because it's a privileged API that is not exposed to web content. In the long-term, we should use it as little as possible.
"""
This might not be the problem you are concerning about. It's just another way for the method.

So I guess what you are asking is setRadioEnabled(serviceId?). Do we have to specify a serviceId? There is only one radio. We just turn it on/off no matter how many subscriptions (ex: airplane mode). How about in DSDA (Dual SIM Dual Active)? Is it possible for user to turn off one of the RFs? 

Hsinyi, is there a user story for requesting setRadioEnabled on some of the subscriptions instead of all of them.

Comment 71

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

(In reply to Szu-Yu Chen [:aknow] from comment #70)
> (In reply to Michael Schwartz [:m4] from comment #68)
> > (In reply to Szu-Yu Chen [:aknow] from comment #46)
> > > (In reply to Michael Schwartz [:m4] from comment #42)
> > > > How does this address the power off issue?
> > > 
> > > Unfortunately, the patch does not address the phone power off issue. The
> > > main reason for changing setting-based operation to API-based one is that we
> > > need a way to configure the radio of a specified sim in multi-sim
> > > architecture.
> > 
> > Hi Szu-Yu, I don't think that what you're talking about is possible. 
> > Airplane mode is either on or off - that's it.  Subscriptions are
> > activated/deactivated but that's completely different and shouldn't affect
> > this at all.
> 
> :m4,
> 
> Another reason for changing from settings to API: Bug 861794.
> """
> We believe we're overusing the Settings API for some settings, because it's
> a privileged API that is not exposed to web content. In the long-term, we
> should use it as little as possible.
> """
> This might not be the problem you are concerning about. It's just another
> way for the method.
> 
> So I guess what you are asking is setRadioEnabled(serviceId?). Do we have to
> specify a serviceId? There is only one radio. We just turn it on/off no
> matter how many subscriptions (ex: airplane mode). How about in DSDA (Dual
> SIM Dual Active)? Is it possible for user to turn off one of the RFs? 
> 
> Hsinyi, is there a user story for requesting setRadioEnabled on some of the
> subscriptions instead of all of them.

Yes, there is. So, what we need to do is having the patch reabase on bug 818353 as I mentioned before (already in m-c). Adding 'clientId' in nsIMobileConnectionProvider::SetRadioEnabled() and ril code. I am sorry if I didn't express clear enough. 

The multisim WebAPI support is in bug 814629. Due to the multisim design for MobileConnection API, we won't need to add 'serviceId' in nsIDOMMozMobileConnection::SetRadioEnabled(). Once bug 814629 is landed, we will be fine with the user story.

Comment 72

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

(In reply to Michael Schwartz [:m4] from comment #68)
> (In reply to Szu-Yu Chen [:aknow] from comment #46)
> > (In reply to Michael Schwartz [:m4] from comment #42)
> > > How does this address the power off issue?
> > 
> > Unfortunately, the patch does not address the phone power off issue. The
> > main reason for changing setting-based operation to API-based one is that we
> > need a way to configure the radio of a specified sim in multi-sim
> > architecture.
> 
> Hi Szu-Yu, I don't think that what you're talking about is possible. 
> Airplane mode is either on or off - that's it.  Subscriptions are
> activated/deactivated but that's completely different and shouldn't affect
> this at all.

Hi :m4,

If they are different, how could we activate/deactivate a subscription? In single sim case, it's just like setting the airplane mode. How about in multi-sim architecture? Do we have another RIL request for that?

Comment 73

[Michael Schwartz [:m4]]

Hi Szu-Yu and Hsin-Yi, airplane mode and subscription are different.  Airplane mode basically just turns the modem on or off - it's the same in DSDS/DSDA or whatever - no client id or any of that business needed.  Subscription is a completely different animal that is outside the scope of this bug.

Comment 74

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

(In reply to Michael Schwartz [:m4] from comment #73)
> Hi Szu-Yu and Hsin-Yi, airplane mode and subscription are different. 
> Airplane mode basically just turns the modem on or off - it's the same in
> DSDS/DSDA or whatever - no client id or any of that business needed. 
> Subscription is a completely different animal that is outside the scope of
> this bug.

Hi :m4, I know it is outside the scope of this bug, but would you share something here. We thought setting radio power for a specified sim could be used to activate/deactivate the subscription in multi-sim case. Looks like the above assumption is not correct.

Airplane mode is set by REQUEST_RADIO_POWER parcel, which is then mapped to at-command "+CFUN". If activate/deactivate subscription is a totally different thing. What parcel should we use in this case? Or how could we achieve this feature?

Comment 75

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

(In reply to Szu-Yu Chen [:aknow] from comment #74)
> (In reply to Michael Schwartz [:m4] from comment #73)
> > Hi Szu-Yu and Hsin-Yi, airplane mode and subscription are different. 
> > Airplane mode basically just turns the modem on or off - it's the same in
> > DSDS/DSDA or whatever - no client id or any of that business needed. 
> > Subscription is a completely different animal that is outside the scope of
> > this bug.
> 
> Hi :m4, I know it is outside the scope of this bug, but would you share
> something here. We thought setting radio power for a specified sim could be
> used to activate/deactivate the subscription in multi-sim case. Looks like
> the above assumption is not correct.
> 
> Airplane mode is set by REQUEST_RADIO_POWER parcel, which is then mapped to
> at-command "+CFUN". If activate/deactivate subscription is a totally
> different thing. What parcel should we use in this case? Or how could we
> achieve this feature?

I echo Aknow's comment. Bug 931160, targeting on activating/deactivating a SIM, is required in v1.3. Looks we are missing a solution to this case. How could we activate/deactivate a subscription? Thanks!

Comment 76

[Michael Schwartz [:m4]]

Hi Szu-Yu and Hsin-Yi, I've checked with our modem guys and the origin impl you proposed is most appropriate ie. radio should be toggled per ril daemon.  That being said, some impls may, as I suggested, have only one modem under-the-hood so toggling one actually toggles both.  I'm still researching more about this and will report back with what I have.  Sorry for the confusion.

Comment 77

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

Some files are still using the key "ril.radio.disabled" for radio on/off detection. They should now change to use the new property "radioState". I will update the patch for this part.

mobilemessage/src/gonk/MmsService.js                                                  
43:const kPrefRilRadioDisabled              = "ril.radio.disabled";                   
193:  settings: ["ril.radio.disabled"],                                               
242:      if (DEBUG) debug("Getting preference 'ril.radio.disabled' fails.");         
433:            if (DEBUG) debug("Updating preference 'ril.radio.disabled' fails.");  
914:            if (DEBUG) debug("Failed to get preference of 'ril.radio.disabled'.");

Comment 78

[Michael Schwartz [:m4]]

Hi Szu-Yu and Hsin-Yi, so indeed toggling one radio may toggle both.  You can choose if you want to abstract that detail from Gaia all together by not exposing the option to toggle them independently.  In DSDS, a user would enable/disable subscriptions rather than toggling whether the modem is on/off - so a single switch seems like the better API unless you have a usecase that differs.  Note that Android uses a single switch.

Comment 79

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

(In reply to Michael Schwartz [:m4] from comment #78)
> Hi Szu-Yu and Hsin-Yi, so indeed toggling one radio may toggle both.  You
> can choose if you want to abstract that detail from Gaia all together by not
> exposing the option to toggle them independently.  In DSDS, a user would
> enable/disable subscriptions rather than toggling whether the modem is
> on/off - so a single switch seems like the better API unless you have a
> usecase that differs.  Note that Android uses a single switch.

Hi :m4,

Thanks for the investigation. It's truly nice to have your and your team's feedback. I now understand that radio off and enable/disable subscriptions are different things. That said, we could deactivate a sim card but let radio still on. And in some implementation such as one-modem case, we couldn't radio off only one ril deamon though we could simply disable one sim card/subscription. As radio-off and subscription-off are totally different implementation and use cases, IMO we will need another API for enabling/disabling subscriptions. Also, per my best knowledge, there's no standard android RIL request for enabling/disabling subscriptions, do you have any ideas about this? What should gecko do to communicate with modem in this case? Thanks again. :)

Comment 80

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #79)

Hi Hsin-Yi, yes we can toggle subscriptions as well as provide a list of available options.  You're right that we need a new API to select which subscription should be enabled.  I'm not as familiar with your overall DSDS API strategy.  Do you have a doc somewhere (or bug you'd recommend I read) so I can make more specific recommendations?

You're welcome.  We're all excited to be contributing!

Comment 81

[Michael Schwartz [:m4]]

I've found https://bugzilla.mozilla.org/show_bug.cgi?id=814629 which I think is already addressing the issue of subscription selection plus please let me know if that's not the case.

Comment 82

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

(In reply to Michael Schwartz [:m4] from comment #81)
> I've found https://bugzilla.mozilla.org/show_bug.cgi?id=814629 which I think
> is already addressing the issue of subscription selection plus please let me
> know if that's not the case.

In DSDS there are going to be several rild/services, and Bug 814629 is providing an API to access them. I guess that is what you said about subscription selection. 

Since each mobileconnection object is bound to a rild/service, user could use 'setRadioEnabled' to turn on/off the whole service that is the idea of this bug. However, this is different from activating/deactivating a SIM card so we filed another bug 936325.

Comment 83

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

Attachment: Part 5#4: add setRadio test

Add a test case to check whether the data is disconnected after set radio to disabled

Comment 84

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

Attachment: Part 5#5: add setRadio test

Part 5#4 is the wrong version.
Please diff with previous r+ version Part5#3

Comment 85

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

Attachment: Part 7: Deactivate data calls from all clientId before set radio to disabled

Based on part4.
When set radio to disabled, we should first deactivate data calls from all clients (radio interface).

Comment 86

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

Comment on attachment 829183 [details] [diff] [review]
Part 5#5: add setRadio test

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

It looks good. r=me with question answered. Thank you.

::: dom/network/tests/marionette/test_mobile_set_radio.js
@@ +134,5 @@
> +function testSwitchRadio() {
> +  log('= testSwitchRadio =');
> +  return waitRadioState('enabled')
> +    .then(setRadioEnabled.bind(null, false, 'disabling', 'disabled'))
> +    .then(waitRadioState.bind(null, 'disabled'))

setRadioEnabled() already ensures the radio state being 'disabled' (or 'enabled). Why do we need 'waitRadioState' here? Oh, looks I missed this in Part5#3.

@@ +147,5 @@
> +    .then(() => {
> +      // Data should be disconnected.
> +      is(connection.data.connected, false);
> +    })
> +    .then(waitRadioState.bind(null, 'disabled'))

ditto.

Comment 87

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #86)
> Comment on attachment 829183 [details] [diff] [review]
> Part 5#5: add setRadio test
> 
> Review of attachment 829183 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good. r=me with question answered. Thank you.
> 
> ::: dom/network/tests/marionette/test_mobile_set_radio.js
> @@ +134,5 @@
> > +function testSwitchRadio() {
> > +  log('= testSwitchRadio =');
> > +  return waitRadioState('enabled')
> > +    .then(setRadioEnabled.bind(null, false, 'disabling', 'disabled'))
> > +    .then(waitRadioState.bind(null, 'disabled'))
> 
> setRadioEnabled() already ensures the radio state being 'disabled' (or
> 'enabled). Why do we need 'waitRadioState' here? Oh, looks I missed this in
> Part5#3.
> 
> @@ +147,5 @@
> > +    .then(() => {
> > +      // Data should be disconnected.
> > +      is(connection.data.connected, false);
> > +    })
> > +    .then(waitRadioState.bind(null, 'disabled'))
> 
> ditto.

We don't need the 'waitRadioState' there. I will remove these two lines. Thank you

Comment 88

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

Attachment: [Final] Part 1: Add setRadioEnabled API (idl). r=hsinyi

Comment 89

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

Attachment: [Final] Part 2: Add setRadioEnabled API (dom). r=khuey

Comment 90

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

Attachment: [Final] Part 3: Add radiostatechange in listener (bluetooth). r=gyeh

Comment 91

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

Attachment: [Final] Part 4: Add setRadioEnabled API (ril). r=hsinyi

Comment 92

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

Attachment: [Final] Part 5: Add setRadioEnabled marionette test. r=hsinyi

Comment 93

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

Comment on attachment 830029 [details] [diff] [review]
[Final] Part 5: Add setRadioEnabled marionette test. r=hsinyi

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

::: dom/network/tests/marionette/test_mobile_set_radio.js
@@ +161,5 @@
> +testSwitchRadio()
> +  .then(testDisableRadioWhenDataConnected)
> +  .then(null, () => {
> +    ok(false, 'promise reject somewhere');
> +  })

Sorry for one more comment: please help provide a consistent coding style - using double quotation marks - in this file. Thank you.

Comment 94

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

Attachment: (x) [Final] Part 5#2: Add setRadioEnabled marionette test. r=hsinyi

Comment 95

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

Attachment: [Final] Part 5#2: Add setRadioEnabled marionette test. r=hsinyi

Comment 96

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

Comment on attachment 829189 [details] [diff] [review]
Part 7: Deactivate data calls from all clientId before set radio to disabled

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

I would like to see this patch again. I feel there's some work for us to enhance the readability.

General comment: Please use 'double quotation marks' in this file. Thanks!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +419,5 @@
>          return null;
>        }
>  
> +      if (msg.name === "RIL:SetRadioEnabled") {
> +        // Special handle for SetRadioEnabled.

s/handle/handler/

@@ +540,5 @@
>  
> +function SetRadioEnabledController(ril) {
> +  this.ril = ril;
> +}
> +SetRadioEnabledController.prototype = {

Use defineLazyGetter for 'gRadioEnabledController'.

For example,

XPCOMUtils.defineLazyGetter(this, "gRadioEnabledController", function () {
    ... ... 
    
    init: function init(ril) {
        this.ril = ril;
    },

    ... ...
}

@@ +557,5 @@
> +    }
> +  },
> +
> +  createTimer: function() {
> +    if (this.timer == null) {

if (!this.timer)

@@ +570,5 @@
> +      this.timer.cancel();
> +    }
> +  },
> +
> +  makeDeactivatingFunction: function(clientId) {

Could we simply name it |deactivateDataCalls|?

@@ +598,5 @@
> +        this.processNextMessage();
> +      }).bind(this);
> +
> +      // In some DSDS architecture with only one modem, toggling one radio may
> +      // toggle both. Therefore, for safely turning off, we should first

nit: s/toggle/toggles/

@@ +603,5 @@
> +      // explicitly deactive all data calls from all clients.
> +      if (DEBUG) debug('setRadioEnabled, deactivating...');
> +
> +      this.createTimer();
> +      this.deactivatingDeferred = {};

You've introduced |deactivatingDeferred| and |deactivatingPromise|... How about just use |this.deactivatingPromise|?

@@ +613,5 @@
> +
> +      promise.then(() => {
> +        if (DEBUG) debug('setRadioEnabled, deactivation done');
> +        this.executeRequest();
> +        this.deactivatingPromise = null;

I don't see |deactivatingPromise| becomes un-null. Looks we don't really need it, removing it please.

@@ +627,5 @@
> +    let msg = this.pendingMessages.shift();
> +    this.handleMessage(msg);
> +  },
> +
> +  addRequest: function(msg) {

'addRequest' is kinda confusing as we don't really add *request* to |this.request|. How about |receiveMessage|?

@@ +635,5 @@
> +      this.processNextMessage();
> +    }
> +  },
> +
> +  finishDeactivation: function(clientId) {

Could we simple name it 'resolveDeactivatingPromise'?

@@ +643,5 @@
> +      deferred.resolve();
> +    }
> +  },
> +
> +  isDeactivating: function() {

I prefer a more descriptive name though it means the name is longer... 
How about 'isDeactivatingDataCalls'?

@@ +652,3 @@
>  function RadioInterfaceLayer() {
>    gMessageManager.init(this);
> +  this.setRadioEnabledController = new SetRadioEnabledController(this);

gRadioEnabledController.init(this);

@@ +672,5 @@
>    debug(numIfaces + " interfaces");
>    this.radioInterfaces = [];
>    for (let clientId = 0; clientId < numIfaces; clientId++) {
>      options.clientId = clientId;
> +    this.radioInterfaces.push(new RadioInterface(this, options));

With gRadioEnabledController, then this isn't necessary.

@@ +2571,5 @@
>      }).bind(this));
>    },
>  
> +  // Return true if successfuly responsed.
> +  setRadioEnabledTryResponse: function setRadioEnabledTryResponse(target, message) {

I feel we do need to come out a better name, but I haven't got a concrete thought yet...

@@ +2576,2 @@
>      if (DEBUG) {
> +      this.debug("setRadioEnabledQuickResponse: " + JSON.stringify(message));

s/setRadioEnabledQuickResponse/setRadioEnabledTryResponse/

Comment 97

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #96)

> @@ +603,5 @@
> > +      // explicitly deactive all data calls from all clients.
> > +      if (DEBUG) debug('setRadioEnabled, deactivating...');
> > +
> > +      this.createTimer();
> > +      this.deactivatingDeferred = {};
> 
> You've introduced |deactivatingDeferred| and |deactivatingPromise|... How
> about just use |this.deactivatingPromise|?
> 
> @@ +613,5 @@
> > +
> > +      promise.then(() => {
> > +        if (DEBUG) debug('setRadioEnabled, deactivation done');
> > +        this.executeRequest();
> > +        this.deactivatingPromise = null;
> 
> I don't see |deactivatingPromise| becomes un-null. Looks we don't really
> need it, removing it please.
> 

Sorry this line conflicts the previous one. Please ignore it. :P

Comment 98

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

Bug 908603 added "powerOffRadioSafely()" function to make sure we disconnect data connection before powering off. However, I learned that later in Bug 909688 we handled data connectivity lost properly. That is, bug 909688 get gecko state and routes right once ril_worker receives "UNSOLICITED_DATA_CALL_LIST_CHANGED".

Given gecko receives "UNSOLICITED_DATA_CALL_LIST_CHANGED" after powering off when there's data connected originally on device, I wonder if we still need bug 908603. However because Android still has a similar 'powerOffRadioSafely()' function, I also wonder if I somehow ignore reasons for keeping that. 

Anshul, do you see some problems from modem if we just remove 'powerOffRadioSafely()'? Or have ideas about the reason that Android keeping that? Thanks!

Comment 99

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

Attachment: Part 7#2: Deactivate data calls from all clientId before set radio to disabled

Comment 100

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

Attachment: Part 4.b: turn on radio when power on

Comment 101

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

Attachment: Part 4.c: single quote to double quote

Comment 102

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

> @@ +598,5 @@
> > +        this.processNextMessage();
> > +      }).bind(this);
> > +
> > +      // In some DSDS architecture with only one modem, toggling one radio may
> > +      // toggle both. Therefore, for safely turning off, we should first
              ^^^^^^
> 
> nit: s/toggle/toggles/

Why not 'toggle'? It is 'may toggle'. 

> @@ +603,5 @@
> > +      // explicitly deactive all data calls from all clients.
> > +      if (DEBUG) debug('setRadioEnabled, deactivating...');
> > +
> > +      this.createTimer();
> > +      this.deactivatingDeferred = {};
> 
> You've introduced |deactivatingDeferred| and |deactivatingPromise|... How
> about just use |this.deactivatingPromise|?

Keep 'deactivatingDeferred' to showing that it's a defer object, which is different from promise.
 
> @@ +613,5 @@
> > +
> > +      promise.then(() => {
> > +        if (DEBUG) debug('setRadioEnabled, deactivation done');
> > +        this.executeRequest();
> > +        this.deactivatingPromise = null;
> 
> I don't see |deactivatingPromise| becomes un-null. Looks we don't really
> need it, removing it please.

Indeed, it's not used.
 
> @@ +635,5 @@
> > +      this.processNextMessage();
> > +    }
> > +  },
> > +
> > +  finishDeactivation: function(clientId) {
> 
> Could we simple name it 'resolveDeactivatingPromise'?

Use 'finishDeactivatingDataCalls'. I don't want to expose too much details to the function caller. Promise is just one of the implementation manner.
 
> @@ +2571,5 @@
> >      }).bind(this));
> >    },
> >  
> > +  // Return true if successfuly responsed.
> > +  setRadioEnabledTryResponse: function setRadioEnabledTryResponse(target, message) {
> 
> I feel we do need to come out a better name, but I haven't got a concrete
> thought yet...

Naming is one of the difficult part in programming...... 
Actually, ...

"""
There are only two hard things in Computer Science: cache invalidation and naming things. -- Phil Karlton
"""

Comment 103

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

Comment on attachment 832788 [details] [diff] [review]
Part 7#2: Deactivate data calls from all clientId before set radio to disabled

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +422,5 @@
> +      if (msg.name === "RIL:SetRadioEnabled") {
> +        // Special handler for SetRadioEnabled.
> +        return gRadioEnabledController.receiveMessage(msg);
> +      } else {
> +        return radioInterface.receiveMessage(msg);

No else after return.

@@ +529,5 @@
> +      } else {
> +        this.request = (function() {
> +          radioInterface.receiveMessage(msg);
> +          this._processNextMessage();
> +        }).bind(this);

nit: add an extra new line here

@@ +532,5 @@
> +          this._processNextMessage();
> +        }).bind(this);
> +        // In some DSDS architecture with only one modem, toggling one radio may
> +        // toggle both. Therefore, for safely turning off, we should first
> +        // explicitly deactive all data calls from all clients.

nit: s/deactive/deactivate

@@ +1640,4 @@
>    /**
>     * Clean up all existing data calls before turning radio off.
>     */
> +  deactivateDataCalls: function deactivateDataCalls() {

The function name is very general and not strongly related to 'radio off.' I'd suggest removing the above comment and debug message below.

@@ +1652,5 @@
>            dataDisconnecting = true;
>          }
>        }
>      }
> +

Please add one comment here:
// No data calls exist. It's safe to proceed the pending radio power off request.

@@ +1655,5 @@
>      }
> +
> +    if (!dataDisconnecting) {
> +      if (DEBUG) this.debug("For setRadioEnabled: No data calls");
> +      gRadioEnabledController.finishDeactivatingDataCalls(this.clientId);

It looks like we are likely to call 'promise.resolve()' in line#504 before returning 'deferred.promise' in line #562. What happens then?

@@ +2565,5 @@
>      }).bind(this));
>    },
>  
> +  // Return true if successfuly responsed.
> +  setRadioEnabledTryResponse: function setRadioEnabledTryResponse(target, message) {

Naming this function is a big challenge ;)

'isSetRadioEnabledProceeding()' is the best one that I could come out.
Return 'true' if we proceed with this request/message; returning  false means we are fine with the current request and we could jump to the next.

With the new name, we'd take care of turning the return value upside down.

Sounds good?

Comment 104

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

(In reply to Szu-Yu Chen [:aknow] from comment #102)
> > @@ +598,5 @@
> > > +        this.processNextMessage();
> > > +      }).bind(this);
> > > +
> > > +      // In some DSDS architecture with only one modem, toggling one radio may
> > > +      // toggle both. Therefore, for safely turning off, we should first
>               ^^^^^^
> > 
> > nit: s/toggle/toggles/
> 
> Why not 'toggle'? It is 'may toggle'. 

You are right, thanks :)

> 
> > @@ +603,5 @@
> > > +      // explicitly deactive all data calls from all clients.
> > > +      if (DEBUG) debug('setRadioEnabled, deactivating...');
> > > +
> > > +      this.createTimer();
> > > +      this.deactivatingDeferred = {};
> > 
> > You've introduced |deactivatingDeferred| and |deactivatingPromise|... How
> > about just use |this.deactivatingPromise|?
> 
> Keep 'deactivatingDeferred' to showing that it's a defer object, which is
> different from promise.
>  

Accept!

> > @@ +613,5 @@
> > > +
> > > +      promise.then(() => {
> > > +        if (DEBUG) debug('setRadioEnabled, deactivation done');
> > > +        this.executeRequest();
> > > +        this.deactivatingPromise = null;
> > 
> > I don't see |deactivatingPromise| becomes un-null. Looks we don't really
> > need it, removing it please.
> 
> Indeed, it's not used.
>  
> > @@ +635,5 @@
> > > +      this.processNextMessage();
> > > +    }
> > > +  },
> > > +
> > > +  finishDeactivation: function(clientId) {
> > 
> > Could we simple name it 'resolveDeactivatingPromise'?
> 
> Use 'finishDeactivatingDataCalls'. I don't want to expose too much details
> to the function caller. Promise is just one of the implementation manner.

The new name looks good to me as well.

>  
> > @@ +2571,5 @@
> > >      }).bind(this));
> > >    },
> > >  
> > > +  // Return true if successfuly responsed.
> > > +  setRadioEnabledTryResponse: function setRadioEnabledTryResponse(target, message) {
> > 
> > I feel we do need to come out a better name, but I haven't got a concrete
> > thought yet...
> 
> Naming is one of the difficult part in programming...... 
> Actually, ...
> 
> """
> There are only two hard things in Computer Science: cache invalidation and
> naming things. -- Phil Karlton
> """

Finally... I came out with a new one... hope it is much clearer.

Comment 105

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

Comment on attachment 832790 [details] [diff] [review]
Part 4.c: single quote to double quote

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

Thanks for reviewing the whole file.

Comment 106

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #103)

> @@ +1655,5 @@
> >      }
> > +
> > +    if (!dataDisconnecting) {
> > +      if (DEBUG) this.debug("For setRadioEnabled: No data calls");
> > +      gRadioEnabledController.finishDeactivatingDataCalls(this.clientId);
> 
> It looks like we are likely to call 'promise.resolve()' in line#504 before
> returning 'deferred.promise' in line #562. What happens then?

It's OK. Just like Promise.resolve().then(), it works well on an already resolved promise.

Comment 107

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

Attachment: Part 7#3: Deactivate data calls from all clientId before set radio to disabled

Rewrite setRadioEnabledTryResponse() part

Comment 108

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

Comment on attachment 8335881 [details] [diff] [review]
Part 7#3: Deactivate data calls from all clientId before set radio to disabled

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

Looks great. Thank you.

Comment 109

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

Attachment: [Final #2] Part 1: Add setRadioEnabled API (idl). r=hsinyi

Comment 110

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

Attachment: [Final #2] Part 2: Add setRadioEnabled API (dom). r=khuey

Comment 111

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

Attachment: [Final #2] Part 3: Add radiostatechange in listener (bluetooth). r=gyeh

Comment 112

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

Attachment: [Final #2] Part 4: Add setRadioEnabled API (ril). r=hsinyi

Merge original part 4, 4b, 4c, 7 into this one.

Comment 113

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

Attachment: [Final #2] Part 5: Add setRadioEnabled marionette test. r=hsinyi

Comment 114

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

Attachment: [Final #2] Part 6: Modify related tests. r=hsinyi

Comment 115

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

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

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bbd421bc4226
https://hg.mozilla.org/integration/b2g-inbound/rev/46bfcce51d51
https://hg.mozilla.org/integration/b2g-inbound/rev/cd4c41890023
https://hg.mozilla.org/integration/b2g-inbound/rev/f57568dfa7c5
https://hg.mozilla.org/integration/b2g-inbound/rev/713964f481e2
https://hg.mozilla.org/integration/b2g-inbound/rev/d1bf8d617dd1

Comment 117

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bbd421bc4226
https://hg.mozilla.org/mozilla-central/rev/46bfcce51d51
https://hg.mozilla.org/mozilla-central/rev/cd4c41890023
https://hg.mozilla.org/mozilla-central/rev/f57568dfa7c5
https://hg.mozilla.org/mozilla-central/rev/713964f481e2
https://hg.mozilla.org/mozilla-central/rev/d1bf8d617dd1

Comment 118

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

Comment on attachment 8336032 [details] [diff] [review]
[Final #2] Part 4: Add setRadioEnabled API (ril). r=hsinyi

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2033,5 @@
>        if (!success) {
>          // At this point we could send a message to content to notify the user
>          // that storing an incoming SMS failed, most likely due to a full disk.
>          if (DEBUG) {
> +          this.debug("Could not store SMS " + message.id + ", error code " + rv);

Tell me why did you reverse these lines?

@@ +2044,5 @@
>      }.bind(this);
>  
>      if (message.messageClass != RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_0]) {
> +      message.id = gMobileMessageDatabaseService.saveReceivedMessage(message,
> +                                                                     notifyReceived);

And this.

Comment 119

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #118)
> Comment on attachment 8336032 [details] [diff] [review]
> [Final #2] Part 4: Add setRadioEnabled API (ril). r=hsinyi
> 
> Review of attachment 8336032 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +2033,5 @@
> >        if (!success) {
> >          // At this point we could send a message to content to notify the user
> >          // that storing an incoming SMS failed, most likely due to a full disk.
> >          if (DEBUG) {
> > +          this.debug("Could not store SMS " + message.id + ", error code " + rv);
> 
> Tell me why did you reverse these lines?
> 
> @@ +2044,5 @@
> >      }.bind(this);
> >  
> >      if (message.messageClass != RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_0]) {
> > +      message.id = gMobileMessageDatabaseService.saveReceivedMessage(message,
> > +                                                                     notifyReceived);
> 
> And this.

Sorry, It must be a defect introduced when I rebasing the code.
Have you landed the patch for correct one? Or I could provide the patch for this.