Closed Bug 937041 Opened 11 years ago Closed 11 years ago

B2G Network Stats: modify availableNetworks method to return all networks having data in database instead of active networks only

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 5 - 11/22
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: albert, Assigned: albert)

References

Details

Attachments

(2 files, 8 obsolete files)

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.
Assignee: nobody → acperez
Blocks: 858003
Depends on: 937067
Attached patch Patch (obsolete) — Splinter Review
Attachment #830221 - Flags: review?(gene.lian)
blocking-b2g: --- → 1.3?
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Attached patch bug-937041-fix (obsolete) — Splinter Review
Fix problems found during updating tests
Attachment #830221 - Attachment is obsolete: true
Attachment #830221 - Flags: review?(gene.lian)
Attachment #830773 - Flags: review?(gene.lian)
Attached patch Update tests (obsolete) — Splinter Review
Attachment #830774 - Flags: review?(gene.lian)
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 :)
Attached patch bug-937041-fix (obsolete) — Splinter Review
Changes from comment 4
Attachment #830773 - Attachment is obsolete: true
Attachment #830773 - Flags: review?(gene.lian)
Attachment #830862 - Flags: review?(jshih)
Attachment #830862 - Flags: review?(gene.lian)
(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 }
Attachment #830862 - Flags: review?(jshih) → review+
(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] });
Attached patch bug-937041-fix (obsolete) — Splinter Review
Changes from comment 8 and moved availableNetworks to getAvailableNetworks
Attachment #830862 - Attachment is obsolete: true
Attachment #830862 - Flags: review?(gene.lian)
Attachment #831745 - Flags: review?(gene.lian)
Attached patch Update tests (obsolete) — Splinter Review
Attachment #830774 - Attachment is obsolete: true
Attachment #830774 - Flags: review?(gene.lian)
Attachment #831747 - Flags: review?(gene.lian)
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
Attachment #831745 - Flags: review?(gene.lian) → review+
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?
Attachment #831747 - Flags: review?(gene.lian) → review+
(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. :)
Attached patch bug-937041-fix (obsolete) — Splinter Review
Changes from comment 11 and rebase
Attachment #831745 - Attachment is obsolete: true
Attachment #833054 - Flags: review+
Attached patch Update testsSplinter Review
Changes from comment 12
Attachment #831747 - Attachment is obsolete: true
Attachment #833056 - Flags: review+
Attached patch bug-937041-fix (obsolete) — Splinter Review
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.
Attachment #833054 - Attachment is obsolete: true
Attachment #8333560 - Flags: review?(gene.lian)
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?
Attachment #8333560 - Flags: review?(gene.lian) → review+
(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.
Attached patch bug-937041-fixSplinter Review
Changes from comment 19
Attachment #8333560 - Attachment is obsolete: true
Attachment #8334460 - Flags: review+
Try server after rebase: https://tbpl.mozilla.org/?tree=Try&rev=d5398b030f47
Keywords: checkin-needed
blocking-b2g: 1.3? → 1.3+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: