Bug 218405 – [macOS] System sounds should be played in the UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

In preparation of blocking the system sound server in the WebContent process, system sounds should be played in the UI process.

[Radar WebKit Bug Importer]

<rdar://problem/70898846>

[Per Arne Vollan]

Created attachment 412803 [details]
Patch

[Per Arne Vollan]

Created attachment 412922 [details]
Patch

[Per Arne Vollan]

Created attachment 412923 [details]
Patch

[Per Arne Vollan]

Created attachment 412930 [details]
Patch

[Per Arne Vollan]

Created attachment 412932 [details]
Patch

[Sam Weinig]

As I mention in many reviews, please consider avoiding a singleton in WebCore as it constrains our options in the future for process sharing. Rather, please consider the standard Page level delegation.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #7)
> As I mention in many reviews, please consider avoiding a singleton in
> WebCore as it constrains our options in the future for process sharing.
> Rather, please consider the standard Page level delegation.

Ah, I see. I will look into that.

Thanks for reviewing!

[Darin Adler]

Comment on attachment 412932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412932&action=review

I’m not sure that an object is the best way to pass a function pointer. Could have used WTF::Function instead.

> Source/WebCore/Modules/mediarecorder/MediaRecorderProvider.h:28
> +#include <wtf/text/WTFString.h>

I would think we’d include Forward.h here.

> Source/WebKit/WebProcess/WebSystemSoundDelegate.h:32
> +class WebSystemSoundDelegate : public WebCore::SystemSoundDelegate {

final

> Source/WebKit/WebProcess/WebSystemSoundDelegate.h:35
> +    void systemBeep() override;

final

[Per Arne Vollan]

Created attachment 413299 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #9)
> Comment on attachment 412932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412932&action=review
> 
> I’m not sure that an object is the best way to pass a function pointer.
> Could have used WTF::Function instead.
> 

That's a good point. My initial thinking was to create a system sound manager that could support new system sounds in the future, although there's a chance that may not happen.

> > Source/WebCore/Modules/mediarecorder/MediaRecorderProvider.h:28
> > +#include <wtf/text/WTFString.h>
> 
> I would think we’d include Forward.h here.
> 
> > Source/WebKit/WebProcess/WebSystemSoundDelegate.h:32
> > +class WebSystemSoundDelegate : public WebCore::SystemSoundDelegate {
> 
> final
> 
> > Source/WebKit/WebProcess/WebSystemSoundDelegate.h:35
> > +    void systemBeep() override;
> 
> final

Fixed in latest patch.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Sam Weinig from comment #7)
> As I mention in many reviews, please consider avoiding a singleton in
> WebCore as it constrains our options in the future for process sharing.
> Rather, please consider the standard Page level delegation.

Yes, I think this sounds good. I have filed https://bugs.webkit.org/show_bug.cgi?id=218616. Instead of using a singleton, I think this could be a member in the Page class.

[Per Arne Vollan]

Created attachment 413303 [details]
Patch

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 413303 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Per Arne Vollan]

Created attachment 413605 [details]
Patch

[Per Arne Vollan]

Created attachment 413608 [details]
Patch

[Per Arne Vollan]

Created attachment 413610 [details]
Patch

[EWS]

Committed r269593: <https://trac.webkit.org/changeset/269593>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 413610 [details].