Lots of runnables don't actually touch content JS. We want a way to label them as such. This bug will add a mechanism for that.

Nathan, are you okay reviewing this? It moves the base class for DocGroup/TabGroup to xpcom/threads. I think this is probably where it should have belonged anyway. I'm adding a new comment in the next patch that might help reviewing a bit. Also had to add a bunch of #includes because unified build stuff shifted.

This patch adds the concept of the SystemGroup. Hopefully the comment explains it.

This patch uses the SystemGroup when dispatching console messages. I don't think we run any content code in this phase (modulo add-on craziness that we'll deal with in XPConnect), so it seems like a safe and easy example of how to use this thing.

It looks from a cursory observation that the SystemGroup acts differently, and fills a different role than, the chrome TabGroup - but I would like a comment explaining the difference between them and when you want to use one vs the other.

(In reply to Michael Layzell [:mystor] from comment #5) > It looks from a cursory observation that the SystemGroup acts differently, > and fills a different role than, the chrome TabGroup - but I would like a > comment explaining the difference between them and when you want to use one > vs the other. Yeah, that makes sense. I think the chrome TabGroup is for runnables that touch chrome-privileged DOM content (especially DOM windows), while SystemGroup is for runnables that don't have anything to do with a specific window. The distinction doesn't really matter for scheduling purposes, but I think it makes sense to have the distinction in case we want to do something different down the road.

(In reply to Bill McCloskey (:billm) from comment #1) > Nathan, are you okay reviewing this? It moves the base class for > DocGroup/TabGroup to xpcom/threads. I think this is probably where it should > have belonged anyway. I'm adding a new comment in the next patch that might > help reviewing a bit. I think I'm OK reviewing this. For avoidance of doubt, we want to move this because we're going to be dispatching runnables using these things from non-DOM places, and we don't want to add a DOM dependency on them? Well, at least for SystemGroup anyway--I'm a little less clear on why Dispatcher itself would need to move. Can you elaborate on that?

I moved it because it seemed fairly generic and because SystemGroup needs to use TaskCategory. We could find other ways of solving that problem though.

With this new SystemGroup introduced, does that means all the use cases of NS_DispatchToMainThread/NS_DispatchToCurrentThread (and maybe AbstractThread::MainThread in the future) without being replaced by one of the Dispatcher::Xxx() has to be replaced with this one explicitly?

Or shall we keep the ones only running in parent process but replace the ones in child process.

Comment on attachment 8828579 [details] [diff] [review] use SystemGroup for LogMessageRunnable Review of attachment 8828579 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsConsoleService.cpp @@ +324,5 @@ > if (r) { > // avoid failing in XPCShell tests > nsCOMPtr<nsIThread> mainThread = do_GetMainThread(); > if (mainThread) { > + SystemGroup::Dispatch("LogMessageRunnable", TaskCategory::Other, r.forget()); I am going to take your word that content code can't execute here, though I am a little skeptical. How is XPConnect going to deal with add-on code executing as a result of this runnable?

Comment on attachment 8828576 [details] [diff] [review] move Dispatcher.{cpp,h} to xpcom/threads Review of attachment 8828576 [details] [diff] [review]: ----------------------------------------------------------------- Let me know if you like either of the two solutions below better, or if you think this patch is better than both of them. ::: xpcom/threads/Dispatcher.h @@ +18,5 @@ > +namespace dom { > +class TabGroup; > +} > + > +enum class TaskCategory { I think I would be a little happier if this class moved into its own header in XPCOM, and then it could be included from Dispatcher and SystemGroup headers. It's not quite perfect for XPCOM, but you might want the TaskCategory when doing scheduling of runnables inside nsThread's event loop? Alternatively, I think you could do the following: - Define this as |enum class TaskCategory : uint32_t { ... };| - TaskCategory can then be forward-declared in SystemGroup as |enum class TaskCategory : uint32_t;| - And you can use TaskCategory as a simple function parameter, since you don't care about its values.

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #9) > With this new SystemGroup introduced, does that means all the use cases of > NS_DispatchToMainThread/NS_DispatchToCurrentThread (and maybe > AbstractThread::MainThread in the future) without being replaced by one of > the Dispatcher::Xxx() has to be replaced with this one explicitly? Yes, that's correct. (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10) > Or shall we keep the ones only running in parent process but replace the > ones in child process. Yes, only in the child.

Comment on attachment 8830084 [details] [diff] [review] create mozilla/TaskCategory.h Review of attachment 8830084 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/98d571ab231a Move TaskCategory definition to xpcom/threads/TaskCategory.h (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/640c238dad72 Add comment to mozilla/dom/Dispatcher.h (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/3afec126bdae Add SystemGroup class similar to TabGroup/DocGroup (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/69dc7a75b782 Use SystemGroup for LogMessageRunnable (r=froydnj)

https://hg.mozilla.org/mozilla-central/rev/98d571ab231a https://hg.mozilla.org/mozilla-central/rev/640c238dad72 https://hg.mozilla.org/mozilla-central/rev/3afec126bdae https://hg.mozilla.org/mozilla-central/rev/69dc7a75b782