986395 - B2G RIL: Correct the API behavior of setPreferredNetworkType()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

+++ This bug was initially created as a clone of Bug #974253 +++

For the radio off case, currently gecko will cache the last value which comes from setPreferredNetworkType() and set it again when radio is on. But in this case, gecko will dispatch an error event to gaia.

I think we should correct the behavior of this api:
1. If gecko already handle the radio off case and make sure the value will be correct set to modem, 
   this api should dispatch success event instead of an error one.
2. Should this api handle the radio off case like this? Other apis doesn't have this kind of behavior.
   So to consist with other apis, we should remove this behavior.

To me, I prefer second one. :)

Comment 1

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

(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> +++ This bug was initially created as a clone of Bug #974253 +++
> 
> For the radio off case, currently gecko will cache the last value which
> comes from setPreferredNetworkType() and set it again when radio is on. But
> in this case, gecko will dispatch an error event to gaia.
> 
> I think we should correct the behavior of this api:
> 1. If gecko already handle the radio off case and make sure the value will
> be correct set to modem, 
>    this api should dispatch success event instead of an error one.
> 2. Should this api handle the radio off case like this? Other apis doesn't
> have this kind of behavior.
>    So to consist with other apis, we should remove this behavior.
> 
> To me, I prefer second one. :)

I think no matter gecko is trying to set preferredNetworkType again once radio is ready, the API should behave as one of the followings:
1) checking radio state, and fire error immediately if radio is off, even without querying REQUEST_SET_PREFERRED_NETWORK_TYPE.
2) we could consider checking radio state first (i.e. 1), but without the check, always fire result as modem responds
3) checking radio state first, if radio is off, we put the request into queue. Once radio is ready, we send the request to modem and respond success/error based on modem response. But this is problematic because what is radio is never on... we will need magic timeout. 

I think Edgar's comment is 1), plus gecko doesn't need to reset it again when radio becomes ready, right? If so, and if we have good communication with Gaia, it looks good to me.

Comment 2

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #1)
> (In reply to Edgar Chen [:edgar][:echen] from comment #0)
> > +++ This bug was initially created as a clone of Bug #974253 +++
> > 
> > For the radio off case, currently gecko will cache the last value which
> > comes from setPreferredNetworkType() and set it again when radio is on. But
> > in this case, gecko will dispatch an error event to gaia.
> > 
> > I think we should correct the behavior of this api:
> > 1. If gecko already handle the radio off case and make sure the value will
> > be correct set to modem, 
> >    this api should dispatch success event instead of an error one.
> > 2. Should this api handle the radio off case like this? Other apis doesn't
> > have this kind of behavior.
> >    So to consist with other apis, we should remove this behavior.
> > 
> > To me, I prefer second one. :)
> 
> I think no matter gecko is trying to set preferredNetworkType again once
> radio is ready, the API should behave as one of the followings:
> 1) checking radio state, and fire error immediately if radio is off, even
> without querying REQUEST_SET_PREFERRED_NETWORK_TYPE.
> 2) we could consider checking radio state first (i.e. 1), but without the
> check, always fire result as modem responds
> 3) checking radio state first, if radio is off, we put the request into
> queue. Once radio is ready, we send the request to modem and respond
> success/error based on modem response. But this is problematic because what
> is radio is never on... we will need magic timeout. 
> 
> I think Edgar's comment is 1), plus gecko doesn't need to reset it again
> when radio becomes ready, right? If so, and if we have good communication
> with Gaia, it looks good to me.

Sorry for doesn't make it clear enough.
My comment is more like 2) without check + gecko doesn't need to reset it again, because most of other api of mobileConnection doesn't check the radio state.

Comment 3

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

(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #1)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #0)
> > > +++ This bug was initially created as a clone of Bug #974253 +++
> > > 
> > > For the radio off case, currently gecko will cache the last value which
> > > comes from setPreferredNetworkType() and set it again when radio is on. But
> > > in this case, gecko will dispatch an error event to gaia.
> > > 
> > > I think we should correct the behavior of this api:
> > > 1. If gecko already handle the radio off case and make sure the value will
> > > be correct set to modem, 
> > >    this api should dispatch success event instead of an error one.
> > > 2. Should this api handle the radio off case like this? Other apis doesn't
> > > have this kind of behavior.
> > >    So to consist with other apis, we should remove this behavior.
> > > 
> > > To me, I prefer second one. :)
> > 
> > I think no matter gecko is trying to set preferredNetworkType again once
> > radio is ready, the API should behave as one of the followings:
> > 1) checking radio state, and fire error immediately if radio is off, even
> > without querying REQUEST_SET_PREFERRED_NETWORK_TYPE.
> > 2) we could consider checking radio state first (i.e. 1), but without the
> > check, always fire result as modem responds
> > 3) checking radio state first, if radio is off, we put the request into
> > queue. Once radio is ready, we send the request to modem and respond
> > success/error based on modem response. But this is problematic because what
> > is radio is never on... we will need magic timeout. 
> > 
> > I think Edgar's comment is 1), plus gecko doesn't need to reset it again
> > when radio becomes ready, right? If so, and if we have good communication
> > with Gaia, it looks good to me.
> 
> Sorry for doesn't make it clear enough.
> My comment is more like 2) without check + gecko doesn't need to reset it
> again, because most of other api of mobileConnection doesn't check the radio
> state.

Ha, I see. If radio state isn't a platform-independent assumption, then 2) should be fine as well.

Comment 4

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

If we are talking to gaia that they should wait till radio is ready, doesn't that imply gecko should simply return error when radio isn't ready, to have a more strict and consistent behaviour, even 2) looks no harm?

Comment 5

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #4)
> If we are talking to gaia that they should wait till radio is ready, doesn't
> that imply gecko should simply return error when radio isn't ready, to have
> a more strict and consistent behaviour, even 2) looks no harm?

Yup, agree with you.
If we do the radio check in gecko for setPreferredNetworkType(), should we do the same thing for other api of mobileConnection as well?

Comment 6

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

(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #4)
> > If we are talking to gaia that they should wait till radio is ready, doesn't
> > that imply gecko should simply return error when radio isn't ready, to have
> > a more strict and consistent behaviour, even 2) looks no harm?
> 
> Yup, agree with you.
> If we do the radio check in gecko for setPreferredNetworkType(), should we
> do the same thing for other api of mobileConnection as well?

I think so, if the function call succeeds only after radio is ready. :)

Comment 7

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 1: RIL changes, v1

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 2: Test, v1

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8421613 [details] [diff] [review]
Part 1: RIL changes, v1

1). Fire "RadioNotAvailable" error if radio is not enabled.
2). Don't reset preferred network type in ril_worker.

Comment 11

[Edgar Chen [:edgar]]

Comment on attachment 8421614 [details] [diff] [review]
Part 2: Test, v1

Add test for setting/getting preferred network type on radio off.

Comment 12

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

Comment on attachment 8421613 [details] [diff] [review]
Part 1: RIL changes, v1

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

Looks good, thanks :)

::: dom/system/gonk/ril_worker.js
@@ +1147,5 @@
>    /**
>     * Set the preferred network type.
>     *
>     * @param options An object contains a valid index of
>     *                RIL_PREFERRED_NETWORK_TYPE_TO_GECKO as its `networkType`

After this patch, |options| no longer has 'networkType', has 'options.type' instead. Please revise the comment appropriately.

@@ -6215,5 @@
>  RilObject.prototype[REQUEST_STK_SEND_TERMINAL_RESPONSE] = null;
>  RilObject.prototype[REQUEST_STK_HANDLE_CALL_SETUP_REQUESTED_FROM_SIM] = null;
>  RilObject.prototype[REQUEST_EXPLICIT_CALL_TRANSFER] = null;
>  RilObject.prototype[REQUEST_SET_PREFERRED_NETWORK_TYPE] = function REQUEST_SET_PREFERRED_NETWORK_TYPE(length, options) {
> -  if (options.networkType == null) {

Nice cleanup!

Comment 13

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

Comment on attachment 8421614 [details] [diff] [review]
Part 2: Test, v1

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

::: dom/mobileconnection/tests/marionette/test_mobile_clir_radio_off.js
@@ -31,5 @@
>      .then(() => testSetClirOnRadioOff(0))
>      .then(() => testGetClirOnRadioOff())
>      // Restore radio state.
>      .then(() => setRadioEnabledAndWait(true),
> -          () => setRadioEnabledAnWait(true));

Thanks for the catch :P

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 1: RIL changes, v2, r=hsinyi

Address review comment #12.

Comment 15

[Edgar Chen [:edgar]]

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

Comment 16

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c00f98915e3e
https://hg.mozilla.org/integration/b2g-inbound/rev/ae3088666d62

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c00f98915e3e
https://hg.mozilla.org/mozilla-central/rev/ae3088666d62