Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1123603002:
Process suspend() and resume() in call order (Closed)
|
Created:
5 years, 7 months ago by Raymond Toy Modified:
5 years, 7 months ago CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionProcess suspend() and resume() in call order
Previously, the promises for suspend and resume were kept on separate
lists and each list was processed at once which then actually took care of suspending the context This is incorrect if there
are back-to-back calls of suspend and resume.
Instead, suspend the context at the call and resolve the promise immediately instead of waiting. For resume, we were already resuming at the call. The list for the promises for resume is still needed because we want to resolve them when the audio hardware has resumed.
BUG=483002, 483269
TEST=Run ManualTests/webaudio/{suspend-resume,suspend-resume-2}.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195476
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 10
Patch Set 3 : Update according to review. #
Total comments: 9
Patch Set 4 : WIP: Not for review #Patch Set 5 : Simplified implementation #
Total comments: 4
Patch Set 6 : Update according to review #
Messages
Total messages: 21 (4 generated)
rtoy@chromium.org changed reviewers: + haraken@chromium.org, hongchan@chromium.org
PTAL. haraken@: please check for oilpan correctness
Thanks for nice changes! LGTM with nits. Taking this CL out of AudioContext is a big change, so I am simply asking your opinion about it. If you think that's too much work for a little benefit, we can just move on. https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:81: AudioContext::SuspendResumeHandler::SuspendResumeHandler( Can we change the name of this class to the wrapper or container? We're already using the name "handler" in a big way. https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:823: m_suspendResumeHandlers.append(SuspendResumeHandler::createResumeHandler(resolver)); At a first glance, I read this code like "create/resume the audio context handler." https://codereview.chromium.org/1123603002/diff/20001/ManualTests/webaudio/su... File ManualTests/webaudio/suspend-resume-2.html (right): https://codereview.chromium.org/1123603002/diff/20001/ManualTests/webaudio/su... ManualTests/webaudio/suspend-resume-2.html:67: var context = new AudioContext(); Can we have a rational or description why this test is needed and why this should be tested manually? I can see we have a link to the issue, but I think it is better to make this self-contained. https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:244: class SuspendResumeResolver { Should we consider take this definition out of the AudioContext? It seems like AudioContext keeps getting longer and longer. Just a thought. Also I believe we need some explanation - regarding why it is needed and what it does.
https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:81: AudioContext::SuspendResumeHandler::SuspendResumeHandler( On 2015/05/04 17:28:38, hoch wrote: > Can we change the name of this class to the wrapper or container? We're already > using the name "handler" in a big way. Are you looking at the right version? I renamed it to SuspendResumeResolver in patch 2. https://codereview.chromium.org/1123603002/diff/20001/ManualTests/webaudio/su... File ManualTests/webaudio/suspend-resume-2.html (right): https://codereview.chromium.org/1123603002/diff/20001/ManualTests/webaudio/su... ManualTests/webaudio/suspend-resume-2.html:67: var context = new AudioContext(); On 2015/05/04 17:28:38, hoch wrote: > Can we have a rational or description why this test is needed and why this > should be tested manually? I can see we have a link to the issue, but I think it > is better to make this self-contained. Doesn't line 20 explain why? Should I expand on it? https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:244: class SuspendResumeResolver { On 2015/05/04 17:28:38, hoch wrote: > Should we consider take this definition out of the AudioContext? It seems like > AudioContext keeps getting longer and longer. Just a thought. Also I believe we > need some explanation - regarding why it is needed and what it does. It seems natural to keep it here since it's only relevant to an AudioContext. I will add some documentation, however.
https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:81: AudioContext::SuspendResumeHandler::SuspendResumeHandler( On 2015/05/04 17:38:34, Raymond Toy wrote: > On 2015/05/04 17:28:38, hoch wrote: > > Can we change the name of this class to the wrapper or container? We're > already > > using the name "handler" in a big way. > > Are you looking at the right version? I renamed it to SuspendResumeResolver in > patch 2. Oh, yes. Resolver is good. https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:244: class SuspendResumeResolver { On 2015/05/04 17:38:34, Raymond Toy wrote: > On 2015/05/04 17:28:38, hoch wrote: > > Should we consider take this definition out of the AudioContext? It seems like > > AudioContext keeps getting longer and longer. Just a thought. Also I believe > we > > need some explanation - regarding why it is needed and what it does. > > It seems natural to keep it here since it's only relevant to an AudioContext. > > I will add some documentation, however. Acknowledged.
https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:823: m_suspendResumeHandlers.append(SuspendResumeHandler::createResumeHandler(resolver)); On 2015/05/04 17:28:38, hoch wrote: > At a first glance, I read this code like "create/resume the audio context > handler." What are you suggesting? This name is bad and I should rename it to something else?
https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/1/Source/modules/webaudio/Aud... Source/modules/webaudio/AudioContext.cpp:823: m_suspendResumeHandlers.append(SuspendResumeHandler::createResumeHandler(resolver)); On 2015/05/04 17:44:07, Raymond Toy wrote: > On 2015/05/04 17:28:38, hoch wrote: > > At a first glance, I read this code like "create/resume the audio context > > handler." > > What are you suggesting? This name is bad and I should rename it to something > else? My suggestion was for the previous patch set, which is wrong. Please have a look at the new comment. https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:89: AudioContext::SuspendResumeResolver* AudioContext::SuspendResumeResolver::createSuspendHandler( Shouldn't the method name be: createSuspendResolver()? https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:95: AudioContext::SuspendResumeResolver* AudioContext::SuspendResumeResolver::createResumeHandler( Same here as line 89. https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:782: m_suspendResumeResolvers.append(SuspendResumeResolver::createSuspendHandler(resolver)); createSuspendHandler => createSuspendResolver() https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:823: m_suspendResumeResolvers.append(SuspendResumeResolver::createResumeHandler(resolver)); createResumeHandler => createResumeResolver() Isn't this better?
https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/20001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:89: AudioContext::SuspendResumeResolver* AudioContext::SuspendResumeResolver::createSuspendHandler( On 2015/05/04 17:50:52, hoch wrote: > Shouldn't the method name be: createSuspendResolver()? Oops. I meant to rename this when I changed the class name to Resolver from Handler.
haraken@chromium.org changed reviewers: + oilpan-reviews@chromium.org
https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (left): https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:872: resolvePromisesForResume(); Why don't we need to call resolvePromisesForSuspendResume() here? https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:933: // promises in the main thread. Let's keep this comment. https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:964: // Resolve any pending promises created by suspend() suspend() or resume(). https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:981: m_suspendResumeResolvers.clear(); Don't we need to set m_isResolvingSuspendResumePromise to false here? https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:246: class SuspendResumeResolver { This needs to be: class SuspendResumeResolver : public GarbageCollected<SuspendResumeResolver> { }; Otherwise m_resolver won't be traced and thus wrongly collected. https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:262: RefPtrWillBeRawPtr<ScriptPromiseResolver> m_resolver; RefPtrWillBeMember<ScriptPromiseResolver> Also you need to trace the m_resolver. https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:309: Vector<SuspendResumeResolver*> m_suspendResumeResolvers; WillBeHeapVector<OwnPtrWillBeMember<SuspendResumeResolver>> In non-oilpan, it is wrong to hold SuspendResumeResolver by a raw pointer. It needs to be an OwnPtr. Otherwise SuspendResumeResolver will leak. In oilpan, you need to hold SuspendResumeResolver by a Member.
https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:246: class SuspendResumeResolver { On 2015/05/04 23:24:41, haraken wrote: > > This needs to be: > > class SuspendResumeResolver : public GarbageCollected<SuspendResumeResolver> { > }; > > Otherwise m_resolver won't be traced and thus wrongly collected. When I do this and make m_resolver be a RefPtrWillBeMember<ScriptPromiseResolver>, I get compiler errors: ../../third_party/WebKit/Source/modules/webaudio/AudioContext.h:246:5: error: [blink-gc] Class 'SuspendResumeResolver' requires finalization. class SuspendResumeResolver : public GarbageCollected<SuspendResumeResolver> { ^ ../../third_party/WebKit/Source/modules/webaudio/AudioContext.h:262:9: note: [blink-gc] Field 'm_resolver' requiring finalization declared here: RefPtrWillBeMember<ScriptPromiseResolver> m_resolver; ^ What else do I need to do?
https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.h (right): https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.h:246: class SuspendResumeResolver { On 2015/05/14 21:26:05, Raymond Toy wrote: > On 2015/05/04 23:24:41, haraken wrote: > > > > This needs to be: > > > > class SuspendResumeResolver : public GarbageCollected<SuspendResumeResolver> > { > > }; > > > > Otherwise m_resolver won't be traced and thus wrongly collected. > > When I do this and make m_resolver be a > RefPtrWillBeMember<ScriptPromiseResolver>, I get compiler errors: > > ../../third_party/WebKit/Source/modules/webaudio/AudioContext.h:246:5: error: > [blink-gc] Class 'SuspendResumeResolver' requires finalization. > class SuspendResumeResolver : public GarbageCollected<SuspendResumeResolver> > { > ^ > ../../third_party/WebKit/Source/modules/webaudio/AudioContext.h:262:9: note: > [blink-gc] Field 'm_resolver' requiring finalization declared here: > RefPtrWillBeMember<ScriptPromiseResolver> m_resolver; > ^ > > What else do I need to do? You'll need to use NoBaseWillBeGarbageCollected. In non-oilpan builds, SuspendResumeResolver should not be a GarbageCollected class.
On 2015/05/14 21:28:27, haraken wrote: > https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... > File Source/modules/webaudio/AudioContext.h (right): > > https://codereview.chromium.org/1123603002/diff/40001/Source/modules/webaudio... > Source/modules/webaudio/AudioContext.h:246: class SuspendResumeResolver { > On 2015/05/14 21:26:05, Raymond Toy wrote: > > On 2015/05/04 23:24:41, haraken wrote: > > > > > > This needs to be: > > > > > > class SuspendResumeResolver : public > GarbageCollected<SuspendResumeResolver> > > { > > > }; > > > > > > Otherwise m_resolver won't be traced and thus wrongly collected. > > > > When I do this and make m_resolver be a > > RefPtrWillBeMember<ScriptPromiseResolver>, I get compiler errors: > > > > ../../third_party/WebKit/Source/modules/webaudio/AudioContext.h:246:5: error: > > [blink-gc] Class 'SuspendResumeResolver' requires finalization. > > class SuspendResumeResolver : public > GarbageCollected<SuspendResumeResolver> > > { > > ^ > > ../../third_party/WebKit/Source/modules/webaudio/AudioContext.h:262:9: note: > > [blink-gc] Field 'm_resolver' requiring finalization declared here: > > RefPtrWillBeMember<ScriptPromiseResolver> m_resolver; > > ^ > > > > What else do I need to do? > > You'll need to use NoBaseWillBeGarbageCollected. > > In non-oilpan builds, SuspendResumeResolver should not be a GarbageCollected > class. Thanks, that fixes everything, of course. Fixing this causes a different issue that I need to investigate, but I'll get back to you when it's fixed. Thanks for your help.
After some discussions with Hongchan, I've concluded that this approach is more complicated than it needs to be. I'll upload a new patch soon with the simplified approach that still achieves the desired goals.
PTAL
LGTM https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (left): https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:768: resolvePromisesForSuspendOnMainThread(); Actually I was wondering why we need to call resolvePromisesForSuspendOnMainThread() whereas AudioContext::suspendContext() is guaranteed to be executed in the main thread (see the ASSERT at line 747). https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:1028: #if 0 You can remove this.
https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (left): https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:768: resolvePromisesForSuspendOnMainThread(); On 2015/05/18 14:47:59, haraken wrote: > > Actually I was wondering why we need to call > resolvePromisesForSuspendOnMainThread() whereas AudioContext::suspendContext() > is guaranteed to be executed in the main thread (see the ASSERT at line 747). Yeah, I have no recollection of why I did it this way. Maybe to match how resume works? https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... File Source/modules/webaudio/AudioContext.cpp (right): https://codereview.chromium.org/1123603002/diff/80001/Source/modules/webaudio... Source/modules/webaudio/AudioContext.cpp:1028: #if 0 On 2015/05/18 14:47:59, haraken wrote: > > You can remove this. Oops. Done
The CQ bit was checked by rtoy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hongchan@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1123603002/#ps100001 (title: "Update according to review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1123603002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195476 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
