Closed Bug 968803 Opened 10 years ago Closed 10 years ago

RUN_ON_THREAD should be able to statically determine dispatch flags for return-valued runnables

Categories

(Core :: WebRTC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(2 files, 1 obsolete file)

This is sort of like bug 825234, I think.
This is not strictly necessary, but I think it makes the next patch slightly
nicer, and runnable_utils.py needs a bit of love anyway.
Attachment #8371578 - Flags: review?(ekr)
The main benefit here is removing the virtual function that we really only care
about in debug builds.  I thought there would be more cases that returned a
result, but such cases only really occur in testcases that do their own
Dispatch() calls anyway.  I feel better knowing that these errors will be
caught at compile time, rather than runtime, though.
Attachment #8371582 - Flags: review?(ekr)
2-months-and-counting ping.
Attachment #8371578 - Flags: review?(ekr) → review+
Comment on attachment 8371582 [details] [diff] [review]
part 2 - statically type runnable classes that return a result

Review of attachment 8371582 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to understand my question below about RefPtr before r+

::: media/mtransport/runnable_utils.h
@@ +21,5 @@
> +  NoResult,
> +  ReturnsResult
> +};
> +
> +// Temporary hack. Really we want to have a template which will do this

Is this comment still necessary?

@@ +25,5 @@
> +// Temporary hack. Really we want to have a template which will do this
> +static inline nsresult
> +RunOnThreadInternal(nsIEventTarget *thread, nsIRunnable *runnable, uint32_t flags)
> +{
> +  nsCOMPtr<nsIRunnable> runnable_ref(runnable);

Why did this become an nsCOMPtr?
Attachment #8371582 - Flags: review?(ekr) → review-
(In reply to Eric Rescorla (:ekr) from comment #4)
> ::: media/mtransport/runnable_utils.h
> @@ +21,5 @@
> > +  NoResult,
> > +  ReturnsResult
> > +};
> > +
> > +// Temporary hack. Really we want to have a template which will do this
> 
> Is this comment still necessary?

Hm.  Probably not.  I can remove it.

> @@ +25,5 @@
> > +// Temporary hack. Really we want to have a template which will do this
> > +static inline nsresult
> > +RunOnThreadInternal(nsIEventTarget *thread, nsIRunnable *runnable, uint32_t flags)
> > +{
> > +  nsCOMPtr<nsIRunnable> runnable_ref(runnable);
> 
> Why did this become an nsCOMPtr?

Because you're supposed to use nsCOMPtr with interface types and nsRefPtr/RefPtr with concrete types.
Ping for comment 5.
Flags: needinfo?(ekr)
OK. r+  if you remove the comment.
Flags: needinfo?(ekr)
Comment removed.  Applying r+ as per comment 7.
Attachment #8371582 - Attachment is obsolete: true
Attachment #8415309 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b30e471400c5
https://hg.mozilla.org/mozilla-central/rev/8a6c3ab28bbc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.