Closed Bug 1368072 Opened 7 years ago Closed 7 years ago

Expose IdleDispatch with timeout to JS

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: smaug, Assigned: farre)

References

Details

Attachments

(6 files, 3 obsolete files)

      No description provided.
Summary: Expose IdleDispatch with timout for JS → Expose IdleDispatch with timout to JS
Blocks: 1365970
Assignee: bugs → ehsan
Attachment #8873252 - Flags: review?(nfroyd)
Summary: Expose IdleDispatch with timout to JS → Expose IdleDispatch with timeout to JS
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-
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)
(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)
(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.
(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: ehsan → afarre
Attachment #8877651 - Flags: review?(nfroyd)
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?
Flags: needinfo?(ehsan)
Attachment #8877655 - Flags: review?(ato)
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+
Attachment #8877651 - Attachment is obsolete: true
Attachment #8877651 - Flags: review?(nfroyd)
Attachment #8877658 - Flags: review?(nfroyd)
(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.
Attachment #8877655 - Flags: review?(ato) → review+
Attachment #8877658 - Flags: review?(nfroyd) → review+
(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 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.)
Attachment #8873252 - Attachment is obsolete: true
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)
Attachment #8879133 - Flags: review?(nfroyd) → review+
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
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
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
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.
See Also: → 1374958
(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.
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+
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
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
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
Flags: needinfo?(afarre)
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)
Attachment #8880276 - Flags: review?(ato) → review+
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: