Closed Bug 1043830 Opened 5 years ago Closed 2 years ago

Refactor NetworkStats a bit

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

Attachments

(17 files, 17 obsolete files)

10.76 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
14.38 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
15.35 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
70.26 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
53.31 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
7.90 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
9.99 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
96.46 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
12.16 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
4.93 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
4.28 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
11.05 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
7.09 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
16.33 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
25.47 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
4.28 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
178.05 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
+++ 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.
Attached patch part 1/7: remove dead code (obsolete) — Splinter Review
Several functions are never referenced in production code. Remove them first.
Assignee: nobody → vyang
Attachment #8463086 - Flags: review?(vchang)
WebIDL dictionary NetworkStatsAlarmOptions is obsoleted by ResourceStatsAlarmOptions, which avoids Date usage.

Gaia CostControl in master branch does not use this dictionary type.
Attachment #8463088 - Flags: review?(ehsan)
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.
Attachment #8463089 - Flags: review?(vchang)
Permission/object type checking lines are removed because related annotations has been added to the WebIDL interface.
Attachment #8463090 - Flags: review?(bzbarsky)
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.
Attachment #8463091 - Flags: review?(vchang)
Use:

  memberFunc: function() { ... }

Instead of:

  memberFunc: function memberFunc() { ... }
Attachment #8463092 - Flags: review?(vchang)
Prefer |{}| over |Object.create(null)|.
Attachment #8463093 - Flags: review?(vchang)
Attachment #8463094 - Flags: review?(vchang)
Rename a lot arguments, variables to reflect the types/meanings in a consistent way.
Attachment #8463096 - Flags: review?(vchang)
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.
Attachment #8463097 - Flags: review?(vchang)
Attachment #8463094 - Attachment description: 5.d/7: remove unused arguments → part 5.d/7: remove unused arguments
Attachment #8463096 - Attachment description: 5.e/7: unify variable naming → part 5.e/7: unify variable naming
'unique' field of the IDBIndexParameters used in ObjectStore::createIndex() has already default value 'false'. See http://www.w3.org/TR/IndexedDB/#idl-def-IDBIndexParameters .
Attachment #8463101 - Flags: review?(vchang)
Attachment #8463101 - Attachment description: 7.a/7: remove redundant unique flag at creating object store index → part 7.a/7: remove redundant unique flag at creating object store index
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.
Attachment #8463103 - Flags: review?(vchang)
Attached patch part 7.c/7: fix alignment (obsolete) — Splinter Review
Additional white space characters are left in previous patch in order to ease the review process. This patch removes them.
Attachment #8463104 - Flags: review?(vchang)
Assign |aTransaction.result| only when that transaction is going to complete.
Attachment #8463105 - Flags: review?(vchang)
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.
Attachment #8463088 - Flags: review?(bzbarsky)
Attachment #8463088 - Flags: review?(ehsan) → review+
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?
Flags: needinfo?(vyang)
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
Attachment #8463090 - Flags: review?(bzbarsky) → review+
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.
Attachment #8463088 - Flags: review?(bzbarsky) → review-
(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.
Flags: needinfo?(vyang)
(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.
Depends on: 1045460
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?
Flags: needinfo?(vyang)
(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.
Flags: needinfo?(vyang)
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.
Attachment #8463086 - Flags: review?(vchang) → review+
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.
Attachment #8463089 - Flags: review?(vchang) → review+
Attachment #8463091 - Flags: review?(vchang) → review+
Comment on attachment 8463092 [details] [diff] [review]
part 5.b/7: remove unnecessary function names

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

Looks good.
Attachment #8463092 - Flags: review?(vchang) → review+
Comment on attachment 8463093 [details] [diff] [review]
part 5.c/7: use object literal instead.

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

Looks good.
Attachment #8463093 - Flags: review?(vchang) → review+
Attachment #8463094 - Flags: review?(vchang) → review+
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.
Attachment #8463096 - Flags: review?(vchang) → review?(btseng)
Attachment #8463097 - Flags: review?(vchang) → review+
Attachment #8463098 - Flags: review?(vchang) → review+
Attachment #8463099 - Flags: review?(vchang) → review+
Attachment #8463100 - Flags: review?(vchang) → review+
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.
Attachment #8463101 - Flags: review?(vchang) → review?(btseng)
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.
Attachment #8463103 - Flags: review?(vchang) → review?(btseng)
Attachment #8463104 - Flags: review?(vchang) → review?(btseng)
Comment on attachment 8463105 [details] [diff] [review]
part 7.d/7: assign |transaction.result| properly.

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

Looks good.
Attachment #8463105 - Flags: review?(vchang) → review+
To be rebased onto bug 1044737.
Depends on: 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!
Attachment #8463096 - Flags: review?(btseng) → review+
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!
Attachment #8463101 - Flags: review?(btseng) → review+
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!
Attachment #8463103 - Flags: review?(btseng) → review+
Comment on attachment 8463104 [details] [diff] [review]
part 7.c/7: fix alignment

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

Looks good to me. :)
Attachment #8463104 - Flags: review?(btseng) → review+
Comment on attachment 8463088 [details] [diff] [review]
part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary

Removed per comment 24.
Attachment #8463088 - Attachment is obsolete: true
Update commit summary only.
Attachment #8463086 - Attachment is obsolete: true
Attachment #8471371 - Flags: review+
Update commit summary and rebase after bug 936367.
Attachment #8463089 - Attachment is obsolete: true
Attachment #8471372 - Flags: review+
Attachment #8471371 - Attachment description: part 1/7: remove dead code : v2 → part 1/6: remove dead code : v2
Update commit summary & accommodate to withdrawal of attachment 8463088 [details] [diff] [review].
Attachment #8463090 - Attachment is obsolete: true
Attachment #8471375 - Flags: review+
Update commit summary and rebase after bug 1044737.
Attachment #8463091 - Attachment is obsolete: true
Attachment #8471378 - Flags: review+
Update commit summary and rebase after bug 1044737.
Attachment #8463092 - Attachment is obsolete: true
Attachment #8471382 - Flags: review+
Update commit summary only.
Attachment #8463093 - Attachment is obsolete: true
Attachment #8471384 - Flags: review+
Update commit summary only.
Attachment #8463094 - Attachment is obsolete: true
Attachment #8471386 - Flags: review+
Update commit summary and rename a few more.
Attachment #8463096 - Attachment is obsolete: true
Attachment #8471389 - Flags: review+
Update commit summary and a few line-wrap style changes.
Attachment #8463097 - Attachment is obsolete: true
Attachment #8471393 - Flags: review+
Update commit summary and a line-wrap style change.
Attachment #8463098 - Attachment is obsolete: true
Attachment #8471395 - Flags: review+
Update commit summary and a few line-wrap style changes.
Attachment #8463099 - Attachment is obsolete: true
Attachment #8471396 - Flags: review+
Update commit summary and a few line-wrap style changes.
Attachment #8463100 - Attachment is obsolete: true
Attachment #8471398 - Flags: review+
Update commit summary only.
Attachment #8463101 - Attachment is obsolete: true
Attachment #8471399 - Flags: review+
Update commit summary only.
Attachment #8463103 - Attachment is obsolete: true
Attachment #8471400 - Flags: review+
Update commit summary only.
Attachment #8463104 - Attachment is obsolete: true
Attachment #8471401 - Flags: review+
Update commit summary only.
Attachment #8463105 - Attachment is obsolete: true
Attachment #8471404 - Flags: review+
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.