+++ This bug was initially created as a clone of Bug #1043132 +++ To be able to merge NetworkStats into ResourceStats, some trivial jobs have to be done to simplify what we have in NetworkStats.

Several functions are never referenced in production code. Remove them first.

WebIDL dictionary NetworkStatsAlarmOptions is obsoleted by ResourceStatsAlarmOptions, which avoids Date usage. Gaia CostControl in master branch does not use this dictionary type.

Renaming nsINetworkStatsServiceProxy to nsINetworkStatsService because: 1) We have two service contract ID "@mozilla.org/networkstatsServiceProxy;1" and "@mozilla.org/netstatsservice;1" and that is pretty confusing. The latter is not really a service but a jsm instead. 2) To move to ResourceStats API, we'd probably have only one ResourceStatsService for all kinds of input sources. Here I'm going to reuse the NetworkStatsService.jsm and land some more patch on it so that some day we may obsolete ResourceStatsService.jsm directly.

Permission/object type checking lines are removed because related annotations has been added to the WebIDL interface.

Before going deeper into the NetworkStats lines, I'd like to clarify which member/methods are private, never have been referenced outside the modules. This patch add '_' to the name of such members/methods. Rename only, no logical changes.

Use: memberFunc: function() { ... } Instead of: memberFunc: function memberFunc() { ... }

Prefer |{}| over |Object.create(null)|.

Rename a lot arguments, variables to reflect the types/meanings in a consistent way.

1) Document fields of the IPC message used in 'NetworkStats:Get' 2) Avoid use of 'Date' object. 3) Returns only an array of stats from NetworkStatsDB::find(). Other parameters can be constructed from NetworkStatsService.jsm. Just don't inject additional complexity into a sub-component when unnecessary.

'unique' field of the IDBIndexParameters used in ObjectStore::createIndex() has already default value 'false'. See http://www.w3.org/TR/IndexedDB/#idl-def-IDBIndexParameters .

Address my bug 854200 comment 110. IndexedDB operations are usually executed asynchronously. That follows one cannot assure the operations queued in upgrading to previous version have been completed when we reach the next version upgrade function. As time goes by, version number increases and the more and more operations gets queued, and such uncertainty begins to bring troubles to newly upgraded devices and may at worse fail the boot process.

Additional white space characters are left in previous patch in order to ease the review process. This patch removes them.

Assign |aTransaction.result| only when that transaction is going to complete.

With these patches, we clean up NetworkStats code a little bit but there is still tons of things to do before we may ever migrate to ResourceStats API. The following-up will probably directly move NetworkStats major functions around, alter DB/IPC object layout, etc..

Comment on attachment 8463088 [details] [diff] [review] part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary Need DOM peer's review for it touches WebIDL files.

Comment on attachment 8463088 [details] [diff] [review] part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary Hmm. ResourceStatsAlarmOptions allows "data" values that NetworkStatsAlarmOptions didn't use to allow. Why do we want to do that?

Comment on attachment 8463090 [details] [diff] [review] part 4/7: Move nsIDOMNetworkStatsManager to WebIDL >+ CheckPermissions="networkstats-manage", I assume you verified that this still leaves the API exposed in chrome? >+ DOMRequest getAvailableNetworks(); // array of MozNetworkStatsInterface. We should really use Promises here, long term. Followup bug? r=me

Comment on attachment 8463088 [details] [diff] [review] part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary In fact, I'm 99% sure that this will introduce a security bug akin to bug 1015540. So let's not do this, please.

(In reply to Boris Zbarsky [:bz] from comment #20) > Comment on attachment 8463088 [details] [diff] [review] > part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary > > Hmm. ResourceStatsAlarmOptions allows "data" values that > NetworkStatsAlarmOptions didn't use to allow. Why do we want to do that? 1) I think that 'Date' type in NetworkStatsAlarmOptions is some kind of typo. It should be of any type as was latterly corrected in ResourceStatsAlarmOptions. 2) No evidence shows that it has to be Date-typed throughout NetworkStats code lines. By design that data field is to be directly stored into database and acts as some callback data when emitting "networkstats-alarm" system messages. Gaia does not utilize this dictionary in any case so far. Based on the two points, I thought we'd better correct it before anyone has an interest in it. I can remove this patch from this bug temporarily, but we'll still have to face it in bug 1043132 -- migrate network stats db to resource stats db.

(In reply to Boris Zbarsky [:bz] from comment #22) > Comment on attachment 8463088 [details] [diff] [review] > part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary > > In fact, I'm 99% sure that this will introduce a security bug akin to bug > 1015540. So let's not do this, please. Not authorized for bug 1015540. But, yes, I'll remove this patch from the queue.

> Not authorized for bug 1015540 I cced you. If we need to allow "any" there, fine, but then we need to do it safely is all.

We're attempting to expose the app manifestURL in Bug 1042149 as part of the current NetworkStatsDB so that we can report data usage in Gaia. Should this change just be worked into the current set of patches?

(In reply to Marshall Culpepper [:marshall_law] from comment #26) > We're attempting to expose the app manifestURL in Bug 1042149 as part of the > current NetworkStatsDB so that we can report data usage in Gaia. Should this > change just be worked into the current set of patches? Feel free to land it first, I'll rebase my stuff after.

Comment on attachment 8463086 [details] [diff] [review] part 1/7: remove dead code Review of attachment 8463086 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your help to clean this up. Looks good.

Comment on attachment 8463089 [details] [diff] [review] part 3/7: rename nsINetworkStatsServiceProxy to nsINetworkStatsService Review of attachment 8463089 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thank you.

Comment on attachment 8463092 [details] [diff] [review] part 5.b/7: remove unnecessary function names Review of attachment 8463092 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8463093 [details] [diff] [review] part 5.c/7: use object literal instead. Review of attachment 8463093 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8463096 [details] [diff] [review] part 5.e/7: unify variable naming Review of attachment 8463096 [details] [diff] [review]: ----------------------------------------------------------------- I am not capable to review the db part, redirect review request to bevis.

Comment on attachment 8463101 [details] [diff] [review] part 7.a/7: remove redundant unique flag at creating object store index Review of attachment 8463101 [details] [diff] [review]: ----------------------------------------------------------------- Redirect the review request to Bevis.

Comment on attachment 8463103 [details] [diff] [review] part 7.b/7: stitch schema upgrade functions up. Review of attachment 8463103 [details] [diff] [review]: ----------------------------------------------------------------- Redirect the review request to Bevis.

Comment on attachment 8463105 [details] [diff] [review] part 7.d/7: assign |transaction.result| properly. Review of attachment 8463105 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

To be rebased onto bug 1044737.

Comment on attachment 8463096 [details] [diff] [review] part 5.e/7: unify variable naming Review of attachment 8463096 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks!

Comment on attachment 8463101 [details] [diff] [review] part 7.a/7: remove redundant unique flag at creating object store index Review of attachment 8463101 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks!

Comment on attachment 8463103 [details] [diff] [review] part 7.b/7: stitch schema upgrade functions up. Review of attachment 8463103 [details] [diff] [review]: ----------------------------------------------------------------- Nice patch! Thank you!

Comment on attachment 8463104 [details] [diff] [review] part 7.c/7: fix alignment Review of attachment 8463104 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. :)

Comment on attachment 8463088 [details] [diff] [review] part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary Removed per comment 24.

Update commit summary only.

Update commit summary and rebase after bug 936367.

Update commit summary & accommodate to withdrawal of attachment 8463088 [details] [diff] [review].

Update commit summary and rebase after bug 1044737.

Update commit summary and rebase after bug 1044737.

Update commit summary only.

Update commit summary only.

Update commit summary and rename a few more.

Update commit summary and a few line-wrap style changes.

Update commit summary and a line-wrap style change.

Update commit summary and a few line-wrap style changes.

Update commit summary and a few line-wrap style changes.

Update commit summary only.

Update commit summary only.

Update commit summary only.

Update commit summary only.

Try all: https://tbpl.mozilla.org/?tree=Try&rev=e173eef5c7e1

Firefox OS is not being worked on