Bug 213043

Created attachment 401572 [details]
Patch

Comment on attachment 401572 [details]
Patch

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

> Source/WTF/ChangeLog:3
> +        Unify some of RunLoop and callOnMainThread

Another possible title?

    [Cocoa] Build callOnMainThread on WTF::RunLoop rather than on NSObject methods

Comment on attachment 401572 [details]
Patch

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

> Source/WTF/wtf/RunLoop.h:66
>      WTF_EXPORT_PRIVATE static void initializeMainRunLoop();
> +#if USE(WEB_THREAD)
> +    WTF_EXPORT_PRIVATE static void initializeWebRunLoop();
> +#endif

Funny that these initialize functions include the word "RunLoop" but the getter functions don’t. Seems we don’t need to repeat the words "run loop".

> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:104
>      if (mainThreadPthread) {

I don’t understand this if statement. (Not new, but I find it really mysterious.)

> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:107
> +        RunLoop::web().dispatch([] {
> +            WTF::dispatchFunctionsFromMainThread();
> +        });

Since this is just calling a function can we write this instead?

    RunLoop::web().dispatch(WTF::dispatchFunctionsFromMainThread);

I suspect it might be slightly more efficient as well.

> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:119
> +    RunLoop::main().dispatch([] {
> +        WTF::dispatchFunctionsFromMainThread();
> +    });

Ditto.

Comment on attachment 401572 [details]
Patch

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

> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:107
> -        [staticMainThreadCaller performSelector:@selector(call) onThread:mainThreadNSThread withObject:nil waitUntilDone:NO];
> +        RunLoop::web().dispatch([] {
> +            WTF::dispatchFunctionsFromMainThread();
> +        });

This seems wrong? The function is supposed to dispatch on the main thread.

> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:119
> +    RunLoop::main().dispatch([] {
> +        WTF::dispatchFunctionsFromMainThread();
> +    });

This also seems wrong.

Comment on attachment 401572 [details]
Patch

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

>>> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:107
>>> +        });
>> 
>> Since this is just calling a function can we write this instead?
>> 
>>     RunLoop::web().dispatch(WTF::dispatchFunctionsFromMainThread);
>> 
>> I suspect it might be slightly more efficient as well.
> 
> This seems wrong? The function is supposed to dispatch on the main thread.

Never mind, confused by the confusion of the past.

>>> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:119
>>> +    });
>> 
>> Ditto.
> 
> This also seems wrong.

Never mind, confused by the confusion of the past.

> > Source/WTF/wtf/RunLoop.h:66
> >      WTF_EXPORT_PRIVATE static void initializeMainRunLoop();
> > +#if USE(WEB_THREAD)
> > +    WTF_EXPORT_PRIVATE static void initializeWebRunLoop();
> > +#endif
> 
> Funny that these initialize functions include the word "RunLoop" but the
> getter functions don’t. Seems we don’t need to repeat the words "run loop".

Why don't I make this style change in a follow-up patch, since it touches 26 more places in the code.

> > Source/WTF/wtf/cocoa/MainThreadCocoa.mm:104
> >      if (mainThreadPthread) {
> 
> I don’t understand this if statement. (Not new, but I find it really
> mysterious.)

It indicates that the web thread has been initialized. Paradoxically, we call the web thread the main thread. Another style change I'd like to take up in a follow-up patch.

> > Source/WTF/wtf/cocoa/MainThreadCocoa.mm:107
> > +        RunLoop::web().dispatch([] {
> > +            WTF::dispatchFunctionsFromMainThread();
> > +        });
> 
> Since this is just calling a function can we write this instead?
> 
>     RunLoop::web().dispatch(WTF::dispatchFunctionsFromMainThread);
> 
> I suspect it might be slightly more efficient as well.
> 
> > Source/WTF/wtf/cocoa/MainThreadCocoa.mm:119
> > +    RunLoop::main().dispatch([] {
> > +        WTF::dispatchFunctionsFromMainThread();
> > +    });
> 
> Ditto.

Okeedokee.

Comment on attachment 401572 [details]
Patch

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

>> Source/WTF/wtf/cocoa/MainThreadCocoa.mm:104
>>      if (mainThreadPthread) {
> 
> I don’t understand this if statement. (Not new, but I find it really mysterious.)

On reflection it seems to be that the idea here is that if "mainThreadPthread" is null, then we haven’t initialized the web thread yet, and so we should run on the main thread. Which seems like peculiar behavior to me, but not nonsensical behavior.

If this is our intent, then for greater clarity this should check if sWebThread is non-null, rather than checking if mainThreadPthread is non-null. An even better alternative in the future might be to ask RunLoop if initializeWebRunLoop has been called.

> An even better alternative in the future might be to ask RunLoop
> if initializeWebRunLoop has been called.

👍🏻

Created attachment 401578 [details]
Patch for landing

ChangeLog entry in Source/WTF/ChangeLog contains OOPS!.

Created attachment 401581 [details]
Patch for landing

Comment on attachment 401581 [details]
Patch for landing

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

> Source/WTF/ChangeLog:14
> +        CFRunLoopSource1 in the future.

CFRunLoopSource1 looks like a typo here, but I assume it’s just something I don’t understand.

Committed r262863: <https://trac.webkit.org/changeset/262863>

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

<rdar://problem/64226370>