926302 - B2G DSDS: need a utility function for DOM to get numRadioInterfaces and default settings

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

Some notes from offline discussion with Vicamo,

It looks better to expose a new API to nsIMobileConnectionProvider to get number of radio interfaces, compared to creating a new ipdl protocol for RadioInterfaceLayer service, as the latter is really expensive... However, we need to be careful of permission checks if we take the former solution. "telephony" and "bluetooth" are known to be added.

Regarding default service id, it will be provided by individual *Provider.idl/*Service.idl.

Comment 2

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

Attachment: part 1/3: interface changes : WIP

Comment 3

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

Attachment: part 2/3: WIP

Comment 4

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

Comment on attachment 817043 [details] [diff] [review]
part 1/3: interface changes : WIP

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

Looks good to me. Don't forget to change uuid.

Comment 5

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

Attachment: part 3/3: test cases : WIP

Comment 6

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

Depends on bug 927432 to run chrome Marionette test cases correctly.

Comment 7

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

Attachment: part 1/3: interface changes

Per offline discuss, the main target is we want to have an unique way to retrieve correct number of radio interfaces.  Based on the face that this number is now only initialized in ctor of RadioInterfaceLayer and has been only available through nsIRadioInterfaceLayer since bug 919982, we have to ensure correct boot up sequence and and have some complex code to work around such access in bug 842239.  The former chains originally independent services up, increase effort to keep modularization.  The latter suffers from permission issues when Bluetooth and other components want numRadioInterfaces as well.

This patch no longer adds attribute 'numRadioInterfaces' to nsIMobileConnectionProvider.  Instead, all components are free to read preference 'ril.numRadioInterfaces', which is synced every time |RadioInterfaceLayer.js| is loaded.  So there is no more dependency to service nsIRadioInterfaceLayer, nor permission issues for other components.

Comment 8

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

Attachment: part 2/3: implementation

Comment 9

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

Attachment: part 3/3: test cases

Depends on bug 927432 to run correctly.

Comment 10

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

Attachment: part 2/3: implementation : v2

fix test case bustage within JB Gonk

Comment 11

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

Attachment: part 3/3: test cases : v2

fix test case bustage within JB Gonk

Comment 12

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

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

Comment 13

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

Comment on attachment 818229 [details] [diff] [review]
part 1/3: interface changes

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

Looks good! Thank you.

Comment 14

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

Comment on attachment 818553 [details] [diff] [review]
part 2/3: implementation : v2

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

Looks good to me, but we still need review from DOM peers. Thank you.

::: dom/mobilemessage/src/gonk/SmsService.cpp
@@ +7,5 @@
>  #include "SmsService.h"
>  #include "jsapi.h"
>  #include "SmsSegmentInfo.h"
> +#include "mozilla/Preferences.h"
> +#include "nsCRT.h"

Seems we don't need this.

@@ +71,5 @@
> +SmsService::Observe(nsISupports* aSubject,
> +                    const char* aTopic,
> +                    const PRUnichar* aData)
> +{
> +  if (!nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {

ditto.

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +17,5 @@
>  #include "mozilla/dom/MobileMessageManagerBinding.h"
>  #include "mozilla/dom/MozMmsMessageBinding.h"
>  #include "mozilla/dom/BindingUtils.h"
> +#include "mozilla/Preferences.h"
> +#include "nsCRT.h"

Seems we don't need this.

@@ +18,5 @@
>  #include "mozilla/dom/MozMmsMessageBinding.h"
>  #include "mozilla/dom/BindingUtils.h"
> +#include "mozilla/Preferences.h"
> +#include "nsCRT.h"
> +#include "nsString.h"

and this ...

::: dom/system/gonk/RILContentHelper.js
@@ +1416,5 @@
> +        } else if (data == kPrefDefaultVoicemailServiceId) {
> +          this.updateDefaultVoicemailServiceId();
> +        }
> +        break;
> +

nit: remove the line

::: dom/telephony/ipc/TelephonyIPCProvider.cpp
@@ +8,4 @@
>  #include "mozilla/dom/ContentChild.h"
>  #include "mozilla/dom/telephony/TelephonyChild.h"
> +#include "mozilla/Preferences.h"
> +#include "nsCRT.h"

Seems we don't need this.

@@ +70,5 @@
> +TelephonyIPCProvider::Observe(nsISupports* aSubject,
> +                              const char* aTopic,
> +                              const PRUnichar* aData)
> +{
> +  if (!nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {

strcmp(aTopic ... ) should be okay.

Comment 15

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

Comment on attachment 818554 [details] [diff] [review]
part 3/3: test cases : v2

Thank you!!!

Comment 16

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

Comment on attachment 818553 [details] [diff] [review]
part 2/3: implementation : v2

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

Bug 818353 has been in b2g-inboud. You will need to rebase afterwards.

Comment 17

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

Attachment: part 2/3: implementation : v3

Address review comment 14.  @khuey, may I have your review?

Comment 18

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

Comment on attachment 818914 [details] [diff] [review]
part 2/3: implementation : v3

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

Please update the commit message and reviewer information.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1197,5 @@
> +function getDefaultServiceId() {
> +  let id = Services.prefs.getIntPref(kPrefDefaultServiceId);
> +  let numRil = Services.prefs.getIntPref(kPrefRilNumRadioInterfaces);
> +
> +  if ((id >= numRil) || (id < 0)) {

You don't need the extra parentheses.  || has much lower precedence than < and >=

::: dom/mobilemessage/src/gonk/SmsService.cpp
@@ +18,5 @@
> +  kPrefDefaultServiceId,
> +  nullptr
> +};
> +
> +uint32_t defaultServiceId = 0;

use a member variable instead of a global

@@ +26,5 @@
> +{
> +  int32_t id = mozilla::Preferences::GetInt(kPrefDefaultServiceId, 0);
> +  int32_t numRil = mozilla::Preferences::GetInt(kPrefRilNumRadioInterfaces, 1);
> +
> +  if ((id >= numRil) || (id < 0)) {

and here

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +34,5 @@
> +  nullptr
> +};
> +
> +uint32_t defaultMmsServiceId = 0;
> +uint32_t defaultSmsServiceId = 0;

here too

@@ +94,5 @@
> +{
> +  int32_t id = mozilla::Preferences::GetInt(aPrefKey, 0);
> +  int32_t numRil = mozilla::Preferences::GetInt(kPrefRilNumRadioInterfaces, 1);
> +
> +  if ((id >= numRil) || (id < 0)) {

and here

::: dom/system/gonk/RILContentHelper.js
@@ +1308,5 @@
> +  updateDefaultVoicemailServiceId: function updateDefaultVoicemailServiceId() {
> +    let id = Services.prefs.getIntPref(kPrefDefaultVoicemailServiceId);
> +    let numRil = Services.prefs.getIntPref(kPrefRilNumRadioInterfaces);
> +
> +    if ((id >= numRil) || (id < 0)) {

and here

::: dom/telephony/ipc/TelephonyIPCProvider.cpp
@@ +20,5 @@
> +  kPrefDefaultServiceId,
> +  nullptr
> +};
> +
> +uint32_t defaultServiceId = 0;

Make this a member variable on TelephonyIPCProvider.  There's no reason to have this be a global variable.

@@ +54,5 @@
>  }
>  
>  TelephonyIPCProvider::~TelephonyIPCProvider()
>  {
> +  Preferences::RemoveObservers(this, kObservedPrefs);

You told the preferences service above to hold a strong reference to this, so the only way the destructor can ever run is if the preference service is clearing out its list of observers at shutdown.  But the Preferences API doesn't expose a good alternative, so I guess this is fine.

Comment 19

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

Attachment: part 2/3: implementation : v4

1) Rebase to tip
2) Address khuey's review comment 18

Comment 20

[Gene Lian (I already quit Mozilla)]

Comment on attachment 819662 [details] [diff] [review]
part 2/3: implementation : v4

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

Looks good! r=gene

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1197,5 @@
> +function getDefaultServiceId() {
> +  let id = Services.prefs.getIntPref(kPrefDefaultServiceId);
> +  let numRil = Services.prefs.getIntPref(kPrefRilNumRadioInterfaces);
> +
> +  if ((id >= numRil) || (id < 0)) {

I don't really like to add too many parenthesis like this but it's OK, though.

@@ +1214,5 @@
>      let minor = MMS.MMS_VERSION & 0x0f;
>      debug("Running protocol version: " + macro + "." + minor);
>    }
>  
> +  Services.prefs.addObserver(kPrefDefaultServiceId, this, false);

Where do you remove the observer while shutting down?

@@ +1820,5 @@
>    },
>  
>    // nsIMmsService
>  
> +  defaultMmsServiceId: 0,

Since you're in the MMS, s/defaultMmsServiceId/defaultServiceId? For example you don't add "mms" for "kPrefDefaultServiceId" or "kPrefRetrievalMode" either. This is also more symmetric to the SMS' mDefaultServiceId.

@@ +2139,5 @@
> +  observe: function observe(aSubject, aTopic, aData) {
> +    switch (aTopic) {
> +      case kPrefenceChangedObserverTopic:
> +        if (aData === kPrefDefaultServiceId) {
> +          this.defaultMmsServiceId = getDefaultServiceId();

Ditto for the naming.

::: dom/mobilemessage/src/gonk/SmsService.cpp
@@ +24,5 @@
> +{
> +  int32_t id = mozilla::Preferences::GetInt(kPrefDefaultServiceId, 0);
> +  int32_t numRil = mozilla::Preferences::GetInt(kPrefRilNumRadioInterfaces, 1);
> +
> +  if (id >= numRil || id < 0) {

Why the parenthesis is different as mentioned above? Better to have the same coding style.

::: dom/system/gonk/RILContentHelper.js
@@ +30,4 @@
>  
> +const kPrefRilNumRadioInterfaces = "ril.numRadioInterfaces";
> +const kPrefRilDebuggingEnabled = "ril.debugging.enabled";
> +const kPrefDefaultVoicemailServiceId = "dom.voicemail.defaultServiceId";

s/kPrefDefaultVoicemailServiceId/kPrefVoicemailDefaultServiceId which is more consistent to other namings.

@@ +455,5 @@
>  
> +  Services.obs.addObserver(this, kXpcomShutdownObserverTopic, false);
> +
> +  Services.prefs.addObserver(kPrefRilDebuggingEnabled, this, false);
> +  Services.prefs.addObserver(kPrefDefaultVoicemailServiceId, this, false);

Ditto for the naming.

@@ +1330,5 @@
>    _iccListeners: null,
>  
>    voicemailStatus: null,
>  
> +  defaultVoicemailServiceId: 0,

Ditto for the naming.

@@ +1331,5 @@
>  
>    voicemailStatus: null,
>  
> +  defaultVoicemailServiceId: 0,
> +  updateDefaultVoicemailServiceId: function updateDefaultVoicemailServiceId() {

s/updateDefaultVoicemailServiceId/updateVoicemailDefaultServiceId.

@@ +1332,5 @@
>    voicemailStatus: null,
>  
> +  defaultVoicemailServiceId: 0,
> +  updateDefaultVoicemailServiceId: function updateDefaultVoicemailServiceId() {
> +    let id = Services.prefs.getIntPref(kPrefDefaultVoicemailServiceId);

Ditto for the naming.

@@ +1339,5 @@
> +    if (id >= numRil || id < 0) {
> +      id = 0;
> +    }
> +
> +    this.defaultVoicemailServiceId = id;

Ditto for the naming.

@@ +1452,5 @@
> +      case kPrefenceChangedObserverTopic:
> +        if (data == kPrefRilDebuggingEnabled) {
> +          this.updateDebugFlag();
> +        } else if (data == kPrefDefaultVoicemailServiceId) {
> +          this.updateDefaultVoicemailServiceId();

Ditto for the naming.

::: dom/telephony/gonk/TelephonyProvider.js
@@ +276,4 @@
>      } catch (e) {}
>    },
>  
> +  _updateDefaultServiceId: function _updateDefaultServiceId() {

Is there any reason why do you use "_update" instead of "_get" like other files?

@@ -515,5 @@
>        case kXpcomShutdownObserverTopic:
>          // Cancel the timer for the call-ring wake lock.
>          this._cancelCallRingWakeLockTimer();
>  
> -        Services.obs.removeObserver(this, kPrefenceChangedObserverTopic);

Why do you remove this?

::: modules/libpref/src/init/all.js
@@ +4486,5 @@
>  pref("dom.mobileconnection.enabled", false);
>  
>  // Voice Mail API
>  pref("dom.voicemail.enabled", false);
> +// Numeric default service id for WebTelephony API calls with |serviceId|

s/WebTelephony/VoiceMail? (not sure if the APIs are separate).

Comment 21

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

Thanks for the review!

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #20)
> part 2/3: implementation : v4
> -----------------------------------------------------------------
> ::: dom/mobilemessage/src/gonk/MmsService.js
> @@ +1214,5 @@
> >      let minor = MMS.MMS_VERSION & 0x0f;
> >      debug("Running protocol version: " + macro + "." + minor);
> >    }
> >  
> > +  Services.prefs.addObserver(kPrefDefaultServiceId, this, false);
> 
> Where do you remove the observer while shutting down?

Nowhere.  nsPrefBranch listens to NS_XPCOM_SHUTDOWN_OBSERVER_ID, and when it's notified, it calls |nsPrefBranch::freeObserverList()| and drops all preference observers. [1]  Besides, during the time nsPrefBranch is freeing observers, no |nsPrefBranch::RemoveObserver()| calls are necessary because they'll be simply ignored. [2]  I should have cleaned all these RemoveObserver() calls.

> @@ +1820,5 @@
> >    },
> >  
> >    // nsIMmsService
> >  
> > +  defaultMmsServiceId: 0,
> 
> Since you're in the MMS, s/defaultMmsServiceId/defaultServiceId? For example
> you don't add "mms" for "kPrefDefaultServiceId" or "kPrefRetrievalMode"
> either. This is also more symmetric to the SMS' mDefaultServiceId.

Can't.  SmsIPCService implements both nsISmsService and nsIMmsService.  Rename both to 'defaultServiceId' results in duplicated member functions in SmsIPCService.

> ::: dom/system/gonk/RILContentHelper.js
> @@ +1330,5 @@
> >    _iccListeners: null,
> >  
> >    voicemailStatus: null,
> >  
> > +  defaultVoicemailServiceId: 0,
> 
> Ditto for the naming.

Same with SmsService/MmsService thing.  I'll have 'smsDefaultServiceId', 'mmsDefaultServiceId', and 'voicemailDefaultServiceId' instead in next revision.

> ::: dom/telephony/gonk/TelephonyProvider.js
> @@ -515,5 @@
> >        case kXpcomShutdownObserverTopic:
> >          // Cancel the timer for the call-ring wake lock.
> >          this._cancelCallRingWakeLockTimer();
> >  
> > -        Services.obs.removeObserver(this, kPrefenceChangedObserverTopic);
> 
> Why do you remove this?

Because that's a mistake.  Register "nsPref:changed" to observer service is meaningless.  I Should have registered to Services.prefs instead.

[1]: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#695
[2]: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/nsPrefBranch.cpp#663

Comment 22

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

Attachment: part 1/3: interface changes : v2

Rename 'defaultFooServiceId' to 'fooDefaultServiceId' for sms, mms, and voicemail.

Comment 23

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

Attachment: part 2.a/3: refactor preference/settings key names

1) Use NS_XPCOM_SHUTDOWN_OBSERVER_ID for "xpcom-shutdown"
2) Use NS_PREFBRANCH_PREFCHANGE_TOPIC_ID for "nsPref:changed"
3) s/PREF_RETRIEVAL_MODE/kPrefRetrievalMode/ in MmsService
4) Remove |Services.prefs.removeObserver()| calls
5) Fix listening preference "ril.debugging.enabled" changes in RILContentHelper and TelephonyProvider
6) Use "kSettingsFoo" for Settings key names

Comment 24

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

Attachment: part 2/3: implementation : v5

Rebase to previous changes.  Address comment 20.

Comment 25

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

Attachment: part 3/3: test cases : v3

Reflect attribute name changes only.

Comment 26

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

Full try yet again: https://tbpl.mozilla.org/?tree=Try&rev=10a95aae0633

Comment 27

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

Comment on attachment 820902 [details] [diff] [review]
part 1/3: interface changes : v2

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

Thank you :)

Comment 28

[Gene Lian (I already quit Mozilla)]

Comment on attachment 820905 [details] [diff] [review]
part 2.a/3: refactor preference/settings key names

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

Nice!

Comment 29

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

Attachment: part 2.a/3: refactor preference/settings key names : v2

Rebase for bug 818353 was backed out and it actually depends on this bug.

Comment 30

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

Attachment: part 2/3: implementation : v6

rebase only

Comment 31

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

https://hg.mozilla.org/integration/b2g-inbound/rev/29149aa9eded
https://hg.mozilla.org/integration/b2g-inbound/rev/f90977536aa9
https://hg.mozilla.org/integration/b2g-inbound/rev/2cf48b7c7a99
https://hg.mozilla.org/integration/b2g-inbound/rev/5452601c459f

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29149aa9eded
https://hg.mozilla.org/mozilla-central/rev/f90977536aa9
https://hg.mozilla.org/mozilla-central/rev/2cf48b7c7a99
https://hg.mozilla.org/mozilla-central/rev/5452601c459f