Bug 170299 – We should pause silent WebAudio rendering in background tabs

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

We should pause silent WebAudio rendering in background tabs since it uses CPU and is not observable by the user.

[Chris Dumez]

<rdar://problem/31289132>

[Chris Dumez]

Created attachment 305902 [details]
WIP Patch

[Chris Dumez]

Created attachment 305904 [details]
WIP Patch

[Chris Dumez]

Created attachment 305915 [details]
WIP Patch

[Chris Dumez]

Created attachment 305917 [details]
WIP Patch

[Chris Dumez]

Created attachment 305923 [details]
Patch

[Chris Dumez]

Created attachment 305933 [details]
Patch

[Eric Carlson]

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?

[Chris Dumez]

(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?

[Chris Dumez]

(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).

[Eric Carlson]

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.

[Chris Dumez]

Comment on attachment 305933 [details]
Patch

Clearing flags on attachment: 305933

Committed r214721: <http://trac.webkit.org/changeset/214721>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.