Bug 218405 - [macOS] System sounds should be played in the UI process
Summary: [macOS] System sounds should be played in the UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-30 15:04 PDT by Per Arne Vollan
Modified: 2020-11-09 12:20 PST (History)
16 users (show)

See Also:


Attachments
Patch (31.99 KB, patch)
2020-10-30 15:17 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.67 KB, patch)
2020-11-02 08:31 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.68 KB, patch)
2020-11-02 08:44 PST, Per Arne Vollan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.05 KB, patch)
2020-11-02 09:09 PST, Per Arne Vollan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.53 KB, patch)
2020-11-02 09:21 PST, Per Arne Vollan
darin: review+
Details | Formatted Diff | Diff
Patch (37.93 KB, patch)
2020-11-05 08:13 PST, Per Arne Vollan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.54 KB, patch)
2020-11-05 08:32 PST, Per Arne Vollan
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (13.62 KB, patch)
2020-11-09 11:17 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.55 KB, patch)
2020-11-09 11:32 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (37.00 KB, patch)
2020-11-09 11:38 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].