Make WKURLSchemeTask thread safe <rdar://problem/50471863>
Created attachment 374047 [details] Patch
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.
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
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.
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.
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.
(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
(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.
Created attachment 374145 [details] Patch
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.
Comment on attachment 374145 [details] Patch Clearing flags on attachment: 374145 Committed r247461: <https://trac.webkit.org/changeset/247461>
All reviewed patches have been landed. Closing bug.
Committed r247473: <https://trac.webkit.org/changeset/247473>