Closed
Bug 1472718
Opened 6 years ago
Closed 6 years ago
Convert ChromeUtils.requestIOActivity to a Promise
Categories
(Core :: Networking, enhancement, P5)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: tarek, Assigned: tarek)
References
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file)
Like in Bug 1471517 - let's convert this API to a promise. This will be much more efficient to collect IOActivity than having notifications sent even if they are not used. I suggest we also get rid of the timer+notification altogether. If needed, this is something that can be done on the JS side by using the ChromeUtil promise.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → tarek
Priority: -- → P2
Updated•6 years ago
|
Priority: P2 → P5
Whiteboard: [necko-triaged]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8989421 [details] Bug 1472718 - Convert ChromeUtils.requestIOActivity to a Promise - https://reviewboard.mozilla.org/r/254496/#review261300 ::: dom/chrome-webidl/ChromeUtils.webidl:361 (Diff revision 2) > + Promise<sequence<IOActivityDataDictionnary>> requestIOActivity(); > +}; > + > + > +/** > + * Used by requestIOActivity() to return the number of bytes extra space here. ::: netwerk/base/IOActivityMonitor.h:24 (Diff revision 2) > #include "private/pprio.h" > #include <stdint.h> > > -namespace mozilla { namespace net { > +namespace mozilla { > > -#define IO_ACTIVITY_ENABLED_PREF "io.activity.enabled" > +namespace dom { class Promise; } same comment as the other patch. ::: netwerk/base/IOActivityMonitor.cpp:296 (Diff revision 2) > -} > - > // static > -nsresult > -IOActivityMonitor::NotifyActivities() > +void > +IOActivityMonitor::RequestActivities(dom::Promise* aPromise) > { MOZ_ASSERT(aPromise) ::: netwerk/base/IOActivityMonitor.cpp:306 (Diff revision 2) > } > - return mon->NotifyActivities_Internal(); > + mon->RequestActivitiesInternal(aPromise); > } > > -nsresult > -IOActivityMonitor::NotifyActivities_Internal() > +void > +IOActivityMonitor::RequestActivitiesInternal(dom::Promise* aPromise) I would merge these 2 methods. but up to you ::: netwerk/base/IOActivityMonitor.cpp:323 (Diff revision 2) > + return; > - } > + } > - } > + } > - return NS_OK; > -} > + } > + aPromise->MaybeResolve(activities); I don't like that you resolve/reject promise inside the mutex. What about if you do: nsresult result = NS_OK; FallibleTArray<dom::IOActivityDataDictionnary> activities; { mozilla::MutexAutoLock lock(mLock); // Remove inactive activities for (... if (NS_WARN_IF(!activities.Append... result = NS_ERROR_OUT_OF_MEMORY; break; .. } if (NS_WARN_IF(NS_FAILED(result))) { aPromise->MaybeReject(rv); return; } aPromise->MaybeResolve(activities);
Attachment #8989421 -
Flags: review?(amarchesini) → review+
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8989421 [details] Bug 1472718 - Convert ChromeUtils.requestIOActivity to a Promise - https://reviewboard.mozilla.org/r/254496/#review261294 Looks good. ::: dom/chrome-webidl/ChromeUtils.webidl:353 (Diff revision 1) > * Request performance metrics to the current process & all ontent processes. > */ > void requestPerformanceMetrics(); > > /** > - * Request IOActivityMonitor to send a notification containing I/O activity > + * Returns a Promise containing a sequence I/O activity nit: sequence of I/O activities. ::: dom/chrome-webidl/ChromeUtils.webidl:361 (Diff revision 1) > + Promise<sequence<IOActivityDataDictionnary>> requestIOActivity(); > +}; > + > + > +/** > + * Used by requestIOActivity() to return the number of bytes nit: trailing WS
Attachment #8989421 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d0e3efb7054f987571b855b02a7ada158cd258
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8989421 [details] Bug 1472718 - Convert ChromeUtils.requestIOActivity to a Promise - https://reviewboard.mozilla.org/r/254494/#review261820
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b4927795c0b6638f312ce2013df5d2dd56cf66a
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f0381da392163df5582793fc9f89720034543f2
Assignee | ||
Comment 14•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ddb09e70c46ff7e4e32d9aab482f3963f0cdf70
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=773ffe8ea671abb5fa19a2018917af41bb541ba9
Comment hidden (mozreview-request) |
Comment 17•6 years ago
|
||
Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c22aa9571139 Convert ChromeUtils.requestIOActivity to a Promise - r=baku,valentin
Assignee | ||
Comment 18•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4378ceba330d1d26755429600ddbb21dbc264077
Comment 19•6 years ago
|
||
Backed out changeset c22aa9571139 (bug 1472718) for failures in browser/browser_test_io_activity.js on a CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c22aa9571139e0437a524f36a070ecdd8b1e6b0f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=186780682 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186780682&repo=autoland&lineNumber=4646 Backout: https://hg.mozilla.org/integration/autoland/rev/33a3fa354ee9451e5204c5a8ae9ca4d233b8c764
Flags: needinfo?(tarek)
Assignee | ||
Comment 20•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2f28f2274f9e1547394d8b8ca67eada1ec95a42
Assignee | ||
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be494b81a176b3c240c05999fca1a3eec72b625b
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a8263446f439 Convert ChromeUtils.requestIOActivity to a Promise - r=baku,valentin
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a8263446f439
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(tarek)
You need to log in
before you can comment on or make changes to this bug.
Description
•