Bug 199764 – Make WKURLSchemeTask thread safe

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Make WKURLSchemeTask thread safe

<rdar://problem/50471863>

[Brady Eidson]

Created attachment 374047 [details]
Patch

[EWS Watchlist]

Attachment 374047 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebURLSchemeTask.cpp:202:  Declaration has space between type name and * in NSURLRequest *WebURLSchemeTask  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 374047 [details]
Patch

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

> Source/WTF/wtf/MainThread.cpp:177
> +enum class MainStyle {

There's no reason not to have : bool here.  I think it's good practice, even if we don't store this anywhere yet.

> Source/WTF/wtf/MainThread.cpp:182
> +static void callOnMainAndWait(WTF::Function<void()>&& function, MainStyle mainStyle)

WTF:: unnecessary many places in this patch.

> Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37
> +static WebKit::WebURLSchemeTask::ExceptionType getExceptionTypeFromMainRunLoop(WTF::Function<WebKit::WebURLSchemeTask::ExceptionType ()>&& function)

Extra space, WTF:: unnecessary

[Geoffrey Garen]

Comment on attachment 374047 [details]
Patch

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

> Source/WTF/wtf/MainThread.cpp:179
> +    Thread,
> +    RunLoop

I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?

> Source/WTF/wtf/MainThread.cpp:185
>      if (isMainThread()) {
>          function();

isMainThread() is true on the WebThread, even when isMainRunLoop() is false. This seems like a bug.

[Alex Christensen]

Comment on attachment 374047 [details]
Patch

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

>> Source/WTF/wtf/MainThread.cpp:179
>> +    RunLoop
> 
> I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?

This is to not cause problems with UIWebView web thread stuff.

>> Source/WTF/wtf/MainThread.cpp:185
>>          function();
> 
> isMainThread() is true on the WebThread, even when isMainRunLoop() is false. This seems like a bug.

You're right.  This should be isMainRunLoop when mainStyle is RunLoop.

[Geoffrey Garen]

Comment on attachment 374047 [details]
Patch

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

>>> Source/WTF/wtf/MainThread.cpp:179
>>> +    RunLoop
>> 
>> I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?
> 
> This is to not cause problems with UIWebView web thread stuff.

For legacy compatibility, callOnMainThread and isMainThread apply to either the main thread or the web thread.

In new code, we don't need to maintain that extremely confusing naming style.

I'd suggest 

    Thread => MainThreadOrWebThread
    RunLoop => MainThread

Also, when practical, we should probably globally rename the older functions too.

[Brady Eidson]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 374047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374047&action=review
> 
> > Source/WTF/wtf/MainThread.cpp:177
> > +enum class MainStyle {
> 
> There's no reason not to have : bool here.  I think it's good practice, even
> if we don't store this anywhere yet.
> 
> > Source/WTF/wtf/MainThread.cpp:182
> > +static void callOnMainAndWait(WTF::Function<void()>&& function, MainStyle mainStyle)
> 
> WTF:: unnecessary many places in this patch.

To not cause a style split inside MainThread.cpp, I intend to leave those.

Fixing others.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37
> > +static WebKit::WebURLSchemeTask::ExceptionType getExceptionTypeFromMainRunLoop(WTF::Function<WebKit::WebURLSchemeTask::ExceptionType ()>&& function)
> 
> Extra space, WTF:: unnecessary

Style-bot says its an extra space because nobody has ever successfully taught style body our established lambda style.

This is our established lambda style.

---

I'm definitely fixing the actual mainrunloop utility function

[Brady Eidson]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 374047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374047&action=review
> 
> >>> Source/WTF/wtf/MainThread.cpp:179
> >>> +    RunLoop
> >> 
> >> I don't understand the distinction you're making between Thread and RunLoop here. There's one RunLoop per thread, so what is the actual difference here?
> > 
> > This is to not cause problems with UIWebView web thread stuff.
> 
> For legacy compatibility, callOnMainThread and isMainThread apply to either
> the main thread or the web thread.
> 
> In new code, we don't need to maintain that extremely confusing naming style.
> 
> I'd suggest 
> 
>     Thread => MainThreadOrWebThread
>     RunLoop => MainThread
> 
> Also, when practical, we should probably globally rename the older functions
> too.

I totally agree with this.

But I'm not sure how I can partially rename things in this patch - before that global renaming - without significantly ADDING to the confusion.

[Brady Eidson]

Created attachment 374145 [details]
Patch

[EWS Watchlist]

Attachment 374145 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebURLSchemeTask.cpp:202:  Declaration has space between type name and * in NSURLRequest *WebURLSchemeTask  [whitespace/declaration] [3]
ERROR: Source/WebKit/UIProcess/API/Cocoa/WKURLSchemeTask.mm:37:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 374145 [details]
Patch

Clearing flags on attachment: 374145

Committed r247461: <https://trac.webkit.org/changeset/247461>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

Committed r247473: <https://trac.webkit.org/changeset/247473>