887699 - B2G Network Stats: support multiple SIM

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Albert [:albert]]

Provide support to multiple SIMs in NetworkStats API.

The idea is to identify SIMs by the iccId, store stats with the id and provide a method to get stats by id.

Comment 1

[Albert [:albert]]

Attachment: Part 1/3: IDL changes (WIP)

Changes to support multiple SIM and some refactoring according to API 2.0 proposal

Comment 2

[Albert [:albert]]

Attachment: Part 1/3: IDL changes

Comment 3

[Albert [:albert]]

Attachment: Part 2/3 Multi SIM implementation

Comment 4

[Albert [:albert]]

Attachment: Part 3/3 Tests

Comment 5

[Marcelino Veiga Tuimil [:sonmarce]]

It seems we forgot to ask for koi? some time ago, as it is an important feature for 1.2, to avoid current limitations of data usage app, not being able to cover all SIM change scenarios, because of the lack of SIM support in Gecko. Not taking this patch implies not being able to fix some bugs detected in data usage app, and in other cases increasing complexity of data usage app for having to fix bugs in Gaia that should be solved in Gecko, so we need this bug to have a fully functional data usage app and much easier to maintain.

Comment 6

[Gene Lian (I already quit Mozilla)]

Will up the priority and review the patches in the Oslo work week.

Comment 7

[Gene Lian (I already quit Mozilla)]

Comment on attachment 773352 [details] [diff] [review]
Part 2/3 Multi SIM implementation

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

This is great! Just some nits. Honestly, I didn't go through all the details of each single line but the basic idea looks great to me. :)

::: dom/network/src/NetworkStatsDB.jsm
@@ +66,5 @@
>          objectStore.createIndex("txTotalBytes", "txTotalBytes", { unique: false });
>          if (DEBUG) {
>            debug("Created object stores and indexes");
>          }
> +      } else if (currVersion == 1) {

You probably need to rebase this since the per-app network metering is under way to check in.

@@ +282,4 @@
>        }
> +
> +      // If all samples for a connection are removed, an empty sample
> +      // have to be saved to keep the totalBytes in order to compute

s/have/has/

@@ +284,5 @@
> +      // If all samples for a connection are removed, an empty sample
> +      // have to be saved to keep the totalBytes in order to compute
> +      // future samples because system counters are not set to 0.
> +      // Thus, if there are no samples after remove, the last sample
> +      // removed is saved after setting bytes to 0.

s/after/by

::: dom/network/src/NetworkStatsManager.js
@@ +148,4 @@
>      this.checkPrivileges();
>  
> +    if (aStart.constructor.name !== "Date" ||
> +        aEnd.constructor.name !== "Date" ||

Is it possible to just use instanceof Date?

::: dom/network/src/NetworkStatsService.jsm
@@ +76,5 @@
> +    // mobile. On the other hand, the connection id is '0' for wifi or the
> +    // iccid for mobile (SIM). Each networkInterface is placed in the
> +    // _networks object at the attribute 'connectionId + connectionType'.
> +    //
> +    // _networks object allows to map available network interfaces at low level

s/allows/allowing

@@ +77,5 @@
> +    // iccid for mobile (SIM). Each networkInterface is placed in the
> +    // _networks object at the attribute 'connectionId + connectionType'.
> +    //
> +    // _networks object allows to map available network interfaces at low level
> +    // (wlan0, rmnet0, etc.) to a connection. Is not mandatory to have a

s/Is/It's

@@ +78,5 @@
> +    // _networks object at the attribute 'connectionId + connectionType'.
> +    //
> +    // _networks object allows to map available network interfaces at low level
> +    // (wlan0, rmnet0, etc.) to a connection. Is not mandatory to have a
> +    // networkInterface per connectionIface but can't exist a networkInterface not

s/but can't exist a/but it's not allowed to have a/

@@ +84,2 @@
>  
> +    this._networks = Object.create(null);

Can you just also some simple examples to describe the object literal? Like [1], so that the followers don't need to imagine that object by text descriptions.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/InterAppCommService.js#102

@@ +192,5 @@
> +   */
> +
> +  notifyIccInfoChanged: function notifyIccInfoChanged() {
> +    // While multi-SIM is not fully supported (bug-799023) lets assume
> +    // that only one SiM is supported without reboot.

s/SiM/SIM

@@ +194,5 @@
> +  notifyIccInfoChanged: function notifyIccInfoChanged() {
> +    // While multi-SIM is not fully supported (bug-799023) lets assume
> +    // that only one SiM is supported without reboot.
> +
> +    // Listening to icc info changes allow to know the available connections

s/allow/allows

@@ +202,5 @@
> +    if (iccid == null) {
> +      return;
> +    }
> +
> +    let netId = this.getNetworkId(iccid, NET_TYPE_MOBILE); 

nit: tailing space.

@@ +259,5 @@
> +    let netId = this.getNetworkId(id, aNetwork.type);
> +
> +    if (!this._networks[netId]) {
> +      this._networks[netId] = Object.create(null);
> +      this._networks[netId].connection = { id: id, 

nit: TS.

@@ +314,5 @@
> +      return;
> +    }
> +
> +    this._db.clearInterface(connection, function onDBCleared(aError, aResult) {
> +        mm.sendAsyncMessage("NetworkStats:Clear:Return",

nit: 2-space alignment.

@@ +339,5 @@
>      // else it is pushed in 'elements' array, which later will be pushed to
>      // the queue array.
> +    for (let netId in this._networks) {
> +      lastElement = { netId: netId,
> +                      queueIndex: this.updateQueueIndex(netId)};

nit: add one space before the last '}'.

@@ +345,2 @@
>        if (lastElement.queueIndex == -1) {
> +        elements.push({netId: lastElement.netId, callbacks: []});

nit: { netId: ... } (adding 2 spaces).

@@ +476,5 @@
> +    let stats = { connectionId:   this._networks[aNetId].connection.id,
> +                  connectionType: this._networks[aNetId].connection.type,
> +                  date:           aDate,
> +                  rxBytes:        aTxBytes,
> +                  txBytes:        aRxBytes};

nit: add one space before the last '}'.

@@ +481,5 @@
>  
> +    debug("Update stats for: " + JSON.stringify(stats));
> +
> +    this._db.saveStats(stats, function onSavedStats(aError, aResult) {
> +      if (aCallback) {

Please use early return:

if (!aCallback) {
  return;
}

...

Comment 8

[Gene Lian (I already quit Mozilla)]

Comment on attachment 773354 [details] [diff] [review]
Part 3/3 Tests

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

Looks good to me!

::: dom/network/tests/test_networkstats_basics.html
@@ +70,5 @@
>  
>    ok(success, "data result has correct dates");
>  }
>  
> +function checkNetowrkInterfaces(networkInterfaceA, networkInterfaceB) {

s/checkNetowrkInterfaces/compareNetowrkInterfaces

@@ +108,5 @@
>        next();
>        return;
>      }
>  
> +    ok(false, "getSamples launch exceptionwhen start is greater than end");

s/exceptionwhen/exception when/

@@ +193,3 @@
>      req.onsuccess = function () {
>        ok(true, "Get stats request ok");
> +      ok(checkNetowrkInterfaces(req.result.networkInterface, networkInterface), "networkInterfaces should be equals");

s/equals/equal/

Comment 9

[Preeti Raghunath(:Preeti)]

koi+ for landing.

Comment 10

[Marcelino Veiga Tuimil [:sonmarce]]

This bug implies a refactor of Cost Control app, that was scheduled for version 1.2. But after analyzing the effort needed, we think it is too late to have all code landed and tested on time. So, we think the best option is to lower the priority and postpone the development for version 1.3.

Comment 11

[Albert [:albert]]

Attachment: Part 1/3: IDL changes

Updated nsINetworkStatsServiceProxy IDL.

Comment 12

[Albert [:albert]]

Attachment: Part 2/3 Multi SIM implementation

Merge with stats per app feature of bug 784575

Comment 13

[Albert [:albert]]

Attachment: Part 3/3 Tests

Updated tests.

Comment 14

[John Shih]

Comment on attachment 811055 [details] [diff] [review]
Part 1/3: IDL changes

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

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +32,5 @@
> +   * Find method get samples with userTimestamp between start and end,
> +   * both included.
> +   *
> +   * If appURL is provided, Find method will retrieve per-app usage, 
> +   * otherwise the target will be system usage.

I think manifestURL or app's manifestURL would be more correct than appURL here :)

If I'm not wrong, this modification is base on the previously proposal of NetworkstatsAPI 2.0[1], right?

[1] https://developer.mozilla.org/en-US/docs/WebAPI/Network_Stats_2_0_proposal

Comment 15

[John Shih]

Comment on attachment 811058 [details] [diff] [review]
Part 2/3 Multi SIM implementation

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

::: dom/network/src/NetworkStatsService.jsm
@@ +512,5 @@
>    /*
>     * Function responsible for receiving per-app stats.
>     */
> +  saveAppStats: function saveAppStats(aAppId, aNetworkInterface, aTimeStamp, aRxBytes, aTxBytes, aCallback) {
> +    let netId = this.getNetworkId(aNetworkInterface.id, aNetworkInterface.type);

so this change make |aConnectionType| change to the |aNetworkInterface| which is a js object. Because there are some c++ code will call this method, is there any way to create the variable corresponding to js object in c++ code?

Comment 16

[Albert [:albert]]

(In reply to John Shih from comment #15)
> Comment on attachment 811058 [details] [diff] [review]
> Part 2/3 Multi SIM implementation
> 
> Review of attachment 811058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsService.jsm
> @@ +512,5 @@
> >    /*
> >     * Function responsible for receiving per-app stats.
> >     */
> > +  saveAppStats: function saveAppStats(aAppId, aNetworkInterface, aTimeStamp, aRxBytes, aTxBytes, aCallback) {
> > +    let netId = this.getNetworkId(aNetworkInterface.id, aNetworkInterface.type);
> 
> so this change make |aConnectionType| change to the |aNetworkInterface|
> which is a js object. Because there are some c++ code will call this method,
> is there any way to create the variable corresponding to js object in c++
> code?

I'm still thinking about it, after see how you use it in the websocket channel. Maybe will be better if you can call saveAppStats passing the nsINetworkInterface that you get with the nsINetworkManager? In that case, then the NetworkStatsServiceProxy can translate that nsINetorkManager into the object used in the NetworkStatsService.

What do you think?

Comment 17

[John Shih]

(In reply to Albert [:albert] from comment #16)
> (In reply to John Shih from comment #15)
> > Comment on attachment 811058 [details] [diff] [review]
> > Part 2/3 Multi SIM implementation
> > 
> > Review of attachment 811058 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/network/src/NetworkStatsService.jsm
> > @@ +512,5 @@
> > >    /*
> > >     * Function responsible for receiving per-app stats.
> > >     */
> > > +  saveAppStats: function saveAppStats(aAppId, aNetworkInterface, aTimeStamp, aRxBytes, aTxBytes, aCallback) {
> > > +    let netId = this.getNetworkId(aNetworkInterface.id, aNetworkInterface.type);
> > 
> > so this change make |aConnectionType| change to the |aNetworkInterface|
> > which is a js object. Because there are some c++ code will call this method,
> > is there any way to create the variable corresponding to js object in c++
> > code?
> 
> I'm still thinking about it, after see how you use it in the websocket
> channel. Maybe will be better if you can call saveAppStats passing the
> nsINetworkInterface that you get with the nsINetworkManager? In that case,
> then the NetworkStatsServiceProxy can translate that nsINetorkManager into
> the object used in the NetworkStatsService.
> 
> What do you think?

sounds good. This might be one of the solutions (though I don't have idea right now).
Let's see if Gene has any feedback on it

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 811055 [details] [diff] [review]
Part 1/3: IDL changes

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

This this looks good to Hsin-Yi, then this is good with me.

Comment 19

[Albert [:albert]]

Attachment: Fix websocket connection to NetworkStats API (WIP)

Comment 20

[Albert [:albert]]

Removed review flags until we decide how to communicate channels with networkstatsserviceproxy

Comment 21

[Gene Lian (I already quit Mozilla)]

Comment on attachment 811058 [details] [diff] [review]
Part 2/3 Multi SIM implementation

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

::: dom/network/src/NetworkStatsService.jsm
@@ +114,5 @@
>      this.cachedAppStatsDate = new Date();
>  
>      this.updateQueue = [];
>      this.isQueueRunning = false;
> +    mobileConnection.registerMobileConnectionMsg(this);

Sorry, this is deprecated. We've already moved the icc stuff out of mobileConnection. You have to use nsIIccProvider.registerIccMsg(this) from now on.

However, even so, I'm still wondering why do you want to use nsIIccProvider (sorry I didn't point this out in the first review). nsIIccProvider is supposed to be used in the content process in general. Can you just use nsIRadioInterfaceLayer to get the RIL service and use:

gRadioInterface.rilContext.iccInfo;

to get the ICC ID (please refer to the MmsService.js for some examples) to meter the traffic? For the scenario of single SIM card, this should be enough to go.

Comment 22

[Gene Lian (I already quit Mozilla)]

Comment on attachment 811923 [details] [diff] [review]
Fix websocket connection to NetworkStats API (WIP)

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

Looks good to me!

Comment 23

[John Shih]

Comment on attachment 811923 [details] [diff] [review]
Fix websocket connection to NetworkStats API (WIP)

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

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1081,5 @@
>    }
>  
>    // obtain active connection type
>    if (mAppId != NECKO_NO_APP_ID) {
> +    GetNetworkInterface(mNetworkInterface);

here also needs some kind of ifdef corresponding to the comment in .h

@@ +3280,5 @@
>    nsresult result;
>    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &result);
>  
>    if (NS_FAILED(result) || !networkManager) {
> +    networkInterface = nullptr;

There should have a return that I forget to add in my patch, can you help on carrying that fix also? Thanks!

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +21,1 @@
>  

since you move this header to here, you should also add the ifdef MOZ_WIDGET in order to avoid the cross-platform issue.

@@ +260,5 @@
>  private:
>    uint64_t                        mCountRecv;
>    uint64_t                        mCountSent;
>    uint32_t                        mAppId;
> +  nsCOMPtr<nsINetworkInterface>   mNetworkInterface;

I think this one also need ifdef MOZ_WIDGET

@@ +265,2 @@
>    bool                            mIsInBrowser;
> +  nsresult                        GetNetworkInterface(nsINetworkInterface *);

ditto, I think ;)

Comment 24

[Albert [:albert]]

(In reply to Gene Lian [:gene] from comment #21)
> Comment on attachment 811058 [details] [diff] [review]
> Part 2/3 Multi SIM implementation
> 
> Review of attachment 811058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsService.jsm
> @@ +114,5 @@
> >      this.cachedAppStatsDate = new Date();
> >  
> >      this.updateQueue = [];
> >      this.isQueueRunning = false;
> > +    mobileConnection.registerMobileConnectionMsg(this);
> 
> Sorry, this is deprecated. We've already moved the icc stuff out of
> mobileConnection. You have to use nsIIccProvider.registerIccMsg(this) from
> now on.
> 
> However, even so, I'm still wondering why do you want to use nsIIccProvider
> (sorry I didn't point this out in the first review). nsIIccProvider is
> supposed to be used in the content process in general. Can you just use
> nsIRadioInterfaceLayer to get the RIL service and use:
> 
> gRadioInterface.rilContext.iccInfo;
> 
> to get the ICC ID (please refer to the MmsService.js for some examples) to
> meter the traffic? For the scenario of single SIM card, this should be
> enough to go.

I used registerIccMsg in order to be able to track iccId changes, if the samplerate is daily and there is a iccId change at the half of the samplerate some data will be added to an incorrect iccId. However with unagi I can't change the SIM without shutting down the phone. And also I can know if there are two iccIds at the same time.

Do you prefer to assume that only one iccid can be enabled?

Comment 25

[Albert [:albert]]

Attachment: Part 1/4 IDL changes

Comment 26

[Albert [:albert]]

Attachment: Part 2/4 Multi SIM implementation

Comment 27

[Albert [:albert]]

Attachment: Part 3/4 Fix access of websocket channel to NetworkStats API

Comment 28

[Albert [:albert]]

Attachment: Part 4/4 Tests

Comment 29

[John Shih]

Comment on attachment 812542 [details] [diff] [review]
Part 3/4 Fix access of websocket channel to NetworkStats API

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

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +3277,2 @@
>  nsresult
> +WebSocketChannel::GetNetworkInterface(nsINetworkInterface *networkInterface)

can you also do the change refer to what I did in bug 746073?
That is, make this function responsible for the mNetworkInterface directly instead of passing variable.

And because of this patch is only necko-related, so I think you may need ask the right person for review (ex. patrick or honzab)

Nice work!

Thanks ;)

Comment 30

[Albert [:albert]]

Attachment: Part 3/4 Fix access of websocket channel to NetworkStats API

Changes from comment 29

Comment 31

[Jason Duell]

Comment on attachment 812533 [details] [diff] [review]
Part 1/4 IDL changes

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

looks fine to me, but I'm not a peer for this code AFAIK.  I'm not sure who is, either--is it just the regular DOM peers?  I can't tell from 

  https://wiki.mozilla.org/Modules/Core

or hg log whether htsai is a peer, for instance.

Comment 32

[Jason Duell]

peterv: who's allowed to review code in dom/networking?

Comment 33

[Jason Duell]

Comment on attachment 812574 [details] [diff] [review]
Part 3/4 Fix access of websocket channel to NetworkStats API

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

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +3271,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +WebSocketChannel::GetNetworkInterface()

IIRC this method is copied in many places now (websockets, HTTP, etc) with essentially the same code.  We should centralize the implementation somewhere.  Either nsNetUtil.h or NeckoCommon.h/.cpp would work.

I'm fine doing that in a followup--we should start landing your patches :)

@@ +3297,5 @@
>  WebSocketChannel::SaveNetworkStats(bool enforce)
>  {
>  #ifdef MOZ_WIDGET_GONK
>    // Check if the connection type and app id are valid.
> +  if(!mNetworkInterface || mAppId == NECKO_NO_APP_ID) {

So here and in the HTTP patches too, you're not recording traffic with id=NO_APP.  But this includes any networking the phone does that's not from an app (like updates, getting phishing lists, etc.).  Since your nsIDOMMozNetworkStatsManager.getSample method says "If appURL is provided, Find method will retrieve per-app usage, otherwise the target will be system usage", I'm guessing you want to actually keep track of NO_APP traffic.  It will be a more accurate measure of actual system data traffic if you do.

I'm fine with doing that work in a followup bug.

Comment 34

[Honza Bambas (:mayhemer)]

Thanks Jason...

Comment 35

[John Shih]

I've filed the bug 922924 of centralizing the code for network metering as the following bug.

Comment 36

[Gene Lian (I already quit Mozilla)]

Comment on attachment 812533 [details] [diff] [review]
Part 1/4 IDL changes

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

Overall, if you want to use nsIDOMMozNetworkStatsInterface, the attribute name should be networkStatsInterface instead of networkInterface which will be easily misunderstood as an instance of nsINetworkInterface.

Steal the review. :) Please ask for hsinyi's review (maybe plus me) again with a better naming.

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +28,2 @@
>     */
> +  readonly attribute nsIDOMMozNetworkStatsInterface networkInterface;

Please see the general comment.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +6,5 @@
>  
>  interface nsIDOMDOMRequest;
>  
> +/**
> + * Represents a data interface for which the manager is recording statistics.

Please drop 'the'.

@@ +14,3 @@
>  {
> +  readonly attribute long type;
> +  readonly attribute DOMString id;

It seems that the id is actually iccId. Can you id/iccId to make it more specific? Or do you have any other concerns? Say, the id could be something different in the future?

@@ +31,5 @@
> +   *
> +   * Find method get samples with userTimestamp between start and end,
> +   * both included.
> +   *
> +   * If appURL is provided, Find method will retrieve per-app usage, 

TS.

s/appURL/manifestURL

@@ +39,2 @@
>     */
> +  nsIDOMDOMRequest getSamples(in nsIDOMMozNetworkStatsInterface networkInterface,

Please see the general comment.

@@ +47,2 @@
>     */
> +  nsIDOMDOMRequest clearStats(in nsIDOMMozNetworkStatsInterface networkInterface);

Please see the general comment.

Comment 37

[Gene Lian (I already quit Mozilla)]

After some thoughts, I think s/networkInterface/network is better. Just like nsINetworkManager.idl for example.

|nsIDOMMozNetworkStatsInterface| stands for an interface and |network| stands for an instance.

Also, s/availableInterfaces/availableNetworks.

Comment 38

[Gene Lian (I already quit Mozilla)]

Comment on attachment 812541 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

Just like mentioned above, can you s/networkInterface/network for the attribute/parameter naming? People will easily misunderstand networkInterface as an instance of nsINetworkInterface which is actually different from the nsIDOMNetworkStatsInterface.

I'd like to give review- for now because of Bug 855951 which has used saveAppStats(...) you're hoping to change, unfortunately. :( Please rebase for that.

Btw, the following reviews might not go though every single lines to change the attribute names. I just glance through them and pick some of them. Please be careful to apply the naming changes to everywhere needed. Thanks!

::: dom/network/src/NetworkStatsDB.jsm
@@ +15,5 @@
>  Cu.import("resource://gre/modules/IndexedDBHelper.jsm");
>  
>  const DB_NAME = "net_stats";
>  const DB_VERSION = 2;
> +const STORE_NAME = "net_stats";

OK. Since you have to delete the whole DB, you can directly assign the STORE_NAME.

@@ +92,5 @@
>        } else if (currVersion == 1) {
>          // In order to support per-app traffic data storage, the original
>          // objectStore needs to be replaced by a new objectStore with new
>          // key path ("appId") and new index ("appId").
> +        // Also, since now connections are identified by their conenctionId and not

I think it should be:

"are identified by their [conenctionId, connectionType] not just by connectionType"

Right?

@@ +345,4 @@
>        }
> +
> +      // If all samples for a connection are removed, an empty sample
> +      // have to be saved to keep the totalBytes in order to compute

s/have/has

@@ +347,5 @@
> +      // If all samples for a connection are removed, an empty sample
> +      // have to be saved to keep the totalBytes in order to compute
> +      // future samples because system counters are not set to 0.
> +      // Thus, if there are no samples after remove, the last sample
> +      // removed is saved after setting bytes to 0.

If you don't mind let's rewrite the last 2 lines to:

// Thus, if there are no samples left, the last sample removed
// will be saved again after setting its bytes to 0.

@@ +365,3 @@
>    },
>  
> +  clearInterface: function clearInterface(aConnection, aResultCb) {

s/clearInterface/clearInterfaceStats
s/aConnection/aNetwork

@@ +367,5 @@
> +  clearInterface: function clearInterface(aConnection, aResultCb) {
> +    let id = [aConnection.id, aConnection.type];
> +    let self = this;
> +
> +    // Clear and save empty sample to keep sync with system counters

s/empty sample/an empty sample

@@ +391,5 @@
> +      };
> +     }, aResultCb);
> +   },
> +
> +  clear: function clear(aConnections, aResultCb) {

s/clear/clearStats
s/aConnections/aNetworks

I'm also wondering if you can call .clearInterface(...) within this function. Well, it's not really a need. Just hoping we can have less duplicated codes.

Btw, please add some comments about the purpose and the logic of this function, like how you recursively collect the stats array and save them at one shot in the end. It took me some time to figure out these codes.

@@ +434,4 @@
>      }, aResultCb);
>    },
>  
> +  find: function find(aResultCb, aConnection, aStart, aEnd,  aAppId, aManifestURL) {

a/aConnection/aNetwork

@@ +472,5 @@
>          // now - VALUES_MAX_LENGTH, fill with empty samples.
>          this.fillResultSamples(start + offset, end + offset, data);
>  
> +        aTxn.result.manifestURL = aManifestURL;
> +        aTxn.result.networkInterface = aConnection;

s/networkInterface/network

::: dom/network/src/NetworkStatsManager.js
@@ +64,5 @@
> +  if (DEBUG) {
> +    debug("NetworkStatsInterface Constructor");
> +  }
> +  this.type = aNetworkInterface.type;
> +  this.id = aNetworkInterface.id;

Please see the IDL comment. I think iccId is better.

@@ +93,5 @@
>    if (DEBUG) {
>      debug("NetworkStats Constructor");
>    }
>    this.manifestURL = aStats.manifestURL || null;
> +  this.networkInterface = new NetworkStatsInterface(aStats.networkInterface);

s/networkInterface/network

@@ +106,5 @@
>  
>  NetworkStats.prototype = {
>    __exposedProps__: {
>                        manifestURL: 'r',
> +                      networkInterface: 'r',

s/networkInterface/network

@@ +145,5 @@
>        throw Components.Exception("Permission denied", Cr.NS_ERROR_FAILURE);
>      }
>    },
>  
> +  getSamples: function getSamples(aNetworkInterface, aStart, aEnd, aManifestURL) {

s/aNetworkInterface/aNetwork

@@ +156,5 @@
>      }
>  
>      let request = this.createRequest();
>      cpmm.sendAsyncMessage("NetworkStats:Get",
> +                          { networkInterface: aNetworkInterface,

s/networkInterface/network
s/aNetworkInterface/aNetwork

@@ +165,4 @@
>      return request;
>    },
>  
> +  clearStats: function clearStats(aNetworkInterface) {

s/aNetworkInterface/aNetwork

@@ +169,5 @@
>      this.checkPrivileges();
>  
>      let request = this.createRequest();
>      cpmm.sendAsyncMessage("NetworkStats:Clear",
> +                          { networkInterface: aNetworkInterface,

s/networkInterface/network
s/aNetworkInterface/aNetwork

@@ +183,5 @@
>                            {id: this.getRequestId(request)});
>      return request;
>    },
>  
> +  get availableInterfaces() {

s/availableInterfaces/availableNetworks

@@ +188,3 @@
>      this.checkPrivileges();
> +
> +    let result = ObjectWrapper.wrap(cpmm.sendSyncMessage("NetworkStats:NetworkInterfaces")[0], this._window);

s/NetworkStats:NetworkInterfaces/NetworkStats:Networks

@@ +188,4 @@
>      this.checkPrivileges();
> +
> +    let result = ObjectWrapper.wrap(cpmm.sendSyncMessage("NetworkStats:NetworkInterfaces")[0], this._window);
> +    let interfaces = this.data = Cu.createArrayIn(this._window);

s/interfaces/networks

::: dom/network/src/NetworkStatsService.jsm
@@ +52,5 @@
> +                                   "nsISettingsService");
> +
> +XPCOMUtils.defineLazyGetter(this, "gRadioInterface", function () {
> +  let ril = Cc["@mozilla.org/ril;1"].getService(Ci["nsIRadioInterfaceLayer"]);
> +  // TODO: Bug 854326 - B2G Multi-SIM: support multiple SIM cards for SMS/MMS

Please file a new bug for network metering.

// TODO: Bug XXXXXX - B2G Multi-SIM: support multiple SIM cards for network metering.

@@ +78,5 @@
> +    // iccid for mobile (SIM). Each networkInterface is placed in the
> +    // _networks object at the attribute 'connectionId + connectionType'.
> +    //
> +    // _networks object allows to map available network interfaces at low level
> +    // (wlan0, rmnet0, etc.) to a connection. Is not mandatory to have a

s/Is/It's

@@ +87,5 @@
> +
> +    // There is no way to know a priori if wifi connection is available,
> +    // just when the wifi driver is loaded, but it is unloaded when
> +    // wifi is switched off. So wifi connection is hardcoded
> +    let netId = this.getNetworkId(0, NET_TYPE_WIFI);

s/0/'0'?

@@ +95,5 @@
>  
>      this.messages = ["NetworkStats:Get",
>                       "NetworkStats:Clear",
> +                     "NetworkStats:ClearAll",
> +                     "NetworkStats:NetworkInterfaces",

s/NetworkStats:NetworkInterfaces/NetworkStats:Networks

@@ +140,3 @@
>          this.clearDB(mm, msg);
>          break;
> +      case "NetworkStats:NetworkInterfaces":

s/NetworkStats:NetworkInterfaces/NetworkStats:Networks

@@ +201,5 @@
>    /*
> +   * nsINetworkStatsService
> +   */
> +
> +  convertNetworkInterface: function(aNetworkInterface) {

s/aNetworkInterface/aNetwork

@@ +212,5 @@
> +    if (aNetworkInterface.type == NET_TYPE_MOBILE) {
> +      // Bug 904542 will provide the serviceId to map the iccId with the
> +      // nsINetworkInterface of the NetworkManager. Now, lets assume that
> +      // network is mapped with the current iccId.
> +      id = gRadioInterface.rilContext.iccInfo.iccid;

Yes, that's enough for now. I actually think the bug title should be changed to:

B2G Network Stats: add ICC ID to do network metering

We'll really solve the multi-sim issue at

// TODO: Bug XXXXXX - B2G Multi-SIM: support multiple SIM cards for network metering.

Btw, don't care too much about the scenario of reinstalling SIM while power is on. Or at least, it should be a separate bug.

@@ +219,5 @@
> +    let netId = this.getNetworkId(id, aNetworkInterface.type);
> +
> +    if (!this._networks[netId]) {
> +      this._networks[netId] = Object.create(null);
> +      this._networks[netId].connection = { id: id,

Let's s/connection/network, please, if it doesn't bother you.

@@ +223,5 @@
> +      this._networks[netId].connection = { id: id,
> +                                           type: aNetworkInterface.type };
> +    }
> +
> +    this._networks[netId].networkInterface = aNetworkInterface.name;

The naming is great here! |networkInterface| means something like an abstract interface (wlan0, rmnet0, etc.). We should even rename it to |interface| or |interfaceName| since it's under _networks[netId]. I'd prefer |interfaceName|.

@@ +227,5 @@
> +    this._networks[netId].networkInterface = aNetworkInterface.name;
> +    return netId;
> +  },
> +
> +  getNetworkId: function getNetworkId(aConnectionId, aNetworkType) {

s/aConnectionId/aIccId

@@ +231,5 @@
> +  getNetworkId: function getNetworkId(aConnectionId, aNetworkType) {
> +    return aConnectionId + '' + aNetworkType;
> +  },
> +
> +  availableConnections: function availableConnections() {

s/availableConnections/availableNetworks

@@ +250,5 @@
>     * it retrieve them from database and return to the manager.
>     */
> +  getSamples: function getSamples(mm, msg) {
> +    let self = this;
> +    let connection = msg.networkInterface;

s/connection/network
s/networkInterface/network

@@ +287,5 @@
> +    });
> +  },
> +
> +  clearInterfaceStats: function clearInterfaceStats(mm, msg) {
> +    let connection = msg.networkInterface;

s/connection/network
s/networkInterface/network

@@ +301,2 @@
>  
> +    this._db.clearInterface(connection, function onDBCleared(aError, aResult) {

s/clearInterface/clearInterfaceStats

You're not clearing an interface. Instead, the stats of interface. :)

@@ +306,4 @@
>    },
>  
>    clearDB: function clearDB(mm, msg) {
> +    let connections = this.availableConnections();

s/connections/networks
s/availableConnections/availableNetworks

@@ +431,5 @@
>        }
>        return;
>      }
>  
> +    let networkName = this._networks[aNetId].networkInterface;

s/networkInterface/interfaceName
s/networkName/interfaceName

if you decide to do it above.

@@ +462,5 @@
>      }
>  
>      let stats = { appId:          0,
> +                  connectionId:   this._networks[aNetId].connection.id,
> +                  connectionType: this._networks[aNetId].connection.type,

s/connection/network

@@ +484,5 @@
>  
>    /*
>     * Function responsible for receiving per-app stats.
>     */
> +  saveAppStats: function saveAppStats(aAppId, aNetworkInterface, aTimeStamp, aRxBytes, aTxBytes, aCallback) {

s/aNetworkInterface/aNetwork

@@ +504,5 @@
>      }
>  
>      let stats = { appId: aAppId,
> +                  connectionId: this._networks[netId].connection.id,
> +                  connectionType: this._networks[netId].connection.type,

s/connection/network for these two lines.

::: dom/network/src/NetworkStatsServiceProxy.js
@@ +28,5 @@
>    /*
>     * Function called in the protocol layer (HTTP, FTP, WebSocket ...etc)
>     * to pass the per-app stats to NetworkStatsService.
>     */
> +  saveAppStats: function saveAppStats(aAppId, aNetworkInterface, aTimeStamp,

s/aNetworkInterface/aNetwork

Comment 39

[Peter Van der Beken [:peterv]]

(In reply to Jason Duell (:jduell) from comment #32)
> peterv: who's allowed to review code in dom/networking?

DOM peers afaik. Seems both Mounir and Sicking either wrote or touched code in there.

Comment 40

[Albert [:albert]]

(In reply to Gene Lian [:gene] from comment #36)
> Comment on attachment 812533 [details] [diff] [review]
> Part 1/4 IDL changes
> 
> Review of attachment 812533 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +14,3 @@
> >  {
> > +  readonly attribute long type;
> > +  readonly attribute DOMString id;
> 
> It seems that the id is actually iccId. Can you id/iccId to make it more
> specific? Or do you have any other concerns? Say, the id could be something
> different in the future?

The id is a concatenation of the network type and the iccid (0 for wifi). We discussed if the iccid should be encrypted in the future, but not now.

Comment 41

[Albert [:albert]]

Attachment: Part 1/4 IDL changes

Changes from comment 36

Comment 42

[Albert [:albert]]

Attachment: Part 2/4 Multi SIM implementation

Changes from comment 38

Comment 43

[Albert [:albert]]

Attachment: Part 4/4 Tests

Tests updated

Comment 44

[Gene Lian (I already quit Mozilla)]

Comment on attachment 813562 [details] [diff] [review]
Part 1/4 IDL changes

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

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +23,5 @@
>     */
>    readonly attribute DOMString    manifestURL;
>  
>    /**
> +   * Interface for which the returned data belongs to.

s/Interface/Netowrk/

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +17,4 @@
>  };
>  
> +
> +[scriptable,  uuid(5fbdcae6-a2cd-47b3-929f-83ac75bd4881)]

Nit: one space only before uuid.

@@ +31,5 @@
> +   *
> +   * Find method get samples with userTimestamp between start and end,
> +   * both included.
> +   *
> +   * If manifestURL is provided, Find method will retrieve per-app usage, 

Nit: trailing white space.

@@ +44,3 @@
>  
>    /**
> +   * Remove all stats related with the provided interface from DB.

s/interface/network

@@ +54,3 @@
>  
>    /**
> +   * Return currently available interfaces.

s/interfaces/networks

@@ +59,5 @@
> +
> +  /**
> +   * Minimum time in milliseconds between samples stored in the database.
> +   */
> +  readonly attribute long  sampleRate;

Nit: one space only before |sampleRate|.

Comment 45

[Gene Lian (I already quit Mozilla)]

Comment on attachment 813564 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

Thank for refining all the naming stuff, Albert! :)

However, I still don't see the rebase for Bug 855951 that I mentioned at the top of comment #38. Giving review- for now because this patch will break the TCP Socket API functions for per-app network metering although the rest of the patch looks pretty nice to me.

::: dom/network/src/NetworkStatsDB.jsm
@@ +74,5 @@
> +        // Also, since now networks are identified by their
> +        // [networkId, networkType] not just by their connectionType,
> +        // to modify the keyPath is mandatory to delete the object store
> +        // and create it again. Old data is going to be deleted because the
> +        // connectionId for each sample can not be set.

s/connectionId/networkId

@@ +396,5 @@
> +    if (!aNetworks[index]) {
> +      aResultCb(null, true);
> +      return;
> +    }
> +    this.clearInterfaceStats(aNetworks[index], callback);

Awesome! Thanks!

@@ +401,3 @@
>    },
>  
> +  find: function find(aResultCb, aNetwork, aStart, aEnd,  aAppId, aManifestURL) {

nit: one space only before aAppId.

::: dom/network/src/NetworkStatsService.jsm
@@ +67,5 @@
> +    // by a network object (network type and network Id) and a interfaceName
> +    // that contains the name of the physical interface (wlan0, rmnet0, etc.).
> +    // The network type can be 0 for wifi or 1 for mobile. On the other hand,
> +    // the network id is '0' for wifi or the iccid for mobile (SIM).
> +    // Each networkInterface is placed in the _networks object at the attribute

s/at the attribute/by the index of/

@@ +72,5 @@
> +    // 'networkId + networkType'.
> +    //
> +    // _networks object allows to map available network interfaces at low level
> +    // (wlan0, rmnet0, etc.) to a network. It's not mandatory to have a
> +    // networkInterface per connectionIface but can't exist a networkInterface not

nit: change connectionIface to something better? I kind of lost the correct name here.

@@ +203,5 @@
> +    let id = '0';
> +    if (aNetwork.type == NET_TYPE_MOBILE) {
> +      // Bug 904542 will provide the serviceId to map the iccId with the
> +      // nsINetworkInterface of the NetworkManager. Now, lets assume that
> +      // network is mapped with the current iccId.

... the current iccId of the single SIM.

Comment 46

[Gene Lian (I already quit Mozilla)]

Comment on attachment 813566 [details] [diff] [review]
Part 4/4 Tests

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

::: dom/network/tests/test_networkstats_basics.html
@@ +59,5 @@
>  
>    ok(success, "data result has correct dates");
>  }
>  
> +function checkNetowrks(networkA, networkB) {

nit: s/checkNetworks/compareNetworks/ and for all the points calling this.

@@ +97,5 @@
>        next();
>        return;
>      }
>  
> +    ok(false, "getSamples launch exceptionwhen start is greater than end");

nit: add a space between "exception" and "when".

@@ +141,5 @@
> +    }
> +
> +    ok(false, "getSamples launch exception when start param is not a Date");
> +  },
> +   function () {

nit: align function to the above "}".

::: dom/network/tests/test_networkstats_enabled_no_perm.html
@@ +22,5 @@
>  
>  var error;
>  try {
> +  navigator.mozNetworkStats.availableInterfaces;
> +  ok(false, "Accessing navigator.mozNetworkStats.availableInterfaces should have thrown!");

s/availableInterfaces/availableNetworks/?

@@ +27,4 @@
>  } catch (ex) {
>    error = ex;
>  }
> +ok(error, "Got an exception accessing navigator.mozNetworkStats.availableInterfaces");

Ditto.

::: dom/network/tests/unit_stats/test_networkstats_db.js
@@ +398,5 @@
>        do_check_eq(error, null);
>        netStatsDb.logAllRecords(function(error, result) {
>          do_check_eq(error, null);
> +        // The clear function clears all records of the datbase but
> +        // insert a new element for each [appId, connectionId, connectionType]

nit: s/insert/inserts/

@@ +549,5 @@
>          if (index == keys.length - 1) {
>            netStatsDb.logAllRecords(function(error, result) {
> +          // Again, result has two samples more than expected samples because
> +          // clear inserts one empty sample for each network to keep totalBytes
> +          // synchronized with netd counters. so the two first sample have

nit: s/two first sample/first two samples/

@@ +550,5 @@
>            netStatsDb.logAllRecords(function(error, result) {
> +          // Again, result has two samples more than expected samples because
> +          // clear inserts one empty sample for each network to keep totalBytes
> +          // synchronized with netd counters. so the two first sample have
> +          // to be discarted.

nit: have to

::: dom/network/tests/unit_stats/test_networkstats_service_proxy.js
@@ +38,5 @@
>    var timestamp = NetworkStatsService.cachedAppStatsDate.getTime();
>    var samples = 5;
>  
> +  var wifi = {type: Ci.nsINetworkInterface.NETWORK_TYPE_WIFI, id: "0"};
> +  var mobile = {type: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE, id: "1234"};

Hope to add some comments before these two vars. Please see below.

@@ +47,4 @@
>    do_check_eq(Object.keys(cachedAppStats).length, 0);
>  
>    for (var i = 0; i < samples; i++) {
> +    nssProxy.saveAppStats(1, wifi, timestamp, 10, 20);

Are you sure this is right? The second parameter should be nsINetworkInterface instead of nsINetworkStatsInterface. Please let me know if misunderstood anything. Thanks!

-----
OK... I see. We just use the nsINetworkInterface.type in the .saveAppStats(...). The .id is just for testing. A bit confusing but it's fine. Can you add some comments like below?

// Making nsINetworkInterface instances where the nsINetworkInterface.type will be used in the .saveAppStats(...).
var wifi = ...
var mobile = ...

@@ +78,5 @@
>    var today = NetworkStatsService.cachedAppStatsDate;
>    var tomorrow = new Date(today.getTime() + (24 * 60 * 60 * 1000));
> +
> +  var wifi = {type: Ci.nsINetworkInterface.NETWORK_TYPE_WIFI, id: "0"};
> +  var mobile = {type: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE, id: "1234"};

Ditto.

@@ +119,5 @@
>  
>  add_test(function test_saveAppStatsWithMaxCachedTraffic() {
>    var timestamp = NetworkStatsService.cachedAppStatsDate.getTime();
>    var maxtraffic = NetworkStatsService.maxCachedTraffic;
> +  var wifi = {type: Ci.nsINetworkInterface.NETWORK_TYPE_WIFI, id: "0"};

Ditto.

@@ +145,5 @@
>    do_get_profile();
>  
>    Cu.import("resource://gre/modules/NetworkStatsService.jsm");
>  
> +  // Function convertNetworkInterface of NetworkStatsService cause errors when dealing

s/cause/causes/

Comment 47

[Albert [:albert]]

Attachment: Part 1/4 IDL changes

Changes from comment 44

Comment 48

[Albert [:albert]]

Attachment: Part 2/4 Multi SIM implementation

Changes from comment 45

Comment 49

[Albert [:albert]]

Attachment: Part 3/4 Fix access of websocket channel and TCPSocket to NetworkStats API

Added rebase for TCPSocket

Comment 50

[Albert [:albert]]

Attachment: Part 4/4 Tests

Changes from comment 46

Comment 51

[Albert [:albert]]

(In reply to Gene Lian [:gene] (Summit + PTOs 10/3 ~ 10/14) from comment #46)
> Comment on attachment 813566 [details] [diff] [review]
> Part 4/4 Tests
> 
> Review of attachment 813566 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/tests/unit_stats/test_networkstats_service_proxy.js
> @@ +38,5 @@
> >    var timestamp = NetworkStatsService.cachedAppStatsDate.getTime();
> >    var samples = 5;
> >  
> > +  var wifi = {type: Ci.nsINetworkInterface.NETWORK_TYPE_WIFI, id: "0"};
> > +  var mobile = {type: Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE, id: "1234"};
> 
> Hope to add some comments before these two vars. Please see below.
> 
> @@ +47,4 @@
> >    do_check_eq(Object.keys(cachedAppStats).length, 0);
> >  
> >    for (var i = 0; i < samples; i++) {
> > +    nssProxy.saveAppStats(1, wifi, timestamp, 10, 20);
> 
> Are you sure this is right? The second parameter should be
> nsINetworkInterface instead of nsINetworkStatsInterface. Please let me know
> if misunderstood anything. Thanks!
> 
> -----
> OK... I see. We just use the nsINetworkInterface.type in the
> .saveAppStats(...). The .id is just for testing. A bit confusing but it's
> fine. Can you add some comments like below?
> 
> // Making nsINetworkInterface instances where the nsINetworkInterface.type
> will be used in the .saveAppStats(...).
> var wifi = ...
> var mobile = ...

You are right, wifi and mobile should be a nsINetworkInterface but I didn't find how to create an instance of it, so I created an object with the same needed properties. I added comments to the code.

Comment 52

[Gene Lian (I already quit Mozilla)]

Comment on attachment 814300 [details] [diff] [review]
Part 3/4 Fix access of websocket channel and TCPSocket to NetworkStats API

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

Just some naming stuff. Thank you!

::: dom/network/src/TCPSocket.js
@@ +166,5 @@
>    // Network statistics (Gonk-specific feature)
>    _txBytes: 0,
>    _rxBytes: 0,
>    _appId: Ci.nsIScriptSecurityManager.NO_APP_ID,
> +  _networkInterface: null,

s/_networkInterface/_activeNetwork/

@@ +346,5 @@
>      if (!nssProxy) {
>        LOG("Error: Ci.nsINetworkStatsServiceProxy service is not available.");
>        return;
>      }
> +    nssProxy.saveAppStats(this._appId, this._networkInterface, Date.now(),

s/_networkInterface/_activeNetwork/

@@ +529,5 @@
>      transport.setEventSink(that, Services.tm.currentThread);
>      that._initStream(that._binaryType);
>  
>  #ifdef MOZ_WIDGET_GONK
> +    // Set _networkInterface, which is only required for network statistics.

s/_networkInterface/_activeNetwork/

@@ +534,5 @@
>      // Note that nsINetworkManager, as well as nsINetworkStatsServiceProxy, is
>      // Gonk-specific.
>      let networkManager = Cc["@mozilla.org/network/manager;1"].getService(Ci.nsINetworkManager);
> +    if (networkManager) {
> +      that._networkInterface = networkManager.active;

s/_networkInterface/_activeNetwork/

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1079,5 @@
>    if (localChannel) {
>      NS_GetAppInfo(localChannel, &mAppId, &mIsInBrowser);
>    }
>  
>    // obtain active connection type

s/connection type/network/

@@ +1081,5 @@
>    }
>  
>    // obtain active connection type
>    if (mAppId != NECKO_NO_APP_ID) {
> +    GetNetworkInterface();

s/GetNetworkInterface/GetActiveNetwork/

@@ +3271,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
> +WebSocketChannel::GetNetworkInterface()

s/GetNetworkInterface/GetActiveNetwork/

@@ +3280,5 @@
>    nsresult result;
>    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &result);
>  
>    if (NS_FAILED(result) || !networkManager) {
> +    mNetworkInterface = nullptr;

s/mNetworkInterface/mActiveNetwork/

@@ +3281,5 @@
>    nsCOMPtr<nsINetworkManager> networkManager = do_GetService("@mozilla.org/network/manager;1", &result);
>  
>    if (NS_FAILED(result) || !networkManager) {
> +    mNetworkInterface = nullptr;
> +    return NS_ERROR_UNEXPECTED; 

nit: trailing white space.

@@ +3286,3 @@
>    }
>  
> +  result = networkManager->GetActive(getter_AddRefs(mNetworkInterface));

s/mNetworkInterface/mActiveNetwork/

@@ +3296,5 @@
>  nsresult
>  WebSocketChannel::SaveNetworkStats(bool enforce)
>  {
>  #ifdef MOZ_WIDGET_GONK
>    // Check if the connection type and app id are valid.

s/connection type/active network/

@@ +3297,5 @@
>  WebSocketChannel::SaveNetworkStats(bool enforce)
>  {
>  #ifdef MOZ_WIDGET_GONK
>    // Check if the connection type and app id are valid.
> +  if(!mNetworkInterface || mAppId == NECKO_NO_APP_ID) {

s/mNetworkInterface/mActiveNetwork/

@@ +3321,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
>  
> +  mNetworkStatsServiceProxy->SaveAppStats(mAppId, mNetworkInterface, PR_Now() / 1000,

s/mNetworkInterface/mActiveNetwork/

::: netwerk/protocol/websocket/WebSocketChannel.h
@@ +267,3 @@
>    bool                            mIsInBrowser;
> +#ifdef MOZ_WIDGET_GONK
> +  nsCOMPtr<nsINetworkInterface>   mNetworkInterface;

s/mNetworkInterface/mActiveNetwork/

@@ +267,5 @@
>    bool                            mIsInBrowser;
> +#ifdef MOZ_WIDGET_GONK
> +  nsCOMPtr<nsINetworkInterface>   mNetworkInterface;
> +#endif
> +  nsresult                        GetNetworkInterface();

s/GetNetworkInterface/GetActiveNetwork/

Comment 53

[John Shih]

Hi Gene,
Although multi-sim part is not ready (i.e, retrieve iccId by serviceId), but I think we can still check this bug in first and open another following bug for that.
So that we can have other related bugs keep in progress, what do you think?

Comment 54

[Albert [:albert]]

Attachment: Part 3/4 Fix access of websocket channel and TCPSocket to NetworkStats API

Changes from comment 52

Comment 55

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

(In reply to Albert [:albert] from comment #47)
> Created attachment 814298 [details] [diff] [review]
> Part 1/4 IDL changes
> 
> Changes from comment 44

I just came back from my PTOs. Sorry for the delay. I'll take a look at it in a day or two.

Comment 56

[Gene Lian (I already quit Mozilla)]

(In reply to John Shih from comment #53)
> Hi Gene,
> Although multi-sim part is not ready (i.e, retrieve iccId by serviceId), but
> I think we can still check this bug in first and open another following bug
> for that.
> So that we can have other related bugs keep in progress, what do you think?

That's exactly what Albert and I were thinking. We've already opened Bug 923382 for that. We can check this in first.

Comment 57

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

Comment on attachment 814298 [details] [diff] [review]
Part 1/4 IDL changes

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

::: dom/network/interfaces/nsIDOMNetworkStats.idl
@@ +23,5 @@
>     */
>    readonly attribute DOMString    manifestURL;
>  
>    /**
> +   * Network for which the returned data belongs to.

Please rewrite as: Network the returned data belongs to.

@@ +30,3 @@
>  
>    /**
>     * Stats for connectionType

We don't have 'connectionType' anymore. Please change the comment accordingly.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +6,5 @@
>  
>  interface nsIDOMDOMRequest;
>  
> +/**
> + * Represents a data interface for which the manager is recording statistics

nit: place a period at the end of a line.

@@ +14,3 @@
>  {
> +  readonly attribute long type;
> +  readonly attribute DOMString id;

id is 0 for type Wifi, and should be iccId for mobile, right?
Please leave a comment to elaborate more on 'id.'

@@ +20,4 @@
>  interface nsIDOMMozNetworkStatsManager : nsISupports
>  {
>    /**
> +   * Constants for known interface types

ditto: a period.

@@ +32,5 @@
> +   * both included.
> +   *
> +   * If manifestURL is provided, Find method will retrieve per-app usage,
> +   * otherwise the target will be system usage.
> +   *

I am a little bit confused by comment 'Find method' in line 31 and line 34.

What is 'Find method'? I don't see a method named 'Find.' Is 'Find method' exactly the one 'getSamples()'? Would you please help revise the comments to make them clearer? Thank you.

@@ +48,3 @@
>  
>    /**
> +   * Remove all stats database.

Remove all stats in the database.

Comment 58

[Albert [:albert]]

Attachment: Part 1/4 IDL changes

Changes from comment 57

Comment 59

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

Comment on attachment 817070 [details] [diff] [review]
Part 1/4 IDL changes

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

Looks good to me. Thank you.

Comment 60

[Albert [:albert]]

Try Server: https://tbpl.mozilla.org/?tree=Try&rev=7f283c6a1851

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/516d0f14f7fd
https://hg.mozilla.org/integration/b2g-inbound/rev/ea06175feb4f
https://hg.mozilla.org/integration/b2g-inbound/rev/57b03d7055e8
https://hg.mozilla.org/integration/b2g-inbound/rev/5c878c48d732

Comment 62

[John Shih]

Hi Salvador,
since this bug are going to checking into master branch,
is there any bug on Cost Control for the NetworkStats API interface change in this bug?

Comment 63

[Gene Lian (I already quit Mozilla)]

+1. This will break the compatibility with Gaia.

Comment 64

[Marcelino Veiga Tuimil [:sonmarce]]

Cost Control refactor is scheduled for version 1.3, it will be started after landing new multi-SIM API

Comment 65

[John Shih]

Also, will the per-app metering be another feature in the Cost Control refactor? Or there is any schedule about that? Thanks!

Comment 66

[Jason Smith [:jsmith]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #63)
> +1. This will break the compatibility with Gaia.

Why did this land then? We can't break daily smoketests on trunk.

Comment 67

[Jason Smith [:jsmith]]

This broke a Cost Control smoketest on trunk, so this should be backed out.

Ryan - Can you back this out?

Comment 68

[Ryan VanderMeulen [:RyanVM]]

John, in the future, when you see that your patches were merged to m-c and the bug wasn't resolved, please ping the person who did the merge because it likely means they didn't do something they were supposed to. Silently resolving the bug makes the errors harder to spot.

https://hg.mozilla.org/mozilla-central/rev/423b9c30c73d

Comment 69

[Salvador de la Puente González [:salva]]

Breaking the Cost Control application was expected since the new API is radically different. There is a message sent to qa list. This should not be backed-out. Now we will start the Cost Control patches to adapt to the new API.

The bug 927615 is the one for this.

Comment 70

[Jason Smith [:jsmith]]

(In reply to Salvador de la Puente González [:salva] from comment #69)
> Breaking the Cost Control application was expected since the new API is
> radically different. There is a message sent to qa list. This should not be
> backed-out. Now we will start the Cost Control patches to adapt to the new
> API.
> 
> The bug 927615 is the one for this.

We do not break smoketests on trunk. Period. You need to figure out what Gaia changes are required here, implement them, and then land the gecko + gaia changes together. But you can't just break the cost control app in the process. That will block testing entirely.

Comment 71

[Salvador de la Puente González [:salva]]

This is why I recommend to disable Cost Control in trunk. I figured out what changes were needed some time ago but I was busy fixing bugs for certification and v1.1. So, do you prefer to disable Cost Control first?

Marce, can you help us?

Comment 72

[Jason Smith [:jsmith]]

(In reply to Salvador de la Puente González [:salva] from comment #71)
> This is why I recommend to disable Cost Control in trunk. I figured out what
> changes were needed some time ago but I was busy fixing bugs for
> certification and v1.1. So, do you prefer to disable Cost Control first?
> 
> Marce, can you help us?

That's not the right workflow to go with. You need to figure out a way to implement this such that you do not break trunk in the process at least a smoketest level. That might mean you might need to land the gecko changes in a separate branch and fix the cost control app against that gecko branch. Or just patch your local gecko to test Gaia against.

Fabrice deals with this problem commonly and can provide suggestions on how to go about this cleanly. I would suggest following up with him on a landing strategy to use here.

Comment 73

[Gene Lian (I already quit Mozilla)]

Since this API is simply used for a certified app, we might not need to take care of all the compatibility issues when it comes to API changes. However, from the point of view of internal testing work flow, we had better make sure everything compatible all the time.

For this case, it's inevitable to break the compatibility since the new APIs are going to be radically different. We still have to get the corresponding Gaia changes prepared and then land the Gecko + Gaia patches at the same time as possible as we can.

Comment 74

[John Shih]

Comment on attachment 814299 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +14,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/IndexedDBHelper.jsm");
>  
>  const DB_NAME = "net_stats";
>  const DB_VERSION = 2;

Hi Albert, as the patch is not landed yet
can you make a simple modification on updating DB_VERSION to 3
and also other corresponding works?

it's because that you just update the code but reuse DB_VERSION 2
that would cause the one who flash with this patch can not get database updated
(i.e, the upgradeSchema is unlikely to work because of cannot aware of the version change)

does it make sense?

Comment 75

[Gene Lian (I already quit Mozilla)]

Comment on attachment 814299 [details] [diff] [review]
Part 2/4 Multi SIM implementation

>diff --git a/dom/network/src/NetworkStatsDB.jsm b/dom/network/src/NetworkStatsDB.jsm
>--- a/dom/network/src/NetworkStatsDB.jsm
>+++ b/dom/network/src/NetworkStatsDB.jsm
>@@ -11,45 +11,43 @@ function debug(s) { dump("-*- NetworkSta
> 
> const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> 
> Cu.import("resource://gre/modules/Services.jsm");
> Cu.import("resource://gre/modules/IndexedDBHelper.jsm");
> 
> const DB_NAME = "net_stats";
> const DB_VERSION = 2;
>-const STORE_NAME = "net_stats"; // Deprecated. Use "net_stats_v2" instead.
>-const STORE_NAME_V2 = "net_stats_v2";
>+const STORE_NAME = "net_stats";

Hi Albert,

Btw, John just discovered another potential compatibility issue. Since this patch has been backed out, we hope it can be taken care of in the next new patch.

We think we should flip the DB version number to 3 to delete and rebuild the database. You can imagine what's happening if anyone has updated its DB to 2 (containing per-app metering functions). The updating process in your original patch will not rebuild the database because its version is already 2, thus keeping using the deprecated DB.

Could you please fix that as well? Thanks!

Comment 76

[Gene Lian (I already quit Mozilla)]

Oh! John and I addressed that at the same time. ;)

Comment 77

[[:fabrice] Fabrice Desré]

In order to land that feature safely, you very likely need to:
- land a gaia patch that supports api v.N and api v.N+1
- wait for this patch to propagate to tbpl
- land the gecko patch that provides api v.N+1 (this one)
- wait for travis to pickup the new gecko
- land another gaia patch that removes support for api v.N

For the first gaia patch you need a way to do feature detection, like testing the presence of a new function offered by the api.

Comment 78

[Albert [:albert]]

Attachment: Part 2/4 Multi SIM implementation

Fix database version.

Comment 79

[Marcelino Veiga Tuimil [:sonmarce]]

Multi-SIM support for data usage is a long-waited feature to fix all technical debt in Cost Control app. We have had to do a lot of tricky things in Gaia because of the lack of Multi-SIM support in Gecko, and even not able to fix all bugs.

As you know data usage API is closely related to per-app data usage API, that it is currently being implemented and partly landed. Any change in per-app API implies modifications in data usage API, so not landing now data usage API will imply more work in following days (rebasing, reviewing, coding, …).

I do not see a problem to have a single smoke test of Cost Control broken for some days. Anyway we will work the way you are saying, and will be working for 3 days in having both old and new APIs available, so that we do not break Cost Control smoke test.

To avoid wasting time in recoding data usage API, I suggest to stop landing anything of per-app API during these 3 days we need to have both old and new API available. This way we can minimize the effect of delaying data usage API landing. 

Gene, do you agree?

Comment 80

[Gene Lian (I already quit Mozilla)]

(In reply to Marcelino Veiga Tuimil [:sonmarce] from comment #79)
> To avoid wasting time in recoding data usage API, I suggest to stop landing
> anything of per-app API during these 3 days we need to have both old and new
> API available. This way we can minimize the effect of delaying data usage
> API landing. 
> 
> Gene, do you agree?

Yes! That would be wonderful if you can have a way to accommodate both old and new APIs on the Gaia side. I don't think we will have any changes for per-app metering recently, which means we won't modify any existing network stats DOM API until you're done. John, Albert and I will watch out.

Comment 81

[Gene Lian (I already quit Mozilla)]

Comment on attachment 818346 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

Yeap! It looks good to me.

John, could you please take another look? We need more eyes on this part to see if it can correctly deal with:

1. version 1 -> version 3
2. version 2 -> version 3

Comment 82

[Gene Lian (I already quit Mozilla)]

(In reply to Fabrice Desré [:fabrice] from comment #77)
> In order to land that feature safely, you very likely need to:
> - land a gaia patch that supports api v.N and api v.N+1
> - wait for this patch to propagate to tbpl
> - land the gecko patch that provides api v.N+1 (this one)
> - wait for travis to pickup the new gecko
> - land another gaia patch that removes support for api v.N
> 
> For the first gaia patch you need a way to do feature detection, like
> testing the presence of a new function offered by the api.

Yeap! Gecko doesn't expose any version info to Gaia. As Fabrice said, maybe what Gaia can do is to directly examine if an API is available in navigator.mozNetworkStats:

  if (newFunc in navigator.mozNetworkStats) {
    newFunc();
  } else if (oldFunc in navigator.mozNetworkStats) {
    oldFunc();
  }

Some Gaia guys suggested we'd better use an independent helper to encapsulate this kind of logic to make it more clear and easier to be removed in the future.

Comment 83

[Marcelino Veiga Tuimil [:sonmarce]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #80)
> Yes! That would be wonderful if you can have a way to accommodate both old
> and new APIs on the Gaia side. I don't think we will have any changes for
> per-app metering recently, which means we won't modify any existing network
> stats DOM API until you're done. John, Albert and I will watch out.

OK, thanks

Comment 84

[Salvador de la Puente González [:salva]]

(In reply to Fabrice Desré [:fabrice] from comment #77)
> In order to land that feature safely, you very likely need to:
> - land a gaia patch that supports api v.N and api v.N+1
> - wait for this patch to propagate to tbpl
> - land the gecko patch that provides api v.N+1 (this one)
> - wait for travis to pickup the new gecko
> - land another gaia patch that removes support for api v.N
> 
> For the first gaia patch you need a way to do feature detection, like
> testing the presence of a new function offered by the api.

Despite I agree on this workflow and it reminds me to the IccHelper case, note this solution brings an over-engineering effort for a solely one application with certified permissions. But it's ok for me and we are working on it.

Comment 85

[John Shih]

Comment on attachment 818346 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

I've checked this logic by myself, it works fine.
Albert, thanks for your work!

Comment 86

[Albert [:albert]]

Attachment: Part 2/4 Multi SIM implementation

Added changes to fix problems when passing dates through asyncmessage between child and parent processes.

Comment 87

[Gene Lian (I already quit Mozilla)]

Comment on attachment 820339 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

What do you think about the following approach? It's also a work-around bug working in a more reasonable way.

Btw, please r=gene,jshih because John used to be involved in the review. :)

::: dom/network/src/NetworkStatsManager.js
@@ +155,4 @@
>        throw Components.results.NS_ERROR_INVALID_ARG;
>      }
>  
> +    // TODO Bug 929410 Date object cannot correctly pass though cpmm/ppmm IPC

s/though/through/

@@ +155,5 @@
>        throw Components.results.NS_ERROR_INVALID_ARG;
>      }
>  
> +    // TODO Bug 929410 Date object cannot correctly pass though cpmm/ppmm IPC
> +    // This is just a work-around.

// This is just a work-around by passing timestamp numbers.

@@ +157,5 @@
>  
> +    // TODO Bug 929410 Date object cannot correctly pass though cpmm/ppmm IPC
> +    // This is just a work-around.
> +    aStart = new Date(aStart.getTime())
> +    aEnd = new Date(aEnd.getTime());

I'd suggest you don't need to convert them to Date objects again. Instead, pass the timestamp numbers:

  aStart = aStart.getTime();
  aEnd = aEnd.getTime();

::: dom/network/src/NetworkStatsService.jsm
@@ +254,3 @@
>  
> +    // TODO Bug 929410 Date object cannot correctly pass though cpmm/ppmm IPC messaging.
> +    // This is just a work-around to recover it.

I noticed you removed the following 2 lines:

  let start = new Date(msg.start);
  let end = new Date(msg.end);

I'd suggest you can keep them and you don't need to make any changes on the parent side (compared to the previous patch).

Comment 88

[Albert [:albert]]

Attachment: Part 2/4 Multi SIM implementation

Changes from comment 87

Comment 89

[Albert [:albert]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #87)
> Comment on attachment 820339 [details] [diff] [review]
> Part 2/4 Multi SIM implementation
> 
> Review of attachment 820339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What do you think about the following approach? It's also a work-around bug
> working in a more reasonable way.

Nice!

Comment 90

[John Shih]

Comment on attachment 820872 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

::: dom/network/src/NetworkStatsServiceProxy.js
@@ +37,4 @@
>      }
>  
> +    NetworkStatsService.saveAppStats(aAppId, aNetwork, aTimeStamp,
> +                                      aRxBytes, aTxBytes, aCallback);

you might need to align this line ;)

Comment 91

[Gene Lian (I already quit Mozilla)]

Comment on attachment 820872 [details] [diff] [review]
Part 2/4 Multi SIM implementation

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

Thank you!

Comment 92

[Albert [:albert]]

Attachment: Part 2/4 Multi SIM implementation

Nit from comment 90 fixed

Comment 93

[Ed Morley [:emorley]]

Please can you clarify that the concerns in comment 77 have been dealt with and then re-mark as checkin-needed...? :-)

Comment 94

[Ed Morley [:emorley]]

(Also ideally a more recent Try server run)

Comment 95

[Albert [:albert]]

Concerns from comment 77 about breaking gaia costcontrol app has been solved in bug 927788. Patch landed in gaia master https://github.com/mozilla-b2g/gaia/commit/ddce49b3dc7b5fc428d2d24d5be134449b4424a5

Comment 96

[Albert [:albert]]

Try server:  https://tbpl.mozilla.org/?tree=Try&rev=830274a9e4ce

Comment 97

[Jason Smith [:jsmith]]

(In reply to Albert [:albert] from comment #95)
> Concerns from comment 77 about breaking gaia costcontrol app has been solved
> in bug 927788. Patch landed in gaia master
> https://github.com/mozilla-b2g/gaia/commit/
> ddce49b3dc7b5fc428d2d24d5be134449b4424a5

And apparently that broke the cost control app again - bug 930555. Once again, this isn't safe to land. I'm backing that patch out.

Comment 98

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #97)
> (In reply to Albert [:albert] from comment #95)
> > Concerns from comment 77 about breaking gaia costcontrol app has been solved
> > in bug 927788. Patch landed in gaia master
> > https://github.com/mozilla-b2g/gaia/commit/
> > ddce49b3dc7b5fc428d2d24d5be134449b4424a5
> 
> And apparently that broke the cost control app again - bug 930555. Once
> again, this isn't safe to land. I'm backing that patch out.

Actually looks like bug 927788 didn't cause this - more likely a different bug. Working on the regression range on that now.

Comment 99

[Albert [:albert]]

Bug 930555 has been closed as duplicated of bug 927724, which is older than changes of bug 927788 and the problem is not related with changes in the API.

Comment 100

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/b2g-inbound/rev/53f2e5066e98
https://hg.mozilla.org/integration/b2g-inbound/rev/78bdbfcca77d
https://hg.mozilla.org/integration/b2g-inbound/rev/7f85957616d4
https://hg.mozilla.org/integration/b2g-inbound/rev/5cdf2c46b548

Comment 101

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/53f2e5066e98
https://hg.mozilla.org/mozilla-central/rev/78bdbfcca77d
https://hg.mozilla.org/mozilla-central/rev/7f85957616d4
https://hg.mozilla.org/mozilla-central/rev/5cdf2c46b548

Comment 102

[Albert [:albert]]

There is a doc draft in https://developer.mozilla.org/en-US/docs/WebAPI/Network_Stats_2_0_proposal, but it needs to be updated.

Comment 103

[Salvador de la Puente González [:salva]]

And please, remove the `_proposal` part x)

Comment 104

[Preeti Raghunath(:Preeti)]

On Master already so will be in 1.3 automatically.