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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: philikon, Assigned: marshall_law)

Tracking

({dev-doc-needed})

unspecified
mozilla16
ARM
Gonk (Firefox OS)
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Hey Marshall, mind taking this? :D
Assignee: nobody → marshall
(Reporter)

Updated

5 years ago
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
(Assignee)

Comment 2

5 years ago
Yup! in fact, I've already done it in preparation for my patch for Bug #759637 :) I'll split out the patch here
(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
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
(Assignee)

Comment 4

5 years ago
Created attachment 631122 [details] [diff] [review]
MobileConnectionInfo.operator - v1
Attachment #631122 - Flags: superreview?(jonas)
Attachment #631122 - Flags: review?(philipp)
(Assignee)

Comment 5

5 years ago
Corresponding Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/1620
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 759637
Attachment #631122 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 7

5 years ago
Created attachment 631468 [details] [diff] [review]
MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v1

Updated to make the API naming more consistent
Attachment #631122 - Attachment is obsolete: true
Attachment #631122 - Flags: review?(philipp)
Attachment #631468 - Flags: superreview?(jonas)
Attachment #631468 - Flags: review?(philipp)
(Assignee)

Comment 8

5 years ago
Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/1633
Attachment #631468 - Flags: superreview?(jonas) → superreview+
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 }
Attachment #631468 - Flags: review?(philipp) → review+
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Created attachment 632337 [details] [diff] [review]
MobileOperatorInfo -> MobileNetworkInfo, MobileConnectionInfo.network - v2

Updated to fix nits, ready for checkin
Attachment #631468 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d93f4177aa6b
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla16
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/d93f4177aa6b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.