Generalize IPDL queueing for e10s protocols

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

unspecified
x86
Windows Server 2003
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

8 years ago
The queueing is pretty easily separated into an EventQueue class that protocols can inherit from.
(Assignee)

Comment 1

8 years ago
Created attachment 476866 [details] [diff] [review]
Create EventQueue class to allow multiple necko protocols to reuse IPDL queueing code.
(Assignee)

Comment 2

8 years ago
Created attachment 476867 [details] [diff] [review]
Update HTTPChannelChild to use new general event queueing infrastructure.
(Assignee)

Updated

8 years ago
Attachment #476866 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

8 years ago
Attachment #476867 - Flags: review?(jduell.mcbugs)
Comment on attachment 476866 [details] [diff] [review]
Create EventQueue class to allow multiple necko protocols to reuse IPDL queueing code.

>+class EventQueue
>+{
>+ public:
>+  EventQueue() : mQueuePhase(PHASE_UNQUEUED) {}
>+  virtual ~EventQueue();

Do you need an empty function body for the destructor, too?  Or = 0?  (In the absence of one of those, aren't you required to provide a definition somewhere?) We don't really need the destructor to be virtual, do we?

>+ protected:

>+  // Workaround for Necko re-entrancy dangers. We buffer IPDL messages in a
>+  // queue if still dispatching previous one(s) to listeners/observers.
>+  // Otherwise synchronous XMLHttpRequests and/or other code that spins the
>+  // event loop (ex: IPDL rpc) could cause listener->OnDataAvailable (for
>+  // instance) to be called before mListener->OnStartRequest has completed.

Let's move this comment up to just above the EventQueue class definition, so it describes the whole class.

>+  virtual void BeginEventQueueing();
>+  virtual void EndEventQueueing();
>+  virtual void EnqueueEvent(ChannelEvent* callback);
>+  virtual bool ShouldEnqueue();

You're declaring these as inline, which I don't *think* plays so well with "virtual" (unless compiler optimizers are reliably smart: they often aren't).  If we don't actually need them to be virtual (and it doesn't look like it AFAICT), let's not use it.

Actually, nothing in this class really needs to be virtual, does it?  We're never in a place where we have a pointer to the base type and don't know the subclass.  If you want to be C++ geeky, you should be able to write this using a template.  Would only be a very minor perf win (non-virtual call to FlushQ), so don't bother, unless you're into that sort of thing...

Otherwise looks good.
Attachment #476866 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 476867 [details] [diff] [review]
Update HTTPChannelChild to use new general event queueing infrastructure.

>@@ -74,6 +75,7 @@
>                        , public nsIApplicationCacheChannel
>                        , public nsIAsyncVerifyRedirectCallback
>                        , public nsIAssociatedContentSecurity
>+                       , public EventQueue

Hmm, mildly allergic reaction all of a sudden to naming it plain old
"EventQueue", instead of "ChannelEventQueue" or something.  I guess that's why
we have namespaces (though it can make navigating mxr trickier if someone winds
up reusing the name later).  I'll let you decide.

> inline bool
> HttpChannelChild::ShouldEnqueue()
> {
>-  return mQueuePhase != PHASE_UNQUEUED || mSuspendCount;
>+  return EventQueue::ShouldEnqueue() || mSuspendCount;

Note that this doesn't require ShouldEnqueue to be virtual.
Attachment #476867 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Updated

8 years ago
Attachment #476866 - Attachment is obsolete: true
(Assignee)

Comment 5

8 years ago
Created attachment 478260 [details] [diff] [review]
Create EventQueue class to allow multiple necko protocols to reuse IPDL queueing code.
(Assignee)

Updated

8 years ago
Attachment #476867 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
Created attachment 478261 [details] [diff] [review]
Update HTTPChannelChild to use new general event queueing infrastructure.
(Assignee)

Comment 7

8 years ago
All comments addressed.
OS: Linux → Windows Server 2003
(Assignee)

Updated

8 years ago
Blocks: 591708
(Assignee)

Updated

8 years ago
Attachment #478261 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 481286 [details] [diff] [review]
Update HTTPChannelChild to use new general event queueing infrastructure.

Bitrot.
This should be marked a fennec blocker at same priority as FTP (bug 536289) since it blocks it.

I vote we change this and FTP to blocking-b2
tracking-fennec: --- → ?

Updated

8 years ago
tracking-fennec: ? → 2.0b2+

Updated

8 years ago
Assignee: nobody → jduell.mcbugs
(Assignee)

Updated

8 years ago
Assignee: jduell.mcbugs → josh
(Assignee)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/f93c9ecd5f28
http://hg.mozilla.org/mozilla-central/rev/651854be26ec
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.