Last Comment Bug 761482 - WebMobileConnection: make {voice|data}.operator an nsIDOMMozMobileOperatorInfo
: WebMobileConnection: make {voice|data}.operator an nsIDOMMozMobileOperatorInfo
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla16
Assigned To: Marshall Culpepper [:marshall_law]
:
Mentors:
Depends on:
Blocks: webmobileconnection 759637
  Show dependency treegraph
 
Reported: 2012-06-04 19:35 PDT by Philipp von Weitershausen [:philikon]
Modified: 2012-06-13 05:59 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
MobileConnectionInfo.operator - v1 (11.96 KB, patch)
2012-06-07 13:59 PDT, Marshall Culpepper [:marshall_law]
jonas: superreview+
Details | Diff | Splinter Review
MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v1 (15.78 KB, patch)
2012-06-08 11:00 PDT, Marshall Culpepper [:marshall_law]
philipp: review+
jonas: superreview+
Details | Diff | Splinter Review
MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v2 (16.02 KB, patch)
2012-06-12 11:29 PDT, Marshall Culpepper [:marshall_law]
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-06-04 19:35:10 PDT
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] 2012-06-04 19:35:41 PDT
Hey Marshall, mind taking this? :D
Comment 2 Marshall Culpepper [:marshall_law] 2012-06-05 08:09:57 PDT
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] 2012-06-05 13:19:43 PDT
(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] 2012-06-07 13:59:27 PDT
Created attachment 631122 [details] [diff] [review]
MobileConnectionInfo.operator - v1
Comment 5 Marshall Culpepper [:marshall_law] 2012-06-07 14:49:11 PDT
Corresponding Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/1620
Comment 6 Marshall Culpepper [:marshall_law] 2012-06-08 09:53:49 PDT
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] 2012-06-08 11:00:58 PDT
Created attachment 631468 [details] [diff] [review]
MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v1

Updated to make the API naming more consistent
Comment 8 Marshall Culpepper [:marshall_law] 2012-06-08 11:05:45 PDT
Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/1633
Comment 9 Philipp von Weitershausen [:philikon] 2012-06-11 15:56:37 PDT
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] 2012-06-12 11:25:03 PDT
(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] 2012-06-12 11:29:42 PDT
Created attachment 632337 [details] [diff] [review]
MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v2

Updated to fix nits, ready for checkin
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-06-12 14:10:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d93f4177aa6b
Comment 13 Ed Morley [:emorley] 2012-06-13 05:59:12 PDT
https://hg.mozilla.org/mozilla-central/rev/d93f4177aa6b

Note You need to log in before you can comment on or make changes to this bug.