Bug 218405

Summary: [macOS] System sounds should be played in the UI process
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bfulgham, darin, eric.carlson, ews-watchlist, glenn, gyuyoung.kim, hi, jer.noble, joepeck, mifenton, philipj, ryuan.choi, sam, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
darin: review+
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch none

Description Per Arne Vollan 2020-10-30 15:04:46 PDT 
In preparation of blocking the system sound server in the WebContent process, system sounds should be played in the UI process.
Comment 1 Radar WebKit Bug Importer 2020-10-30 15:05:10 PDT 
<rdar://problem/70898846>
Comment 2 Per Arne Vollan 2020-10-30 15:17:24 PDT 
Created attachment 412803 [details]
Patch
Comment 3 Per Arne Vollan 2020-11-02 08:31:07 PST 
Created attachment 412922 [details]
Patch
Comment 4 Per Arne Vollan 2020-11-02 08:44:31 PST 
Created attachment 412923 [details]
Patch
Comment 5 Per Arne Vollan 2020-11-02 09:09:02 PST 
Created attachment 412930 [details]
Patch
Comment 6 Per Arne Vollan 2020-11-02 09:21:46 PST 
Created attachment 412932 [details]
Patch
Comment 7 Sam Weinig 2020-11-02 09:31:12 PST 
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.
Comment 8 Per Arne Vollan 2020-11-02 09:49:07 PST 
(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!
Comment 9 Darin Adler 2020-11-03 11:22:51 PST 
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
Comment 10 Per Arne Vollan 2020-11-05 08:13:54 PST 
Created attachment 413299 [details]
Patch
Comment 11 Per Arne Vollan 2020-11-05 08:18:11 PST 
(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!
Comment 12 Per Arne Vollan 2020-11-05 08:24:27 PST 
(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.
Comment 13 Per Arne Vollan 2020-11-05 08:32:19 PST 
Created attachment 413303 [details]
Patch
Comment 14 EWS 2020-11-09 09:01:01 PST 
Tools/Scripts/svn-apply failed to apply attachment 413303 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 15 Per Arne Vollan 2020-11-09 11:17:27 PST 
Created attachment 413605 [details]
Patch
Comment 16 Per Arne Vollan 2020-11-09 11:32:22 PST 
Created attachment 413608 [details]
Patch
Comment 17 Per Arne Vollan 2020-11-09 11:38:01 PST 
Created attachment 413610 [details]
Patch
Comment 18 EWS 2020-11-09 12:20:30 PST 
Committed r269593: <https://trac.webkit.org/changeset/269593>

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