Refactor NetworkStats a bit

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
4 years ago
7 months ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments, 17 obsolete attachments)

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
(Assignee)

Description

4 years ago
+++ 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.
(Assignee)

Comment 1

4 years ago
Created attachment 8463086 [details] [diff] [review]
part 1/7: remove dead code

Several functions are never referenced in production code. Remove them first.
Assignee: nobody → vyang
Attachment #8463086 - Flags: review?(vchang)
(Assignee)

Comment 2

4 years ago
Created attachment 8463088 [details] [diff] [review]
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.
Attachment #8463088 - Flags: review?(ehsan)
(Assignee)

Comment 3

4 years ago
Created attachment 8463089 [details] [diff] [review]
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.
Attachment #8463089 - Flags: review?(vchang)
(Assignee)

Comment 4

4 years ago
Created attachment 8463090 [details] [diff] [review]
part 4/7: Move nsIDOMNetworkStatsManager to WebIDL

Permission/object type checking lines are removed because related annotations has been added to the WebIDL interface.
Attachment #8463090 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

4 years ago
Created attachment 8463091 [details] [diff] [review]
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.
Attachment #8463091 - Flags: review?(vchang)
(Assignee)

Comment 6

4 years ago
Created attachment 8463092 [details] [diff] [review]
part 5.b/7: remove unnecessary function names

Use:

  memberFunc: function() { ... }

Instead of:

  memberFunc: function memberFunc() { ... }
Attachment #8463092 - Flags: review?(vchang)
(Assignee)

Comment 7

4 years ago
Created attachment 8463093 [details] [diff] [review]
part 5.c/7: use object literal instead.

Prefer |{}| over |Object.create(null)|.
Attachment #8463093 - Flags: review?(vchang)
(Assignee)

Comment 8

4 years ago
Created attachment 8463094 [details] [diff] [review]
part 5.d/7: remove unused arguments
Attachment #8463094 - Flags: review?(vchang)
(Assignee)

Comment 9

4 years ago
Created attachment 8463096 [details] [diff] [review]
part 5.e/7: unify variable naming

Rename a lot arguments, variables to reflect the types/meanings in a consistent way.
Attachment #8463096 - Flags: review?(vchang)
(Assignee)

Comment 10

4 years ago
Created attachment 8463097 [details] [diff] [review]
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.
Attachment #8463097 - Flags: review?(vchang)
(Assignee)

Comment 11

4 years ago
Created attachment 8463098 [details] [diff] [review]
part 6.b/7: clean up 'NetworkStats:{Clear,ClearAll}' IPC messages
Attachment #8463098 - Flags: review?(vchang)
(Assignee)

Updated

4 years ago
Attachment #8463094 - Attachment description: 5.d/7: remove unused arguments → part 5.d/7: remove unused arguments
(Assignee)

Updated

4 years ago
Attachment #8463096 - Attachment description: 5.e/7: unify variable naming → part 5.e/7: unify variable naming
(Assignee)

Comment 12

4 years ago
Created attachment 8463099 [details] [diff] [review]
part 6.c/7: clean up 'NetworkStats:{GetAvailableNetworks,GetAvailableServiceTypes}' IPC messages
Attachment #8463099 - Flags: review?(vchang)
(Assignee)

Comment 13

4 years ago
Created attachment 8463100 [details] [diff] [review]
part 6.d/7: clean up 'NetworkStats:{SetAlarm,GetAlarms,RemoveAlarms}' IPC messages
Attachment #8463100 - Flags: review?(vchang)
(Assignee)

Comment 14

4 years ago
Created attachment 8463101 [details] [diff] [review]
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 .
Attachment #8463101 - Flags: review?(vchang)
(Assignee)

Updated

4 years ago
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
(Assignee)

Comment 15

4 years ago
Created attachment 8463103 [details] [diff] [review]
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.
Attachment #8463103 - Flags: review?(vchang)
(Assignee)

Comment 16

4 years ago
Created attachment 8463104 [details] [diff] [review]
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.
Attachment #8463104 - Flags: review?(vchang)
(Assignee)

Comment 17

4 years ago
Created attachment 8463105 [details] [diff] [review]
part 7.d/7: assign |transaction.result| properly.

Assign |aTransaction.result| only when that transaction is going to complete.
Attachment #8463105 - Flags: review?(vchang)
(Assignee)

Comment 18

4 years ago
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..
(Assignee)

Comment 19

4 years ago
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)

Updated

4 years ago
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-
(Assignee)

Comment 23

4 years ago
(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)
(Assignee)

Comment 24

4 years ago
(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.
(Assignee)

Updated

4 years ago
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)
(Assignee)

Comment 27

4 years ago
(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+
(Assignee)

Comment 36

4 years ago
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+
(Assignee)

Comment 41

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

Removed per comment 24.
Attachment #8463088 - Attachment is obsolete: true
(Assignee)

Comment 42

4 years ago
Created attachment 8471371 [details] [diff] [review]
part 1/6: remove dead code : v2

Update commit summary only.
Attachment #8463086 - Attachment is obsolete: true
Attachment #8471371 - Flags: review+
(Assignee)

Comment 43

4 years ago
Created attachment 8471372 [details] [diff] [review]
part 2/6: rename nsINetworkStatsServiceProxy to nsINetworkStatsService : v2

Update commit summary and rebase after bug 936367.
Attachment #8463089 - Attachment is obsolete: true
Attachment #8471372 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8471371 - Attachment description: part 1/7: remove dead code : v2 → part 1/6: remove dead code : v2
(Assignee)

Comment 44

4 years ago
Created attachment 8471375 [details] [diff] [review]
part 3/6: Move nsIDOMNetworkStatsManager to WebIDL : v2

Update commit summary & accommodate to withdrawal of attachment 8463088 [details] [diff] [review].
Attachment #8463090 - Attachment is obsolete: true
Attachment #8471375 - Flags: review+
(Assignee)

Comment 45

4 years ago
Created attachment 8471378 [details] [diff] [review]
part 4.a/6: rename private member functions : v2

Update commit summary and rebase after bug 1044737.
Attachment #8463091 - Attachment is obsolete: true
Attachment #8471378 - Flags: review+
(Assignee)

Comment 46

4 years ago
Created attachment 8471382 [details] [diff] [review]
part 4.b/6: remove unnecessary function names : v2

Update commit summary and rebase after bug 1044737.
Attachment #8463092 - Attachment is obsolete: true
Attachment #8471382 - Flags: review+
(Assignee)

Comment 47

4 years ago
Created attachment 8471384 [details] [diff] [review]
part 4.c/6: use object literal instead : v2

Update commit summary only.
Attachment #8463093 - Attachment is obsolete: true
Attachment #8471384 - Flags: review+
(Assignee)

Comment 48

4 years ago
Created attachment 8471386 [details] [diff] [review]
part 4.d/6: remove unused arguments : v2

Update commit summary only.
Attachment #8463094 - Attachment is obsolete: true
Attachment #8471386 - Flags: review+
(Assignee)

Comment 49

4 years ago
Created attachment 8471389 [details] [diff] [review]
part 4.e/6: unify variable naming : v2

Update commit summary and rename a few more.
Attachment #8463096 - Attachment is obsolete: true
Attachment #8471389 - Flags: review+
(Assignee)

Comment 50

4 years ago
Created attachment 8471393 [details] [diff] [review]
part 5.a/6: clean up NetworkStats:Get IPC messages : v2

Update commit summary and a few line-wrap style changes.
Attachment #8463097 - Attachment is obsolete: true
Attachment #8471393 - Flags: review+
(Assignee)

Comment 51

4 years ago
Created attachment 8471395 [details] [diff] [review]
part 5.b/6: clean up 'NetworkStats:{Clear,ClearAll}' IPC messages : v2

Update commit summary and a line-wrap style change.
Attachment #8463098 - Attachment is obsolete: true
Attachment #8471395 - Flags: review+
(Assignee)

Comment 52

4 years ago
Created attachment 8471396 [details] [diff] [review]
part 5.c/6: clean up 'NetworkStats:{GetAvailableNetworks,GetAvailableServiceTypes}' IPC messages : v2

Update commit summary and a few line-wrap style changes.
Attachment #8463099 - Attachment is obsolete: true
Attachment #8471396 - Flags: review+
(Assignee)

Comment 53

4 years ago
Created attachment 8471398 [details] [diff] [review]
part 5.d/6: clean up 'NetworkStats:{SetAlarm,GetAlarms,RemoveAlarms}' IPC messages : v2

Update commit summary and a few line-wrap style changes.
Attachment #8463100 - Attachment is obsolete: true
Attachment #8471398 - Flags: review+
(Assignee)

Comment 54

4 years ago
Created attachment 8471399 [details] [diff] [review]
part 6.a/6: remove redundant unique flag at creating object store index : v2

Update commit summary only.
Attachment #8463101 - Attachment is obsolete: true
Attachment #8471399 - Flags: review+
(Assignee)

Comment 55

4 years ago
Created attachment 8471400 [details] [diff] [review]
part 6.b/6: stitch schema upgrade functions up : v2

Update commit summary only.
Attachment #8463103 - Attachment is obsolete: true
Attachment #8471400 - Flags: review+
(Assignee)

Comment 56

4 years ago
Created attachment 8471401 [details] [diff] [review]
part 6.c/6: fix alignment : v2

Update commit summary only.
Attachment #8463104 - Attachment is obsolete: true
Attachment #8471401 - Flags: review+
(Assignee)

Comment 57

4 years ago
Created attachment 8471404 [details] [diff] [review]
part 6.d/6: assign |transaction.result| properly : v2

Update commit summary only.
Attachment #8463105 - Attachment is obsolete: true
Attachment #8471404 - Flags: review+

Comment 59

7 months ago
Firefox OS is not being worked on
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.