Closed
Bug 1316215
Opened 4 years ago
Closed 4 years ago
Convert GMPService to MozPromise
Categories
(Core :: Audio/Video: GMP, defect, P3)
Core
Audio/Video: GMP
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
The GMPService should use MozPromises to make control flow and threading simpler. I also need this to help make the work in bug 1315850 easier.
Assignee | ||
Comment 1•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d2d669bb0cc
Assignee | ||
Comment 2•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f0837a51802
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•4 years ago
|
||
Green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a663ef1d763edc632c14aea9276f03b16debc0f
Comment 8•4 years ago
|
||
mozreview-review |
Comment on attachment 8813883 [details] Bug 1316215 - Promisify GMPService GetGMPContentParent and GetServiceChild. https://reviewboard.mozilla.org/r/94930/#review95394 r+ as-is, unless my questions/suggestions unveil some needed changes: ::: dom/media/gmp/GMPParent.cpp:1077 (Diff revision 1) > - RefPtr<RunCreateContentParentCallbacks> runCallbacks = > - new RunCreateContentParentCallbacks(mGMPContentParent); > + RefPtr<nsIRunnable> task(NewRunnableMethod(this, &GMPParent::ResolveGetContentParentPromises)); > + NS_DispatchToCurrentThread(task); I can see how it made sense before to dispatch a task that would directly call all callbacks. But MozPromise does all the necessary dispatching itself when it gets resolved. (Though I am not sure this is a guarantee of its API?) So I think you could just inline ResolveGetContentParentPromises right here. But if there is any doubt, it's fine to keep this dispatch, it's a small cost for a rare-enough operation. ::: dom/media/gmp/GMPParent.cpp:1099 (Diff revision 1) > if (!EnsureProcessLoaded() || !PGMPContent::Open(this)) { > - return false; > + MOZ_ASSERT(mGetContentParentPromises.Length() == 1); > + mGetContentParentPromises[0]->Reject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__); > + return; Should we empty mGetContentParentPromises? (To quickly release the promise holder we don't need anymore.) I see it was not done before either. Are you assuming that the rejection will get the GMPParent destroyed soon enough, along with its mGetContentParentPromises? ::: dom/media/gmp/GMPServiceChild.cpp:251 (Diff revision 1) > + UniquePtr<GetNodeIdCallback> callback(rawCallback); > - nsCString outId; > + nsCString outId; > - if (!aGMPServiceChild->SendGetGMPNodeId(mOrigin, mTopLevelOrigin, > - mGMPName, > - mInPrivateBrowsing, &outId)) { > - mCallback->Done(NS_ERROR_FAILURE, EmptyCString()); > + if (!child->SendGetGMPNodeId(origin, topLevelOrigin, > + gmpName, > + pb, &outId)) { > + callback->Done(NS_ERROR_DOM_MEDIA_FATAL_ERR, EmptyCString()); Is this error-code change really related to this promisification patch? (more below)
Attachment #8813883 -
Flags: review?(gsquelart) → review+
Comment 9•4 years ago
|
||
mozreview-review |
Comment on attachment 8813884 [details] Bug 1316215 - Block GMPContentParent close while a GMPService::GetContentParent is being processed. https://reviewboard.mozilla.org/r/94932/#review95400 r+ with typos fixed: ::: dom/media/gmp/GMPContentParent.h:1 (Diff revision 1) > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ In commit description: - 'MozPromimse' -> 'MozPromise' - 'closes it's' -> 'closes its'
Attachment #8813884 -
Flags: review?(gsquelart) → review+
Comment 10•4 years ago
|
||
mozreview-review |
Comment on attachment 8813885 [details] Bug 1316215 - Make GMPParent::IsUsed() take into account whether there are pending GetContentParent calls. https://reviewboard.mozilla.org/r/94934/#review95402
Attachment #8813885 -
Flags: review?(gsquelart) → review+
Comment 11•4 years ago
|
||
mozreview-review |
Comment on attachment 8813886 [details] Bug 1316215 - Merge SelectGMP and LaunchGMP into one synchronous IPC operation. https://reviewboard.mozilla.org/r/94936/#review95406 r+ with commit description fixed: ::: dom/media/gmp/GMPServiceChild.cpp:1 (Diff revision 1) > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ At the end of the commit description: 'in Bug 1267918 to and', spurious 'to' or missing blurb?
Attachment #8813886 -
Flags: review?(gsquelart) → review+
Assignee | ||
Comment 12•4 years ago
|
||
(In reply to Gerald Squelart [:gerald] from comment #8) > Comment on attachment 8813883 [details] > Bug 1316215 - Promisify GMPService GetGMPContentParent and GetServiceChild. > > https://reviewboard.mozilla.org/r/94930/#review95394 > > r+ as-is, unless my questions/suggestions unveil some needed changes: > > ::: dom/media/gmp/GMPParent.cpp:1077 > (Diff revision 1) > > - RefPtr<RunCreateContentParentCallbacks> runCallbacks = > > - new RunCreateContentParentCallbacks(mGMPContentParent); > > + RefPtr<nsIRunnable> task(NewRunnableMethod(this, &GMPParent::ResolveGetContentParentPromises)); > > + NS_DispatchToCurrentThread(task); > > I can see how it made sense before to dispatch a task that would directly > call all callbacks. > > But MozPromise does all the necessary dispatching itself when it gets > resolved. (Though I am not sure this is a guarantee of its API?) > So I think you could just inline ResolveGetContentParentPromises right here. > > But if there is any doubt, it's fine to keep this dispatch, it's a small > cost for a rare-enough operation. I had just made it async as it was already, but yes, the promise resolve should make this async, so it's probably safe to just call it directly. > ::: dom/media/gmp/GMPParent.cpp:1099 > (Diff revision 1) > > if (!EnsureProcessLoaded() || !PGMPContent::Open(this)) { > > - return false; > > + MOZ_ASSERT(mGetContentParentPromises.Length() == 1); > > + mGetContentParentPromises[0]->Reject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__); > > + return; > > Should we empty mGetContentParentPromises? (To quickly release the promise > holder we don't need anymore.) > > I see it was not done before either. > Are you assuming that the rejection will get the GMPParent destroyed soon > enough, along with its mGetContentParentPromises? I had been assuming that the rejection would get the GMPParent destroyed. But it would make the code more robust if we rejected all the promises and free'd their holders, just to be sure. It shouldn't be possible for other promises to be added while we're loading a GMP, but it doesn't hurt to be careful. > > ::: dom/media/gmp/GMPServiceChild.cpp:251 > (Diff revision 1) > > + UniquePtr<GetNodeIdCallback> callback(rawCallback); > > - nsCString outId; > > + nsCString outId; > > - if (!aGMPServiceChild->SendGetGMPNodeId(mOrigin, mTopLevelOrigin, > > - mGMPName, > > - mInPrivateBrowsing, &outId)) { > > - mCallback->Done(NS_ERROR_FAILURE, EmptyCString()); > > + if (!child->SendGetGMPNodeId(origin, topLevelOrigin, > > + gmpName, > > + pb, &outId)) { > > + callback->Done(NS_ERROR_DOM_MEDIA_FATAL_ERR, EmptyCString()); > > Is this error-code change really related to this promisification patch? > (more below) hmmm... yeah, not strictly, I will revert them.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•4 years ago
|
||
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cb29571e8491 Promisify GMPService GetGMPContentParent and GetServiceChild. r=gerald https://hg.mozilla.org/integration/autoland/rev/4be3169b9d02 Block GMPContentParent close while a GMPService::GetContentParent is being processed. r=gerald https://hg.mozilla.org/integration/autoland/rev/efff1ff587e3 Make GMPParent::IsUsed() take into account whether there are pending GetContentParent calls. r=gerald https://hg.mozilla.org/integration/autoland/rev/0390e2080381 Merge SelectGMP and LaunchGMP into one synchronous IPC operation. r=gerald
Comment 18•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cb29571e8491 https://hg.mozilla.org/mozilla-central/rev/4be3169b9d02 https://hg.mozilla.org/mozilla-central/rev/efff1ff587e3 https://hg.mozilla.org/mozilla-central/rev/0390e2080381
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•