Closed Bug 1350436 Opened 8 years ago Closed 8 years ago

Remove Dispatcher and renaming ValidatingDispatcher to SchedulerGroup

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(4 files)

This is a refactoring I've been considering since bug 1337537. I would like to eliminate the Dispatcher class. It serves as a base class to DocGroup and ValidatingDispatcher. However, DocGroup doesn't really use its functionality--it just delegates to its TabGroup. So ValidatingDispatcher is really only one subclass. I might as well fold Dispatcher into ValidatingDispatcher. Then I can rename that class to SchedulerGroup, which is a better name.
This patch makes DocGroup no longer inherit from Dispatcher.
Attachment #8851102 - Flags: review?(nfroyd)
Attached patch Get rid of Dispatcher (deleted) — Splinter Review
This patch gets rid of Dispatcher so that we only have ValidatingDispatcher.
Attachment #8851103 - Flags: review?(nfroyd)
This patch does the renaming to SchedulerGroup.
Attachment #8851104 - Flags: review?(nfroyd)
Comment on attachment 8851102 [details] [diff] [review] DocGroup no longer inherits from Dispatcher Review of attachment 8851102 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/DocGroup.h @@ +75,5 @@ > > + nsIEventTarget* EventTargetFor(TaskCategory aCategory) const; > + > + AbstractThread* > + AbstractMainThreadFor(TaskCategory aCategory); I'm unsure if we're concerned that we now/will have a set of functions in DocGroup and a set of functions in Dispatcher/SystemGroup that share names and functionality/contracts, but there's no way of ensuring that the two implementations stay "in sync". I guess if TabGroup doesn't implement the Dispatcher interface, then this is kind of a moot point, because we're already in that sort of situation today.
Attachment #8851102 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #4) > I guess if TabGroup doesn't implement the > Dispatcher interface, then this is kind of a moot point, because we're > already in that sort of situation today. Oh, I guess TabGroup inherits from ValidatingDispatcher, so TabGroup will at least have a consistent interface by virtue of that.
Attachment #8851103 - Flags: review?(nfroyd) → review+
Attachment #8851104 - Flags: review?(nfroyd) → review+
Attached patch networking changes (deleted) — Splinter Review
While rebasing this patch over bug 1343750, I had to make significant changes. I think it deserves another review. I'm trying to change the code so that Dispatcher is no longer a shared base class for DocGroup and TabGroup. Instead, I would like to use nsIEventTarget as a generic type to be used to dispatch to a DocGroup or TabGroup. This patch does that for the HTTP and FTP code. I had to remove an assertion in the HTTP code. It's hard to see how to keep it with the code refactored the way it is. The property being asserted isn't really necessary for the code to be correct. It's just a nice thing to have.
Attachment #8854216 - Flags: review?(honzab.moz)
Attachment #8854216 - Flags: review?(honzab.moz) → review+
Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7509b5c557ee Use nsIEventTarget instead of Dispatcher in netwerk code (r=mayhemer) https://hg.mozilla.org/integration/mozilla-inbound/rev/2e1cf2787080 DocGroup should not inherit from Dispatcher (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff47d6ae913 Collapse Dispatcher into ValidatingDispatcher (r=froydnj) https://hg.mozilla.org/integration/mozilla-inbound/rev/68dcc900c5bd Rename ValidatingDispatcher to SchedulerGroup (r=froydnj)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: