Closed Bug 1365098 Opened 7 years ago Closed 7 years ago

Convert NS_GetCurrentThread uses in dom/media

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
This conversion makes us operate on event targets instead of threads. The advantage of event targets is that there is only one main thread event target in the Quantum DOM world, while there are potentially many main threads.

I'll send out an email about this change to dev-platform tomorrow. However, I want to get some patches posted so people can see what the change will look like.

There's some discussion of the API in bug 1361164 in case that helps.
Attachment #8868328 - Flags: review?(cpearce)
Comment on attachment 8868328 [details] [diff] [review]
patch

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

::: dom/media/VideoUtils.cpp
@@ +352,4 @@
>    if (aTarget) {
>      target = aTarget;
>    } else {
> +    target = GetMainThreadEventTarget();

In MediaManager you kept the checks for GetMainThreadEventTarget() failing; should you do that here too?

I don't think we could run this code after XPCOM shutdown has progressed past xpcom-shutdown-threads, but the prescence of "Get" in the name of GetMainThreadEventTarget() implies it can fail and that we should check for failure.

Should there be a "Get" in GetMainThreadEventTarget()?

::: dom/media/gmp/GMPContentParent.cpp
@@ +73,5 @@
>  
>  void
>  GMPContentParent::CheckThread()
>  {
> +  MOZ_ASSERT(mGMPEventTarget->IsOnCurrentThread());

For consistency, may as well use GMPEventTarget() instead of mGMPEventTarget.

::: dom/media/gmp/GMPParent.cpp
@@ +385,5 @@
>    // Note: GeckoMediaPluginService::GetThread() is threadsafe, and returns
>    // nullptr if the GeckoMediaPluginService has started shutdown.
>    nsCOMPtr<nsIThread> gmpThread;
>    mps->GetThread(getter_AddRefs(gmpThread));
> +  return gmpThread->EventTarget();

We need to null check gmpThread here, as per the comment starting line 385 above.
Attachment #8868328 - Flags: review?(cpearce) → review+
Component: Audio/Video → XPCOM
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f926563d9d
Convert NS_GetCurrentThread uses in dom/media (r=cpearce)
https://hg.mozilla.org/mozilla-central/rev/54f926563d9d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: