[webkit-reviews] review denied: [Bug 179627] Replace dispatch_async with callOnMainThread : [Attachment 326784] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 179627: Replace dispatch_async with callOnMainThread
https://bugs.webkit.org/show_bug.cgi?id=179627

Attachment 326784: Patch

https://bugs.webkit.org/attachment.cgi?id=326784&action=review




--- Comment #2 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 326784
  --> https://bugs.webkit.org/attachment.cgi?id=326784
Patch

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

I think this blanket replacement is dangerous. Some of those call sites almost
certainly want the actual main thread. I think you need to consult file owners
for each area.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:318
> -    RetainPtr<WebMediaSessionHelper> strongSelf = self;
> -    dispatch_async(dispatch_get_main_queue(), [strongSelf]() {
> +    callOnMainThread([strongSelf = retainPtr(self)]() {
>	   BEGIN_BLOCK_OBJC_EXCEPTIONS
>	   RetainPtr<MPVolumeView> volumeView =
adoptNS([allocMPVolumeViewInstance() init]);
>	   callOnWebThreadOrDispatchAsyncOnMainThread([strongSelf,
volumeView]() {

I think this one was correct.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:364
>	   [[UIApplication sharedApplication]
beginReceivingRemoteControlEvents];

This probably really should be the main thread, so undo this part.

> Source/WebCore/platform/audio/mac/AudioSampleDataSource.mm:185
> -    dispatch_async(dispatch_get_main_queue(), [sampleCount, sampleTime,
presentationTime, absoluteTime = mach_absolute_time(), startFrame1, endFrame1,
startFrame2, endFrame2] {
> +    callOnMainThread([sampleCount, sampleTime, presentationTime,
absoluteTime = mach_absolute_time(), startFrame1, endFrame1, startFrame2,
endFrame2] {
>	   LOG(MediaCaptureSamples, "@@ pushSamples: added %ld samples for time
= %s (was %s), mach time = %lld", sampleCount,
toString(sampleTime).utf8().data(), toString(presentationTime).utf8().data(),
absoluteTime);
>	   LOG(MediaCaptureSamples, "@@ pushSamples: buffered range was [%lld
.. %lld], is [%lld .. %lld]", startFrame1, endFrame1, startFrame2, endFrame2);

Why would we do anything here if we're not logging?