Closed
Bug 1368072
Opened 7 years ago
Closed 7 years ago
Expose IdleDispatch with timeout to JS
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: smaug, Assigned: farre)
References
Details
Attachments
(6 files, 3 obsolete files)
1.84 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
9.43 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
852 bytes,
text/x-patch
|
Details | |
1.11 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
1.00 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•7 years ago
|
Summary: Expose IdleDispatch with timout for JS → Expose IdleDispatch with timout to JS
Updated•7 years ago
|
Assignee: bugs → ehsan
Comment 1•7 years ago
|
||
Attachment #8873252 -
Flags: review?(nfroyd)
Updated•7 years ago
|
Summary: Expose IdleDispatch with timout to JS → Expose IdleDispatch with timeout to JS
Comment 2•7 years ago
|
||
Comment on attachment 8873252 [details] [diff] [review] Expose idle dispatch with timeout to JS Review of attachment 8873252 [details] [diff] [review]: ----------------------------------------------------------------- As part of the Quantum DOM work, we've been trying to move away from exposing more scriptable methods on nsIThread; script shouldn't really have to think about threads, if at all possible. I think I'd prefer to add nsIThreadManager::idleDispatchToMainThread instead. Thoughts on that approach in general welcome.
Attachment #8873252 -
Flags: review?(nfroyd) → review-
Comment 3•7 years ago
|
||
I don't understand. What makes this the special snowflake? Note that we already expose idleDispatch on nsIThread. Are you saying that's not OK either? FWIW I only did it this way because of how bug 1353206 was done, but as you can see on the C++ side our APIs aren't bound to nsIThread.
Flags: needinfo?(nfroyd)
Comment 4•7 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #3) > I don't understand. What makes this the special snowflake? Note that we > already expose idleDispatch on nsIThread. Are you saying that's not OK > either? The special snowflakeness vs. nsIThread::idleDispatch comes about because I was thinking about not adding new scriptable APIs to nsIThread when I reviewed this patch, but unfortunately not when I reviewed the other patch. Vanilla idle dispatch should go on nsIThreadManager as well.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #4) > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from > comment #3) > > I don't understand. What makes this the special snowflake? Note that we > > already expose idleDispatch on nsIThread. Are you saying that's not OK > > either? > > The special snowflakeness vs. nsIThread::idleDispatch comes about because I > was thinking about not adding new scriptable APIs to nsIThread when I > reviewed this patch, but unfortunately not when I reviewed the other patch. > Vanilla idle dispatch should go on nsIThreadManager as well. Created Bug 1371629 to track this. Let's move it before it gains too much use.
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Andreas Farre [:farre] from comment #5) > > Created Bug 1371629 to track this. Let's move it before it gains too much > use. Or I'll just re-use this bug. I have some patches, do you mind if I take this from you Ehsan?
Flags: needinfo?(ehsan)
Assignee | ||
Comment 7•7 years ago
|
||
Assignee: ehsan → afarre
Attachment #8877651 -
Flags: review?(nfroyd)
Comment 8•7 years ago
|
||
Comment on attachment 8877651 [details] [diff] [review] 0001-Bug-1368072-Move-idle-dispatch-to-thread-manager.-r-.patch Review of attachment 8877651 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/nsIThreadManager.idl @@ +93,5 @@ > + > + /** > + * This queues a runnable to the main thread's idle queue. It's a > + * shortcut for JS callers to be used instead of > + * .mainThread.idleDispatch(runnable); You are making nsIThread.idleDispatch [noscript], so this is not a shortcut. @@ +98,5 @@ > + * C++ callers should instead use NS_IdleDispatchToThread or > + * NS_IdleDispatchToCurrentThread > + */ > + void idleDispatchToMainThread(in nsIRunnable event, > + [optional] in uint32_t timeout); Could you please document the 'timeout' parameter in the comment above this method? Any plan to make the features of nsIIdleRunnable available to JavaScript callers?
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8877654 -
Flags: review?(mconley)
Assignee | ||
Comment 10•7 years ago
|
||
Flags: needinfo?(ehsan)
Attachment #8877655 -
Flags: review?(ato)
Comment hidden (obsolete) |
Comment 12•7 years ago
|
||
Comment on attachment 8877654 [details] [diff] [review] 0002-Bug-1368072-Use-idleDispatchToMainThread-instead.-r-.patch Review of attachment 8877654 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8877654 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8877651 -
Attachment is obsolete: true
Attachment #8877651 -
Flags: review?(nfroyd)
Attachment #8877658 -
Flags: review?(nfroyd)
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #8) > Comment on attachment 8877651 [details] [diff] [review] > 0001-Bug-1368072-Move-idle-dispatch-to-thread-manager.-r-.patch > > Review of attachment 8877651 [details] [diff] [review]: > ----------------------------------------------------------------- Thanks for the review! > Any plan to make the features of nsIIdleRunnable available to JavaScript > callers? Ideally yes, at the moment, no. I'll create a bug about it.
Updated•7 years ago
|
Attachment #8877655 -
Flags: review?(ato) → review+
Updated•7 years ago
|
Attachment #8877658 -
Flags: review?(nfroyd) → review+
Comment 15•7 years ago
|
||
(In reply to Andreas Farre [:farre] from comment #6) > (In reply to Andreas Farre [:farre] from comment #5) > > > > Created Bug 1371629 to track this. Let's move it before it gains too much > > use. > > Or I'll just re-use this bug. I have some patches, do you mind if I take > this from you Ehsan? Not at all, and thank you!
Comment 17•7 years ago
|
||
Comment on attachment 8877658 [details] [diff] [review] 0001-Bug-1368072-Move-idle-dispatch-to-thread-manager.-r-.patch Review of attachment 8877658 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/nsIThreadManager.idl @@ +101,5 @@ > + * queue to the regular queue if it hasn't been executed by then. If not > + * passed or a zero value is specified, the event will never be moved to > + * the regular queue. > + */ > + void idleDispatchToMainThread(in nsIRunnable event, Random thought: if we don't want scripts to think about threads, should we remove "ToMainThread" from the name? eg. [binaryname(IdleDispatchToMainThread)] void idleDispatch(in nsIRunnable event, (The same change could apply to dispatchToMainThread, and if we do it I'm willing to provide a scripted patch for the mass rename.)
Assignee | ||
Updated•7 years ago
|
Attachment #8873252 -
Attachment is obsolete: true
Assignee | ||
Comment 18•7 years ago
|
||
For some reason extern nsresult NS_IdleDispatchToThread(already_AddRefed<nsIRunnable>&& aEvent, nsIThread* aThread); Dropped out of the patch. I'll add an interdiff to show it.
Attachment #8877658 -
Attachment is obsolete: true
Attachment #8879133 -
Flags: review?(nfroyd)
Assignee | ||
Comment 19•7 years ago
|
||
Updated•7 years ago
|
Attachment #8879133 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 20•7 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcd517825dc27aa08532c59b658513247f000925
Keywords: checkin-needed
Comment 21•7 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4782401394a9 Move idle dispatch to thread manager. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/1797afe16a2a Use idleDispatchToMainThread instead. r=mconley https://hg.mozilla.org/integration/mozilla-inbound/rev/357635c84e49 Use idleDispatchToMainThread instead. r=ato
Keywords: checkin-needed
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4782401394a9 https://hg.mozilla.org/mozilla-central/rev/1797afe16a2a https://hg.mozilla.org/mozilla-central/rev/357635c84e49
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 23•7 years ago
|
||
Backed out in https://hg.mozilla.org/mozilla-central/rev/e1e4a481b7e8 for an unhappy collision with something in https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=c55e582aee5f4dd7c28cd9820156ecd0335e4e79 Before that merge everything was fine, but afterward there were both understandably related failures like https://treeherder.mozilla.org/logviewer.html#?job_id=108707792&repo=mozilla-central and then if telemetry was broken that makes https://treeherder.mozilla.org/logviewer.html#?job_id=108707759&repo=mozilla-central understandable as well, and slightly more odd things like devtools only on Win8 hitting the same "TypeError: Services.tm.mainThread.idleDispatch is not a function", and very odd things like making test_fullscreen-api.html time out only on Linux32 and Linux64 opt (unless, as is entirely possible, those are the only two places it's still enabled).
Status: RESOLVED → REOPENED
status-firefox56:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
Comment 24•7 years ago
|
||
Ah, we separately broke and merged around the bustage of test_fullscreen-api.html, that's why it makes so little sense, it's something else.
Comment 25•7 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #23) > Backed out in https://hg.mozilla.org/mozilla-central/rev/e1e4a481b7e8 for an > unhappy collision with something in > https://hg.mozilla.org/mozilla-central/ > pushloghtml?changeset=c55e582aee5f4dd7c28cd9820156ecd0335e4e79 > > Before that merge everything was fine, but afterward there were both > understandably related failures like > https://treeherder.mozilla.org/logviewer.html#?job_id=108707792&repo=mozilla- > central and then if telemetry was broken that makes > https://treeherder.mozilla.org/logviewer.html#?job_id=108707759&repo=mozilla- > central understandable as well, and slightly more odd things like devtools > only on Win8 hitting the same "TypeError: > Services.tm.mainThread.idleDispatch is not a function", and very odd things > like making test_fullscreen-api.html time out only on Linux32 and Linux64 > opt (unless, as is entirely possible, those are the only two places it's > still enabled). Unlucky coincidence: in bug 1369734 I landed a patch that uses the |Services.tm.mainThread.idleDispatch| (https://hg.mozilla.org/mozilla-central/rev/f495235291e7#l1.48) and this bug renamed right after. We both pushed to try before landing our patches, but that happened at the very same time and we landed at the very same time :-D But 1374958 it's related fallout from this too.
Assignee | ||
Comment 26•7 years ago
|
||
Attachment #8879863 -
Flags: review?(alessio.placitelli)
Comment 27•7 years ago
|
||
Comment on attachment 8879863 [details] [diff] [review] 0004-Bug-1368072-Use-idleDispatchToMainThread-instead.-r-.patch Review of attachment 8879863 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me!
Attachment #8879863 -
Flags: review?(alessio.placitelli) → review+
Comment 28•7 years ago
|
||
Pushed by afarre@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1cdae5a8d6 Move idle dispatch to thread manager. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/c4a1ddf9be0d Use idleDispatchToMainThread instead. r=mconley https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7e008d7d70 Use idleDispatchToMainThread instead. r=ato
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ba1cdae5a8d6 https://hg.mozilla.org/mozilla-central/rev/c4a1ddf9be0d https://hg.mozilla.org/mozilla-central/rev/1d7e008d7d70
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 30•7 years ago
|
||
How are you ever going to get this landed? Backed out in https://hg.mozilla.org/mozilla-central/rev/edb7e1ddd9b6 for breaking the use of Services.tm.mainThread.idleDispatch in https://hg.mozilla.org/mozilla-central/rev/5dc0f2cd4bc7 which got to mozilla-central before you did.
Status: RESOLVED → REOPENED
status-firefox56:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
Updated•7 years ago
|
Flags: needinfo?(afarre)
Assignee | ||
Comment 31•7 years ago
|
||
Yeah, the bounces. I'll try one more time, if I can't land it this time I'll have to add back nsIThread.idleDispatch just to get nsThreadManager.idleDispatchToMainThread to land.
Flags: needinfo?(afarre)
Attachment #8880276 -
Flags: review?(ato)
Updated•7 years ago
|
Attachment #8880276 -
Flags: review?(ato) → review+
Comment 32•7 years ago
|
||
Pushed by afarre@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d22dce71a78 Move idle dispatch to thread manager. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/30f931980859 Use idleDispatchToMainThread instead. r=mconley https://hg.mozilla.org/integration/mozilla-inbound/rev/822c8c11b2b3 Use idleDispatchToMainThread instead. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/295e163e66f8 Use idleDispatchToMainThread instead. r=ato
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d22dce71a78 https://hg.mozilla.org/mozilla-central/rev/30f931980859 https://hg.mozilla.org/mozilla-central/rev/822c8c11b2b3 https://hg.mozilla.org/mozilla-central/rev/295e163e66f8
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•