If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove Dispatcher and renaming ValidatingDispatcher to SchedulerGroup

RESOLVED FIXED in Firefox 55

Status

()

Core
XPCOM
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

6 months ago
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)

Updated

6 months ago
Blocks: 1350432
(Assignee)

Comment 1

6 months ago
Created attachment 8851102 [details] [diff] [review]
DocGroup no longer inherits from Dispatcher

This patch makes DocGroup no longer inherit from Dispatcher.
Attachment #8851102 - Flags: review?(nfroyd)
(Assignee)

Comment 2

6 months ago
Created attachment 8851103 [details] [diff] [review]
Get rid of Dispatcher

This patch gets rid of Dispatcher so that we only have ValidatingDispatcher.
Attachment #8851103 - Flags: review?(nfroyd)
(Assignee)

Comment 3

6 months ago
Created attachment 8851104 [details] [diff] [review]
Rename ValidatingDispatcher to SchedulerGroup

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.

Updated

6 months ago
Attachment #8851103 - Flags: review?(nfroyd) → review+

Updated

6 months ago
Attachment #8851104 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 6

6 months ago
Created attachment 8854216 [details] [diff] [review]
networking changes

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+

Comment 7

5 months ago
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

5 months 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
Last Resolved: 5 months 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.