Bug 223073 - Replace some usage of dispatch_queue with WorkQueue now that it supports sync dispatching
Summary: Replace some usage of dispatch_queue with WorkQueue now that it supports sync...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 223049
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-11 09:48 PST by Chris Dumez
Modified: 2021-03-11 12:48 PST (History)
13 users (show)

See Also:


Attachments
Patch (27.00 KB, patch)
2021-03-11 10:02 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.09 KB, patch)
2021-03-11 11:10 PST, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (27.09 KB, patch)
2021-03-11 11:34 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-03-11 09:48:01 PST
Replace some usage of dispatch_queue with WorkQueue now that it supports sync dispatching.
Comment 1 Chris Dumez 2021-03-11 10:02:37 PST
Created attachment 422941 [details]
Patch
Comment 2 Darin Adler 2021-03-11 10:16:07 PST
Comment on attachment 422941 [details]
Patch

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

> Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:107
> -    OSObjectPtr<dispatch_queue_t> m_dispatchQueue;
> +    RefPtr<WorkQueue> m_workQueue;
>      RetainPtr<WebInterruptionObserverHelper> m_interruptionObserverHelper;

This now adds a non-thread-safe reference count to this object. I’m assuming that didn’t matter.

Also, given that everything here is public, this seems more like a struct than a class.

> Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:271
> +        NSError* error = nil;

We normally format this as "NSError *". The old code above didn’t do that, but the code below did.

> Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:272
> +        m_private->m_workQueue->dispatchSync([&error] {

Can we make a bool come out of this thing instead of a possibly-already-deallocated NSError that we check against nil?
Comment 3 Chris Dumez 2021-03-11 11:05:07 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 422941 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=422941&action=review
> 
> > Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:107
> > -    OSObjectPtr<dispatch_queue_t> m_dispatchQueue;
> > +    RefPtr<WorkQueue> m_workQueue;
> >      RetainPtr<WebInterruptionObserverHelper> m_interruptionObserverHelper;
> 
> This now adds a non-thread-safe reference count to this object. I’m assuming
> that didn’t matter.

What non-thread-safe reference count? WorkQueue definitely subclasses ThreadSafeRefCounted.

> 
> Also, given that everything here is public, this seems more like a struct
> than a class.
> 
> > Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:271
> > +        NSError* error = nil;
> 
> We normally format this as "NSError *". The old code above didn’t do that,
> but the code below did.

OK.

> 
> > Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:272
> > +        m_private->m_workQueue->dispatchSync([&error] {
> 
> Can we make a bool come out of this thing instead of a
> possibly-already-deallocated NSError that we check against nil?

OK.
Comment 4 Chris Dumez 2021-03-11 11:10:36 PST
Created attachment 422947 [details]
Patch
Comment 5 Darin Adler 2021-03-11 11:23:54 PST
(In reply to Chris Dumez from comment #3)
> What non-thread-safe reference count? WorkQueue definitely subclasses
> ThreadSafeRefCounted.

My mistake.
Comment 6 Darin Adler 2021-03-11 11:30:13 PST
Comment on attachment 422947 [details]
Patch

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

> Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:272
> +        m_private->m_workQueue->dispatchSync([&error] {

Should be &success, not &error
Comment 7 Chris Dumez 2021-03-11 11:34:59 PST
Created attachment 422951 [details]
Patch
Comment 8 EWS 2021-03-11 12:47:37 PST
Committed r274296: <https://commits.webkit.org/r274296>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422951 [details].
Comment 9 Radar WebKit Bug Importer 2021-03-11 12:48:16 PST
<rdar://problem/75328714>