Closed Bug 926302 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 3 - 10/25
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: hsinyi, Assigned: vicamo)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(4 files, 12 obsolete files)

4.73 KB, patch
hsinyi
: review+
airpingu
: review+
Details | Diff | Splinter Review
15.51 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
31.39 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
36.77 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 814629
Blocks: 814625
Blocks: 814637
Assignee: nobody → vyang
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.
Attachment #817043 - Flags: feedback?(htsai)
Attached patch part 2/3: WIP (obsolete) — Splinter Review
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.
Attachment #817043 - Flags: feedback?(htsai) → feedback+
Attached patch part 3/3: test cases : WIP (obsolete) — Splinter Review
Blocks: 814634
Depends on bug 927432 to run chrome Marionette test cases correctly.
Depends on: 927432
Attached patch part 1/3: interface changes (obsolete) — Splinter Review
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.
Attachment #817043 - Attachment is obsolete: true
Attachment #818229 - Flags: review?(htsai)
Attached patch part 2/3: implementation (obsolete) — Splinter Review
Attachment #817044 - Attachment is obsolete: true
Attachment #818230 - Flags: review?(htsai)
Attachment #818230 - Flags: review?(gene.lian)
Attachment #818229 - Flags: review?(htsai) → superreview?(htsai)
Attached patch part 3/3: test cases (obsolete) — Splinter Review
Depends on bug 927432 to run correctly.
Attachment #817104 - Attachment is obsolete: true
Attachment #818233 - Flags: review?(htsai)
blocking-b2g: --- → 1.3+
Attached patch part 2/3: implementation : v2 (obsolete) — Splinter Review
fix test case bustage within JB Gonk
Attachment #818230 - Attachment is obsolete: true
Attachment #818230 - Flags: review?(htsai)
Attachment #818230 - Flags: review?(gene.lian)
Attachment #818553 - Flags: review?(htsai)
Attachment #818553 - Flags: review?(gene.lian)
Attached patch part 3/3: test cases : v2 (obsolete) — Splinter Review
fix test case bustage within JB Gonk
Attachment #818233 - Attachment is obsolete: true
Attachment #818233 - Flags: review?(htsai)
Attachment #818554 - Flags: review?(htsai)
Attachment #818553 - Attachment description: part 2/3: implementation → part 2/3: implementation : v2
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Comment on attachment 818229 [details] [diff] [review]
part 1/3: interface changes

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

Looks good! Thank you.
Attachment #818229 - Flags: superreview?(htsai) → review+
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.
Attachment #818553 - Flags: review?(htsai) → review+
Comment on attachment 818554 [details] [diff] [review]
part 3/3: test cases : v2

Thank you!!!
Attachment #818554 - Flags: review?(htsai) → review+
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.
Attached patch part 2/3: implementation : v3 (obsolete) — Splinter Review
Address review comment 14.  @khuey, may I have your review?
Attachment #818553 - Attachment is obsolete: true
Attachment #818553 - Flags: review?(gene.lian)
Attachment #818914 - Flags: review?(khuey)
Attachment #818914 - Flags: review?(gene.lian)
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.
Attachment #818914 - Flags: review?(khuey) → review+
Attached patch part 2/3: implementation : v4 (obsolete) — Splinter Review
1) Rebase to tip
2) Address khuey's review comment 18
Attachment #818914 - Attachment is obsolete: true
Attachment #818914 - Flags: review?(gene.lian)
Attachment #819662 - Flags: review?(gene.lian)
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).
Attachment #819662 - Flags: review?(gene.lian) → review+
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
Whiteboard: [FT:RIL]
Rename 'defaultFooServiceId' to 'fooDefaultServiceId' for sms, mms, and voicemail.
Attachment #818229 - Attachment is obsolete: true
Attachment #820902 - Flags: review?(htsai)
Attachment #820902 - Flags: review?(gene.lian)
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
Attachment #820905 - Flags: review?(gene.lian)
Attachment #820905 - Attachment description: part 2.a/2: refactor preference/settings key names → part 2.a/3: refactor preference/settings key names
Attached patch part 2/3: implementation : v5 (obsolete) — Splinter Review
Rebase to previous changes.  Address comment 20.
Attachment #819662 - Attachment is obsolete: true
Attachment #820907 - Flags: review?(gene.lian)
Reflect attribute name changes only.
Attachment #818554 - Attachment is obsolete: true
Attachment #820909 - Flags: review+
Comment on attachment 820902 [details] [diff] [review]
part 1/3: interface changes : v2

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

Thank you :)
Attachment #820902 - Flags: review?(htsai) → review+
Attachment #820902 - Flags: review?(gene.lian) → review+
Attachment #820907 - Flags: review?(gene.lian) → review+
Comment on attachment 820905 [details] [diff] [review]
part 2.a/3: refactor preference/settings key names

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

Nice!
Attachment #820905 - Flags: review?(gene.lian) → review+
Blocks: 928280
Rebase for bug 818353 was backed out and it actually depends on this bug.
Attachment #820905 - Attachment is obsolete: true
Attachment #821479 - Flags: review+
rebase only
Attachment #820907 - Attachment is obsolete: true
Attachment #821480 - Flags: review+
Blocks: 926343
You need to log in before you can comment on or make changes to this bug.