761482 - WebMobileConnection: make {voice|data}.operator an nsIDOMMozMobileOperatorInfo

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

This will allow us to read the MCC and MNC from content. Which is useful for all sorts of things, e.g. automatically configuring mobile network parameters.

Comment 1

[Philipp von Weitershausen [:philikon]]

Hey Marshall, mind taking this? :D

Comment 2

[Marshall Culpepper [:marshall_law]]

Yup! in fact, I've already done it in preparation for my patch for Bug #759637 :) I'll split out the patch here

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to Marshall Culpepper [:marshall_law] from comment #2)
> Yup! in fact, I've already done it in preparation for my patch for Bug
> #759637 :) I'll split out the patch here

<3

Comment 4

[Marshall Culpepper [:marshall_law]]

Attachment: MobileConnectionInfo.operator - v1

Comment 5

[Marshall Culpepper [:marshall_law]]

Corresponding Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/1620

Comment 6

[Marshall Culpepper [:marshall_law]]

One thing that has been bugging me is the inconsistency of nomenclature in the DOM API, i.e. "operator" vs "network".

This is mainly because of nomenclature differences in the Android RIL constant values, but those can easily be hidden as implementation details. Along with this change, I'm going to make the following DOM API updates:

rename MobileOperatorInfo -> MobileNetworkInfo
rename MobilleConnectionInfo.operator -> MobileConnectionInfo.network

With this change, getNetworks() will return an array of MobileNetworkInfo, which sounds much better :)

This will also pave the way for clearer names for the upcoming network selection APIs in Bug #759637

Comment 7

[Marshall Culpepper [:marshall_law]]

Attachment: MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v1

Updated to make the API naming more consistent

Comment 8

[Marshall Culpepper [:marshall_law]]

Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/1633

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 631468 [details] [diff] [review]
MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v1

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

Great patch! Only a few minor coding style nits.

::: dom/system/gonk/RILContentHelper.js
@@ +122,5 @@
>                                                   Ci.nsIRILContentHelper]}),
>  
> +  updateConnectionInfo: function updateConnectionInfo(srcInfo, destInfo) {
> +    for (let key in srcInfo) {
> +      if (key != 'network') {

Nit: double quotes

@@ +135,5 @@
> +    }
> +
> +    let network = destInfo.network;
> +    if (!network) {
> +        network = destInfo.network = new MobileNetworkInfo();

Nit: indent by 2 spaces

::: dom/system/gonk/ril_worker.js
@@ +2833,5 @@
> +      this.operator.longName != operator[0] ||
> +      this.operator.shortName != operator[1] ||
> +      this.operator.numeric != operator[2]) {
> +
> +    this.operator = {

If I was really picky, I would suggest normalizing this to:

  if (!this.operator) {
    this.operator = {type: "operatorchange"};
  }
  if (this.operator.longName != operator[0] ||
      this.operator.shortName != operator[1] ||
      this.operator.numeric != operator[2]) {
    this.operator.longName = ...
    ...
  }

but the operator will change so infrequently, it really doesn't matter.

@@ +2836,5 @@
> +
> +    this.operator = {
> +      longName: operator[0],
> +      shortName: operator[1],
> +      mcc: 0, mnc: 0,

Nit: put each field on its own line.

@@ +2845,5 @@
> +    try {
> +      this._processNetworkTuple(networkTuple, this.operator);
> +    } catch (e) {
> +      debug("Error processing operator tuple: " + e);
> +     }

Nit: align }

Comment 10

[Marshall Culpepper [:marshall_law]]

(In reply to Philipp von Weitershausen [:philikon] from comment #9)

> ::: dom/system/gonk/ril_worker.js
> @@ +2833,5 @@
> > +      this.operator.longName != operator[0] ||
> > +      this.operator.shortName != operator[1] ||
> > +      this.operator.numeric != operator[2]) {
> > +
> > +    this.operator = {
> 
> If I was really picky, I would suggest normalizing this to:
> 
>   if (!this.operator) {
>     this.operator = {type: "operatorchange"};
>   }
>   if (this.operator.longName != operator[0] ||
>       this.operator.shortName != operator[1] ||
>       this.operator.numeric != operator[2]) {
>     this.operator.longName = ...
>     ...
>   }
> 
> but the operator will change so infrequently, it really doesn't matter.

I'm actually glad you called this out, as "this.operator.numeric" is no longer used, but this.operator.mnc / this.operator.mcc are :) I've made your suggested changes as well as cleaned up the logic (and retested/rebased), and all is looking good.

Comment 11

[Marshall Culpepper [:marshall_law]]

Attachment: MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v2

Updated to fix nits, ready for checkin

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d93f4177aa6b

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d93f4177aa6b