Closed
Bug 993311
Opened 11 years ago
Closed 9 years ago
Convert Network Stats API to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: johnshih.bugs, Assigned: peterv)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
This bug is a follow-up of Bug 986837, which is to convent the remaining part of NetworkStats API to WebIDL.
Attachment #8403137 -
Attachment is obsolete: true
Attachment #8403138 -
Attachment is obsolete: true
Comment 4•11 years ago
|
||
Please don't land this unless bug 983502 has been fixed, this will break the MarketPlace feature detection without that bug.
Depends on: 983502
Attachment #8403885 -
Attachment is obsolete: true
Attachment #8404619 -
Flags: review?(bzbarsky)
fix typo
Attachment #8404619 -
Attachment is obsolete: true
Attachment #8404619 -
Flags: review?(bzbarsky)
Attachment #8405145 -
Flags: review?(bzbarsky)
Attachment #8405145 -
Attachment is obsolete: true
Attachment #8405145 -
Flags: review?(bzbarsky)
Attachment #8405216 -
Flags: review?(bzbarsky)
Comment 9•11 years ago
|
||
Comment on attachment 8405216 [details] [diff] [review]
Bug 993311 - Convert Network Stats API to WebIDL. r=bzbarsky
>+++ b/dom/network/src/NetworkStatsManager.js
>+ if (aAlarmId == 0 || typeof aAlarmId == 'undefined') {
> aAlarmId = -1;
If you want "not passed" to be treated as 1, just make -1 the default value in the IDL, please:
DOMRequest removeAlarms(optional long alarmId = -1);
>+++ b/dom/network/tests/test_networkstats_alarms.html
>+ ok(ex == msg, "addAlarm() throws \"" + msg + "\" when no parameters");
is(ex, msg, "addAlarm() throws \"" + msg + "\" when no parameters");
and similar in the other three places.
>+++ b/dom/network/tests/test_networkstats_enabled_no_perm.html
As Ehsan said, these changes break the feature detection story we have right this moment. It might be better to go ahead and expose the object even if the permission is not available, but have it end up null in that case.
>+++ b/dom/network/tests/test_networkstats_enabled_perm.html
>+ ok(typeof navigator.mozNetworkStats === 'object',
That would test true for null as well. Maybe better to test that it's instanceof the right thing?
>+++ b/dom/webidl/NetworkStatsManager.webidl
>+ DOMRequest getSamples(any network,
>+ any start,
>+ any end,
>+ optional any options /* NetworkStatsGetOptions */);
Are there really no more restrictive types than "any" here? That seems a bit weird to me.
It seems like it would make more sense if start/end were Date and network were MozNetworkStatsInterface. Then you can remove the corresponding checks from the implementation.
options should be a NetworkStatsGetOptions, for sure, like the comment says.
>+ DOMRequest addAlarm(any network,
>+ long threshold,
>+ optional any options /* NetworkStatsAlarmOptions */);
Similar here: use better types for network and options.
>+ DOMRequest getAllAlarms(optional any network);
And here.
r-, since I think the uses of "any" there are wrong...
Attachment #8405216 -
Flags: review?(bzbarsky) → review-
Reporter | ||
Comment 10•11 years ago
|
||
(In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #4)
> Please don't land this unless bug 983502 has been fixed, this will break the
> MarketPlace feature detection without that bug.
Hi Ehsan,
Can you elaborate more on the "break" you mentioned? Although bug 983502 is landed, I still not sure about the relation between that bug and this work. Thanks!
Comment 11•11 years ago
|
||
(In reply to John Shih from comment #10)
> (In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #4)
> > Please don't land this unless bug 983502 has been fixed, this will break the
> > MarketPlace feature detection without that bug.
>
> Hi Ehsan,
>
> Can you elaborate more on the "break" you mentioned? Although bug 983502 is
> landed, I still not sure about the relation between that bug and this work.
> Thanks!
Currently marketplace uses the fact that we expose some of these types of properties on navigator and set their values to null when you lack enough permissions to access it as an indication that the platform supports these feature if you had sufficient permissions. Moving these APIs to WebIDL will properly hide them from the non-privileged pages, hence breaking marketplace.
We're trying to address this issue using navigator.getFeature(). Bug 983502 happened to implement the parts of that API which are not useful for this use case, I filed bug 1009645 for implementing the interesting bits, and moved the dependencies to that bug.
Sorry for the confusion!
Flags: needinfo?(ehsan)
Comment 12•10 years ago
|
||
Ehsan, is this ready to go, or do we also need to block on a server-side Marketplace bug?
Flags: needinfo?(ehsan)
Comment 13•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #12)
> Ehsan, is this ready to go, or do we also need to block on a server-side
> Marketplace bug?
Wil? I'm not sure how MarketPlace is going to use this, but from the Gecko perspective, I think it can land now. But I'll let Wil confirm.
Flags: needinfo?(ehsan) → needinfo?(clouserw)
Comment 14•10 years ago
|
||
Marking as blocked on the Marketplace bug
Depends on: 983880
Flags: needinfo?(clouserw)
Updated•9 years ago
|
Keywords: dev-doc-needed
Comment 15•9 years ago
|
||
We can now convert this all to WebIDL, there is no market place dependency to worry about any more.
Assignee | ||
Comment 16•9 years ago
|
||
Let's drive this in.
Assignee: johnshih.bugs → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8405216 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8713683 [details] [diff] [review]
Convert MozNetworkStatsManager to WebIDL
Review of attachment 8713683 [details] [diff] [review]:
-----------------------------------------------------------------
I've named the interface MozNetworkStatsManager, as all the related interfaces kept the Moz prefix when they were converted to WebIDL.
Attachment #8713683 -
Flags: review?(bzbarsky)
Comment 19•9 years ago
|
||
Comment on attachment 8713683 [details] [diff] [review]
Convert MozNetworkStatsManager to WebIDL
>+++ b/dom/network/NetworkStatsManager.js
>+ let appManifestURL = aOptions.appManifestURL;
>+ let serviceType = aOptions.serviceType;
>+ let browsingTrafficOnly = aOptions.browsingTrafficOnly;
This is a subtle behavior change. These used to default to null, null, false. now they default to undefined.
Seems to me like you should keep the old defaults, either explicitly here or via adding default values to the dictionary.
r=me with that addressed.
Attachment #8713683 -
Flags: review?(bzbarsky) → review+
Comment 20•9 years ago
|
||
Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/3ddc1096f248 for Gregor to fix up b2g builds after the webidl conversion.
Comment 23•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/216059cd4bca
https://hg.mozilla.org/mozilla-central/rev/3ddc1096f248
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•