John Shih Assignee
	Description • 11 years ago

As we want to track system-only network traffic, the traffic through wifi-hotspot
and usb tethering are two of important information.

This bug is intended to find a way to catch these traffic.

	John Shih Assignee
	Comment 1 • 11 years ago

Support the command to retrieve tethering traffic in netd through NetworkManager.js.

The command is

bandwidth gettetherstats ifname1 ifname2

The result returned by netd is

ifname1(receive) ifname2(transmit) rxbytes rxpackets txbytes txpackets

so we are going to hold the values of rxbytes and txbytes here.

	John Shih Assignee
	Comment 3 • 11 years ago

Comment on attachment 825829 [details] [diff] [review]
Bug 928289 - Part 1: Support obtain tethergin stats from netd through NetworkManager.js r=vchang

we may also need to add some notification to allow NetworkStatsService to listen the state of tethering.

The most important issue here is that we need to collect the data before tethering is closed/disabled, otherwise we'll lose these data.

	Vincent Chang[:vchang][changyihsin]
	Comment 4 • 11 years ago

Comment on attachment 825829 [details] [diff] [review]
Bug 928289 - Part 1: Support obtain tethergin stats from netd through NetworkManager.js r=vchang

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

Thanks for your patch. It looks good.

::: dom/system/gonk/NetworkManager.js
@@ +1066,5 @@
>      debug("Can't find rndis interface in possible lists.");
>      return DEFAULT_USB_INTERFACE_NAME;
>    },
>  
> +  getTetheringStats: function getTetheringStats(ifname1, ifname2, callback) {

s/ifname1/ifaceOut, s/ifname2/ifaceIn

Can you implement tethering statistics for both wifi and usb tethering ?

@@ +1080,5 @@
> +    params.isAsync = true;
> +
> +    this.controlMessage(params, function(result) {
> +      let success = result.resultCode >= NETD_COMMAND_OKAY &&
> +                    result.resultCode < NETD_COMMAND_ERROR;

You can use isError(result.resultCode).

::: dom/system/gonk/nsINetworkManager.idl
@@ +296,5 @@
> +   * @param callback
> +   *        Callback to notify result and provide stats
> +   *        and the date when stats are retrieved
> +   */
> +  void getTetheringStats(in DOMString ifname1, in DOMString

s/ifname1/ifaceOut

@@ +297,5 @@
> +   *        Callback to notify result and provide stats
> +   *        and the date when stats are retrieved
> +   */
> +  void getTetheringStats(in DOMString ifname1, in DOMString
> +  ifname2, in nsITetheringStatsCallback callback);

s/ifname2/ifaceIn

	John Shih Assignee
	Comment 5 • 11 years ago

In this patch, there are several modifications include:

1. make sure tethering stats is retrieved before tethering is disabled/changed (e.g., switch connection type between wifi and 3g)

2. then the stats will be saved in a cached by Tethering Type (USB, WiFi)

3. NetworkManager will send out notification to tell that there is cached
stats available.

4. use last stats to handle the case if tethering stats are retrieved when tethering is enabled. (since we always get the accumulated stats from netd)
   ex. first time get: newStats = stats;
                       lastStats = stats;
       second time:    newStats = stats - lastStats;
                       lastStats = stats; ...etc.

	John Shih Assignee
	Comment 6 • 11 years ago

Comment on attachment 831445 [details] [diff] [review]
Bug 928289 - Part 1: Support obtain tetherging stats from netd through NetworkManager.js r=vchang

I'm going to rebase the patch.

	John Shih Assignee
	Comment 7 • 11 years ago

Base on the separated purposes of NetworkManager and NetworService, the work of obtaining tethering stats will modified.

In short, NetworkService will firstly get tethering interface pair from NetworkManger and then use the pair to query tethering stats through NetworkService.

	John Shih Assignee
	Comment 16 • 11 years ago

Tethering statistics can be obtained by a similar way as how we get interface statistics. That is, get statistics through netd. However, there also several problems we need to deal with:

1. It requires tethering interface pair (internal interface and external interface) to query statistics from netd.

2. If tethering is disabled or under internal interface change (only for USB tethering, internal interface can be switched between WiFi and 3G), the previous statistics will lose.

To solve problem 1., the getTetheringStats interface is exposed by NetworkManager, which is in charge of network information. Thus, networkStatsService can simply query tethering statistics by type (WiFi or USB) without knowing interface pair.

For problem 2., cases including tethering disable and interface change are forced to obtain tethering statistics and save them through networkStatsServiceProxy before taking action.

If tethering statistics is queried actively by networkStatsService, it use the original way as how we deal with obtaining interface statistics.

	Albert [:albert]
	Comment 17 • 11 years ago

(In reply to John Shih from comment #15)
> Created attachment 8357698 [details] [diff] [review]
> Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

I did a quick review to see your design and I have two points that I would discuss with you:

1) You add a new '_tethering' array to save tethering types, which is very similar to '_networks' array. That makes you to duplicate some code to process both arrays ( in 'updateAllStats', 'updateStats', 'updateQueueIndex' and 'update'). As tethering can be conceptualized as a network I think you can add it to the '_networks' array, so you won't need to duplicate code, just check network type when updating in order to know if you have to call 'getTetheringStats' or 'getNetworkStats'. Does it make sense to you?

2) In first versions of NetworkStats API I placed the 'getNetworkStats' netd connector in the NetworkManager like you did with 'getTetheringStats', but then someone move it to the NetworkService. Do you know where it should be placed? (is just to understand why one function goes in NetworkService and why the other goes to NetworkManager).

	John Shih Assignee
	Comment 18 • 11 years ago

Thanks for your quick feedback!

(In reply to Albert [:albert] from comment #17)
> (In reply to John Shih from comment #15)
> > Created attachment 8357698 [details] [diff] [review]
> > Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> 
> I did a quick review to see your design and I have two points that I would
> discuss with you:
> 
> 1) You add a new '_tethering' array to save tethering types, which is very
> similar to '_networks' array. That makes you to duplicate some code to
> process both arrays ( in 'updateAllStats', 'updateStats', 'updateQueueIndex'
> and 'update'). As tethering can be conceptualized as a network I think you
> can add it to the '_networks' array, so you won't need to duplicate code,
> just check network type when updating in order to know if you have to call
> 'getTetheringStats' or 'getNetworkStats'. Does it make sense to you?

Yes, I've also considered about it. Then I decided to have separate arrays in order to reduce the impact on original logic. But with your suggestion, I will integrate them.

> 
> 2) In first versions of NetworkStats API I placed the 'getNetworkStats' netd
> connector in the NetworkManager like you did with 'getTetheringStats', but
> then someone move it to the NetworkService. Do you know where it should be
> placed? (is just to understand why one function goes in NetworkService and
> why the other goes to NetworkManager).

This is actually an issue I've met. The work of splitting NetworkManager into NetworkManager and NetworkService is aiming at making NetworkManager purely take charge of network information. The code related to sending commands to net_worker is moved to NetworkService, so some works can be done without going through NetworkManager.

Under this architecture, there is no specific impact for getNetworkStats because it only require interface name, which can be easily obtained and maintained without the help of 'new' NetworkManager. However, it is little complicated for getTetheringStats due to two main problems:

1. It requires interface pair (internal interface and external interface) when querying tethering stats.
2. It is unable to map tethering interface pair to nsINetworkInterface (which is needed to query icc Id).

Both tethering interface pairs and nsINetworkInterface are hold in NetworkManager. As a result, it would require lots of communication (among NetworkStatsService, NetworkManager, NetworkService) and additional interfaces (such as obtain tethering pair, query nsINetworkInterface) if placing getTetheringStats in NetworkService. After having some discussion with RIL guys, we finally decide to use this design to make the work more simple. (but it still needs their review)

IMO, getNetworkStats is at the right place (NetworkManager) based on the responsibility of current NetworkManager and NetworkService, while there are some compromises to have getTetheringStats placed in NetworkManager.

	John Shih Assignee
	Comment 19 • 11 years ago

Comment on attachment 8357698 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

remove the review flag, I'll upload a new patch later

	John Shih Assignee
	Comment 20 • 11 years ago

Patch updated.
The original object _networks is changed to _connections, which integrates both available networks and tethering.

	John Shih Assignee
	Comment 21 • 11 years ago

patch updated.

	Albert [:albert]
	Comment 22 • 11 years ago

Comment on attachment 8359645 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

::: dom/network/src/NetworkStatsService.jsm
@@ +95,4 @@
>      // networkInterface per network but can't exist a networkInterface not
>      // being mapped to a network.
>  
> +    this._connections = Object.create(null);

Previous versions of the API already called the object 'connections' but we decided to change it to 'networks'. I think we can keep 'networks' because is more representative of the object contents, you also use 'network tethering' words in above comments :)

Let's see Gene's opinion.

@@ +406,5 @@
>  
>      // Check if the network is currently active. If yes, we need to update
>      // the cached stats first before retrieving stats from the DB.
> +    if (this._connections[netId]) {
> +      let updateList = [netId, TETHERING_TYPE_WIFI, TETHERING_TYPE_USB];

I see that you change netId parameter of updateStats from a single value to an array just to pass this array. Do we need to update tethering always?

@@ +552,5 @@
>        this.logAllRecords();
>      }
>    },
>  
> +  updateStats: function updateStats(aIdList, aCallback) {

From my point of view, as commented before I don't see why do we need to change aNetId to aIdList, what are the benefits of increasing complexity?

If finally we decide to use the array, we can remove updateAllStats because is a particular case of updateStats using a list of ids.

@@ +561,5 @@
> +    for (let i = 0; i < aIdList.length; i++) {
> +      // only put the callback to the last queue element.
> +      let callback = null;
> +      if (i == aIdList.length - 1) {
> +        callback = aCallback;

You put the callback in the last element of aIdList but it can be processed before other ones if another request with the same id is in the queue. In that case you are calling the callback before all elements in aIdList are processed.

	John Shih Assignee
	Comment 23 • 11 years ago

patch update.

	John Shih Assignee
	Comment 25 • 11 years ago

(In reply to Albert [:albert] from comment #22)
> Comment on attachment 8359645 [details] [diff] [review]
> Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> 
> Review of attachment 8359645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/src/NetworkStatsService.jsm
> @@ +95,4 @@
> >      // networkInterface per network but can't exist a networkInterface not
> >      // being mapped to a network.
> >  
> > +    this._connections = Object.create(null);
> 
> Previous versions of the API already called the object 'connections' but we
> decided to change it to 'networks'. I think we can keep 'networks' because
> is more representative of the object contents, you also use 'network
> tethering' words in above comments :)
> 
> Let's see Gene's opinion.
> 
> @@ +406,5 @@
> >  
> >      // Check if the network is currently active. If yes, we need to update
> >      // the cached stats first before retrieving stats from the DB.
> > +    if (this._connections[netId]) {
> > +      let updateList = [netId, TETHERING_TYPE_WIFI, TETHERING_TYPE_USB];
> 
> I see that you change netId parameter of updateStats from a single value to
> an array just to pass this array. 


I've refined the patch, turning an array back to single value and recursively call the updateStats function.
Is it ok with you?

>Do we need to update tethering always?
 
This is another issue. Currently there is no way to observe tethering status as how we did for network interfaces, so that's why I try to update them (both two types) every time when getSamples() is called and use their result to check if they are available. I think it needs some support from NetworkManager/NetworkService (like add an event when tethering is enabled/disabled) to solve this problem, let's loop Vincent to see if he has any thoughts.

> @@ +552,5 @@
> >        this.logAllRecords();
> >      }
> >    },
> >  
> > +  updateStats: function updateStats(aIdList, aCallback) {
> 
> From my point of view, as commented before I don't see why do we need to
> change aNetId to aIdList, what are the benefits of increasing complexity?
> 
> If finally we decide to use the array, we can remove updateAllStats because
> is a particular case of updateStats using a list of ids.
> 
> @@ +561,5 @@
> > +    for (let i = 0; i < aIdList.length; i++) {
> > +      // only put the callback to the last queue element.
> > +      let callback = null;
> > +      if (i == aIdList.length - 1) {
> > +        callback = aCallback;
> 
> You put the callback in the last element of aIdList but it can be processed
> before other ones if another request with the same id is in the queue. In
> that case you are calling the callback before all elements in aIdList are
> processed.

Now with the recursive way, this issue could be avoided, right?
Initially, the goal to do so is to make sure self._db.find is called after updating each network (interface/tethering) in getSamples

Thanks

	John Shih Assignee
	Comment 26 • 11 years ago

Hi Vincent,
Do you have any thoughts on obtaining tethering status in NetworkStatsService?
Thanks

	Vincent Chang[:vchang][changyihsin]
	Comment 27 • 11 years ago

Comment on attachment 8359724 [details] [diff] [review]
Bug 928289 - Part 1: Support obtain tetherging stats from netd through NetworkManager.js r=vchang

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

::: dom/system/gonk/NetworkManager.js
@@ +50,5 @@
>  const KERNEL_NETWORK_ENTRY = "/sys/class/net";
>  
>  const TETHERING_TYPE_WIFI = "WiFi";
>  const TETHERING_TYPE_USB  = "USB";
> +const TETHERING_SERVICE_TYPE = "tethering";

I am a little confuse about the naming here. Do we have other TETHERING_SERVICE_TYPE ?

@@ +766,5 @@
> +	  gNetworkService.setWifiTethering(enable, config, (function (error) {
> +	    let resetSettings = error;
> +	    this.notifyError(resetSettings, callback, error);
> +	  }).bind(this));
> +  },

Nit: two spaces please.

@@ +799,5 @@
> +                               .bind(this, this.active, enable, config,
> +                                     callback, this.setWifiTetheringResult));
> +	  } else {
> +	    this.setWifiTetheringResult(enable, config, callback);
> +	  }

Nit: two space please. 
if () {
  abc;
  def; 
}

@@ +803,5 @@
> +	  }
> +  },
> +
> +  setUSBTetheringResult: function(enable, config, callback) {
> +	  gNetworkService.setUSBTethering(enable, config, callback);

Nit: ditto.

@@ +824,5 @@
> +                               .bind(this, this.active, enable, params,
> +                                     callback, this.setUSBTetheringResult));
> +	} else {
> +      this.setUSBTetheringResult(enable, params, callback);
> +	}

Nit: ditto.

::: dom/system/gonk/NetworkService.js
@@ +434,5 @@
> +      let success = !isError(result.resultCode);
> +      callback.tetheringStatsAvailable(success, result.rxBytes, result.txBytes,
> +                                       result.date);
> +    })
> +  },

This should be re-written to C++ because bug 864931 is about to land.

::: dom/system/gonk/net_worker.js
@@ +206,5 @@
> +
> +  postMessage(params);
> +  return true;
> +}
> +

This should be re-written to C++ because bug 864931 is about to land.

	Albert [:albert]
	Comment 28 • 11 years ago

(In reply to John Shih from comment #25)
> (In reply to Albert [:albert] from comment #22)
> > Comment on attachment 8359645 [details] [diff] [review]
> > Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> > 
> > Review of attachment 8359645 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +406,5 @@
> > >  
> > >      // Check if the network is currently active. If yes, we need to update
> > >      // the cached stats first before retrieving stats from the DB.
> > > +    if (this._connections[netId]) {
> > > +      let updateList = [netId, TETHERING_TYPE_WIFI, TETHERING_TYPE_USB];
> > 
> > I see that you change netId parameter of updateStats from a single value to
> > an array just to pass this array. 
> 
> 
> I've refined the patch, turning an array back to single value and
> recursively call the updateStats function.
> Is it ok with you?

Yes, fine.

> >Do we need to update tethering always?
>  
> This is another issue. Currently there is no way to observe tethering status
> as how we did for network interfaces, so that's why I try to update them
> (both two types) every time when getSamples() is called and use their result
> to check if they are available. I think it needs some support from
> NetworkManager/NetworkService (like add an event when tethering is
> enabled/disabled) to solve this problem, let's loop Vincent to see if he has
> any thoughts.

When tethering is disabled can you get tethering stats from netd?

	John Shih Assignee
	Comment 29 • 11 years ago

(In reply to Albert [:albert] from comment #28)
> (In reply to John Shih from comment #25)
> > (In reply to Albert [:albert] from comment #22)
> > > Comment on attachment 8359645 [details] [diff] [review]
> > > Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> > > 
> > > Review of attachment 8359645 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > @@ +406,5 @@
> > > >  
> > > >      // Check if the network is currently active. If yes, we need to update
> > > >      // the cached stats first before retrieving stats from the DB.
> > > > +    if (this._connections[netId]) {
> > > > +      let updateList = [netId, TETHERING_TYPE_WIFI, TETHERING_TYPE_USB];
> > > 
> > > I see that you change netId parameter of updateStats from a single value to
> > > an array just to pass this array. 
> > 
> > 
> > I've refined the patch, turning an array back to single value and
> > recursively call the updateStats function.
> > Is it ok with you?
> 
> Yes, fine.
> 
> > >Do we need to update tethering always?
> >  
> > This is another issue. Currently there is no way to observe tethering status
> > as how we did for network interfaces, so that's why I try to update them
> > (both two types) every time when getSamples() is called and use their result
> > to check if they are available. I think it needs some support from
> > NetworkManager/NetworkService (like add an event when tethering is
> > enabled/disabled) to solve this problem, let's loop Vincent to see if he has
> > any thoughts.
> 
> When tethering is disabled can you get tethering stats from netd?

Yes, I get tethering stats before it is truly disabled. But it's a internal work in NetworkManager.

	Albert [:albert]
	Comment 30 • 11 years ago

(In reply to John Shih from comment #29)
> (In reply to Albert [:albert] from comment #28)
> > (In reply to John Shih from comment #25)
> > > (In reply to Albert [:albert] from comment #22)
> > > > Comment on attachment 8359645 [details] [diff] [review]
> > > > Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> > > > 
> > > > Review of attachment 8359645 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > @@ +406,5 @@
> > > > >  
> > > > >      // Check if the network is currently active. If yes, we need to update
> > > > >      // the cached stats first before retrieving stats from the DB.
> > > > > +    if (this._connections[netId]) {
> > > > > +      let updateList = [netId, TETHERING_TYPE_WIFI, TETHERING_TYPE_USB];
> > > > 
> > > > I see that you change netId parameter of updateStats from a single value to
> > > > an array just to pass this array. 
> > > 
> > > 
> > > I've refined the patch, turning an array back to single value and
> > > recursively call the updateStats function.
> > > Is it ok with you?
> > 
> > Yes, fine.
> > 
> > > >Do we need to update tethering always?
> > >  
> > > This is another issue. Currently there is no way to observe tethering status
> > > as how we did for network interfaces, so that's why I try to update them
> > > (both two types) every time when getSamples() is called and use their result
> > > to check if they are available. I think it needs some support from
> > > NetworkManager/NetworkService (like add an event when tethering is
> > > enabled/disabled) to solve this problem, let's loop Vincent to see if he has
> > > any thoughts.
> > 
> > When tethering is disabled can you get tethering stats from netd?
> 
> Yes, I get tethering stats before it is truly disabled. But it's a internal
> work in NetworkManager.

Then, in NetworkStatsService you don't need to update stats until some request wants to check tethering stats or there is a system shutdown. Sorry, I guess I miss something..

	John Shih Assignee
	Comment 31 • 11 years ago

(In reply to Albert [:albert] from comment #30)
> (In reply to John Shih from comment #29)
> > (In reply to Albert [:albert] from comment #28)
> > > (In reply to John Shih from comment #25)
> > > > (In reply to Albert [:albert] from comment #22)
> > > > > Comment on attachment 8359645 [details] [diff] [review]
> > > > > Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> > > > > 
> > > > > Review of attachment 8359645 [details] [diff] [review]:
> > > > > -----------------------------------------------------------------
> > > > > 
> > > > > @@ +406,5 @@
> > > > > >  
> > > > > >      // Check if the network is currently active. If yes, we need to update
> > > > > >      // the cached stats first before retrieving stats from the DB.
> > > > > > +    if (this._connections[netId]) {
> > > > > > +      let updateList = [netId, TETHERING_TYPE_WIFI, TETHERING_TYPE_USB];
> > > > > 
> > > > > I see that you change netId parameter of updateStats from a single value to
> > > > > an array just to pass this array. 
> > > > 
> > > > 
> > > > I've refined the patch, turning an array back to single value and
> > > > recursively call the updateStats function.
> > > > Is it ok with you?
> > > 
> > > Yes, fine.
> > > 
> > > > >Do we need to update tethering always?
> > > >  
> > > > This is another issue. Currently there is no way to observe tethering status
> > > > as how we did for network interfaces, so that's why I try to update them
> > > > (both two types) every time when getSamples() is called and use their result
> > > > to check if they are available. I think it needs some support from
> > > > NetworkManager/NetworkService (like add an event when tethering is
> > > > enabled/disabled) to solve this problem, let's loop Vincent to see if he has
> > > > any thoughts.
> > > 
> > > When tethering is disabled can you get tethering stats from netd?
> >
> > Yes, I get tethering stats before it is truly disabled. But it's a internal
> > work in NetworkManager.
> 
> Then, in NetworkStatsService you don't need to update stats until some
> request wants to check tethering stats or there is a system shutdown. Sorry,
> I guess I miss something..

You are right. There are two cases for updating tethering stats:
1. NetworkStatsService actively queries when getSample() or updateAllStats() is called
2. NetworkStatsService passively updates when tethering is disabled or connection changed (these two situations would cause data lose so that we need to get data in advance)

Note that case 2. is an internal work in NetworkManager.

	Gene Lian (I already quit Mozilla)
	Comment 32 • 11 years ago

Comment on attachment 8360232 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

::: dom/network/src/NetworkStatsService.jsm
@@ +24,5 @@
>  
>  const TOPIC_BANDWIDTH_CONTROL = "netd-bandwidth-control"
>  
>  const TOPIC_INTERFACE_REGISTERED   = "network-interface-registered";
>  const TOPIC_INTERFACE_UNREGISTERED = "network-interface-unregistered";

Add one blank line here.

@@ +28,5 @@
>  const TOPIC_INTERFACE_UNREGISTERED = "network-interface-unregistered";
>  const NET_TYPE_WIFI = Ci.nsINetworkInterface.NETWORK_TYPE_WIFI;
>  const NET_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
> +const NET_TYPE_MOBILE_MMS = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS;
> +const NET_TYPE_MOBILE_SUPL = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;

Add one blank line here.

@@ +30,5 @@
>  const NET_TYPE_MOBILE = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE;
> +const NET_TYPE_MOBILE_MMS = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS;
> +const NET_TYPE_MOBILE_SUPL = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;
> +const TETHERING_TYPE_WIFI = "WiFi";
> +const TETHERING_TYPE_USB = "USB";

Add one blank line here.

@@ +31,5 @@
> +const NET_TYPE_MOBILE_MMS = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS;
> +const NET_TYPE_MOBILE_SUPL = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;
> +const TETHERING_TYPE_WIFI = "WiFi";
> +const TETHERING_TYPE_USB = "USB";
> +const TETHERING_SERVICE_TYPE = "tethering";

I think it should be:

const SERVICE_TYPE_TETHERING = "tethering";

Right?

@@ +77,5 @@
>  
>      this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>  
> +    // Object to store available networks, including network interfaces and
> +    // network tethering.

The comment here is a bit messy. I'd rewrite them as:

// Object to store available networks, including network interfaces and
// network tethering.
//
// 1. For network interfaces
//    Blah...
//
// 2. For  network tethering
//    Blah...
//
// Common part if needed.

@@ +87,5 @@
> +    // Each networkInterface is placed in the _networks object by the index
> +    // of 'networkId + networkType'.
> +    // Different from network interfaces, network tethering only contains type
> +    // (WiFi, USB, etc). Therefore different tethering is placed in the
> +    // _networks object by the index of 'tetheringType'.

s/'tetheringType'/'TETHERING_TYPE_*'

@@ +98,5 @@
>      this._networks = Object.create(null);
>  
>      // 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

Please align the comments (80-char rule) since you're here.

@@ +106,4 @@
>                                interfaceName: null };
>  
> +    // Due to tethering types are constant and required before querying,
> +    // they are also hardcoded here.

// Because tethering types are constant and can be required before querying,
// they are also hardcoded here.

@@ +106,5 @@
>                                interfaceName: null };
>  
> +    // Due to tethering types are constant and required before querying,
> +    // they are also hardcoded here.
> +    this._networks[TETHERING_TYPE_WIFI] = { tetheringType: TETHERING_TYPE_WIFI,

Why do you want to keep a |tetheringType| in the object? where its index already specified.

@@ +107,5 @@
>  
> +    // Due to tethering types are constant and required before querying,
> +    // they are also hardcoded here.
> +    this._networks[TETHERING_TYPE_WIFI] = { tetheringType: TETHERING_TYPE_WIFI,
> +                                            interfaceName: null };

Where do you use the |interfaceName|?

@@ +108,5 @@
> +    // Due to tethering types are constant and required before querying,
> +    // they are also hardcoded here.
> +    this._networks[TETHERING_TYPE_WIFI] = { tetheringType: TETHERING_TYPE_WIFI,
> +                                            interfaceName: null };
> +    this._networks[TETHERING_TYPE_USB] = { tetheringType: TETHERING_TYPE_USB,

Ditto.

@@ +109,5 @@
> +    // they are also hardcoded here.
> +    this._networks[TETHERING_TYPE_WIFI] = { tetheringType: TETHERING_TYPE_WIFI,
> +                                            interfaceName: null };
> +    this._networks[TETHERING_TYPE_USB] = { tetheringType: TETHERING_TYPE_USB,
> +                                           interfaceName: null };

Where do you use the |interfaceName|?

@@ +279,5 @@
> +      return null;
> +    }
> +
> +    // TODO Bug 939046 Receive a wrong network type when data connection is
> +    // disconnected.

I don't understand this part. Could you please come by to discuss? Thanks!

@@ +282,5 @@
> +    // TODO Bug 939046 Receive a wrong network type when data connection is
> +    // disconnected.
> +    let type = aNetwork.type;
> +    if (type == NET_TYPE_MOBILE_SUPL ||
> +        type == NET_TYPE_MOBILE_MMS) {

Don't need to wrap this condition.

@@ +287,5 @@
> +      type = NET_TYPE_MOBILE;
> +    }
> +
> +    if (type != NET_TYPE_WIFI &&
> +        type != NET_TYPE_MOBILE) {

Don't need to wrap this condition.

@@ +307,5 @@
>  
>      if (!this._networks[netId]) {
>        this._networks[netId] = Object.create(null);
>        this._networks[netId].network = { id: id,
> +                                        type: type };

Don't need to wrap this object.

@@ +355,5 @@
>      debug("Init usage alarms");
>      let self = this;
>  
>      for (let netId in this._networks) {
> +      if (this._networks[netId].tetheringType) {

Please directly check the nedId to break.

@@ +420,5 @@
> +                mm.sendAsyncMessage("NetworkStats:Get:Return",
> +                                    { id: msg.id, error: aError, result: aResult });
> +              }, appId, serviceType, network, start, end, appManifestURL);
> +            });
> +          return;

Align this.

@@ +525,5 @@
>  
>      let elements = [];
>      let lastElement;
>  
> +    // For each available network and tethering create an object containning

Drop "and tethering" since available networks includes network interfaces and network tethering.

@@ +526,5 @@
>      let elements = [];
>      let lastElement;
>  
> +    // For each available network and tethering create an object containning
> +    // the identification (netId or tetheringType) and the 'queueIndex', the

s/, the/, where the/

@@ +528,5 @@
>  
> +    // For each available network and tethering create an object containning
> +    // the identification (netId or tetheringType) and the 'queueIndex', the
> +    // 'queueIndex' is an integer representing the index of a network or
> +    // tethering in the global queue array. So, if the network or tethering is

Drop /or tethering/

@@ +580,5 @@
>        this.updateQueue[index].callbacks.push(aCallback);
>        return;
>      }
>  
> +    if (this.isQueueRunning) {

Need discussion.

@@ +645,5 @@
>        return;
>      }
>  
> +    let network = null;
> +    if (this._networks[aNetId].tetheringType) {

Can we just use the aNetId which equals to the _networks[aNetId].tetheringType?

@@ +649,5 @@
> +    if (this._networks[aNetId].tetheringType) {
> +      network = networkManager.active;
> +    }
> +
> +    if (network) {

Don't use network to branch. Please just move the logic into:

if (this._networks[aNetId].tetheringType) {
  ...
}

@@ +733,5 @@
>    saveStats: function saveStats(aAppId, aServiceType, aNetwork, aTimeStamp,
>                                  aRxBytes, aTxBytes, aIsAccumulative,
>                                  aCallback) {
> +    let callback = aCallback;
> +    if (callback && callback.notify) {

Is that possible to unify the callers of saveStats so that we don't need to do this?

@@ +770,5 @@
>                    txBytes:        aTxBytes,
>                    isAccumulative: aIsAccumulative };
>  
> +    // If coming stats is accumulative, it will be directly saved into database.
> +    if (aIsAccumulative) {

I don't understand this part. Need discussions.

	Gene Lian (I already quit Mozilla)
	Comment 35 • 11 years ago

Comment on attachment 8362871 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

::: dom/network/src/NetworkStatsService.jsm
@@ +32,5 @@
> +const NET_TYPE_MOBILE_MMS = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_MMS;
> +const NET_TYPE_MOBILE_SUPL = Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE_SUPL;
> +
> +const TETHERING_TYPE_WIFI = "WiFi";
> +const TETHERING_TYPE_USB = "USB";

After some thoughts, may we do? What do you think?

const long NETWORK_TYPE_UNKNOWN        = -1;
const long NETWORK_TYPE_WIFI           = 0;
const long NETWORK_TYPE_MOBILE         = 1;
const long NETWORK_TYPE_MOBILE_MMS     = 2;
const long NETWORK_TYPE_MOBILE_SUPL    = 3;
const long NETWORK_TYPE_TETHERING_WIFI = 4;
const long NETWORK_TYPE_TETHERING_USB  = 5;

@@ +34,5 @@
> +
> +const TETHERING_TYPE_WIFI = "WiFi";
> +const TETHERING_TYPE_USB = "USB";
> +
> +const SERVICE_TYPE_TETHERING_WIFI = "Tethering";

Why not SERVICE_TYPE_TETHERING?

@@ +90,1 @@
>      // The network type can be 0 for wifi or 1 for mobile. On the other hand,

Don't need to change a new line.

@@ +102,2 @@
>      // networkInterface per network but can't exist a networkInterface not
>      // being mapped to a network.

The above paragraph should be moved to the: 1. For network interfaces. Also, lots of redundant descriptions like (wlan0, rmnet0, etc.). Would you please rephrase them as well?

@@ +115,5 @@
> +    // Because tethering types are constant and can be required before querying,
> +    // they are also hardcoded here.
> +    netId = this.getNetworkId('', TETHERING_TYPE_WIFI);
> +    this._networks[netId] = { tetheringType: TETHERING_TYPE_WIFI }};
> +    netId = this.getNetworkId('', TETHERING_TYPE_USB);

Add one blank line above.

@@ +351,5 @@
>      let self = this;
>  
>      for (let netId in this._networks) {
> +      // Do not set alarm for network tethering.
> +      if (this._networks[netId].tetheringType) {

I'd prefer add a utility to check the netId:

isTetheringNetwork(netId) {
  return (netId == TETHERING_TYPE_WIFI || netId == TETHERING_TYPE_USB);
}

Then you don't need tetheringType. It's more clear to me to check it instead of relying on the presence of a property.

@@ +402,5 @@
>  
>      // Check if the network is currently active. If yes, we need to update
>      // the cached stats first before retrieving stats from the DB.
>      if (this._networks[netId]) {
> +      let updateList = [netId, TETHERING_TYPE_WIFI, TETHERING_TYPE_USB];

Still don't understand this part. getSamples is aimed to get samples and before doing this we have to update the cached. Why not just updating the tethering cache when we really want to get its samples?

@@ +653,5 @@
>        return;
>      }
>  
> +    let network = networkManager.active;
> +    let tetheringType = this._networks[aNetId].tetheringType;

Ditto. Use that utility function instead.

@@ +654,5 @@
>      }
>  
> +    let network = networkManager.active;
> +    let tetheringType = this._networks[aNetId].tetheringType;
> +    if (!network && !tetheringType) {

Why not |tetheringType|?

@@ +718,5 @@
> +
> +    debug("tetheringStatsAvailable: " + aType + " " + aNetwork.type + " " +
> +          aResult + " " + aRxBytes + " " + aTxBytes + " " + aDate);
> +
> +    let serviceType = aType + ' ' + SERVICE_TYPE_TETHERING;

Why?

	John Shih Assignee
	Comment 37 • 11 years ago

Hi Gene, Albert,
Since the way to get tethering stats is so mush similar to interface stats, it makes me to think how to integrate them. After some consideration, I decide to move the logic of handling accumulative stats (such as the use of  rxSystemBytes/txSystemBytes) from NetworkStatsDB.jsm to NetworkStatsService.jsm. In short, with this work, all network stats (per-app/per-service/per-interface) will all firstly be cached in NetworkStatsService.jsm and then get update when getSample() or updateAllStats() is called. I think this design can make our current architecture more clear and also solve some problems.
Indeed, it needs your feedback first, thanks!

	John Shih Assignee
	Comment 38 • 11 years ago

patch updated.

	Albert [:albert]
	Comment 40 • 11 years ago

Comment on attachment 8377018 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

Lot of changes, great work but there is something wrong.

I've been doing some testing and reported stats are greater than the real ones. In order to detect it, launch usage application and compare results with the report of the system:
adb shell cat /proc/net/dev

It seems that stats from the API are the double of the real stats.

BTW, Part 1 and Part 2 need a small rebase.

	John Shih Assignee
	Comment 41 • 11 years ago

(In reply to Albert [:albert] from comment #40)
> Comment on attachment 8377018 [details] [diff] [review]
> Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> 
> Review of attachment 8377018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Lot of changes, great work but there is something wrong.
> 
> I've been doing some testing and reported stats are greater than the real
> ones. In order to detect it, launch usage application and compare results
> with the report of the system:
> adb shell cat /proc/net/dev
> 
> It seems that stats from the API are the double of the real stats.
> 
> BTW, Part 1 and Part 2 need a small rebase.

Thanks for pointing this out!
After some investigation, it is caused by continuously receives NetworkStats:Get in very short time.
Cached can not be cleared before the next getSample() is executed.
I'm working on it.

LOG:
I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats: Tue Feb 18 2014 15:23:46 GMT+0800 (CST)
I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats with key: 000
I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:MaxStorageAge
I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
I/Gecko   (  573): -*- NetworkStatsService: getstats for network 0 type 0
I/Gecko   (  573): -*- NetworkStatsService: appId: 0 from appManifestURL: null
I/Gecko   (  573): -*- NetworkStatsService: serviceType: 
I/Gecko   (  573): -*- NetworkStatsService: Get for interface stats 00
I/Gecko   (  573): -*- NetworkStatsService: Update stats for wlan0
I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:MaxStorageAge
I/Gecko   (  573): -*- NetworkStatsService: networkStatsAvailable: 00 true 2087871 204247 Tue Feb 18 2014 15:30:46 GMT+0800 (CST)
I/Gecko   (  573): -*- NetworkStatsService: saveStats: 0  00 Tue Feb 18 2014 15:30:46 GMT+0800 (CST) 204247 2087871
I/Gecko   (  573): -*- NetworkStatsService: CachedStats: {"000":{"appId":0,"serviceType":"","networkId":"0","networkType":0,"date":"2014-02-18T07:30:46.000Z","rxBytes":5791,"txBytes":125313,"rxSystemBytes":204247,"txSystemBytes":2087871,"isAccumulative":true},"100900":{"appId":1009,"serviceType":"","networkId":"0","networkType":0,"date":"2014-02-18T07:25:20.175Z","rxBytes":505849,"txBytes":30188,"rxSystemBytes":0,"txSystemBytes":0,"isAccumulative":false}}
I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats: Tue Feb 18 2014 15:23:46 GMT+0800 (CST)
I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats with key: 000
I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
I/Gecko   (  573): -*- NetworkStatsService: getstats for network 0 type 0
I/Gecko   (  573): -*- NetworkStatsService: appId: 0 from appManifestURL: null
I/Gecko   (  573): -*- NetworkStatsService: serviceType: 
I/Gecko   (  573): -*- NetworkStatsService: Get for interface stats 00
I/Gecko   (  573): -*- NetworkStatsService: Update stats for wlan0
I/Gecko   (  573): -*- NetworkStatsService: networkStatsAvailable: 00 true 2087871 204247 Tue Feb 18 2014 15:30:46 GMT+0800 (CST)
I/Gecko   (  573): -*- NetworkStatsService: saveStats: 0  00 Tue Feb 18 2014 15:30:46 GMT+0800 (CST) 204247 2087871
I/Gecko   (  573): -*- NetworkStatsService: CachedStats: {"000":{"appId":0,"serviceType":"","networkId":"0","networkType":0,"date":"2014-02-18T07:30:46.000Z","rxBytes":5791,"txBytes":125313,"rxSystemBytes":204247,"txSystemBytes":2087871,"isAccumulative":true},"100900":{"appId":1009,"serviceType":"","networkId":"0","networkType":0,"date":"2014-02-18T07:25:20.175Z","rxBytes":505849,"txBytes":30188,"rxSystemBytes":0,"txSystemBytes":0,"isAccumulative":false}}
I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats: Tue Feb 18 2014 15:23:46 GMT+0800 (CST)
I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats with key: 000
I/Gecko   (  573): -*- NetworkStatsService: Application stats inserted in indexedDB
I/Gecko   (  573): -*- NetworkStatsService: Application stats inserted in indexedDB
I/Gecko   (  573): -*- NetworkStatsService: Application stats inserted in indexedDB

	Albert [:albert]
	Comment 42 • 11 years ago

(In reply to John Shih from comment #41)
> (In reply to Albert [:albert] from comment #40)
> > Comment on attachment 8377018 [details] [diff] [review]
> > Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert
> > 
> > Review of attachment 8377018 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Lot of changes, great work but there is something wrong.
> > 
> > I've been doing some testing and reported stats are greater than the real
> > ones. In order to detect it, launch usage application and compare results
> > with the report of the system:
> > adb shell cat /proc/net/dev
> > 
> > It seems that stats from the API are the double of the real stats.
> > 
> > BTW, Part 1 and Part 2 need a small rebase.
> 
> Thanks for pointing this out!
> After some investigation, it is caused by continuously receives
> NetworkStats:Get in very short time.
> Cached can not be cleared before the next getSample() is executed.
> I'm working on it.
> 
> LOG:
> I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats: Tue Feb 18
> 2014 15:23:46 GMT+0800 (CST)
> I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats with key: 000
> I/Gecko   (  573): -*- NetworkStatsService: receiveMessage
> NetworkStats:MaxStorageAge
> I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
> I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
> I/Gecko   (  573): -*- NetworkStatsService: getstats for network 0 type 0
> I/Gecko   (  573): -*- NetworkStatsService: appId: 0 from appManifestURL:
> null
> I/Gecko   (  573): -*- NetworkStatsService: serviceType: 
> I/Gecko   (  573): -*- NetworkStatsService: Get for interface stats 00
> I/Gecko   (  573): -*- NetworkStatsService: Update stats for wlan0
> I/Gecko   (  573): -*- NetworkStatsService: receiveMessage
> NetworkStats:MaxStorageAge
> I/Gecko   (  573): -*- NetworkStatsService: networkStatsAvailable: 00 true
> 2087871 204247 Tue Feb 18 2014 15:30:46 GMT+0800 (CST)
> I/Gecko   (  573): -*- NetworkStatsService: saveStats: 0  00 Tue Feb 18 2014
> 15:30:46 GMT+0800 (CST) 204247 2087871
> I/Gecko   (  573): -*- NetworkStatsService: CachedStats:
> {"000":{"appId":0,"serviceType":"","networkId":"0","networkType":0,"date":
> "2014-02-18T07:30:46.000Z","rxBytes":5791,"txBytes":125313,"rxSystemBytes":
> 204247,"txSystemBytes":2087871,"isAccumulative":true},"100900":{"appId":1009,
> "serviceType":"","networkId":"0","networkType":0,"date":"2014-02-18T07:25:20.
> 175Z","rxBytes":505849,"txBytes":30188,"rxSystemBytes":0,"txSystemBytes":0,
> "isAccumulative":false}}
> I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats: Tue Feb 18
> 2014 15:23:46 GMT+0800 (CST)
> I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats with key: 000
> I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
> I/Gecko   (  573): -*- NetworkStatsService: receiveMessage NetworkStats:Get
> I/Gecko   (  573): -*- NetworkStatsService: getstats for network 0 type 0
> I/Gecko   (  573): -*- NetworkStatsService: appId: 0 from appManifestURL:
> null
> I/Gecko   (  573): -*- NetworkStatsService: serviceType: 
> I/Gecko   (  573): -*- NetworkStatsService: Get for interface stats 00
> I/Gecko   (  573): -*- NetworkStatsService: Update stats for wlan0
> I/Gecko   (  573): -*- NetworkStatsService: networkStatsAvailable: 00 true
> 2087871 204247 Tue Feb 18 2014 15:30:46 GMT+0800 (CST)
> I/Gecko   (  573): -*- NetworkStatsService: saveStats: 0  00 Tue Feb 18 2014
> 15:30:46 GMT+0800 (CST) 204247 2087871
> I/Gecko   (  573): -*- NetworkStatsService: CachedStats:
> {"000":{"appId":0,"serviceType":"","networkId":"0","networkType":0,"date":
> "2014-02-18T07:30:46.000Z","rxBytes":5791,"txBytes":125313,"rxSystemBytes":
> 204247,"txSystemBytes":2087871,"isAccumulative":true},"100900":{"appId":1009,
> "serviceType":"","networkId":"0","networkType":0,"date":"2014-02-18T07:25:20.
> 175Z","rxBytes":505849,"txBytes":30188,"rxSystemBytes":0,"txSystemBytes":0,
> "isAccumulative":false}}
> I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats: Tue Feb 18
> 2014 15:23:46 GMT+0800 (CST)
> I/Gecko   (  573): -*- NetworkStatsService: updateCachedStats with key: 000
> I/Gecko   (  573): -*- NetworkStatsService: Application stats inserted in
> indexedDB
> I/Gecko   (  573): -*- NetworkStatsService: Application stats inserted in
> indexedDB
> I/Gecko   (  573): -*- NetworkStatsService: Application stats inserted in
> indexedDB

Yes, this is because usage app has two parts, the web application and the widget, and both are requesting the get

	John Shih Assignee
	Comment 43 • 11 years ago

(In reply to Albert [:albert] from comment #42)
> (In reply to John Shih from comment #41)
> > (In reply to Albert [:albert] from comment #40)
> 
> Yes, this is because usage app has two parts, the web application and the
> widget, and both are requesting the get
>

I think we may need a queue for getSample() to solve the problem, how do you think?

	Albert [:albert]
	Comment 44 • 11 years ago

(In reply to John Shih from comment #43)
> (In reply to Albert [:albert] from comment #42)
> > (In reply to John Shih from comment #41)
> > > (In reply to Albert [:albert] from comment #40)
> > 
> > Yes, this is because usage app has two parts, the web application and the
> > widget, and both are requesting the get
> >
> 
> I think we may need a queue for getSample() to solve the problem, how do you
> think?

Maybe is better to enqueue the cached array operations, as it is the resource that need a lock.

BTW, I would appreciate if you use more functions, some ones are big and makes harder to understand the code. :)

	John Shih Assignee
	Comment 45 • 11 years ago

(In reply to Albert [:albert] from comment #44)
> (In reply to John Shih from comment #43)
> > (In reply to Albert [:albert] from comment #42)
> > > (In reply to John Shih from comment #41)
> > > > (In reply to Albert [:albert] from comment #40)
> > > 
> > > Yes, this is because usage app has two parts, the web application and the
> > > widget, and both are requesting the get
> > >
> > 
> > I think we may need a queue for getSample() to solve the problem, how do you
> > think?
> 
> Maybe is better to enqueue the cached array operations, as it is the
> resource that need a lock.
> 

Agree.

> BTW, I would appreciate if you use more functions, some ones are big and
> makes harder to understand the code. :)

no problem!

	Albert [:albert]
	Comment 46 • 11 years ago

Comment on attachment 8377020 [details] [diff] [review]
Bug 928289 - Part 3: Tests. r=gene, albert

Removing review flag until part 2 finished.

	John Shih Assignee
	Comment 47 • 11 years ago

Comment on attachment 8375280 [details] [diff] [review]
Bug 928289 - Part 1: Support obtain tetherging stats from netd. r=vchang

Remove review flag since the patch is needed to re-base.

	John Shih Assignee
	Comment 48 • 11 years ago

Comment on attachment 8377018 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

Remove review flag since the patch is needed to re-base.

	John Shih Assignee
	Comment 49 • 11 years ago

Comment on attachment 8377020 [details] [diff] [review]
Bug 928289 - Part 3: Tests. r=gene, albert

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

Remove review flag since the patch is needed to re-base.

	John Shih Assignee
	Comment 50 • 11 years ago

remove needinfo request also ;)

	John Shih Assignee
	Comment 51 • 11 years ago

patch update.

	John Shih Assignee
	Comment 52 • 11 years ago

Patch update.

	Albert [:albert]
	Comment 53 • 11 years ago

Comment on attachment 8401200 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

Looks fine

::: dom/network/src/NetworkStatsDB.jsm
@@ +28,5 @@
>  // Constant defining the rate of the samples. Daily.
>  const SAMPLE_RATE = 1000 * 60 * 60 * 24;
>  
> +// Default service type.
> +const SERVICE_NO_TYPE = "";

You have this const in each of networkstats src files, should be better to move it to interface definition.

@@ +31,5 @@
> +// Default service type.
> +const SERVICE_NO_TYPE = "";
> +
> +// Default app id.
> +const APP_NO_ID = 0;

ditto

	Gene Lian (I already quit Mozilla)
	Comment 54 • 11 years ago

Comment on attachment 8401200 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

::: dom/network/src/NetworkStatsService.jsm
@@ +33,5 @@
> +const TETHERING_TYPE_WIFI = "WiFi";
> +const TETHERING_TYPE_USB = "USB";
> +
> +const SERVICE_TYPE_TETHERING = "tethering";
> +const SERVICE_NO_TYPE = "";

Please use SERVICE_TYPE_NONE which is symmetric to other SERVICE_TYPE_*.

@@ +35,5 @@
> +
> +const SERVICE_TYPE_TETHERING = "tethering";
> +const SERVICE_NO_TYPE = "";
> +
> +const APP_NO_ID = 0;

APP_ID_NONE is better too?

@@ +99,4 @@
>      //
> +    // 1. For network interfaces
> +    // Each network interface is composed by a network object (network type and
> +    // network Id) and a interfaceName that contains the name of the physical

s/a/an

	Gene Lian (I already quit Mozilla)
	Comment 55 • 11 years ago

Comment on attachment 8401200 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

One more run.

::: dom/network/src/NetworkStatsDB.jsm
@@ +315,5 @@
>              }
>            }, this);
>          };
> +      } else if (currVersion == 7) {
> +        // Remove rxSystemBytes/txSystemBytes from db.

Please add some comments here to explain why.

::: dom/network/src/NetworkStatsService.jsm
@@ +35,5 @@
> +
> +const SERVICE_TYPE_TETHERING = "tethering";
> +const SERVICE_NO_TYPE = "";
> +
> +const APP_NO_ID = 0;

After some thoughts, you can keep this if you found other codes like this (in Necko). Otherwise, APP_INVALID_ID might be better.

@@ +103,5 @@
> +    // 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 by the index of 'networkId + networkType'. Note that it's not
> +    // mandatory to have a networkInterface per network but can't exist a

s/exist/allow/

@@ +1023,5 @@
> +        terminal = keys.length - 1;
> +      }
> +    }
> +
> +    debug("updateStatsList: " +JSON.stringify(updateStatsList));

Add one space after '+'.

@@ +1097,5 @@
> +                    networkId:      this._networks[netId].network.id,
> +                    networkType:    this._networks[netId].network.type,
> +                    txBytes:        0,
> +                    rxBytes:        0,
> +                    date:           this.cachedStatsDate };

Too many spaces between properties and values.

@@ +1100,5 @@
> +                    rxBytes:        0,
> +                    date:           this.cachedStatsDate };
> +
> +      // Try to find tethering stats with the same netId in |cachedStats|.
> +      // If found, accumulative txBytes/rxBytes into |stats| and also reset

s/accumulative/accumulate/

@@ +1109,5 @@
> +        stats.txBytes += cachedTetheringStats.txBytes;
> +        stats.rxBytes += cachedTetheringStats.rxBytes;
> +        cachedTetheringStats.txBytes = cachedTetheringStats.rxBytes = 0;
> +      }
> +      if (this.cachedStats[wifiKey] != undefined) {

Please combine the common logic between this condition and the above one.

@@ +1115,5 @@
> +        stats.txBytes += cachedTetheringStats.txBytes;
> +        stats.rxBytes += cachedTetheringStats.rxBytes;
> +        cachedTetheringStats.txBytes = cachedTetheringStats.rxBytes = 0;
> +      }
> +      // Check if there is found tethering stats in |cachedStats|.

s/there is found tethering stats/tethering stats is found/

	Ken Chang[:kenkai]
	Comment 56 • 11 years ago

Put this bug into backlog.

	John Shih Assignee
	Comment 57 • 10 years ago

Gene, can you take another look? Thanks!

	John Shih Assignee
	Comment 58 • 10 years ago

patch update

	Gene Lian (I already quit Mozilla)
	Comment 59 • 10 years ago

Comment on attachment 8448554 [details] [diff] [review]
Bug 928289 - Part 2: NetworkStatsService.jsm modification. r=gene, albert

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +323,5 @@
>          if (DEBUG) {
>            debug("Create index of 'serviceType' for version 8");
>          }
> +      } else if (currVersion == 8) {
> +        // Since rxSystemBytes/txSystemBytes are now handle in

s/handle/handled/

@@ +326,5 @@
> +      } else if (currVersion == 8) {
> +        // Since rxSystemBytes/txSystemBytes are now handle in
> +        // NetworkStatsService by using cache, these data is no longer needed to
> +        // be stored in database.
> +        // Remove rxSystemBytes/txSystemBytes from db.

Combine this line into the above line.

::: dom/network/src/NetworkStatsService.jsm
@@ +100,5 @@
> +    // 1. For network interfaces
> +    // Each network interface is composed by a network object (network type and
> +    // network Id) and an 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

s/is/can be/

@@ +103,5 @@
> +    // 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 by the index of 'networkId + networkType'. Note that it's not
> +    // mandatory to have a networkInterface per network but can't allow a

Change the sentence

but can't allow a networkInterface not being mapped to a network.

to

but it's not allowed to have a networkInterface not being mapped to a network.

@@ +747,5 @@
>        }
>        return;
>      }
>  
> +    // Request tethering stats to NetworkManager, which will get stats from

s/to/from/

@@ +808,3 @@
>  
> +  /*
> +   * Callback of request tethering stats. Save stats into cache.

s/request/requesting/

@@ +937,5 @@
>        }.bind(this));
>        return;
>      }
>  
> +    // If not found matched row, save the incoming data into |cachedStats|.

s/.../If no matched row is found/

@@ +957,3 @@
>  
> +    // Date in the row may be old due to cached update process, so keep it
> +    // up-to-date in each time receving new data.

s/in each time/whenever/

::: dom/network/src/NetworkStatsServiceProxy.js
@@ +17,5 @@
>  const NETWORKSTATSSERVICEPROXY_CONTRACTID = "@mozilla.org/networkstatsServiceProxy;1";
>  const NETWORKSTATSSERVICEPROXY_CID = Components.ID("705c01d6-8574-464c-8ec9-ac1522a45546");
>  const nsINetworkStatsServiceProxy = Ci.nsINetworkStatsServiceProxy;
>  
> +const SERVICE_NO_TYPE = "";

s/SERVICE_NO_TYPE/SERVICE_TYPE_NONE/

@@ +18,5 @@
>  const NETWORKSTATSSERVICEPROXY_CID = Components.ID("705c01d6-8574-464c-8ec9-ac1522a45546");
>  const nsINetworkStatsServiceProxy = Ci.nsINetworkStatsServiceProxy;
>  
> +const SERVICE_NO_TYPE = "";
> +const APP_NO_TYPE = 0;

s/APP_NO_TYPE/APP_INVALID_ID/?

@@ +50,5 @@
>      if (aCallback) {
>        aCallback = aCallback.notify;
>      }
>  
> +    NetworkStatsService.saveStats(aAppId, SERVICE_NO_TYPE, aNetwork, aTimeStamp,

Ditto.

@@ +79,5 @@
>      if (aCallback) {
>        aCallback = aCallback.notify;
>      }
>  
> +    NetworkStatsService.saveStats(APP_NO_TYPE, aServiceType ,aNetwork,

DItto.

	Gene Lian (I already quit Mozilla)
	Comment 60 • 10 years ago

Comment on attachment 8448555 [details] [diff] [review]
Bug 928289 - Part 3: Tests. r=gene, albert

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

::: dom/network/tests/unit_stats/test_networkstats_service.js
@@ +9,5 @@
>  
>  const QUEUE_TYPE_UPDATE_STATS = 0;
>  
>  var wifiId = '00';
> +var wifiTethering = 'WiFi';

s/wifiTethering/tetheringTypeWifi

::: dom/network/tests/unit_stats/test_networkstats_service_proxy.js
@@ +196,5 @@
>  
> +add_test(function test_saveTetheringStats() {
> +  var timestamp = NetworkStatsService.cachedStatsDate.getTime();
> +
> +  // Create to fake nsINetworkInterfaces. As nsINetworkInterface can not

s/to fake/a fake/

	Vincent Chang[:vchang][changyihsin]
	Comment 61 • 10 years ago

Comment on attachment 8401199 [details] [diff] [review]
Bug 928289 - Part 1: Support obtain tetherging stats from netd. r=vchang

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

I think this should be rebased.

	BMO Automation
	Comment 62 • 7 years ago

Firefox OS is not being worked on