Bug 223073 – Replace some usage of dispatch_queue with WorkQueue now that it supports sync dispatching

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

[Chris Dumez]

Replace some usage of dispatch_queue with WorkQueue now that it supports sync dispatching.

[Chris Dumez]

Created attachment 422941 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

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

[Chris Dumez]

Created attachment 422947 [details]
Patch

[Darin Adler]

(In reply to Chris Dumez from comment #3)
> What non-thread-safe reference count? WorkQueue definitely subclasses
> ThreadSafeRefCounted.

My mistake.

[Darin Adler]

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

[Chris Dumez]

Created attachment 422951 [details]
Patch

[EWS]

Committed r274296: <https://commits.webkit.org/r274296>

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

[Radar WebKit Bug Importer]

<rdar://problem/75328714>