1043830 - Refactor NetworkStats a bit

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

+++ 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.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/7: remove dead code

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

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary

WebIDL dictionary NetworkStatsAlarmOptions is obsoleted by ResourceStatsAlarmOptions, which avoids Date usage.

Gaia CostControl in master branch does not use this dictionary type.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/7: rename nsINetworkStatsServiceProxy to nsINetworkStatsService

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.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/7: Move nsIDOMNetworkStatsManager to WebIDL

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

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.a/7: rename private member functions

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.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.b/7: remove unnecessary function names

Use:

  memberFunc: function() { ... }

Instead of:

  memberFunc: function memberFunc() { ... }

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.c/7: use object literal instead.

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

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.d/7: remove unused arguments

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.e/7: unify variable naming

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

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.a/7: clean up NetworkStats:Get IPC messages

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.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.b/7: clean up 'NetworkStats:{Clear,ClearAll}' IPC messages

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.c/7: clean up 'NetworkStats:{GetAvailableNetworks,GetAvailableServiceTypes}' IPC messages

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.d/7: clean up 'NetworkStats:{SetAlarm,GetAlarms,RemoveAlarms}' IPC messages

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 7.a/7: remove redundant unique flag at creating object store index

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

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 7.b/7: stitch schema upgrade functions up.

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.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 7.c/7: fix alignment

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

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 7.d/7: assign |transaction.result| properly.

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

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

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 19

[Vicamo Yang [:vicamo][:vyang]]

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 20

[Boris Zbarsky [:bzbarsky]]

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 21

[Boris Zbarsky [:bzbarsky]]

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 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

(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.

Comment 24

[Vicamo Yang [:vicamo][: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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 26

[Marshall Culpepper [:marshall_law]]

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?

Comment 27

[Vicamo Yang [:vicamo][: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.

Comment 28

[Vincent Chang[:vchang][changyihsin]]

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 29

[Vincent Chang[:vchang][changyihsin]]

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 30

[Vincent Chang[:vchang][changyihsin]]

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 31

[Vincent Chang[:vchang][changyihsin]]

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 32

[Vincent Chang[:vchang][changyihsin]]

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 33

[Vincent Chang[:vchang][changyihsin]]

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 34

[Vincent Chang[:vchang][changyihsin]]

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 35

[Vincent Chang[:vchang][changyihsin]]

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

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

Looks good.

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

To be rebased onto bug 1044737.

Comment 37

[Bevis Tseng[:bevis][:btseng](Exited)]

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 38

[Bevis Tseng[:bevis][:btseng](Exited)]

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 39

[Bevis Tseng[:bevis][:btseng](Exited)]

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 40

[Bevis Tseng[:bevis][:btseng](Exited)]

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 41

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8463088 [details] [diff] [review]
part 2/7: remove duplicated NetworkStatsAlarmOptions dictionary

Removed per comment 24.

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/6: remove dead code : v2

Update commit summary only.

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/6: rename nsINetworkStatsServiceProxy to nsINetworkStatsService : v2

Update commit summary and rebase after bug 936367.

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3/6: Move nsIDOMNetworkStatsManager to WebIDL : v2

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

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.a/6: rename private member functions : v2

Update commit summary and rebase after bug 1044737.

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.b/6: remove unnecessary function names : v2

Update commit summary and rebase after bug 1044737.

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.c/6: use object literal instead : v2

Update commit summary only.

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.d/6: remove unused arguments : v2

Update commit summary only.

Comment 49

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4.e/6: unify variable naming : v2

Update commit summary and rename a few more.

Comment 50

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.a/6: clean up NetworkStats:Get IPC messages : v2

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

Comment 51

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.b/6: clean up 'NetworkStats:{Clear,ClearAll}' IPC messages : v2

Update commit summary and a line-wrap style change.

Comment 52

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.c/6: clean up 'NetworkStats:{GetAvailableNetworks,GetAvailableServiceTypes}' IPC messages : v2

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

Comment 53

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 5.d/6: clean up 'NetworkStats:{SetAlarm,GetAlarms,RemoveAlarms}' IPC messages : v2

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

Comment 54

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.a/6: remove redundant unique flag at creating object store index : v2

Update commit summary only.

Comment 55

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.b/6: stitch schema upgrade functions up : v2

Update commit summary only.

Comment 56

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.c/6: fix alignment : v2

Update commit summary only.

Comment 57

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 6.d/6: assign |transaction.result| properly : v2

Update commit summary only.

Comment 58

[Vicamo Yang [:vicamo][:vyang]]

Attachment: squashed patch

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

Comment 59

[BMO Automation]

Firefox OS is not being worked on