Albert [:albert] Assignee
	Description • 11 years ago

Enable alarms to notify when data usage raise to the specified threshold

	Albert [:albert] Assignee
	Comment 1 • 11 years ago

I made a proof of concept and I am able to put an alarm and notify it when the amount of usage has been reached using the bandwidth controller of netd (which uses iptables).

There are a couple of things needed to implement it:

- Fix bug 783966 to be able to get netd broadcast messages (which notifies gecko that the amount of data is reached).

- Fix a bug in netd that produces a segmentation fault, it is solved in newer versions of netd.

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

Comment on attachment 740884 [details] [diff] [review]
Netd integration

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

Looks good, thank you.

	Albert [:albert] Assignee
	Comment 5 • 11 years ago

Added definition of UsageAlarm object and options to set an alarm

	Albert [:albert] Assignee
	Comment 6 • 11 years ago

Try to update alarm and enable it if process fails. Before this logic was placed in the remote service.

	Albert [:albert] Assignee
	Comment 8 • 11 years ago

While I work on adding tests I would like to get feedback from Salva if he can test patches in costcontrol.

	Salvador de la Puente González [:salva]
	Comment 9 • 11 years ago

Ok, we have another priorities now, but I'm aware.

	Albert [:albert] Assignee
	Comment 16 • 11 years ago

Comment on attachment 753698 [details] [diff] [review]
Part 2/4: Netd integration

Since you gave the r+, implementation patch makes me add some functions. Now implementation patch is finished.

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

I'm struggling with some tef+ bugs. I'll come back to review this ASAP before Wednesday or Thursday. Thanks!

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

I haven't really looked through the patches in details. Just having a quick question: why do we need an alarm to notify users at a specific time point? I cannot figure the role of alarm playing here by reading the bug title. Can we simply fire system message for that?

	Albert [:albert] Assignee
	Comment 19 • 11 years ago

(In reply to Gene Lian [:gene] from comment #18)
> I haven't really looked through the patches in details. Just having a quick
> question: why do we need an alarm to notify users at a specific time point?
> I cannot figure the role of alarm playing here by reading the bug title. Can
> we simply fire system message for that?

Actually the application is notified through a system message, but internally I'm using an alarm to allow apps (costcontrol) to set a data usage alarm based on the usage threshold || expiration date. Your point here is that you don't see useful to set an expiration date?

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

What I don't really understand is you're going to warn the costcontrol app when the data usage reaches the specified threshold. Right? How does this purpose relate to the ideas of *alarms* (which can help you fire notifications at the specific time points). My question is why do you need alarms? Can we just fire system message immediately when the data usage exceeds the threshold?

It seems it helps the *expiration date* you're talking about here. Could you please give me a brief explanation about its purpose?

I'm reviewing your patch now. That would be helpful if you could return me some feedback at the same time. Thanks!

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

Comment on attachment 756374 [details] [diff] [review]
Part 3/4: Alarm implementation

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

This patch is really big to me. I need to take more reviews on this. I haven't really looked into all the details of the state machines in the NetworkStatsService.jsm. However, there is at least a wrong usage of the system message, That is, you cannot use the "alarm" again because that will break the current compatibility with the Gaia apps that used to register system message handlers for it, so giving review- for now.

Could you please answer my question at comment #20 as well? Thanks!

::: dom/network/src/NetworkStatsDB.jsm
@@ +73,5 @@
> +        }
> +
> +      } else if (currVersion == 1) {
> +        objectStore = db.createObjectStore(ALARM_STORE_NAME, { keyPath: "id", autoIncrement: true });
> +        objectStore.createIndex("iface", ['connectionId','threshold'], { unique: false});

Add a space between "false" and "}".

@@ +74,5 @@
> +
> +      } else if (currVersion == 1) {
> +        objectStore = db.createObjectStore(ALARM_STORE_NAME, { keyPath: "id", autoIncrement: true });
> +        objectStore.createIndex("iface", ['connectionId','threshold'], { unique: false});
> +        objectStore.createIndex("data", "data", { unique: false});

Add a space between "false" and "}".

@@ +75,5 @@
> +      } else if (currVersion == 1) {
> +        objectStore = db.createObjectStore(ALARM_STORE_NAME, { keyPath: "id", autoIncrement: true });
> +        objectStore.createIndex("iface", ['connectionId','threshold'], { unique: false});
> +        objectStore.createIndex("data", "data", { unique: false});
> +        objectStore.createIndex("manifestURL", "manifestURL", { unique: false});

Add a space between "false" and "}".

@@ +76,5 @@
> +        objectStore = db.createObjectStore(ALARM_STORE_NAME, { keyPath: "id", autoIncrement: true });
> +        objectStore.createIndex("iface", ['connectionId','threshold'], { unique: false});
> +        objectStore.createIndex("data", "data", { unique: false});
> +        objectStore.createIndex("manifestURL", "manifestURL", { unique: false});
> +        objectStore.createIndex("pageURL", "pageURL", { unique: false});

Add a space between "false" and "}".

@@ +151,5 @@
>        throw new Error("Error processing samples");
>      }
>  
>      if (DEBUG) {
>        debug("New: " + newSample.timestamp + " - Last: " + lastSample.timestamp + " - diff: " + diff);

Since you're here, please fold this line.

@@ +160,3 @@
>      if (rxDiff < 0 || txDiff < 0) {
> +      rxDiff = newSample.rxSystemBytes;
> +      txDiff = newSample.txSystemBytes;

I don't really understand this logic. Could you please provide some hints to me about this if-block?

I sounds very strange to me to assign |rxDiff| and |txDiff| with summations and add them to the summations below again.

@@ +189,5 @@
> +                      txBytes:         0,
> +                      rxSystemBytes:   lastSample.rxSystemBytes,
> +                      txSystemBytes:   lastSample.txSystemBytes,
> +                      rxTotalBytes:    lastSample.rxTotalBytes,
> +                      txTotalBytes:    lastSample.txTotalBytes};

Since you're here please add spaces around |{| and |}|:

{ ...,
  ...,
  ... };

which looks a general coding style within this file.

@@ +274,5 @@
> +      request.onsuccess = function onsuccess(event) {
> +        txn.result = null;
> +        let cursor = event.target.result;
> +        if (cursor) {
> +           txn.result = cursor.value;

Two-space indention is enough.

@@ +459,5 @@
> +      };
> +    }.bind(this), aResultCb);
> +  },
> +
> +  getFirstAlarm: function getFirstAlarm(connectionId, aResultCb) {

s/connectionId/aConnectionId

@@ +468,5 @@
> +        debug("Get first alarm for connection " + connectionId);
> +      }
> +
> +      let lowFilter = [connectionId, 0];
> +      let upFilter = [connectionId, Number.MAX_VALUE];

s/lowFilter/lowerFilter
s/upFilter/upperFilter

which sounds better English words in my dictionary. However, you use these names everywhere within this file, so just keep as it is. Never mind.

Also, could you please use empty string for the upperFilter as below?

let upFilter = [connectionId, ""];

Numeric 0 is smaller than any time stamp and empty string is larger than all numeric values. In theory |Number.MAX_VALUE| is also a valid number although it's never going to happen.

@@ +498,5 @@
> +      }
> +    }, aResultCb);
> +  },
> +
> +  removeAppAlarm: function removeAppAlarm(aAlarmId, aManifestURL, aResultCb) {

Could you please combine .removeAlarm() and .removeAppAlarm() into:

removeAlarm: function removeAlarm(aAlarmId, aManifestURL, aResultCb) {
  ...
},

1. Consider |aManifestURL| as an extra filter. When it's null, don't check that; otherwise, delete the record when it's matched.

2. Use store.get(...) like what you did in the original .removeAppAlarm(). Using store.openCursor(...) sounds weird to me since you don't want to go through multiple records.

@@ +513,5 @@
> +      }
> +    }, aResultCb);
> +  },
> +
> +  removeAppAllAlarms: function removeAppAllAlarms(aManifestURL, aResultCb) {

If you don't have strong opinion, I'd prefer:

s/removeAppAllAlarms/removeAlarms/

which sounds simpler to me.

Also, maybe you need to return true/false for this function for symmetry, since you did it for .removeAlarm(...)/.updateAlarm(...).

@@ +516,5 @@
> +
> +  removeAppAllAlarms: function removeAppAllAlarms(aManifestURL, aResultCb) {
> +    this.dbNewTxn(ALARM_STORE_NAME, "readwrite", function(txn, store) {
> +      if (DEBUG) {
> +        debug("Remove alarms of " + aManifestURL.aAlarmId);

s/aManifestURL.aAlarmId/aManifestURL

aManifestURL should be a string. Right?

@@ +519,5 @@
> +      if (DEBUG) {
> +        debug("Remove alarms of " + aManifestURL.aAlarmId);
> +      }
> +
> +      store.index("manifestURL").openCursor(aManifestURL).onsuccess = function onsuccess(event) {

Please fold the above line like:

store.index("manifestURL").openCursor(aManifestURL)
                          .onsuccess = function onsuccess(event) {
  ...
}

@@ +537,5 @@
> +      if (DEBUG) {
> +        debug("Update alarm " + aAlarm.id);
> +      }
> +
> +      store.openCursor(record.id).onsuccess = function onsuccess(event) {

I'd prefer using .get(...) instead of .openCursor(...). Anyway, it should no big deal. Maybe just keep it as it is if you like.

@@ +548,5 @@
> +      }
> +    }, aResultCb);
> +  },
> +
> +  getAlarms: function getAlarms(connectionId, manifestURL, aResultCb) {

s/connectionId/aConnectionId
s/manifestURL/aManifestURL

@@ +555,5 @@
> +        debug("Get alarms for " + manifestURL);
> +      }
> +
> +      txn.result = [];
> +      store.index("manifestURL").openCursor(manifestURL).onsuccess = function onsuccess(event) {

Fold this line:

store.index("manifestURL").openCursor(aManifestURL)
                          .onsuccess = function onsuccess(event) {
  ...
}

@@ +563,5 @@
> +          if (connectionId == -1 || cursor.value.connectionId == connectionId) {
> +            let alarm = { id: cursor.value.id,
> +                          iface: cursor.value.connectionId,
> +                          threshold: cursor.value.threshold,
> +                          data: cursor.value.data };

Why not using this.recordToAlarm(cursor.value)?

@@ +575,5 @@
> +      }
> +    }, aResultCb);
> +  },
> +
> +  logAlarms: function logAlarms(manifestURL, aResultCb) {

Could you please combine .logAlarms() and .getAlarms() into a single one? When |aConnectionId| is null, don't filter out records by that.

::: dom/network/src/NetworkStatsManager.js
@@ +96,5 @@
> +// UsageAlarm
> +const USAGEALARM_CID      = Components.ID("{2dcd5394-ad90-45d0-a755-3da80095f22f}");
> +const nsIDOMMozUsageAlarm = Components.interfaces.nsIDOMMozUsageAlarm;
> +
> +function UsageAlarm(aAlarm) {

UsageAlarm sound a very general term to me. It's a bit implicit, although I don't understand yet what the main purpose it is. Let's figure out if we could have a better name for it to make it more related to networks statistics.

@@ +165,5 @@
>                            {id: this.getRequestId(request)});
>      return request;
>    },
>  
> +  addUsageAlarm: function addUsageAlarm(connectionType, threshold, options) {

s/connectionType/aConnectionType
s/threshold/aThreshold
s/options/aOptions

@@ +169,5 @@
> +  addUsageAlarm: function addUsageAlarm(connectionType, threshold, options) {
> +    this.checkPrivileges();
> +
> +    if (!options) {
> +      options = Object.create(null); 

Please fix the trailing space (TS).

@@ +178,5 @@
> +                          {id: this.getRequestId(request),
> +                           data: {connectionType: connectionType, threshold: threshold,
> +                                  alarmStart: options.alarmStart, alarmEnd: options.alarmEnd,
> +                                  data: options.data, manifestURL: this.manifestURL,
> +                                  pageURL: this.pageURL}});

Please fold these lines:

cpmm.sendAsyncMessage("NetworkStats:SetAlarm",
                      {id: this.getRequestId(request),
                       data: {connectionType: connectionType,
                              threshold: threshold,
                              alarmStart: options.alarmStart,
                              alarmEnd: options.alarmEnd,
                              data: options.data,
                              manifestURL: this.manifestURL,
                              pageURL: this.pageURL}});

@@ +182,5 @@
> +                                  pageURL: this.pageURL}});
> +    return request;
> +  },
> +
> +  getAllUsageAlarms: function getAllUsageAlarms(connectionType) {

s/connectionType/aConnectionType

Why not s/getAllUsageAlarms/getUsageAlarms? It doesn't get all because it's actually filtered by |aConnectionType|.

@@ +188,5 @@
> +
> +    let request = this.createRequest();
> +    cpmm.sendAsyncMessage("NetworkStats:GetAlarms",
> +                          {id: this.getRequestId(request),
> +                           data: {connectionType: connectionType, manifestURL: this.manifestURL}});

Wrap the lines in |data: {...}| like above.

@@ +192,5 @@
> +                           data: {connectionType: connectionType, manifestURL: this.manifestURL}});
> +    return request;
> +  },
> +
> +  removeUsageAlarms: function removeUsageAlarms(alarmId) {

s/alarmId/aAlarmId

Also, why not s/removeUsageAlarms/removeUsageAlarm/? You only remove single alarm. Right?

@@ +200,5 @@
> +      alarmId = -1;
> +    }
> +
> +    let request = this.createRequest();
> +    cpmm.sendAsyncMessage("NetworkStats:RemoveAlarms",

s/NetworkStats:RemoveAlarms/NetworkStats:RemoveAlarm/, maybe.

@@ +317,5 @@
>      this.initHelper(aWindow, ["NetworkStats:Get:Return",
> +                              "NetworkStats:Clear:Return",
> +                              "NetworkStats:SetAlarm:Return",
> +                              "NetworkStats:GetAlarms:Return",
> +                              "NetworkStats:RemoveAlarms:Return"]);

s/NetworkStats:RemoveAlarms:Return/NetworkStats:RemoveAlarm:Return/, maybe.

@@ +319,5 @@
> +                              "NetworkStats:SetAlarm:Return",
> +                              "NetworkStats:GetAlarms:Return",
> +                              "NetworkStats:RemoveAlarms:Return"]);
> +
> +    // init app properties

// "I"nit app properties"."

There is a convention for the comment:

//{one-space}{upper-case-letter}{...}{period}

@@ +325,5 @@
> +                        .getService(Ci.nsIAppsService);
> +
> +    this.manifestURL = appsService.getManifestURLByLocalId(principal.appId);
> +
> +    this.isApp = !(this.manifestURL.length == 0);

s/this.isApp/let isApp/because you don't use |this.isApp| else where.

Use the following looks cooler!

let isApp = !!this.manifestURL.length;

::: dom/network/src/NetworkStatsService.jsm
@@ +4,5 @@
>  
>  "use strict";
>  
>  const DEBUG = false;
> +function debug(s) { 

Please fix TS.

@@ +40,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "networkManager",
>                                     "@mozilla.org/network/manager;1",
>                                     "nsINetworkManager");
>  
> +XPCOMUtils.defineLazyGetter(this, "messenger", function() {

It's doesn't really matter but please use:

XPCOMUtils.defineLazyServiceGetter(...) which is used for others.

@@ +66,5 @@
>      this.messages = ["NetworkStats:Get",
>                       "NetworkStats:Clear",
> +                     "NetworkStats:SetAlarm",
> +                     "NetworkStats:GetAlarms",
> +                     "NetworkStats:RemoveAlarms",

s/NetworkStats:RemoveAlarms/NetworkStats:RemoveAlarm/

@@ +143,5 @@
> +        debug("Network " + network.name + " of type " +
> +              network.type + " registered");
> +
> +        let currentAlarm = this._currentAlarm[network.type];
> +        if (!(Object.getOwnPropertyNames(currentAlarm).length === 0) && !currentAlarm.registered) {

Use .length !== 0 instead of !(.length === 0).

@@ +163,5 @@
> +        for( let i in networkManager.networkInterfaces) {
> +          let iface = data.substring(data.lastIndexOf(" ") + 1);
> +          if (iface == networkManager.networkInterfaces[i].name) {
> +            let currentAlarm = this._currentAlarm[networkManager.networkInterfaces[i].type];
> +            if (!(Object.getOwnPropertyNames(currentAlarm).length === 0)) {

!== 0.

@@ +206,5 @@
> +      this._db.getFirstAlarm(connection,
> +                             function getResult(error, result) {
> +        if (!error && result) {
> +          self._setAlarm(result, function onSet(error, success){
> +            if (error == "InvalidStateError") {

Why do you fire alarms for invalid state errors during initialization? I don't get it. Could you please elaborate on this?

@@ +401,5 @@
>     * Callback of request stats. Store stats in database.
>     */
> +  networkStatsAvailable: function networkStatsAvailable(callback, result,
> +                                                        connType, rxBytes,
> +                                                        txBytes, date) {

OK. I kind of give up asking to add |a| for parameters since sometimes you used prefix |a| but sometimes you didn't. Well... when you see this comment just apply |a| for parameters *per* function. It's fine to me. :)

@@ +494,5 @@
> +      }
> +
> +      for (let i in self._currentAlarm) {
> +        let currentAlarm = self._currentAlarm[i].alarm;
> +        if (currentAlarm && ((alarmId == currentAlarm.id) || 

TS.

@@ +586,5 @@
> +      self._db.addUsageAlarm(aAlarm, function addSuccessCb(error, aNewId) {
> +        aAlarm.id = aNewId;
> +
> +        if (!error && aAlarm.alarmEnd) {
> +          let timeAlarm = { date: aAlarm.alarmEnd.getTime(),

Also, |timeAlarm| is a very weird term. Please just use |alarm|.

@@ +588,5 @@
> +
> +        if (!error && aAlarm.alarmEnd) {
> +          let timeAlarm = { date: aAlarm.alarmEnd.getTime(),
> +                            ignoreTimezone: false,
> +                            data: aAlarm};

Space before |}|.

@@ +605,5 @@
> +      });
> +    });
> +  },
> +
> +  _validateAlarm: function _validateAlarm(aAlarm, callback) {

s/callback/aCallback

@@ +606,5 @@
> +    });
> +  },
> +
> +  _validateAlarm: function _validateAlarm(aAlarm, callback) {
> +    // Validate correct alarm end

Add a period.

@@ +608,5 @@
> +
> +  _validateAlarm: function _validateAlarm(aAlarm, callback) {
> +    // Validate correct alarm end
> +    let now = Date.now();
> +    if ((aAlarm.alarmStart && aAlarm.alarmStart.getTime() >= now) || 

TS.

@@ +614,5 @@
> +      callback("InvalidDate", null);
> +      return;
> +    }
> +
> +    // Update Threshold will check if threshold is valid

Add period.

@@ +628,5 @@
> +    }
> +
> +    let self = this;
> +
> +    this._updateThreshold(aAlarm, function onUpdate(error, _threshold){

Add |a| for parameters.

@@ +660,5 @@
> +      }
> +    });
> +  },
> +
> +  _updateThreshold: function _updateThreshold(aAlarm, callback) {

aParameter.

@@ +665,5 @@
> +    let self = this;
> +
> +    this.updateStats(aAlarm.connectionId, function onStatsUpdated(aResult, aMessage) {
> +      self._db.getCurrentStats(self._connectionTypes[aAlarm.connectionId],
> +                               aAlarm.alarmStart, function onStatsFound(error, result) {

Wrap this line:

self._db.getCurrentStats(self._connectionTypes[aAlarm.connectionId],
                         aAlarm.alarmStart,
                         function onStatsFound(error, result) {

@@ +667,5 @@
> +    this.updateStats(aAlarm.connectionId, function onStatsUpdated(aResult, aMessage) {
> +      self._db.getCurrentStats(self._connectionTypes[aAlarm.connectionId],
> +                               aAlarm.alarmStart, function onStatsFound(error, result) {
> +        if (error) {
> +          debug("Error getting stats for " + self._connectionTypes[connection] + ": " + error);

Wrap the debug message.

@@ +680,5 @@
> +          return;
> +        }
> +
> +        let _threshold = { systemThreshold: result.rxSystemBytes + result.txSystemBytes + offset,
> +                           absoluteThreshold: result.rxTotalBytes + result.txTotalBytes + offset}

let _threshold = {
  systemThreshold: result.rxSystemBytes + result.txSystemBytes + offset,
  absoluteThreshold: result.rxTotalBytes + result.txTotalBytes + offset
};

Note: please add |;| for each statement.

@@ +687,5 @@
> +      });
> +    });
> +  },
> +
> +  _fireAlarm: function _fireAlarm(aAlarm, expired) {

s/expired/aExpired

@@ +691,5 @@
> +  _fireAlarm: function _fireAlarm(aAlarm, expired) {
> +    debug("Fire alarm");
> +
> +    let self = this;
> +    this._db.removeAlarm(aAlarm.id, function onRemove(error, result){

s/error/aError/
s/result/aResult/

@@ +729,5 @@
> +      });
> +    });
> +  },
> +
> +  _fireSystemMessage: function _fireSystemMessage(aAlarm, expired) {

s/expired/aExpired/

@@ +738,5 @@
> +
> +    let alarm = { "id":              aAlarm.id,
> +                  "threshold":       aAlarm.threshold,
> +                  "data":            aAlarm.data,
> +                  "expired":         expired};

Add a space before "}".

@@ +740,5 @@
> +                  "threshold":       aAlarm.threshold,
> +                  "data":            aAlarm.data,
> +                  "expired":         expired};
> +
> +    messenger.sendMessage("alarm", alarm, pageURI, manifestURI);

I don't like you reuse the "alarm" system message type here because the system message handler would think the "alarm" message object should have a specific set of properties. Please see AlarmService._publicAlarm(...).

Please choose another system message type for the network metering purpose.

@@ +743,5 @@
> +
> +    messenger.sendMessage("alarm", alarm, pageURI, manifestURI);
> +  },
> +
> +  _onAlarmExpired: function _onAlarmExpired(timeAlarm) {

s/timeAlarm/aAlarm

	Albert [:albert] Assignee
	Comment 22 • 11 years ago

(In reply to Gene Lian [:gene] from comment #20)
> What I don't really understand is you're going to warn the costcontrol app
> when the data usage reaches the specified threshold. Right?

Yes you are right.

> How does this purpose relate to the ideas of *alarms* (which can help you fire
> notifications at the specific time points). My question is why do you need
> alarms? Can we just fire system message immediately when the data usage
> exceeds the threshold?

That's the idea.

> It seems it helps the *expiration date* you're talking about here. Could you
> please give me a brief explanation about its purpose?

We thought that will be interesting to allow the user to set an expiration date to the usage alarm, then if threshold is not achieved in that date, the usage alarm is removed. However we have no use case where it is necessary. I can remove this feature while it is not necessary. Then I won't need to internally deal with alarms. Does it make sense to you?

> I'm reviewing your patch now. That would be helpful if you could return me
> some feedback at the same time. Thanks!

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

(In reply to Albert from comment #22)
> (In reply to Gene Lian [:gene] from comment #20)
> > It seems it helps the *expiration date* you're talking about here. Could you
> > please give me a brief explanation about its purpose?
> 
> We thought that will be interesting to allow the user to set an expiration
> date to the usage alarm, then if threshold is not achieved in that date, the
> usage alarm is removed. However we have no use case where it is necessary. I
> can remove this feature while it is not necessary. Then I won't need to
> internally deal with alarms. Does it make sense to you?

OK. now I understand better. :)

I'm totally glad to see new features. However, new features have to be fully discussed before implementations, especially when it needs to change the public APIs. We have to avoid designing API that is hard to use or is not going to be used. I cannot make the final decision whether to include the idea of *usage alarms* in the Network Stats API or not (maybe Jonas can decide that).

Btw, I'm thinking since we've already had Alarm API [1], is that possible for the Costcontrol App to directly set alarms through Alarm API? The alarm can carry the threshold, network interface type and other miscellaneous data you want to persist. When the alarm fires, its system message handler can then query and check if the network traffic exceeds the threshold at that time point through the existing Network Stats API. It seems to me everything can be done within the content site and you don't even need to add new APIs at all. Lot's of work you've done here have almost the same logic of the Alarm API implementation.

[1] https://wiki.mozilla.org/WebAPI/AlarmAPI

	Salvador de la Puente González [:salva]
	Comment 24 • 11 years ago

Hello Gene

(In reply to Gene Lian [:gene] from comment #23)
> Btw, I'm thinking since we've already had Alarm API [1], is that possible
> for the Costcontrol App to directly set alarms through Alarm API? The alarm
> can carry the threshold, network interface type and other miscellaneous data
> you want to persist.

I totally agree with you. Probably it's better to move this to the FE and only if we encounter a BE issue, then move the problematic to the BE.

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

Comment on attachment 753698 [details] [diff] [review]
Part 2/4: Netd integration

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

::: dom/system/gonk/NetworkManager.js
@@ +395,5 @@
> +    params.report = true;
> +    params.isAsync = true;
> +
> +    this.controlMessage(params, function(result) {
> +      if (result.resultCode >= NETD_COMMAND_OKAY && result.resultCode < NETD_COMMAND_FAIL) {

How about use isError() to check the error code ?

@@ +400,5 @@
> +        callback.networkUsageAlarmResult(null);
> +        return;
> +      }
> +
> +      this._enableNetworkInterfaceAlarm(iface, threshold, callback);

If I read the code correctly, we have a error from netd if we run to here right ?
Why do we need to call _enableNetworkInterfaceAlarm() when we fail to set network interface alarm ?

	Albert [:albert] Assignee
	Comment 26 • 11 years ago

(In reply to Vincent Chang[:vchang] from comment #25)
> Comment on attachment 753698 [details] [diff] [review]
> Part 2/4: Netd integration
> 
> Review of attachment 753698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +400,5 @@
> > +        callback.networkUsageAlarmResult(null);
> > +        return;
> > +      }
> > +
> > +      this._enableNetworkInterfaceAlarm(iface, threshold, callback);
> 
> If I read the code correctly, we have a error from netd if we run to here
> right ?
> Why do we need to call _enableNetworkInterfaceAlarm() when we fail to set
> network interface alarm ?

When you want to set an alarm and there are no alarms yet (every time after boot and when all alarms are removed) you need to send the following commands to netd:

1 - bandwidth enable
2 - bandwidth setiquota 'quota'
3 - bandwidth setinterfacealert 'iface name' 'threshold'

Once an alarm is set, you just need to send:

1 - bandwidth setinterfacealert 'iface name' 'threshold'

So, to avoid to check if an alarm has been enabled, first I try to set the alarm as if it is already enabled, and if it fails (only the first time) it means we have to enable, set quota and set threshold.

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

Comment on attachment 760093 [details] [diff] [review]
Part 2/4: Netd integration

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

This patch looks good, thank you.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 29 • 11 years ago

Is it really ok that all this API provides is an "alarm" once quota is reached? Wouldn't we want to enable the cost control app to say "application X can only use Y MB more data over 3g, once it reaches that limit cut the connection immediately".

The current approach seems to require the cost control app to open and then the cost control app is in charge or cutting the connection? As we've seen in other scenarios, sometimes starting an app can take 10 or even 30 seconds. It seems bad if we keep downloading data that long.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 30 • 11 years ago

I'm also not sure that we need the start and end dates? Couldn't that be solved by letting the cost control app simply set a limit. The cost control app could then use the alarm API to wake itself up and reset that limit on whatever schedule the user has configured.

	Salvador de la Puente González [:salva]
	Comment 31 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #30)
> I'm also not sure that we need the start and end dates? Couldn't that be
> solved by letting the cost control app simply set a limit. The cost control
> app could then use the alarm API to wake itself up and reset that limit on
> whatever schedule the user has configured.

I think the end date is not necessary, it was added just for completion as the starting date is required. It allow us to introduce a date since starting count usage which enable us to count in time periods and without need of deleting the historical data.

	Salvador de la Puente González [:salva]
	Comment 32 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #29)
> Is it really ok that all this API provides is an "alarm" once quota is
> reached? Wouldn't we want to enable the cost control app to say "application
> X can only use Y MB more data over 3g, once it reaches that limit cut the
> connection immediately".

Of course, it is possible but for the current US, we need this feature. If we want to expand functionality let's write some US. AFAIK, there is no definition about next steps in Cost Control.

> 
> The current approach seems to require the cost control app to open and then
> the cost control app is in charge or cutting the connection? As we've seen
> in other scenarios, sometimes starting an app can take 10 or even 30
> seconds. It seems bad if we keep downloading data that long.

Yes, a system message is delivered so the application must wake up before acting in any way. Although I think we really need is some kind of Network Restrictions API to address these kind of US. I.e: the alarm could be one of two possibilities, this is the "soft" alarm and you can have a "hard" as well. The hard alarm is a constrain so once that usage is reached, the traffic is forbidden.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 33 • 11 years ago

(In reply to Salvador de la Puente González [:salva] from comment #31)
> I think the end date is not necessary, it was added just for completion as
> the starting date is required. It allow us to introduce a date since
> starting count usage which enable us to count in time periods and without
> need of deleting the historical data.

Rather than keeping a start date, we could track the alarm usage separate from the historical data. I.e. at the time when an alarm is added, we initialize the counter for that alarm to 0 and then add to that as data is used.

I would imagine that we need to do that anyway since we don't want to constantly query the historical database? So the main difference would be that we need to store the data separately between restarts as well.

Come to think of it, don't we need to store the usage data for the alarm separately from the historical data anyway? Since we only store historical data on a pretty course-grained level, something like one data point per day, it's hard to not lose accuracy after a restart if we use the historical data as basis for the alarm?


Good point that we need to create US before we add anything to the API. If the front-end team for cost control isn't planning on creating any UX for allowing "hard" limits, then adding something to the API wouldn't make much of a difference anyway.

Come to think of it. It "hard" limits makes more sense for per-app alarms. That way we can do things like "As a user, I want to limit youtube to only use 1MB of data in a single day", or "As a user, I want to allow gmail to use 100kB of data per day".

	Salvador de la Puente González [:salva]
	Comment 34 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #33)
> (In reply to Salvador de la Puente González [:salva] from comment #31)
> > I think the end date is not necessary, it was added just for completion as
> > the starting date is required. It allow us to introduce a date since
> > starting count usage which enable us to count in time periods and without
> > need of deleting the historical data.
> 
> Rather than keeping a start date, we could track the alarm usage separate
> from the historical data. I.e. at the time when an alarm is added, we
> initialize the counter for that alarm to 0 and then add to that as data is
> used.

Another reason for keeping the starting time are system clock changes (not the time zone). Our UX team decided it is more important to keep the dates relative to the User point of view, it is more important to keep the notion of "yesterday I saw this large movie clip" rather than trying to correctly place the consumption when it really happen so we "move" the historical (we don't move anything, only apply some fixing offsets). Keeping the start date is convenient for these cases but it could be an implementation detail more than a feature we should expose to the user.

Maybe Albert could clarify why using starting dates. My opinion is it is useful If the want to establish a global alarm taking in count all the data ever used.

> I would imagine that we need to do that anyway since we don't want to
> constantly query the historical database? So the main difference would be
> that we need to store the data separately between restarts as well.

I didn't understand this point but I think we don't rely on querying the DB for watching the alarm. I think this happen in a lower level.

> Come to think of it, don't we need to store the usage data for the alarm
> separately from the historical data anyway? Since we only store historical
> data on a pretty course-grained level, something like one data point per
> day, it's hard to not lose accuracy after a restart if we use the historical
> data as basis for the alarm?

I'm asking Albert to clarify these points as he implemented that part.

> Good point that we need to create US before we add anything to the API. If
> the front-end team for cost control isn't planning on creating any UX for
> allowing "hard" limits, then adding something to the API wouldn't make much
> of a difference anyway.
> 
> Come to think of it. It "hard" limits makes more sense for per-app alarms.
> That way we can do things like "As a user, I want to limit youtube to only
> use 1MB of data in a single day", or "As a user, I want to allow gmail to
> use 100kB of data per day".

Totally agree.

	Albert [:albert] Assignee
	Comment 35 • 11 years ago

(In reply to Salvador de la Puente González [:salva] from comment #34)
> (In reply to Jonas Sicking (:sicking) from comment #33)
> > (In reply to Salvador de la Puente González [:salva] from comment #31)
> > > I think the end date is not necessary, it was added just for completion as
> > > the starting date is required. It allow us to introduce a date since
> > > starting count usage which enable us to count in time periods and without
> > > need of deleting the historical data.
> > 
> > Rather than keeping a start date, we could track the alarm usage separate
> > from the historical data. I.e. at the time when an alarm is added, we
> > initialize the counter for that alarm to 0 and then add to that as data is
> > used.
> 
> Another reason for keeping the starting time are system clock changes (not
> the time zone). Our UX team decided it is more important to keep the dates
> relative to the User point of view, it is more important to keep the notion
> of "yesterday I saw this large movie clip" rather than trying to correctly
> place the consumption when it really happen so we "move" the historical (we
> don't move anything, only apply some fixing offsets). Keeping the start date
> is convenient for these cases but it could be an implementation detail more
> than a feature we should expose to the user.
> 
> Maybe Albert could clarify why using starting dates. My opinion is it is
> useful If the want to establish a global alarm taking in count all the data
> ever used.

I think start date is useful, for example when the user sets the threshold limit in costcontrol. When the threshold is configured the usage alarm have to be set since the bill period starts not the current day, to be able to count usage for the whole period. In this case if the start date is not available, costcontrol app should check the current usage to substract it to the user threshold and set the result as the threshold for the system.   

> > I would imagine that we need to do that anyway since we don't want to
> > constantly query the historical database? So the main difference would be
> > that we need to store the data separately between restarts as well.
> 
> I didn't understand this point but I think we don't rely on querying the DB
> for watching the alarm. I think this happen in a lower level.

As Salva said, the usage alarm does not work checking the database constantly. The database stores two values for each day, the system value to store the usage since the mobile boots and the system total value to store the usage since the beginning. When a new alarm is set, the threshold is computed based in the system total value to be able to track it between restarts, then the alarm is set using the network daemon service (netd) that works at kernel level through the iptables.

> > Come to think of it, don't we need to store the usage data for the alarm
> > separately from the historical data anyway? Since we only store historical
> > data on a pretty course-grained level, something like one data point per
> > day, it's hard to not lose accuracy after a restart if we use the historical
> > data as basis for the alarm?
> 
> I'm asking Albert to clarify these points as he implemented that part.

This is answered in the previous explanation.

> > Good point that we need to create US before we add anything to the API. If
> > the front-end team for cost control isn't planning on creating any UX for
> > allowing "hard" limits, then adding something to the API wouldn't make much
> > of a difference anyway.
> > 
> > Come to think of it. It "hard" limits makes more sense for per-app alarms.
> > That way we can do things like "As a user, I want to limit youtube to only
> > use 1MB of data in a single day", or "As a user, I want to allow gmail to
> > use 100kB of data per day".
> 
> Totally agree.

I agree as well, but from my point of view this is related with the app policy bugs. Bugs 784575, 746074 and 746065 will provide a mechanism to set policies for apps, so in coordination with the usage alarm will be a good way to set hard limits.

	Albert [:albert] Assignee
	Comment 36 • 11 years ago

Changes from comment 21
Removed the usage of alarm API

	Albert [:albert] Assignee
	Comment 37 • 11 years ago

(In reply to Gene Lian [:gene] from comment #21)
> Comment on attachment 756374 [details] [diff] [review]
> Part 3/4: Alarm implementation
> 
> Review of attachment 756374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +563,5 @@
> > +          if (connectionId == -1 || cursor.value.connectionId == connectionId) {
> > +            let alarm = { id: cursor.value.id,
> > +                          iface: cursor.value.connectionId,
> > +                          threshold: cursor.value.threshold,
> > +                          data: cursor.value.data };
> 
> Why not using this.recordToAlarm(cursor.value)?

Because this alarm object is not used internally, it is just passed back to the requester through the manager. 

> ::: dom/network/src/NetworkStatsManager.js
> @@ +96,5 @@
> > +// UsageAlarm
> > +const USAGEALARM_CID      = Components.ID("{2dcd5394-ad90-45d0-a755-3da80095f22f}");
> > +const nsIDOMMozUsageAlarm = Components.interfaces.nsIDOMMozUsageAlarm;
> > +
> > +function UsageAlarm(aAlarm) {
> 
> UsageAlarm sound a very general term to me. It's a bit implicit, although I
> don't understand yet what the main purpose it is. Let's figure out if we
> could have a better name for it to make it more related to networks
> statistics.

The usage alarm is the network stats alarm object to deal with apps regarding a threshold usage alarm. Do you prefer UsageLimit, UsageLimitAlarm or NetworkStatsAlarm?

> @@ +192,5 @@
> > +                           data: {connectionType: connectionType, manifestURL: this.manifestURL}});
> > +    return request;
> > +  },
> > +
> > +  removeUsageAlarms: function removeUsageAlarms(alarmId) {
> 
> s/alarmId/aAlarmId
> 
> Also, why not s/removeUsageAlarms/removeUsageAlarm/? You only remove single
> alarm. Right?

No, it depends on the param. If the requester doesn't provide an alarmId parameter, all alarms for the app will be removed.

> @@ +206,5 @@
> > +      this._db.getFirstAlarm(connection,
> > +                             function getResult(error, result) {
> > +        if (!error && result) {
> > +          self._setAlarm(result, function onSet(error, success){
> > +            if (error == "InvalidStateError") {
> 
> Why do you fire alarms for invalid state errors during initialization? I
> don't get it. Could you please elaborate on this?

The InvalidStateError is thrown when the requester sets an alarm at a threshold lower than the current data usage amount. Alarms API also throws an InvalidStateError when you set an alarm to a past clock time.

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

Sorry for the delay (busy with the urgent MMS bugs).

One question. Again, do we really have any user stories supporting these new APIs? Honestly, I still don't understand is there any use case we cannot satisfy by using the existing Network Stats API + Alarm API?

As far as I know, the main purpose of these APIs is to do something at specific time points (i.e. checking if the traffic hits the threshold), but it's exactly what the Alarm API is behaving. That is, the Cost Control App can still do this check in the 'alarm' system message handler. Right?

Please provide some practical examples to prove the necessary of these new APIs. I'll try to catch up the review if we really need to support this...

	Salvador de la Puente González [:salva]
	Comment 39 • 11 years ago

Gene, sorry but can you specify for which changes do you need the US? Albert is on vacations but I can answer you.

Thank you.

	Salvador de la Puente González [:salva]
	Comment 43 • 11 years ago

There is a US already implemented in the Gaia side but not performing optimal due to the lack of BE support. I'll try to summarize the problematic:

US1: The user wants to be warned as soon as reaching a limit of data usage.

Now the Gaia trick is very ugly. When detecting data transmission a (short) periodic check is performed. As the event of transmission does not carry any information about how many data is being transmitted I'm counting number of events before performing a new check.

So the new implementation allows me to install an alarm for a number of bytes. Once reached, the BE sends the application a system-message (currently, the same used in alarms, I think but not sure).

The alarm require two parameters, a start-date and a limit threshold. Note how the end date was removed precisely for the reasons you are pointing. But the start-date is relevant because we need to now when to start counting. Consider these two US:

US2: The user can set a tracking period, after it, counters are reset.
US3: The user can see previous periods.

The latter one is not implemented nor defined yet but has been demanded several times from user's feedback.

As the user can see the previous periods we can not simply empty the database or reset the counters. We need to specify the BE to start counting since some date. This is one of the reasons we need the start date.

Furthermore, this enables some new user stories:

US4: The user want to set a global usage limit.

This can be implemented by installing an alarm with start date equal to the oldest one.

It is convenient to deal with a starting date when dealing with time-travel. We give the name of time-travel to the issue raising when the user performs a big system time change.

After a lot of discussion about how to model this change in the historical and how to depict it, we end thinking the best option is to keep the user-centric perspective focused on the concept of "today" where the important thing is to keep the notion of today, yesterday, the day before yesterday... in such a way the user could say:

"Yesterday I download a lot of data" with no dependencies of system time changes.

So, when the user changes the system time, we "move" the historical by the same amount of time the user has move in the timeline. Alarms should be moved as well so we need to keep track of when they start to count.

Hope this helps.

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

Thanks Salvador for the detailed clarification. I'll catch up the review in recent two days. :)

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

Comment on attachment 767629 [details] [diff] [review]
Part 3/4: Alarm implementation

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

Patch is great!!! Most of the comments are just nits. There are so many details in this patch and I hope to have another check. If there's no more issues, should be fine to give review+ in the next round. Could you please come back with the nits fixed? Thanks!

::: dom/network/src/NetworkStatsDB.jsm
@@ +105,5 @@
> +        let lastTx = 0;
> +
> +        objectStore.openCursor().onsuccess = function(event) {
> +          let cursor = event.target.result;
> +          if (cursor) {

nit: use early return:

if (cursor) {
  return;
}

//...
cursor.continue();

@@ +127,5 @@
> +
> +            cursor.update(cursor.value);
> +            cursor.continue();
> +          }
> +        }; 

nit: TS.

@@ +326,5 @@
> +      debug("Get current stats for " + aConnectionType + " since " + aDate);
> +    }
> +    this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
> +      let request = null;
> +      if(aDate) {

nit: one space after "if".

@@ +327,5 @@
> +    }
> +    this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {
> +      let request = null;
> +      if(aDate) {
> +        let start = this.convertDate(aDate);

We used to change this to .normalizeDate(...). Right?

@@ +503,5 @@
> +
> +    if (aRecord.id) {
> +      alarm.id = aRecord.id;
> +    }
> +    

nit: TS.

@@ +613,5 @@
> +
> +      txn.result = [];
> +      store.index("manifestURL").openCursor(aManifestURL)
> +                                .onsuccess = function onsuccess(event) {
> +

nit: don't need this blank line after the function declaration.

@@ +615,5 @@
> +      store.index("manifestURL").openCursor(aManifestURL)
> +                                .onsuccess = function onsuccess(event) {
> +
> +        let cursor = event.target.result;
> +        if (cursor) {

nit:

if (!cursor) {
  return;
}

//...
cursor.continue();

@@ +626,5 @@
> +            txn.result.push(alarm);
> +          }
> +
> +          cursor.continue();
> +          return;

nit: don't need this return.

::: dom/network/src/NetworkStatsManager.js
@@ +109,5 @@
> +                      alarmId: 'r',
> +                      iface: 'r',
> +                      threshold:  'r',
> +                      data:  'r',
> +                     },

Nit:

  __exposedProps__: {
    alarmId: 'r',
    iface: 'r',
    threshold: 'r',
    data: 'r'
  },

Notes:
1. It looks more comfortable by this alignment.
2. One space is enough before 'r'.

However, since you did this way for other __exposedProps__ within this file, so fixing #2 is enough to me.

::: dom/network/src/NetworkStatsService.jsm
@@ +156,5 @@
>          break;
> +      case TOPIC_BANDWIDTH_CONTROL:
> +        debug("Bandwidth message from netd: " + JSON.stringify(data));
> +
> +        for( let i in networkManager.networkInterfaces) {

nit: s/for( ...)/for (...)/

@@ +205,5 @@
> +
> +      this._db.getFirstAlarm(connection,
> +                             function getResult(error, result) {
> +        if (!error && result) {
> +          self._setAlarm(result, function onSet(error, success){

nit: add a space before "{".

@@ +206,5 @@
> +      this._db.getFirstAlarm(connection,
> +                             function getResult(error, result) {
> +        if (!error && result) {
> +          self._setAlarm(result, function onSet(error, success){
> +            if (error == "InvalidStateError") {

We used to remove this kind of error at bug 876936. So you don't need this check.

@@ +414,5 @@
>  
> +    let stats = { connectionType: this._connectionTypes[aConnType].name,
> +                  date:           aDate,
> +                  rxBytes:        aTxBytes,
> +                  txBytes:        aRxBytes};

nit: add one space before "};".

@@ +533,5 @@
> +    let now = Date.now();
> +    if (options.alarmStart && options.alarmStart.getTime() >= now) {
> +      mm.sendAsyncMessage("NetworkStats:SetAlarm:Return",
> +                          { id: msg.id, error: "InvalidDate", result: null });
> +      return;

You don't need this. Please see bug 876936. We found out it's meaningless to check if the alarm is added in the past.

@@ +537,5 @@
> +      return;
> +    }
> +
> +    for (let i in this._connectionTypes) {
> +      if (this._connectionTypes[i].name == options.connectionType) {

nit: please use early return:

if (this._connectionTypes[i].name != options.connectionType) {
  continue;
}

// ...
return;

@@ +558,5 @@
> +            return;
> +          }
> +
> +          newAlarm.absoluteThreshold = _threshold.absoluteThreshold;
> +          self._db.addUsageAlarm(newAlarm, function addSuccessCb(error, aNewId) {

nit: s/aNewId/newId/

@@ +559,5 @@
> +          }
> +
> +          newAlarm.absoluteThreshold = _threshold.absoluteThreshold;
> +          self._db.addUsageAlarm(newAlarm, function addSuccessCb(error, aNewId) {
> +            mm.sendAsyncMessage("NetworkStats:SetAlarm:Return",

I know you're wanting to return error here by the same call, but it looks safer to me to call this in the callback of ._setAlarm(...) for the non-error case. Note that the child process will get this return even *before* the parent process actually sets this alarm. It doesn't sound right logically and it's kind of racing.

@@ +562,5 @@
> +          self._db.addUsageAlarm(newAlarm, function addSuccessCb(error, aNewId) {
> +            mm.sendAsyncMessage("NetworkStats:SetAlarm:Return",
> +                                { id: msg.id, error: error, result: aNewId });
> +
> +            if (!error) {

nit: use early return:

if (error) {
  return;
}

// The rest of the codes.

@@ +564,5 @@
> +                                { id: msg.id, error: error, result: aNewId });
> +
> +            if (!error) {
> +              newAlarm.id = aNewId;
> +              self._setAlarm(newAlarm, function onSet(error, success){

nit: add one space before "{".

@@ +565,5 @@
> +
> +            if (!error) {
> +              newAlarm.id = aNewId;
> +              self._setAlarm(newAlarm, function onSet(error, success){
> +                if (error == "InvalidStateError") {

Ditto: you don't nee this check anymore.

@@ +592,5 @@
> +
> +    let self = this;
> +
> +    this._updateThreshold(aAlarm, function onUpdate(aError, aThreshold){
> +

nit: don't need a blank line after the function declaration.

@@ +685,5 @@
> +        return;
> +      }
> +
> +      self._setAlarm(result, function onSet(error, success){
> +        if (error == "InvalidStateError") {

Ditto.

@@ +700,5 @@
> +    let manifestURI = Services.io.newURI(aAlarm.manifestURL, null, null);
> +    let pageURI = Services.io.newURI(aAlarm.pageURL, null, null);
> +
> +    let alarm = { "id":              aAlarm.id,
> +                  "threshold":       aAlarm.threshold,

nit: one space after "threshold": is enough.

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

(In reply to Albert [:albert] from comment #37)
> (In reply to Gene Lian [:gene] from comment #21)
> > UsageAlarm sound a very general term to me. It's a bit implicit, although I
> > don't understand yet what the main purpose it is. Let's figure out if we
> > could have a better name for it to make it more related to networks
> > statistics.
> 
> The usage alarm is the network stats alarm object to deal with apps
> regarding a threshold usage alarm. Do you prefer UsageLimit, UsageLimitAlarm
> or NetworkStatsAlarm?

I'd prefer NetworkStatsAlarm since the purpose of this kind of alarm is to fire warnings regarding the dynamic of network statistics. You also need to fix the names in the IDL.

> The InvalidStateError is thrown when the requester sets an alarm at a
> threshold lower than the current data usage amount. Alarms API also throws
> an InvalidStateError when you set an alarm to a past clock time.

If the InvalidStateError is for the threshold purpose then you can keep it. If it's for checking adding alarm in the past, we used to have a conclusion at bug 876926 that it's meaningless to return such an error since we might be under the risk of race condition.

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

(In reply to Gene Lian [:gene] from comment #46)
> I'd prefer NetworkStatsAlarm since the purpose of this kind of alarm is to
> fire warnings regarding the dynamic of network statistics. You also need to
> fix the names in the IDL.

After some thoughts, let's use NetworkUsageAlarm so that you won't have too many changes in your original codes.

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

Comment on attachment 774298 [details] [diff] [review]
Part 1/4: IDL definition

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

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +17,5 @@
>    jsval start;              // date
>    jsval end;                // date
>  };
>  
> +dictionary AlarmUsageOptions

s/AlarmUsageOptions/NetworkUsageAlarmOptions

@@ +24,5 @@
> +  jsval data;
> +};
> +
> +[scriptable, builtinclass, uuid(2dcd5394-ad90-45d0-a755-3da80095f22f)]
> +interface nsIDOMMozUsageAlarm : nsISupports

s/nsIDOMMozUsageAlarm/nsIDOMMozNetworkUsageAlarm

This is going to be exposed to web content so the naming has to be more specific.

Also, you need to s/@mozilla.org/usagealarm;1/@mozilla.org/networkusagealarm;1/ in the manifest and s/UsageAlarm/NetworkUsageAlarm in all of your codes.

The component stuff in the Mozilla code base is open to all the Gecko codes (i.e. everyone can create one at anywhere), so the naming should be more specific to specify its purpose.

@@ +58,5 @@
> +   * object and it is included inside the alarm system message.
> +   *
> +   * If success, the result field of the request keeps the alarm Id.
> +   */
> +  nsIDOMDOMRequest               addUsageAlarm(in DOMString iface,

It's fine to keep the original function name because you're calling these APIs under nsIDOMMozNetworkStatsManager.

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

(In reply to Gene Lian [:gene] from comment #46)
> If the InvalidStateError is for the threshold purpose then you can keep it.
> If it's for checking adding alarm in the past, we used to have a conclusion
> at bug 876926 that it's meaningless to return such an error since we might
         ^^^^^^ Sorry I mean bug 876936.
> be under the risk of race condition.

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

Comment on attachment 774298 [details] [diff] [review]
Part 1/4: IDL definition

># HG changeset patch
># User Albert Crespell <acperez@tid.es>
># Date 1369387622 -7200
># Node ID 7981f5b25677571ae923dcb9d5bfdfe046e2e4c5
># Parent ac7663828c8823ccadc73614671309eb7e9f9d06
>Bug 858005 - Part 1/4 IDL definition. r=jonas
>
>diff --git a/dom/network/interfaces/nsIDOMNetworkStatsManager.idl b/dom/network/interfaces/nsIDOMNetworkStatsManager.idl
>--- a/dom/network/interfaces/nsIDOMNetworkStatsManager.idl
>+++ b/dom/network/interfaces/nsIDOMNetworkStatsManager.idl
>@@ -13,33 +13,83 @@ dictionary NetworkStatsOptions
>    * 'mobile', 'wifi' or null.
>    * If null, stats for both mobile and wifi are returned.
>    */
>   DOMString connectionType;
>   jsval start;              // date
>   jsval end;                // date
> };
> 
>-[scriptable,  uuid(87529a6c-aef6-11e1-a595-4f034275cfa6)]
>+dictionary AlarmUsageOptions
>+{
>+  jsval alarmStart;              // date

Also, s/alarmStart/startTime sounds better.

>+  jsval data;
>+};

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

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

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

The tests are nice but I'm concerned about the usages of |alarmStart| and |alarmEnd|. Could you please correct or clarify them? Thanks!

::: dom/network/tests/test_networkstats_alarms.html
@@ +12,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +function test() {

Where did you run this test()? I cannot find it. Should it put into the steps array?

@@ +48,5 @@
> +      next();
> +    }
> +  },
> +  function () {
> +    ok(true, "Test set alarms error when threshold is negative and parameters are missing");

s/are missing/are missing or invalid/

@@ +51,5 @@
> +  function () {
> +    ok(true, "Test set alarms error when threshold is negative and parameters are missing");
> +
> +    try {
> +          navigator.mozNetworkStats.addUsageAlarm();

nit: 2-space indentation is enough.

@@ +53,5 @@
> +
> +    try {
> +          navigator.mozNetworkStats.addUsageAlarm();
> +    } catch(ex) {
> +      ok(ex.result == 2153185281, "addUsageAlarm launch Not enough arguments exception wehn no parameters");

Can you use SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS? I found we can use in this way in other test cases. We should avoid using magic numbers which is hard to understand its purpose.

s/wehn/when/

@@ +57,5 @@
> +      ok(ex.result == 2153185281, "addUsageAlarm launch Not enough arguments exception wehn no parameters");
> +    }
> +
> +    try {
> +          navigator.mozNetworkStats.addUsageAlarm(100000);

Ditto.

@@ +59,5 @@
> +
> +    try {
> +          navigator.mozNetworkStats.addUsageAlarm(100000);
> +    } catch(ex) {
> +      ok(ex.result == 2153185281, "addUsageAlarm launch Not enough arguments exception wehn no interface");

Ditto.

@@ +63,5 @@
> +      ok(ex.result == 2153185281, "addUsageAlarm launch Not enough arguments exception wehn no interface");
> +    }
> +
> +    try {
> +          navigator.mozNetworkStats.addUsageAlarm("wifi");

Ditto.

@@ +65,5 @@
> +
> +    try {
> +          navigator.mozNetworkStats.addUsageAlarm("wifi");
> +    } catch(ex) {
> +      ok(ex.result == 2153185281, "addUsageAlarm launch Not enough arguments exception wehn no threshold");

Ditto.

@@ +84,5 @@
> +      req.onsuccess = function () {
> +        ok(false, "Set alarm with startDate greater than now should return error");
> +      };
> +      req.onerror = function () {
> +        ok(req.error.name == "InvalidDate", "Get Invalid date error when start is greater than now");

I might misunderstand this bit. I originally thought the design is: add an alarm in the future and the alarm will fire and then check the threshold. Why cannot we add an alarm in the future? Please correct my understanding if I'm wrong. Thanks!

@@ +88,5 @@
> +        ok(req.error.name == "InvalidDate", "Get Invalid date error when start is greater than now");
> +
> +        var date = new Date(new Date().getTime() - 10000);
> +
> +        req = navigator.mozNetworkStats.addUsageAlarm("mobile", 100000, {alarmEnd: date});

We don't use "alarmEnd" based on your latest design. Right?

::: dom/network/tests/unit_stats/test_networkstats_service.js
@@ +113,5 @@
> +                alarmStart: null,
> +                alarmEnd: null,
> +                data: null,
> +                pageURL: "http://test.com",
> +                manifestURL: "http://test.com/manifest.webapp"};

nit: add one space before the last "}" since you did the same thing for the "}" at the beginning.

@@ +165,5 @@
> +});
> +
> +add_test(function test_updateThreshold() {
> +  let alarm = { connectionId: 2,
> +                threshold: 10000};

Ditto.

@@ +180,5 @@
> +                                    "http://test.com/manifest.webapp", function onCompleted(error, result) {
> +    do_check_eq(error, null);
> +    do_check_eq(result.length, 1);
> +    NetworkStatsService._fireAlarm(result[0], false);
> +    NetworkStatsService._db.getAlarms(Ci.nsINetworkInterface.NETWORK_TYPE_WIFI, 

nit: TS.

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

Comment on attachment 774300 [details] [diff] [review]
Part 2/4: Netd integration

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

Sorry for my late response. 
The patch looks good. Thank you.

	Albert [:albert] Assignee
	Comment 53 • 11 years ago

(In reply to Gene Lian [:gene] from comment #45)
> Comment on attachment 767629 [details] [diff] [review]
> Part 3/4: Alarm implementation
> 
> Review of attachment 767629 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +206,5 @@
> > +      this._db.getFirstAlarm(connection,
> > +                             function getResult(error, result) {
> > +        if (!error && result) {
> > +          self._setAlarm(result, function onSet(error, success){
> > +            if (error == "InvalidStateError") {
> 
> We used to remove this kind of error at bug 876936. So you don't need this
> check.

As you pointed in comment 46, "InvalidStateError" is used for threshold purposes, so I keep it.

> @@ +533,5 @@
> > +    let now = Date.now();
> > +    if (options.alarmStart && options.alarmStart.getTime() >= now) {
> > +      mm.sendAsyncMessage("NetworkStats:SetAlarm:Return",
> > +                          { id: msg.id, error: "InvalidDate", result: null });
> > +      return;
> 
> You don't need this. Please see bug 876936. We found out it's meaningless to
> check if the alarm is added in the past.

Is not the same than bug 876936, here I am checking the alarmStart date, conceptually the user is setting a future alarm as the alarmStart is used to determine since when I have to check the amount of data usage. I can remove the validation of the startDate greater than now, then if the user sets an alarm since next month it will remain inactive in the database until next month. Is it ok?

> @@ +565,5 @@
> > +
> > +            if (!error) {
> > +              newAlarm.id = aNewId;
> > +              self._setAlarm(newAlarm, function onSet(error, success){
> > +                if (error == "InvalidStateError") {
> 
> Ditto: you don't nee this check anymore.
> 
> @@ +685,5 @@
> > +        return;
> > +      }
> > +
> > +      self._setAlarm(result, function onSet(error, success){
> > +        if (error == "InvalidStateError") {
> 
> Ditto.

Both like the first comment, "InvalidStateError" for threshold.

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

(In reply to Albert [:albert] from comment #53)
> > You don't need this. Please see bug 876936. We found out it's meaningless to
> > check if the alarm is added in the past.
> 
> Is not the same than bug 876936, here I am checking the alarmStart date,
> conceptually the user is setting a future alarm as the alarmStart is used to
> determine since when I have to check the amount of data usage. I can remove
> the validation of the startDate greater than now, then if the user sets an
> alarm since next month it will remain inactive in the database until next
> month. Is it ok?

Sounds good!

My point was you can never check if a time point is *in the future or not* in the implementation. We would be under the risk of race condition.

	Albert [:albert] Assignee
	Comment 55 • 11 years ago

(In reply to Gene Lian [:gene] from comment #47)
> (In reply to Gene Lian [:gene] from comment #46)
> > I'd prefer NetworkStatsAlarm since the purpose of this kind of alarm is to
> > fire warnings regarding the dynamic of network statistics. You also need to
> > fix the names in the IDL.
> 
> After some thoughts, let's use NetworkUsageAlarm so that you won't have too
> many changes in your original codes.

I also prefer NetworkStatsAlarm, seems better according to the rest of names in the IDL, I modified it.

Regarding the rest of functions sounds better to change names as well because 'usage' does not have sense now, so:

mozNetworkStats.addUsageAlarm     -> mozNetworkStats.addAlarm
mozNetworkStats.getAllUsageAlarms -> mozNetworkStats.getAllAlarms
mozNetworkStats.removeUsageAlarms -> mozNetworkStats.removeAlarms

	Albert [:albert] Assignee
	Comment 56 • 11 years ago

Modified alarm functions names

	Albert [:albert] Assignee
	Comment 57 • 11 years ago

Rebase

	Albert [:albert] Assignee
	Comment 58 • 11 years ago

Changes requested

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

Comment on attachment 784849 [details] [diff] [review]
Part 1/4: IDL definition

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

Looks great! Thanks!

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +67,5 @@
> +   * Obtain all alarms for those interfaces in availableInterfaces.
> +   * If an interface is provided, only retrieves the alarms
> +   * for that interface. The interface must be one of those in
> +   * availebleInterfaces or an InvalidInterface exception will be raised.
> +   * 

nit: TS.

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

Will take the part-3 review tomorrow.

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

Comment on attachment 784851 [details] [diff] [review]
Part 3/4: Alarm implementation

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

Looks great! Thanks Albert! Btw, we still need Jonas' super review regarding the IDL.

::: dom/messages/SystemMessagePermissionsChecker.jsm
@@ +97,5 @@
>    },
>    "wappush-received": {
>      "sms": []
>    },
> +  "networkstats-alarm": {

nit: please put this new entry in a correct alphabet order.

::: dom/network/src/NetworkStatsDB.jsm
@@ +90,5 @@
>          if (DEBUG) {
>            debug("Database initialized");
>          }
> +      } else if (currVersion == 1) {
> +        // In order to maange alarms is necessary to add a global counter

nit: s/maange/manage

@@ +91,5 @@
>            debug("Database initialized");
>          }
> +      } else if (currVersion == 1) {
> +        // In order to maange alarms is necessary to add a global counter
> +        // that will increase instead of system reboot.

If I don't misunderstand the meaning, let's rewrite the comments a bit:

// In order to manage alarms, it is necessary to use a global counter
// (totalBytes) that will increase regardless of the system reboot.

@@ +558,5 @@
> +        debug("Remove alarm " + aAlarmId);
> +      }
> +
> +      store.get(aAlarmId).onsuccess = function onsuccess(event) {
> +        let result = event.target.result;

Can you s/result/record/ to make it clear it's not the value going to return?

@@ +560,5 @@
> +
> +      store.get(aAlarmId).onsuccess = function onsuccess(event) {
> +        let result = event.target.result;
> +        txn.result = false;
> +        if (!result || (aManifestURL && result.manifestURL != aManifestURL)) {

s/result/record/

::: dom/network/src/NetworkStatsManager.js
@@ +94,5 @@
>  }
>  
> +// NetworkStatsAlarm
> +const NETWORKSTATSALARM_CID      = Components.ID("{063ebeb2-5c6e-47ae-bdcd-5e6ebdc7a68c}");
> +const nsIDOMMozNetworkStatsAlarm = Components.interfaces.nsIDOMMozNetworkStatsAlarm;

nit: s/Components.interfaces/Ci/

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

Btw, where is the new patch for test case?

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

Oh, I saw that. Is that ready for review?

	Albert [:albert] Assignee
	Comment 64 • 11 years ago

(In reply to Gene Lian [:gene] from comment #63)
> Oh, I saw that. Is that ready for review?

Tests are not ready, I wanted to wait to implementation patch to be more stable in order to start working on the tests.

	Albert [:albert] Assignee
	Comment 65 • 11 years ago

Rebase

	Albert [:albert] Assignee
	Comment 66 • 11 years ago

Changes from comment 61

Removed timerId in NetworkStatsDB

	Albert [:albert] Assignee
	Comment 67 • 11 years ago

Tests updated

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

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

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

The tests are very nice. Just having some nits. Thank you! :)

::: dom/network/tests/test_networkstats_alarms.html
@@ +13,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +
> +function test() {
> +  ok(true, "Check no alarms set");

grammar nit (gnit): "Checking if no alarms are set."

@@ +18,5 @@
> +
> +  req = navigator.mozNetworkStats.getAllAlarms();
> +
> +  req.onsuccess = function () {
> +    ok(true, "Get alarms request ok");

gnit: "Succeeded to get alarms."

@@ +20,5 @@
> +
> +  req.onsuccess = function () {
> +    ok(true, "Get alarms request ok");
> +    ok(Array.isArray(req.result) && req.result.length == 0,
> +       "There are no usage alarms set.");

nit: drop "usage".

@@ +25,5 @@
> +    next();
> +  };
> +
> +  req.onerror = function () {
> +    ok(false, "Get alarms failure!");

gnit: "getAllAlarms() shouldn't fail!"

@@ +34,5 @@
> +var index = -1;
> +
> +var steps = [
> +  function () {
> +    ok(true, "Check get alarms for invalid interface");

gnit: "Calling getAllAlarms() with invalid interface."

@@ +39,5 @@
> +
> +    req = navigator.mozNetworkStats.getAllAlarms("foo");
> +
> +    req.onsuccess = function () {
> +      ok(false, "Get alarms request for invalid interface");

gnit: "getAllAlarms() shouldn't succeed!"

Don't need to emphasize invalid interface again since it has been claimed at the beginning of this function.

@@ +48,5 @@
> +      next();
> +    }
> +  },
> +  function () {
> +    ok(true, "Test set alarms error when threshold is negative and parameters are missing or invalid");

gnit: "Calling addAlarm() with invalid interface or parameters."

@@ +53,5 @@
> +
> +    try {
> +      navigator.mozNetworkStats.addAlarm();
> +    } catch(ex) {
> +      ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS, "addUsageAlarm launch Not enough arguments exception when no parameters");

nit: fold this line. I'm sure this exceeds 80 chars. Also, s/addUsageAlarm/addAlarm.

ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS,
   "addAlarm() throws NS_ERROR_XPC_NOT_ENOUGH_ARGS exception when no parameters");

@@ +59,5 @@
> +
> +    try {
> +      navigator.mozNetworkStats.addAlarm(100000);
> +    } catch(ex) {
> +      ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS, "addUsageAlarm launch Not enough arguments exception when no interface");

ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS,
   "addAlarm() throws NS_ERROR_XPC_NOT_ENOUGH_ARGS exception when no interface");

@@ +65,5 @@
> +
> +    try {
> +      navigator.mozNetworkStats.addAlarm("wifi");
> +    } catch(ex) {
> +      ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS, "addUsageAlarm launch Not enough arguments exception when no threshold");

ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS,
   "addAlarm() throws NS_ERROR_XPC_NOT_ENOUGH_ARGS exception when no threshold");

@@ +71,5 @@
> +
> +    req = navigator.mozNetworkStats.addAlarm("mobile", -100000);
> +
> +    req.onsuccess = function () {
> +      ok(false, "Set alarm with negative threshold should return error");

gnit: "addAlarm() shouldn't succeed with negative threshold."

@@ +80,5 @@
> +      next();
> +    }
> +  },
> +  function () {
> +    ok(true, "Test set alarm for mobile and wifi");

gnit: "Calling addAlarm() with mobile or wifi interface."

@@ +85,5 @@
> +
> +    req = navigator.mozNetworkStats.addAlarm("mobile", 1000000);
> +
> +    req.onsuccess = function () {
> +      ok(true, "Set mobile alarm success. AlarmId: " + req.result);

gnit: "Succeeded to add alarm for mobile. ..."

@@ +90,5 @@
> +
> +      req = navigator.mozNetworkStats.addAlarm("wifi", 1000000);
> +
> +      req.onsuccess = function () {
> +        ok(true, "Set wifi alarm success. AlarmId: " + req.result);

gnit: "Succeeded to add alarm for wifi. ..."

@@ +94,5 @@
> +        ok(true, "Set wifi alarm success. AlarmId: " + req.result);
> +        next();
> +      };
> +      req.onerror = function () {
> +        ok(false, "Set alarm error");

gnit: "addAlarm() shouldn't fail."

@@ +99,5 @@
> +      };
> +    };
> +
> +    req.onerror = function () {
> +      ok(false, "Set alarm error");

gnit: "addAlarm() shouldn't fail."

@@ +103,5 @@
> +      ok(false, "Set alarm error");
> +    }
> +  },
> +  function () {
> +    ok(true, "Test get alarm for mobile and wifi");

gnit: "Calling getAllAlarms() with mobile or wifi interface."

@@ +108,5 @@
> +
> +    req = navigator.mozNetworkStats.getAllAlarms("mobile");
> +
> +    req.onsuccess = function () {
> +      ok(req.result[0].alarmId == 1, "Get correct alarmId for mobile");

Could you please add a check for the length of the req.result array? Should be 1. To make sure only the alarm for "mobile" is returned.

@@ +113,5 @@
> +
> +      req = navigator.mozNetworkStats.getAllAlarms("wifi", 1000000);
> +
> +      req.onsuccess = function () {
> +        ok(req.result[0].alarmId == 2, "Get correct alarmId for wifi");

Ditto.

@@ +117,5 @@
> +        ok(req.result[0].alarmId == 2, "Get correct alarmId for wifi");
> +        next();
> +      };
> +      req.onerror = function () {
> +        ok(false, "Get alarm error");

gnit: "getAllAlarms() shouldn't fail."

@@ +122,5 @@
> +      };
> +    };
> +
> +    req.onerror = function () {
> +      ok(false, "Get alarm error");

gnit: "getAllAlarms() shouldn't fail."

@@ +126,5 @@
> +      ok(false, "Get alarm error");
> +    }
> +  },
> +  function () {
> +    ok(true, "Test get all alarms");

gnit: "Calling getAllAlarms() to get all alarms."

@@ +132,5 @@
> +    req = navigator.mozNetworkStats.getAllAlarms();
> +
> +    req.onsuccess = function () {
> +      ok(Array.isArray(req.result) && req.result.length == 2,
> +         "Get all alarms ok.");

gnit: "Succeeded to get all alarms."

@@ +137,5 @@
> +      next();
> +    };
> +
> +    req.onerror = function () {
> +      ok(false, "Get alarms error");

gnit: "getAllAlarms() shouldn't fail."

@@ +141,5 @@
> +      ok(false, "Get alarms error");
> +    }
> +  },
> +  function () {
> +    ok(true, "Remove alarms");

gnit: "Calling removeAlarms() to remove alarms."

@@ +146,5 @@
> +
> +    req = navigator.mozNetworkStats.removeAlarms();
> +
> +    req.onsuccess = function () {
> +      ok(req.result, "Remove alarms ok.");

gnit: "Succeeded to remove alarms."

@@ +151,5 @@
> +      next();
> +    };
> +
> +    req.onerror = function () {
> +      ok(false, "Remove alarms error");

gnit: "removeAlarms() shouldn't fail."

@@ +155,5 @@
> +      ok(false, "Remove alarms error");
> +    }
> +  },
> +  function () {
> +    ok(true, "Test that all alarms are removed");

gnit: "Checking if all alarms are removed."

@@ +161,5 @@
> +    req = navigator.mozNetworkStats.getAllAlarms();
> +
> +    req.onsuccess = function () {
> +      ok(Array.isArray(req.result) && req.result.length == 0,
> +         "No alarms in database.");

gnit: "Succeeded to remove all alarms."

@@ +166,5 @@
> +      next();
> +    };
> +
> +    req.onerror = function () {
> +      ok(false, "Get alarms error");

gnit: "getAllAlarms() shouldn't fail."

::: dom/network/tests/unit_stats/test_networkstats_db.js
@@ +400,5 @@
>      }, {start: start, end: end, connectionType: "wifi"});
>    });
>  });
>  
> +var alarms = [{ id:                null,

Add:

var examplePageURL = "http://example.com/index.html";
var exampleManifestURL= "http://example.com/manifest.webapp";

var testPageURL = "http://test.com/index.html";
var testManifestURL = "http://test.com/manifest.webapp";

before var alarms = ...;

So that it makes clear to input examplePageURL/exampleManifestURL as parameters when testing functions below, instead of using alarms[0].pageURL/alarms[0].manifestURL.

@@ +405,5 @@
> +                connectionId:      Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                threshold:         10000,
> +                data:              {foo: "something"},
> +                pageURL:           "http://example.com/index.html",
> +                manifestURL:       "http://example.com/manifest.webapp" },

...
pageURL:     examplePageURL,
manifestURL: exampleManifestURL },

@@ +411,5 @@
> +                connectionId:      Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                threshold:         1000,
> +                data:              {foo: "else"},
> +                pageURL:           "http://example.com/index.html",
> +                manifestURL:       "http://example.com/manifest.webapp" },

Ditto.

@@ +417,5 @@
> +                connectionId:      Ci.nsINetworkInterface.NETWORK_TYPE_MOBILE,
> +                threshold:         100,
> +                data:              {foo: "to"},
> +                pageURL:           "http://example.com/index.html",
> +                manifestURL:       "http://example.com/manifest.webapp" },

Ditto.

@@ +423,5 @@
> +                connectionId:      Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                threshold:         10,
> +                data:              {foo: "test"},
> +                pageURL:           "http://test.com/index.html",
> +                manifestURL:       "http://test.com/manifest.webapp" }];

...
pageURL:     testPageURL,
manifestURL: testManifestURL },

@@ +428,5 @@
> +
> +var alarmsDbId = 0;
> +
> +add_test(function test_addAlarm() {
> +  // add alarms[0]

// Add alarms[0] -> DB: [ alarms[0] (id: 1) ]

@@ +429,5 @@
> +var alarmsDbId = 0;
> +
> +add_test(function test_addAlarm() {
> +  // add alarms[0]
> +  // check insertion ok

gnit: // Check the insertion is OK.

@@ +436,5 @@
> +    alarmsDbId += 1;
> +    netStatsDb.addAlarm(alarms[0], function(error, result) {
> +      do_check_eq(error, null);
> +      do_check_eq(result, 1);
> +      netStatsDb.getAlarms(alarms[0].connectionId, alarms[0].manifestURL, function(error, result) {

s/alarms[0].connectionId/Ci.nsINetworkInterface.NETWORK_TYPE_WIFI
s/alarms[0].manifestURL/exampleManifestURL

It sounds strange to input alarm[0] info and check the result by alarm[0] again, although it's logically right. We should make it more general.

@@ +442,5 @@
> +        do_check_eq(result.length, 1);
> +        do_check_eq(result[0].id, alarmsDbId);
> +        do_check_eq(result[0].iface, alarms[0].connectionId);
> +        do_check_eq(result[0].threshold, alarms[0].threshold);
> +        do_check_eq(result[0].data.foo, alarms[0].data.foo);

Do we need to check pageURL/manifestURL here?

@@ +450,5 @@
> +  });
> +});
> +
> +add_test(function test_getFirstAlarm() {
> +  // add alarms[1] -> DB: alarms[0] (1) - alarms[1] (2)

I realize (1) means (id: 1) when seeing more codes. Maybe a better comment could be:

// Add alarms[1] -> DB: [ alarms[0] (id: 1), alarms[1] (id: 2) ]

@@ +451,5 @@
> +});
> +
> +add_test(function test_getFirstAlarm() {
> +  // add alarms[1] -> DB: alarms[0] (1) - alarms[1] (2)
> +  // check first alarm is alarms[1] because threshold is lower

gnit: // "C"... lower"."

@@ +470,5 @@
> +  });
> +});
> +
> +add_test(function test_removeAlarm() {
> +  // remove alarm 2 -> DB: alarms[0] (1)

// Remove alarms[1] (id: 2) -> DB: [ alarms[0] (id: 1) ]

"2" confused me about the id and index.

@@ +471,5 @@
> +});
> +
> +add_test(function test_removeAlarm() {
> +  // remove alarm 2 -> DB: alarms[0] (1)
> +  // check get first return alarms[0]

gnit: // "C"... alarms[0]"."

OK, I call this gnit: comment. A formal comment should start with a upper case and end with a period. This is picky I know but it seems a coding convention.

@@ +488,5 @@
> +  });
> +});
> +
> +add_test(function test_removeAppAlarm() {
> +  // remove alarm 1 -> empty DB

// Remove alarms[0] (id: 1) -> DB: [ ]

@@ +491,5 @@
> +add_test(function test_removeAppAlarm() {
> +  // remove alarm 1 -> empty DB
> +  netStatsDb.removeAlarm(1, alarms[0].manifestURL, function (error, result) {
> +    do_check_eq(error, null);
> +    netStatsDb.getAlarms(alarms[0].connectionId, alarms[0].manifestURL, function(error, result) {

s/alarms[0].connectionId/Ci.nsINetworkInterface.NETWORK_TYPE_WIFI
s/alarms[0].manifestURL/exampleManifestURL

@@ +500,5 @@
> +  });
> +});
> +
> +add_test(function test_getAlarms() {
> +  // add all alarms -> DB: alarms[0] (3) - alarms[1] (4) - alarms[2] (5) - alarms[3] (6)

// Add all alarms -> DB: [ alarms[0] (id: 3),
                           alarms[1] (id: 4),
                           alarms[2] (id: 5),
                           alarms[3] (id: 6) ]

You miss one element. Right?

@@ +501,5 @@
> +});
> +
> +add_test(function test_getAlarms() {
> +  // add all alarms -> DB: alarms[0] (3) - alarms[1] (4) - alarms[2] (5) - alarms[3] (6)
> +  // check that getAlarms for wifi return 2 alarms

gnit: comment.
gnit: s/return/returns

@@ +502,5 @@
> +
> +add_test(function test_getAlarms() {
> +  // add all alarms -> DB: alarms[0] (3) - alarms[1] (4) - alarms[2] (5) - alarms[3] (6)
> +  // check that getAlarms for wifi return 2 alarms
> +  // check that getAlarms for all connections return 3 alarms

gnit: comment
gnit: s/return/returns

@@ +503,5 @@
> +add_test(function test_getAlarms() {
> +  // add all alarms -> DB: alarms[0] (3) - alarms[1] (4) - alarms[2] (5) - alarms[3] (6)
> +  // check that getAlarms for wifi return 2 alarms
> +  // check that getAlarms for all connections return 3 alarms
> +  var index = 0;

Please move this line before var addFunction = ... below.

@@ +506,5 @@
> +  // check that getAlarms for all connections return 3 alarms
> +  var index = 0;
> +
> +  var callback = function () {
> +    netStatsDb.getAlarms(alarms[0].connectionId, alarms[0].manifestURL, function(error, result) {

s/alarms[0].connectionId/Ci.nsINetworkInterface.NETWORK_TYPE_WIFI
s/alarms[0].manifestURL/exampleManifestURL

@@ +509,5 @@
> +  var callback = function () {
> +    netStatsDb.getAlarms(alarms[0].connectionId, alarms[0].manifestURL, function(error, result) {
> +      do_check_eq(error, null);
> +      do_check_eq(result.length, 2);
> +      netStatsDb.getAlarms(-1, alarms[0].manifestURL, function(error, result) {

s/alarms[0].manifestURL/exampleManifestURL

@@ +535,5 @@
> +  addFunction();
> +});
> +
> +add_test(function test_removeAppAllAlarms() {
> +  // remove all alarms for example.com -> DB: alarms[3] (6)

// Remove all alarms for exampleManifestURL -> DB: [ alarms[3] (id: 6) ]

@@ +536,5 @@
> +});
> +
> +add_test(function test_removeAppAllAlarms() {
> +  // remove all alarms for example.com -> DB: alarms[3] (6)
> +  netStatsDb.removeAlarms(alarms[0].manifestURL, function (error, result) {

s/alarms[0].manifestURL/exampleManifestURL

@@ +538,5 @@
> +add_test(function test_removeAppAllAlarms() {
> +  // remove all alarms for example.com -> DB: alarms[3] (6)
> +  netStatsDb.removeAlarms(alarms[0].manifestURL, function (error, result) {
> +    do_check_eq(error, null);
> +    netStatsDb.getAlarms(alarms[0].connectionId, alarms[0].manifestURL, function(error, result) {

s/alarms[0].connectionId/Ci.nsINetworkInterface.NETWORK_TYPE_WIFI
s/alarms[0].manifestURL/exampleManifestURL

@@ +541,5 @@
> +    do_check_eq(error, null);
> +    netStatsDb.getAlarms(alarms[0].connectionId, alarms[0].manifestURL, function(error, result) {
> +      do_check_eq(error, null);
> +      do_check_eq(result.length, 0);
> +      netStatsDb.getAlarms(alarms[3].connectionId, alarms[3].manifestURL, function(error, result) {

s/alarms[3].connectionId/Ci.nsINetworkInterface.NETWORK_TYPE_WIFI
s/alarms[3].manifestURL/testManifestURL

@@ +551,5 @@
> +  });
> +});
> +
> +add_test(function test_updateAlarm() {
> +  // update alarm 6 -> DB: alarms[3]* (6)

// Update alarms[3] (id: 6) -> DB: [ alarms[3]* (id: 6) ]

@@ +553,5 @@
> +
> +add_test(function test_updateAlarm() {
> +  // update alarm 6 -> DB: alarms[3]* (6)
> +
> +  var updateAlarm = alarms[1];

s/updateAlarm/updatedAlarm and below.

::: dom/network/tests/unit_stats/test_networkstats_service.js
@@ +79,5 @@
> +add_test(function test_updateThreshold() {
> +  let alarm = { connectionId: Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                threshold: 10000};
> +
> +  NetworkStatsService._updateThreshold(alarm, function onSet(error, threshold){

Do we need to check error as well?

@@ +86,5 @@
> +    run_next_test();
> +  });
> +});
> +
> +add_test(function test_alarmSetup_ok() {

s/test_alarmSetup_ok/test_setAlarm

@@ +88,5 @@
> +});
> +
> +add_test(function test_alarmSetup_ok() {
> +  let alarm = { id: null,
> +                timerId: null,

Do we still need timerId?

@@ +96,5 @@
> +                alarmStart: null,
> +                alarmEnd: null,
> +                data: null,
> +                pageURL: "http://test.com",
> +                manifestURL: "http://test.com/manifest.webapp" };

...
pageURL: testPageURL,
manifestURL: testManifestURL };

@@ +98,5 @@
> +                data: null,
> +                pageURL: "http://test.com",
> +                manifestURL: "http://test.com/manifest.webapp" };
> +
> +  NetworkStatsService._setAlarm(alarm, function onSetup(error, result) {

s/onSetup/onSet

@@ +104,5 @@
> +    run_next_test();
> +  });
> +});
> +
> +add_test(function test_alarmSetup_threshold_failure() {

s/test_alarmSetup_threshold_failure/test_setAlarm_invalid_threshold

@@ +114,5 @@
> +                alarmStart: null,
> +                alarmEnd: null,
> +                data: null,
> +                pageURL: "http://test.com",
> +                manifestURL: "http://test.com/manifest.webapp" };

...
pageURL: testPageURL,
manifestURL: testManifestURL };

@@ +116,5 @@
> +                data: null,
> +                pageURL: "http://test.com",
> +                manifestURL: "http://test.com/manifest.webapp" };
> +
> +  NetworkStatsService._setAlarm(alarm, function onSetup(error, result) {

s/onSetup/onSet

@@ +123,5 @@
> +  });
> +});
> +
> +add_test(function test_fireAlarm() {
> +  // Add fake alarm into database

gnit: comment.

@@ +133,5 @@
> +                alarmStart: null,
> +                alarmEnd: null,
> +                data: null,
> +                pageURL: "http://test.com",
> +                manifestURL: "http://test.com/manifest.webapp" };

...
pageURL: testPageURL,
manifestURL: testManifestURL };

@@ +137,5 @@
> +                manifestURL: "http://test.com/manifest.webapp" };
> +
> +  NetworkStatsService._db.addAlarm(alarm, function addSuccessCb(error, newId) {
> +    NetworkStatsService._db.getAlarms(Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                                      "http://test.com/manifest.webapp", function onCompleted(error, result) {

s/.../testManifestURL
s/onCompleted/onGet

@@ +142,5 @@
> +      do_check_eq(error, null);
> +      do_check_eq(result.length, 1);
> +      NetworkStatsService._fireAlarm(result[0], false);
> +      NetworkStatsService._db.getAlarms(Ci.nsINetworkInterface.NETWORK_TYPE_WIFI,
> +                                        "http://test.com/manifest.webapp", function onCompleted(error, result) {

s/.../testManifestURL
s/onCompleted/onGet

	Albert [:albert] Assignee
	Comment 69 • 11 years ago

nits fixed

	Albert [:albert] Assignee
	Comment 70 • 11 years ago

(In reply to Gene Lian [:gene] from comment #68)
> Comment on attachment 791226 [details] [diff] [review]
> Part 4/4: Tests
> 
> Review of attachment 791226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/network/tests/unit_stats/test_networkstats_db.js
> 
> @@ +442,5 @@
> > +        do_check_eq(result.length, 1);
> > +        do_check_eq(result[0].id, alarmsDbId);
> > +        do_check_eq(result[0].iface, alarms[0].connectionId);
> > +        do_check_eq(result[0].threshold, alarms[0].threshold);
> > +        do_check_eq(result[0].data.foo, alarms[0].data.foo);
> 
> Do we need to check pageURL/manifestURL here?

No, we don't. The result of getAlarms() does not contain the pageURL/manifestURL, it just use these values for security reasons in the NetworkStatsDB, to restrict alarms access depending on the pair pageURL/manifestURL.

> @@ +500,5 @@
> > +  });
> > +});
> > +
> > +add_test(function test_getAlarms() {
> > +  // add all alarms -> DB: alarms[0] (3) - alarms[1] (4) - alarms[2] (5) - alarms[3] (6)
> 
> // Add all alarms -> DB: [ alarms[0] (id: 3),
>                            alarms[1] (id: 4),
>                            alarms[2] (id: 5),
>                            alarms[3] (id: 6) ]
> 
> You miss one element. Right?

I'm not sure, which one?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 71 • 11 years ago

Comment on attachment 784849 [details] [diff] [review]
Part 1/4: IDL definition

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

sr=me, but I think we should start working on implementing blocking alarms and other policies that don't just warn the user, but helps the user manage their consumption.

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +60,5 @@
> +   * If success, the result field of the request keeps the alarm Id.
> +   */
> +  nsIDOMDOMRequest         addAlarm(in DOMString iface,
> +                                    in long threshold,
> +                                    [optional] in jsval options /* NetworkStatsAlarmOptions */);

I think it'd be cleaner to add all arguments to the "options" object. I realize that the first two arguments here are required, but it still feels cleaner to not mix inline and in-object arguments here. And we could still throw if the interface or threshold is missing.

	Marcelino Veiga Tuimil [:sonmarce]
	Comment 72 • 11 years ago

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 have real time alerts about surpassing data usage limits. Not taking this patch will impact in user feedback, because of not being warned on time about exceeding data usage limit will increase user expenses.

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

(In reply to Albert [:albert] from comment #70)
> (In reply to Gene Lian [:gene] from comment #68)
> > @@ +500,5 @@
> > > +  });
> > > +});
> > > +
> > > +add_test(function test_getAlarms() {
> > > +  // add all alarms -> DB: alarms[0] (3) - alarms[1] (4) - alarms[2] (5) - alarms[3] (6)
> > 
> > // Add all alarms -> DB: [ alarms[0] (id: 3),
> >                            alarms[1] (id: 4),
> >                            alarms[2] (id: 5),
> >                            alarms[3] (id: 6) ]
> > 
> > You miss one element. Right?
> 
> I'm not sure, which one?

Sorry. I probably missed this. Please go ahead to land. Thanks! Btw, please contact Michael Wu for the FFOS' dependency to codeaurora (I just replied to your e-mail).

	Albert [:albert] Assignee
	Comment 74 • 11 years ago

Since Bug 784575 has landed I need to rebase and make some modifications to the database management.

	Marcelino Veiga Tuimil [:sonmarce]
	Comment 75 • 11 years ago

This bug is closely related to bug 887699, as both of them are needed to refactor Cost Control app, for avoiding current limitations, and 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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 76 • 11 years ago

Gregor, do you know the status of this effort?  Does it fall within systems and is it still active?

	Albert [:albert] Assignee
	Comment 77 • 11 years ago

I am currently working on it. I rebased all patches except the one for tests, as soon as I have it I will update, no more than one day.

	John Shih
	Comment 82 • 11 years ago

Comment on attachment 8335900 [details] [diff] [review]
Part 3/4: Alarm implementation

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

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +31,5 @@
>  {
>    readonly attribute unsigned long alarmId;
> +  readonly attribute nsIDOMMozNetworkStatsInterface network;
> +  readonly attribute long threshold;
> +  readonly attribute jsval data;

why didn't you put these in the idl definition patch?

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

please refers to Bug 939061, which simply remove redundant indexes.
since it's more simple, I think we can have it landed first.
does it make sense to you?

@@ +96,5 @@
> +      } else if (currVersion == 3) {
> +        // In order to manage alarms, it is necessary to use a global counter
> +        // (totalBytes) that will increase regardless of the system reboot.
> +        objectStore = aTransaction.objectStore(STATS_STORE_NAME);
> +        objectStore.createIndex("rxSystemBytes", "rxSystemBytes", { unique: false });

Also (refers to bug 939061), actually we don't need to create an index if we don't do search for it.
After creating an index, there will be some reference data created each time we put new data into DB.
This is only used to make search by index available (that is, objectstore.index("xxx")) and has no impact on the structure of objectstore.
So we don't need to do createIndex for every "index" we want in the objectstore.

@@ +98,5 @@
> +        // (totalBytes) that will increase regardless of the system reboot.
> +        objectStore = aTransaction.objectStore(STATS_STORE_NAME);
> +        objectStore.createIndex("rxSystemBytes", "rxSystemBytes", { unique: false });
> +        objectStore.createIndex("txSystemBytes", "txSystemBytes", { unique: false });
> +

ditto.

@@ +106,5 @@
> +        let txCounter = 0;
> +        let lastRx = 0;
> +        let lastTx = 0;
> +
> +        objectStore.openCursor().onsuccess = function(event) {

seems like you are going through all data in DB (regardless system or per-app, and wifi or mobile), are you sure it is what you want?

@@ +137,5 @@
> +
> +        // Create DB for alarms
> +        objectStore = db.createObjectStore(ALARMS_STORE_NAME, { keyPath: "id", autoIncrement: true });
> +        objectStore.createIndex("alarm", ['networkId','threshold'], { unique: false });
> +        objectStore.createIndex("data", "data", { unique: false });

ditto.

@@ +139,5 @@
> +        objectStore = db.createObjectStore(ALARMS_STORE_NAME, { keyPath: "id", autoIncrement: true });
> +        objectStore.createIndex("alarm", ['networkId','threshold'], { unique: false });
> +        objectStore.createIndex("data", "data", { unique: false });
> +        objectStore.createIndex("manifestURL", "manifestURL", { unique: false });
> +        objectStore.createIndex("pageURL", "pageURL", { unique: false });

ditto.

@@ +155,5 @@
> +                  timestamp:     aStats.timestamp,
> +                  rxBytes:       aStats.rxBytes,
> +                  txBytes:       aStats.txBytes,
> +                  rxSystemBytes: aStats.rxSystemBytes,
> +                  txSystemBytes: aStats.txSystemBytes,

Can we have better naming here?
As I'm working on bug obtaining tethering traffic, I found that both interface traffic (which you defined here) and tethering traffic are both accumulative. So These two kinds of traffic data may both need them (which make SystemBytes not that accurate).
However, I don't have good currently.. maybe something like lastTxBytes, lastRxBytes?

::: dom/network/src/NetworkStatsService.jsm
@@ +114,5 @@
>      this.updateQueue = [];
>      this.isQueueRunning = false;
> +
> +    this._currentAlarm = [];
> +    this.initAlarms(); 

nit: extra space

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

Comment on attachment 8335898 [details] [diff] [review]
Part 2/4: Netd integration

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

Looks good, thank you.

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

I'll try to catch up the reviews by the end of this week (or early next week). Before that, Albert, I'd like to know is it just rebasing work or did you add any extra codes that weren't present in the previous patches? If it's the latter, that would help a lot if you could briefly summarize what you've done for the new patches.

	Albert [:albert] Assignee
	Comment 85 • 11 years ago

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #84)
> I'll try to catch up the reviews by the end of this week (or early next
> week). Before that, Albert, I'd like to know is it just rebasing work or did
> you add any extra codes that weren't present in the previous patches? If
> it's the latter, that would help a lot if you could briefly summarize what
> you've done for the new patches.

Just rebasing work, but as last patches were uploaded 3 months ago I had to change some things. Most important changes are in Netd integration patch because changes on NetworkManager and for the Alarms implementation patch the big change is to move from connectionType (string) to network (nsIDOMMozNetworkStatsInterface) and add compatibility with the per-app stats.

	Albert [:albert] Assignee
	Comment 86 • 11 years ago

(In reply to John Shih from comment #82)
> Comment on attachment 8335900 [details] [diff] [review]
> Part 3/4: Alarm implementation
> 
> Review of attachment 8335900 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +106,5 @@
> > +        let txCounter = 0;
> > +        let lastRx = 0;
> > +        let lastTx = 0;
> > +
> > +        objectStore.openCursor().onsuccess = function(event) {
> 
> seems like you are going through all data in DB (regardless system or
> per-app, and wifi or mobile), are you sure it is what you want?

Yes, because now we have a new field systembytes and it should be updated for all records in the database. Now the semantic of totalBytes is different. Before this patch, totalBytes are the number of bytes since system boot up, and now, bytes since boot are stored in systemBytes while totalBytes keep the track of bytes since the first value was inserted even if reboot. 

> @@ +155,5 @@
> > +                  timestamp:     aStats.timestamp,
> > +                  rxBytes:       aStats.rxBytes,
> > +                  txBytes:       aStats.txBytes,
> > +                  rxSystemBytes: aStats.rxSystemBytes,
> > +                  txSystemBytes: aStats.txSystemBytes,
> 
> Can we have better naming here?
> As I'm working on bug obtaining tethering traffic, I found that both
> interface traffic (which you defined here) and tethering traffic are both
> accumulative. So These two kinds of traffic data may both need them (which
> make SystemBytes not that accurate).
> However, I don't have good currently.. maybe something like lastTxBytes,
> lastRxBytes?

Now that I have explained the meaning of systemBytes vs totalBytes do you feel that lastBytes is a good name?

	Albert [:albert] Assignee
	Comment 87 • 11 years ago

Changes from comment 82

	Albert [:albert] Assignee
	Comment 88 • 11 years ago

Changes from comment 82

	John Shih
	Comment 89 • 11 years ago

(In reply to Albert [:albert] from comment #86)
> (In reply to John Shih from comment #82)
> > Comment on attachment 8335900 [details] [diff] [review]
> > Part 3/4: Alarm implementation
> > 
> > Review of attachment 8335900 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > @@ +106,5 @@
> > > +        let txCounter = 0;
> > > +        let lastRx = 0;
> > > +        let lastTx = 0;
> > > +
> > > +        objectStore.openCursor().onsuccess = function(event) {
> > 
> > seems like you are going through all data in DB (regardless system or
> > per-app, and wifi or mobile), are you sure it is what you want?
> 
> Yes, because now we have a new field systembytes and it should be updated
> for all records in the database. Now the semantic of totalBytes is
> different. Before this patch, totalBytes are the number of bytes since
> system boot up, and now, bytes since boot are stored in systemBytes while
> totalBytes keep the track of bytes since the first value was inserted even
> if reboot. 
> 

I understand why you need to do this change, which can allow to trace the total traffic and avoid the interference with reboot.
What I mean is that I think per-app stats and per-network stats need to be accumulated separately. That is, each app and network should have its own total bytes, so that the alarm can be set for different target. However, current implementation seems take all records as the same.

> > @@ +155,5 @@
> > > +                  timestamp:     aStats.timestamp,
> > > +                  rxBytes:       aStats.rxBytes,
> > > +                  txBytes:       aStats.txBytes,
> > > +                  rxSystemBytes: aStats.rxSystemBytes,
> > > +                  txSystemBytes: aStats.txSystemBytes,
> > 
> > Can we have better naming here?
> > As I'm working on bug obtaining tethering traffic, I found that both
> > interface traffic (which you defined here) and tethering traffic are both
> > accumulative. So These two kinds of traffic data may both need them (which
> > make SystemBytes not that accurate).
> > However, I don't have good currently.. maybe something like lastTxBytes,
> > lastRxBytes?
> 
> Now that I have explained the meaning of systemBytes vs totalBytes do you
> feel that lastBytes is a good name?

I initially want to make the naming more clear, since systemBytes can help on tracking both
per-interface (wifi/mobile) and tethering data obtained from netd. But it's fine to keep this ;)

	John Shih
	Comment 90 • 11 years ago

Hi Albert,
I have a few questions here:
1. If there are multiple alarms saved in db with same network interface, what will the behavior be? Since my understanding was that each network interface can be set only one quota, so I got little confused with why need to save multiple alarms. (I think I miss UCs here)

2. How will the behavior be if there are multiple alarms added by different apps? Will there be any interference among these alarms?

Thanks!

	Albert [:albert] Assignee
	Comment 91 • 11 years ago

(In reply to John Shih from comment #90)
> Hi Albert,
> I have a few questions here:
> 1. If there are multiple alarms saved in db with same network interface,
> what will the behavior be? Since my understanding was that each network
> interface can be set only one quota, so I got little confused with why need
> to save multiple alarms. (I think I miss UCs here)

Netd only allows to set one alarm for one interface but the API allows to set more than one alarm per interface. So the idea is to store all alarms in the database and set the one with the lower quota, when the alarm is fired then it is deleted from the database and the next is set.

> 2. How will the behavior be if there are multiple alarms added by different
> apps? Will there be any interference among these alarms?

There is no problem if alarms are set by different apps, as explained before alarms are set sequentially, the difference will be the receiver of the notification. 

> Thanks!

	John Shih
	Comment 92 • 11 years ago

(In reply to Albert [:albert] from comment #91)
> (In reply to John Shih from comment #90)
> > Hi Albert,
> > I have a few questions here:
> > 1. If there are multiple alarms saved in db with same network interface,
> > what will the behavior be? Since my understanding was that each network
> > interface can be set only one quota, so I got little confused with why need
> > to save multiple alarms. (I think I miss UCs here)
> 
> Netd only allows to set one alarm for one interface but the API allows to
> set more than one alarm per interface. So the idea is to store all alarms in
> the database and set the one with the lower quota, when the alarm is fired
> then it is deleted from the database and the next is set.
> 
> > 2. How will the behavior be if there are multiple alarms added by different
> > apps? Will there be any interference among these alarms?
> 
> There is no problem if alarms are set by different apps, as explained before
> alarms are set sequentially, the difference will be the receiver of the
> notification. 
> 
> > Thanks!

Oh I see! much clear now, thanks :)

	John Shih
	Comment 93 • 11 years ago

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

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

looks good to me.

::: dom/network/tests/unit_stats/test_networkstats_db.js
@@ +146,2 @@
>                  rxTotalBytes: 1234,
>                  txTotalBytes: 1234 };

let's align all the values here

@@ +189,2 @@
>                     rxTotalBytes: 1234,
>                     txTotalBytes: 1234 });

ditto.

	John Shih
	Comment 94 • 11 years ago

Comment on attachment 8339210 [details] [diff] [review]
Part 3/4: Alarm implementation

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

Besides the problem I leave in DB part, others look good to me.
You just need to check if it is needed to fix, thanks.

::: dom/network/src/NetworkStatsDB.jsm
@@ +132,5 @@
> +        let txCounter = 0;
> +        let lastRx = 0;
> +        let lastTx = 0;
> +
> +        objectStore.openCursor().onsuccess = function(event) {

Again, at least I think you should accumulate per-app and per-network data interface separately.

	Albert [:albert] Assignee
	Comment 95 • 11 years ago

Changes from comment 93.

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

Comment on attachment 8339208 [details] [diff] [review]
Part 1/4: IDL definition

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

::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl
@@ +21,5 @@
>  };
>  
> +dictionary NetworkStatsAlarmOptions
> +{
> +  jsval startTime;          // date

s/date/Date object/

And one space after ";" is enough.

@@ +57,5 @@
>                                in jsval end,
>                                [optional] in DOMString manifestURL);
>  
>    /**
> +   * Install an alarm on a network. The network must be in availableNetworks

s/availableNetworks/the return of getAvailableNetworks()

since you already had a new way to query available networks.

@@ +58,5 @@
>                                [optional] in DOMString manifestURL);
>  
>    /**
> +   * Install an alarm on a network. The network must be in availableNetworks
> +   * or an InvalidNetwork exception will be raised.

s/or/otherwise, which is more clear.
s/InvalidNetwork/"InvalidNetwork", to emphasize the name of exception.

@@ +61,5 @@
> +   * Install an alarm on a network. The network must be in availableNetworks
> +   * or an InvalidNetwork exception will be raised.
> +   *
> +   * When total data usage reaches threshold bytes, a system message is
> +   * sent to the application. Optional parameter data must be a cloneable

s/data/|data|/

@@ +62,5 @@
> +   * or an InvalidNetwork exception will be raised.
> +   *
> +   * When total data usage reaches threshold bytes, a system message is
> +   * sent to the application. Optional parameter data must be a cloneable
> +   * object and it is included inside the alarm system message.

If my recall is right, it's not an "alarm" system message. Right? Please correct it:

s/alarm/"the-name-I-forget"

I'd simplify the comment:

* When total data usage reaches threshold bytes, a "the-name-I-forget"
* system message is sent to the application, where the optional parameter
* |data| must be a cloneable object.

@@ +64,5 @@
> +   * When total data usage reaches threshold bytes, a system message is
> +   * sent to the application. Optional parameter data must be a cloneable
> +   * object and it is included inside the alarm system message.
> +   *
> +   * If success, the result field of the request keeps the alarm Id.

s/result/|result|
s/request/DOMRequest/

@@ +71,5 @@
> +                            in long threshold,
> +                            [optional] in jsval options /* NetworkStatsAlarmOptions */);
> +
> +  /**
> +   * Obtain all alarms for those networks in availableNetworks.

s/in availableNetworks/returned by getAvailableNetworks()/

@@ +74,5 @@
> +  /**
> +   * Obtain all alarms for those networks in availableNetworks.
> +   * If a network is provided, only retrieves the alarms
> +   * for that network. The network must be one of those in
> +   * availebleNetworks or an InvalidNetwork exception will be raised.

s/in availableNetworks/returned by getAvailableNetworks()/
s/InvalidNetwork/"InvalidNetwork"

Btw, please don't wrap comments if it doesn't exceed 80 chars.

@@ +75,5 @@
> +   * Obtain all alarms for those networks in availableNetworks.
> +   * If a network is provided, only retrieves the alarms
> +   * for that network. The network must be one of those in
> +   * availebleNetworks or an InvalidNetwork exception will be raised.
> +   * 

Trailing white space.

@@ +77,5 @@
> +   * for that network. The network must be one of those in
> +   * availebleNetworks or an InvalidNetwork exception will be raised.
> +   * 
> +   * Each alarm object has the same fields as that
> +   * in the system message:

Please don't wrap comments if it doesn't exceed 80 chars.

@@ +86,5 @@
> +   */
> +  nsIDOMDOMRequest getAllAlarms([optional] in nsIDOMMozNetworkStatsInterface network);
> +
> +  /**
> +   * Remove all network alarms. If an alarmId is provided, then only that

s/alarmId/|alarmId|/

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

Comment on attachment 8339210 [details] [diff] [review]
Part 3/4: Alarm implementation

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +132,5 @@
> +        let txCounter = 0;
> +        let lastRx = 0;
> +        let lastTx = 0;
> +
> +        objectStore.openCursor().onsuccess = function(event) {

As John mentioned, this part is wrong. Note that we don't have system reboot issue for that per-app metering. This logic only works for system metering.

Since the keyPath is ["appId", "network", "timestamp"], I assume (please double check) all the records would be sorted like:

appId  rxTotalBytes 
--------------------
    0             0
    0            10 
    0          1000
    0             0
    0            20
    0           200
    1           500
    1          5000
    1         50000
    2
    2
    2

As you can see, there are system reboots in the block of appId == 0. However, you cannot do the same thing when it crosses the gap between 0 and 1.

So, I think you can just apply the logic for appId == 0 (watch out the edge case in the switch of 0 and 1). For appId != 0, please just copy rxTotalBytes to rxSystemBytes for consistency.

What do you think?

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

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

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

Looks nice. Just having some picky nits. ;)

::: dom/network/tests/test_networkstats_alarms.html
@@ +57,5 @@
> +    try {
> +      navigator.mozNetworkStats.addAlarm();
> +    } catch(ex) {
> +      ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS,
> +        "addAlarm() throws NS_ERROR_XPC_NOT_ENOUGH_ARGS exception when no parameters");

I don't want to be picky but please align this line with the above parameter.

@@ +64,5 @@
> +    try {
> +      navigator.mozNetworkStats.addAlarm(100000);
> +    } catch(ex) {
> +      ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS,
> +        "addAlarm() throws NS_ERROR_XPC_NOT_ENOUGH_ARGS exception when no network");

Ditto.

@@ +71,5 @@
> +    try {
> +      navigator.mozNetworkStats.addAlarm(wifi);
> +    } catch(ex) {
> +      ok(ex.result == SpecialPowers.Cr.NS_ERROR_XPC_NOT_ENOUGH_ARGS,
> +        "addAlarm() throws NS_ERROR_XPC_NOT_ENOUGH_ARGS exception when no threshold");

Ditto.

::: dom/network/tests/unit_stats/test_networkstats_db.js
@@ +19,5 @@
>    }, callback);
>  }
>  
> +add_test(function prepareDatabase() {
> +  // Clear whole database to avoid start tests with unknown state

s/start/starting/

@@ +20,5 @@
>  }
>  
> +add_test(function prepareDatabase() {
> +  // Clear whole database to avoid start tests with unknown state
> +  // due to previous tests.

s/previous/the previous/

@@ +604,5 @@
>  
> +            do_check_eq(error, null);
> +            do_check_eq(result.length, 4);
> +            do_check_eq(result[0].appId, 1);
> +            do_check_true(compareNetworks(result[0].network,[networkWifi.id, networkWifi.type]));

Add one space after ",".

::: dom/network/tests/unit_stats/test_networkstats_service.js
@@ +147,5 @@
>  
> +var wifiId = '00';
> +
> +add_test(function test_updateThreshold() {
> +  let alarm = { networkId: wifiId, threshold: 10000};

Add one space before "}".

@@ +195,5 @@
> +  });
> +});
> +
> +add_test(function test_fireAlarm() {
> +  // Add fake alarm into database.

s/fake/a fake/

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

I'd suggest landing this after branching V1.3 because:

1. This bug has never been marked as V1.3+, which shows it's not really a critical blocker.
2. This bug will have lots of DB schema changes, which might a bit risky to pollute the existing codes. 
3. Gaia also needs extra work to use the new APIs. If we only have Gecko ready in V1.3 then it's just a vain.

So personally I'd suggest targeting this as V1.4.

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

I just want to get the comment #100. :P

	Marcelino Veiga Tuimil [:sonmarce]
	Comment 101 • 11 years ago

I think we cannot wait for version 1.4 to have alarms landed, currently in Cost Control we have no way to warn user about reaching top limit usage in real time, so it is really important to have this bug landed to fix Cost Control bugs related to real time usage alarms.

	Albert [:albert] Assignee
	Comment 102 • 11 years ago

Changes from comment 96

	Albert [:albert] Assignee
	Comment 103 • 11 years ago

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #100)
> I just want to get the comment #100. :P

:D

	Albert [:albert] Assignee
	Comment 104 • 11 years ago

Changes from comment 97.

Thanks for the example! It helps a lot to understand John's concern, I missed the appId.

	Albert [:albert] Assignee
	Comment 105 • 11 years ago

Changes from comment 98.

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

Comment on attachment 8342570 [details] [diff] [review]
Part 3/4: Alarm implementation

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

Looks very nice! Just some nits. r=gene

Before landing, I'd like to see John's double check again, especially for the bytes/totalBytes/systemBytes calculations.

::: dom/network/src/NetworkStatsDB.jsm
@@ +166,5 @@
> +          cursor.update(cursor.value);
> +          cursor.continue();
> +        };
> +
> +        // Create DB for alarms

s/DB/object store/ and add a period at the end of the comment.

@@ +172,5 @@
> +        objectStore.createIndex("alarm", ['networkId','threshold'], { unique: false });
> +        objectStore.createIndex("manifestURL", "manifestURL", { unique: false });
> +
> +        if (DEBUG) {
> +          debug("Created alarms store for upgrade 5");

s/upgrade 5/version 5/ to have a more consistent log with others.

@@ +368,4 @@
>        // If diff < 0, clock or timezone changed back. Place data in the last sample.
>  
> +      // Old |rxTotalBytes|/|txTotalBytes| needs to get updated by
> +      // adding the last |rxTotalBytes|/|txTotalBytes|.

Don't need to wrap if lines don't exceed 80 chars.

Please do this for all the comments starting from "// New element...".

@@ +496,5 @@
> +  getCurrentStats: function getCurrentStats(aNetwork, aDate, aResultCb) {
> +    if (DEBUG) {
> +      debug("Get current stats for " + JSON.stringify(aNetwork) + " since " + aDate);
> +    }
> +    this.dbNewTxn(STATS_STORE_NAME, "readonly", function(txn, store) {

Add one blank line above this line.

@@ +670,5 @@
> +  },
> +
> +  addAlarm: function addAlarm(aAlarm, aResultCb) {
> +    if (DEBUG) {
> +      debug("addAlarm");

This debug message is a bit redundant, I think, since you're already printed:

debug("Going to add " + JSON.stringify(aAlarm));

@@ +747,5 @@
> +    }, aResultCb);
> +  },
> +
> +  updateAlarm: function updateAlarm(aAlarm, aResultCb) {
> +    let record = this.alarmToRecord(aAlarm);

Please move this line into dbNewTxn(...). Don't do this until we really need it.

@@ +781,5 @@
> +        }
> +
> +        if (!aNetworkId || cursor.value.networkId == aNetworkId) {
> +          let alarm = { id: cursor.value.id,
> +                        network: cursor.value.networkId,

s/network/networkId

::: dom/network/src/NetworkStatsService.jsm
@@ +117,5 @@
>  
>      this.updateQueue = [];
>      this.isQueueRunning = false;
> +
> +    this._currentAlarm = [];

s/_currentAlarm/_currentAlarms/ since this var will contain multiple alarms based on networkId. Please also rename all the codes using it.

Also, why is this not an object? I mean:

this._currentAlarms = {};

@@ +780,5 @@
> +        return;
> +      }
> +    }
> +
> +    let alarms = [];

This var is not needed.

@@ +789,5 @@
> +                            { id: msg.id, error: error, result: result });
> +        return;
> +      }
> +
> +      for (let i = 0; i < result.length; i++) {

Add a comment before this line:

// Add |network| property for the alarms to return.

@@ +790,5 @@
> +        return;
> +      }
> +
> +      for (let i = 0; i < result.length; i++) {
> +        result[i].network = self._networks[result[i].network].network;

result[i].network = self._networks[result[i].networkId].network;

However, since you don't want to expose |networkId| then you'd better prepare a new alarms array to return. So please move the above |let alarms = []| into here and assign the needed properties for it.

@@ +794,5 @@
> +        result[i].network = self._networks[result[i].network].network;
> +      }
> +
> +      mm.sendAsyncMessage("NetworkStats:GetAlarms:Return",
> +                          { id: msg.id, error: null, result: result });

So, { id: msg.id, error: null, result: alarms }

@@ +909,5 @@
> +        aCallback(aError, null);
> +        return;
> +      }
> +
> +      let _callback = function onAlarmSet(aError) {

Please just use s/_callback/callback/ since you use |aCallback| as param. So, no conflicts.

@@ +926,5 @@
> +      let interfaceName = self._networks[aAlarm.networkId].interfaceName;
> +      if (interfaceName) {
> +        networkService.setNetworkInterfaceAlarm(interfaceName,
> +                                                aThreshold.systemThreshold,
> +                                                _callback);

s/_callback/callback/

@@ +955,5 @@
> +          aCallback("InvalidStateError", null);
> +          return;
> +        }
> +
> +        let _threshold = {

s/_threshold/threshold/

@@ +960,5 @@
> +          systemThreshold: result.rxSystemBytes + result.txSystemBytes + offset,
> +          absoluteThreshold: result.rxTotalBytes + result.txTotalBytes + offset
> +        };
> +
> +        aCallback(null, _threshold);

s/_threshold/threshold/

	John Shih
	Comment 107 • 11 years ago

Comment on attachment 8342570 [details] [diff] [review]
Part 3/4: Alarm implementation

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

::: dom/network/src/NetworkStatsDB.jsm
@@ +141,5 @@
> +
> +          cursor.value.rxSystemBytes = cursor.value.rxTotalBytes;
> +          cursor.value.txSystemBytes = cursor.value.txTotalBytes;
> +
> +          if (cursor.value.appId == 0) {

Great! We are finally aligned ;)
BTW, have you also considered if the same issue happened on data with different networks?

Something like:
AppId    Networks           rxTotalBytes
0        {id: 0, type: 0}   0
0        {id: 0, type: 0}   1000
0        {id: 0, type: 0}   10000
0        {id: 0, type: 0}   0
0        {id: 0, type: 0}   1000
0        {id: 0, type: 0}   20
0        {id: 0, type: 1}   0
0        {id: 0, type: 1}   100
0        {id: 0, type: 1}   10000
...

	Albert [:albert] Assignee
	Comment 108 • 11 years ago

(In reply to John Shih from comment #107)
> Comment on attachment 8342570 [details] [diff] [review]
> Part 3/4: Alarm implementation
> 
> Review of attachment 8342570 [details] [diff] [review]:
> -----------------------------------------------------------------
> Great! We are finally aligned ;)
> BTW, have you also considered if the same issue happened on data with
> different networks?

That's a good point, fixed

	Albert [:albert] Assignee
	Comment 109 • 11 years ago

Added the check of same network when upgrading db to version 5

	Albert [:albert] Assignee
	Comment 110 • 11 years ago

After changing implementation, tests needed some fixes:
- Change network by networkId
- Add function to clear alarm store in order to ensure that testing environment is the expected.

	Albert [:albert] Assignee
	Comment 111 • 11 years ago

Tests revealed an error:

NetworkStatsDB:760 -> this not defined, so added self.

	Albert [:albert] Assignee
	Comment 112 • 11 years ago

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

	John Shih
	Comment 113 • 11 years ago

Comment on attachment 8343135 [details] [diff] [review]
Part 3/4: Alarm implementation

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

Looks good for me! I think we can have this bug landed very soon :)

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

Comment on attachment 8343135 [details] [diff] [review]
Part 3/4: Alarm implementation

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

Nice! Please r=gene,jshih

::: dom/network/src/NetworkStatsDB.jsm
@@ +146,5 @@
> +              counters[netId] = {};
> +              counters[netId].rxCounter = 0;
> +              counters[netId].txCounter = 0;
> +              counters[netId].lastRx = 0;
> +              counters[netId].lastTx = 0;

Please just do:

if (!counters[netId]) {
  counters[netId] = {
    rxCounter: 0,
    txCounter: 0,
    lastRx: 0,
    lastTx: 0
  };
}

	Albert [:albert] Assignee
	Comment 115 • 11 years ago

Added 'r=gene,jshih' and change from comment 114

	Ryan VanderMeulen [:RyanVM]
	Comment 116 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/2052ba4c936c
https://hg.mozilla.org/integration/b2g-inbound/rev/97b690fd7806
https://hg.mozilla.org/integration/b2g-inbound/rev/7cbdd95bf371
https://hg.mozilla.org/integration/b2g-inbound/rev/34d915243969

	Ryan VanderMeulen [:RyanVM]
	Comment 117 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/2052ba4c936c
https://hg.mozilla.org/mozilla-central/rev/97b690fd7806
https://hg.mozilla.org/mozilla-central/rev/7cbdd95bf371
https://hg.mozilla.org/mozilla-central/rev/34d915243969

	Preeti Raghunath(:Preeti)
	Comment 118 • 11 years ago

Gene,

What is the impact to the user? Is it better experience to the user?

	John Shih
	Comment 119 • 11 years ago

Hi Preeti,
with this patch, user is able to set a usage alarm for a specific network interface.

a simple use case can be
1. User can set a threshold of 3g traffic to 100 MB.
2. When the 3g traffic reaches the threshold, an alarm will fire.

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

I'd like to let :salva answer this, who is in charge of the Network Metering features. I guess :Preeti needs to decide whether or not to mark this as V1.3+. Right?

	Albert [:albert] Assignee
	Comment 121 • 11 years ago

The leak of this feature causes that alarms in data usage application are not fired when the threshold is reached, there is an important delay notifying users, as can be seen in bug 850125.

	Marcelino Veiga Tuimil [:sonmarce]
	Comment 122 • 11 years ago

As Albert said, not taking this bug implies not having real time usage alarm when reaching top limit, what in the end implies users cannot control their expenses

	Tony Chung [:tchung]
	Comment 123 • 11 years ago

koi+.  please comment if there are any major side effects that could affect the branch from this patch.  but it's been landed on trunk with tests over a month now so seems safe.

	Tony Chung [:tchung]
	Comment 124 • 11 years ago

(In reply to Tony Chung [:tchung] from comment #123)
> koi+. 

Sorry, i meant 1.3+!

	Ryan VanderMeulen [:RyanVM]
	Comment 125 • 11 years ago

This landed when trunk was v1.3.