Closed
Bug 1365098
Opened 7 years ago
Closed 7 years ago
Convert NS_GetCurrentThread uses in dom/media
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
86.34 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
See https://bugzilla.mozilla.org/show_bug.cgi?id=1365096#c0. This bug covers all uses in dom/media.
Assignee | ||
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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+
Updated•7 years ago
|
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)
Comment 4•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/54f926563d9d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•