Closed
Bug 1350436
Opened 8 years ago
Closed 8 years ago
Remove Dispatcher and renaming ValidatingDispatcher to SchedulerGroup
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
References
Details
Attachments
(4 files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
This patch makes DocGroup no longer inherit from Dispatcher.
Attachment #8851102 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•8 years ago
|
||
This patch gets rid of Dispatcher so that we only have ValidatingDispatcher.
Attachment #8851103 -
Flags: review?(nfroyd)
Assignee | ||
Comment 3•8 years ago
|
||
This patch does the renaming to SchedulerGroup.
Attachment #8851104 -
Flags: review?(nfroyd)
Comment 4•8 years ago
|
||
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+
Comment 5•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8851103 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8851104 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 6•8 years ago
|
||
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)
Updated•8 years ago
|
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)
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7509b5c557ee
https://hg.mozilla.org/mozilla-central/rev/2e1cf2787080
https://hg.mozilla.org/mozilla-central/rev/1ff47d6ae913
https://hg.mozilla.org/mozilla-central/rev/68dcc900c5bd
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•