Summary: | We should pause silent WebAudio rendering in background tabs | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | Web Audio | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | barraclough, eric.carlson, jer.noble | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2017-03-30 13:40:13 PDT
Created attachment 305902 [details]
WIP Patch
Created attachment 305904 [details]
WIP Patch
Created attachment 305915 [details]
WIP Patch
Created attachment 305917 [details]
WIP Patch
Created attachment 305923 [details]
Patch
Created attachment 305933 [details]
Patch
Comment on attachment 305933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305933&action=review > Source/WebCore/dom/VisibilityChangeClient.h:35 > +class VisibilityChangeClient { > +public: > + virtual ~VisibilityChangeClient() { } > + > + virtual void visibilityStateChanged() = 0; > +}; Anders recently suggested I use a lambda rather than a new "client" class for an asynch callback. Maybe you should do the same thing here as long as you have to change all of the call sites? (In reply to Eric Carlson from comment #8) > Comment on attachment 305933 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305933&action=review > > > Source/WebCore/dom/VisibilityChangeClient.h:35 > > +class VisibilityChangeClient { > > +public: > > + virtual ~VisibilityChangeClient() { } > > + > > + virtual void visibilityStateChanged() = 0; > > +}; > > Anders recently suggested I use a lambda rather than a new "client" class > for an asynch callback. Maybe you should do the same thing here as long as > you have to change all of the call sites? Doesn't this make unregistering from the document a bit more awkward though? Previous, we did: document()->unregisterForVisibilityStateChangedCallbacks(this); But now, I think we would need to unregister the lambda, which I think would require HTMLMediaElement / AudioContext to keep the lambda they register in a member so they can unregister it later. Or am I missing something easier? (In reply to Eric Carlson from comment #8) > Comment on attachment 305933 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=305933&action=review > > > Source/WebCore/dom/VisibilityChangeClient.h:35 > > +class VisibilityChangeClient { > > +public: > > + virtual ~VisibilityChangeClient() { } > > + > > + virtual void visibilityStateChanged() = 0; > > +}; > > Anders recently suggested I use a lambda rather than a new "client" class > for an asynch callback. Maybe you should do the same thing here as long as > you have to change all of the call sites? Note that I totally agree lambdas are better for callback to async operations. But this is not exactly the case for this code. We just want to register / unregister a listener for a specific event (visibility change). Comment on attachment 305933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305933&action=review >>>> Source/WebCore/dom/VisibilityChangeClient.h:35 >>>> +}; >>> >>> Anders recently suggested I use a lambda rather than a new "client" class for an asynch callback. Maybe you should do the same thing here as long as you have to change all of the call sites? >> >> Doesn't this make unregistering from the document a bit more awkward though? Previous, we did: >> document()->unregisterForVisibilityStateChangedCallbacks(this); >> >> But now, I think we would need to unregister the lambda, which I think would require HTMLMediaElement / AudioContext to keep the lambda they register in a member so they can unregister it later. Or am I missing something easier? > > Note that I totally agree lambdas are better for callback to async operations. But this is not exactly the case for this code. We just want to register / unregister a listener for a specific event (visibility change). I agree, it would be more awkward to use a lambda in a case like this. Comment on attachment 305933 [details] Patch Clearing flags on attachment: 305933 Committed r214721: <http://trac.webkit.org/changeset/214721> All reviewed patches have been landed. Closing bug. |