Currently, availableNetworks method from NetworkStatsMAnager returns only the active networks when it gets called. CostControl needs to know all networks having data in the database to make it visible to users.

Fix problems found during updating tests

Comment on attachment 830773 [details] [diff] [review] bug-937041-fix Review of attachment 830773 [details] [diff] [review]: ----------------------------------------------------------------- simple suggestion here ;) ::: dom/network/src/NetworkStatsDB.jsm @@ +473,5 @@ > } > }, > > + networks: function networks(aResultCb) { > + let self = this; do you still need this |self|? @@ +485,5 @@ > + request.onsuccess = function onsuccess(event) { > + let cursor = event.target.result; > + if (cursor) { > + aTxn.result.push({ id: cursor.key[0], > + type: cursor.key[1]}); I think you need to align this line :) and also add a space before } ::: dom/network/src/NetworkStatsManager.js @@ +290,5 @@ > return null; > } > > this.initDOMRequestHelper(aWindow, ["NetworkStats:Get:Return", > + "NetworkStats:Networks:Return", Same here, I suggest to use an "action" term instead of just an noun what do you think? ::: dom/network/src/NetworkStatsService.jsm @@ +294,5 @@ > }); > }, > > clearDB: function clearDB(mm, msg) { > + let self = this; you don't need this |self| anymore, right? @@ +295,5 @@ > }, > > clearDB: function clearDB(mm, msg) { > + let self = this; > + this._db.networks(function onGetNetworks(aError, aResult) { maybe we can use more clear naming instead of just |networks| just a little suggestion :)

Changes from comment 4

(In reply to John Shih from comment #4) > Comment on attachment 830773 [details] [diff] [review] > bug-937041-fix > > Review of attachment 830773 [details] [diff] [review]: > ----------------------------------------------------------------- > > simple suggestion here ;) > > ::: dom/network/src/NetworkStatsService.jsm > @@ +294,5 @@ > > }); > > }, > > > > clearDB: function clearDB(mm, msg) { > > + let self = this; > > you don't need this |self| anymore, right? This self is still necessary, it is used inside the callback, at line 307. Thanks for your review!

Comment on attachment 830862 [details] [diff] [review] bug-937041-fix Review of attachment 830862 [details] [diff] [review]: ----------------------------------------------------------------- nice work! with only one little nit :) I'll leave other detail for Gene to review. ::: dom/network/src/NetworkStatsDB.jsm @@ +484,5 @@ > + request.onsuccess = function onsuccess(event) { > + let cursor = event.target.result; > + if (cursor) { > + aTxn.result.push({ id: cursor.key[0], > + type: cursor.key[1]}); it would be better to do like: aTxn.result.push({ id: cursor.key[0], type: cursor.key[1] }); e.g, align the cursor and leave a space between ] and }

(In reply to John Shih from comment #7) > Comment on attachment 830862 [details] [diff] [review] > bug-937041-fix > > Review of attachment 830862 [details] [diff] [review]: > ----------------------------------------------------------------- > > nice work! with only one little nit :) > I'll leave other detail for Gene to review. > > ::: dom/network/src/NetworkStatsDB.jsm > @@ +484,5 @@ > > + request.onsuccess = function onsuccess(event) { > > + let cursor = event.target.result; > > + if (cursor) { > > + aTxn.result.push({ id: cursor.key[0], > > + type: cursor.key[1]}); > > it would be better to do like: > aTxn.result.push({ id: cursor.key[0], > type: cursor.key[1] }); > > e.g, align the cursor and leave a space between ] and } sorry, the following was the correct one. aTxn.result.push({ id: cursor.key[0], type: cursor.key[1] });

Changes from comment 8 and moved availableNetworks to getAvailableNetworks

Comment on attachment 831745 [details] [diff] [review] bug-937041-fix Review of attachment 831745 [details] [diff] [review]: ----------------------------------------------------------------- Nice patch! Please fix some nits. r=gene Also, please land bug 937067 first to ensure the backward-compatibility with Gaia. QA won't be happy any possibilities of breaking functions. ;) ::: dom/network/interfaces/nsIDOMNetworkStatsManager.idl @@ +19,5 @@ > */ > readonly attribute DOMString id; > }; > > +[scriptable, uuid(5f033d31-c9a2-4e2d-83aa-6a807f1e0c11)] nit: since you're here, please remove one space after ",". @@ +52,5 @@ > */ > nsIDOMDOMRequest clearAllStats(); > > /** > + * Return networks with data in the database. Return available networks that used to be saved in the database. @@ +57,2 @@ > */ > + nsIDOMDOMRequest getAvailableNetworks(); // array of nsIDOMMozNetworkStatsInterface. Hmm... I think we can keep the original name since "available" here means available in the DB. ::: dom/network/src/NetworkStatsDB.jsm @@ +472,5 @@ > date: new Date(aData[aData.length - 1].date.getTime() + SAMPLE_RATE) }); > } > }, > > + getNetworks: function getNetworks(aResultCb) { I'd prefer: s/getNetworks/getAvailableNetworks/ as mentioned above, to emphasize the networks available in the DB. @@ +473,5 @@ > } > }, > > + getNetworks: function getNetworks(aResultCb) { > + // Clear and save an empty sample to keep sync with system counters Please drop this comment. ;) ::: dom/network/src/NetworkStatsManager.js @@ +193,4 @@ > this.checkPrivileges(); > > + let request = this.createRequest(); > + cpmm.sendAsyncMessage("NetworkStats:GetNetworks", I'd prefer s/NetworkStats:GetNetworks/NetworkStats:GetAvailableNetworks/ if you don't mind. @@ +235,5 @@ > } > Services.DOMRequest.fireSuccess(req, result); > break; > > + case "NetworkStats:GetNetworks:Return": Ditto. I'd prefer "NetworkStats:GetAvailableNetworks:Return". @@ +290,5 @@ > return null; > } > > this.initDOMRequestHelper(aWindow, ["NetworkStats:Get:Return", > + "NetworkStats:GetNetworks:Return", Ditto. ::: dom/network/src/NetworkStatsService.jsm @@ +82,5 @@ > > this.messages = ["NetworkStats:Get", > "NetworkStats:Clear", > "NetworkStats:ClearAll", > + "NetworkStats:GetNetworks", I'd prefer "NetworkStats:GetAvailableNetworks". @@ +124,5 @@ > break; > case "NetworkStats:ClearAll": > this.clearDB(mm, msg); > break; > + case "NetworkStats:GetNetworks": Ditto. "NetworkStats:GetAvailableNetworks". @@ +222,5 @@ > return aIccId + '' + aNetworkType; > }, > > + getAvailableNetworks: function getAvailableNetworks(mm, msg) { > + this._db.getNetworks(function onGetNetworks(aError, aResult) { s/getNetworks/getAvailableNetworks @@ +223,5 @@ > }, > > + getAvailableNetworks: function getAvailableNetworks(mm, msg) { > + this._db.getNetworks(function onGetNetworks(aError, aResult) { > + mm.sendAsyncMessage("NetworkStats:GetNetworks:Return", Ditto. "NetworkStats:GetAvailableNetworks:Return". @@ +295,5 @@ > }, > > clearDB: function clearDB(mm, msg) { > + let self = this; > + this._db.getNetworks(function onGetNetworks(aError, aResult) { s/getNetworks/getAvailableNetworks

Comment on attachment 831747 [details] [diff] [review] Update tests Review of attachment 831747 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks nice to me. Please run try again before landing. Thanks! r=gene ::: dom/network/tests/unit_stats/test_networkstats_service.js @@ +3,5 @@ > > const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > > +function getNetworks(callback) { > + NetworkStatsService._db.networks(function onGetNetworks(aError, aResult) { getAvailableNetworks? How could networks work?

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #11) > Comment on attachment 831745 [details] [diff] [review] > bug-937041-fix > > Review of attachment 831745 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice patch! Please fix some nits. r=gene > > Also, please land bug 937067 first to ensure the backward-compatibility with > Gaia. QA won't be happy any possibilities of breaking functions. ;) Sure! > @@ +57,2 @@ > > */ > > + nsIDOMDOMRequest getAvailableNetworks(); // array of nsIDOMMozNetworkStatsInterface. > > Hmm... I think we can keep the original name since "available" here means > available in the DB. I don't understand why do you suggest to change all 'availableNetworks' references to 'getAvailableNetworks' in whole NetworkStats API but you want to keep 'availableNetworks' for the interface. The change of the function name (s/availableNetworks/getAvailableNetworks) comes as a proposal from costcontrol's owner. Now that it is a function instead of an attribute, I prefer 'getAvailableNetworks' because it represents better an action. Are you agree?

(In reply to Albert [:albert] from comment #13) > I don't understand why do you suggest to change all 'availableNetworks' > references to 'getAvailableNetworks' in whole NetworkStats API but you want > to keep 'availableNetworks' for the interface. Oh! Sorry for letting you confuse! Actually, I support for |getAvailableNetworks|! What I said is to emphasize we can keep the keyword "available", compared to the alternative |getNetworks|, because you're now hoping to get "networks in the DB" instead of "available networks" that are currently on. So, what I mean is to reinterpret the meaning of "available" as "available networks that have been used for metering", so we still can reuse the keyword "available". Please go ahead to land after fixing the nits. :)

Changes from comment 11 and rebase

Changes from comment 12

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

I found that is not enough to modify the 'getAvailableNetworks' function because costcontrol will request data for non-active networks and currently it is not possible, 'getSamples' will return 'Invalid connectionType'. To allow it I changed the 'getSamples' function of NetworkStatsService, I also had to add a function in the NetworkStatsDb to check if the network provided to the API is in the database.

Comment on attachment 8333560 [details] [diff] [review] bug-937041-fix Review of attachment 8333560 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/network/src/NetworkStatsDB.jsm @@ +491,5 @@ > + }; > + }, aResultCb); > + }, > + > + isNetworkValid: function isNetworkValid(aNetwork, aResultCb) { s/isNetworkValid/isNetworkAvailable to have a more consistent semantics with |getAvailableNetworks|. ::: dom/network/src/NetworkStatsService.jsm @@ +262,5 @@ > > let start = new Date(msg.start); > let end = new Date(msg.end); > > + if (this._networks[netId]) { Please add a comment here before this condition: // Check if the network is currently active. If yes, we need to update // the cached stats first before retrieving stats from the DB. @@ +277,5 @@ > + }); > + return; > + } > + > + this._db.isNetworkValid(network, function(aError, aResult) { s/isNetworkValid/isNetworkAvailable/ and add a comment before this condition: // Check if the network is available in the DB. If yes, we also // retrieve the stats for it from the DB. Btw, I'm wondering why _db.find() cannot play the same role of _db.isNetworkAvailable()? That is, if no records are found, then return "Invalid connectionType". Do we really need to create another function _db.isNetworkAvailable()? Review+'ed but that would be nice if you could answer the above question. Thanks! Btw, the change on getSamples() won't break the tests. Right?

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #19) > Comment on attachment 8333560 [details] [diff] [review] > bug-937041-fix > > Review of attachment 8333560 [details] [diff] [review]: > ----------------------------------------------------------------- > > Btw, I'm wondering why _db.find() cannot play the same role of > _db.isNetworkAvailable()? That is, if no records are found, then return > "Invalid connectionType". Do we really need to create another function > _db.isNetworkAvailable()? The reason to create the isNetworkAvailable is to make the request to the database simpler. The find function has two negative points to find if a network is in the database: - It is boundered to dates. - It does some processing with result before returning to the callback. So, isNetworkAvailable is faster and requires less logic. > Btw, the change on getSamples() won't break the tests. Right? I launched tests and all goes fine, however I'm going to launch a try server.

Changes from comment 19

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

Try server after rebase: https://tbpl.mozilla.org/?tree=Try&rev=d5398b030f47

https://hg.mozilla.org/integration/b2g-inbound/rev/bbc9461bdd13 https://hg.mozilla.org/integration/b2g-inbound/rev/f067a2bcf079

https://hg.mozilla.org/mozilla-central/rev/bbc9461bdd13 https://hg.mozilla.org/mozilla-central/rev/f067a2bcf079