WKURLSchemeHandler doesn't handle sync XHR <rdar://problem/40955884>
Created attachment 343295 [details] Patch Have ideas for test enhancements I plan to get to tomorrow, but the code itself is reviewable.
Attachment 343295 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:99: The parameter name "identifier" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:471: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:476: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:480: Extra space before ( in function call [whitespace/parens] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:486: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 343308 [details] Patch
Attachment 343308 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:101: The parameter name "identifier" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:467: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:472: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:476: Extra space before ( in function call [whitespace/parens] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:482: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 5 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 343363 [details] Patch
Attachment 343363 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:482: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:488: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:492: Extra space before ( in function call [whitespace/parens] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:498: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 343363 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343363&action=review > Source/WebKit/Shared/WebErrors.cpp:62 > + return ResourceError(errorDomainWebKitInternal, 0, request.url(), WEB_UI_STRING("Error handling synchronous load with custom protocol", "Custom protocol synchronous load failure description")); Don't you need to run Tools/Scripts/update-webkit-localizable-strings too? > Source/WebKit/UIProcess/WebURLSchemeHandler.h:46 > +typedef CompletionHandler<void(const WebCore::ResourceResponse&, const WebCore::ResourceError&, const IPC::DataReference&)> SyncLoadCompletionHandler; I think we like "using SyncLoadCompletionHandler = CompletionHandler<void(const WebCore::ResourceResponse&, const WebCore::ResourceError&, const IPC::DataReference&)>;" better in new code. > Source/WebKit/UIProcess/WebURLSchemeTask.h:51 > +typedef CompletionHandler<void(const WebCore::ResourceResponse&, const WebCore::ResourceError&, const IPC::DataReference&)> SyncLoadCompletionHandler; using ? > Source/WebKit/UIProcess/WebURLSchemeTask.h:82 > + bool isSync() const { return !!m_syncCompletionHandler; } It may not matter but note that isSync() will start returning false after the m_syncCompletionHandler has been called. So it will only be sync until it is completed.
(In reply to Chris Dumez from comment #7) > Comment on attachment 343363 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=343363&action=review > > > Source/WebKit/Shared/WebErrors.cpp:62 > > + return ResourceError(errorDomainWebKitInternal, 0, request.url(), WEB_UI_STRING("Error handling synchronous load with custom protocol", "Custom protocol synchronous load failure description")); > > Don't you need to run Tools/Scripts/update-webkit-localizable-strings too? Yah. > > > Source/WebKit/UIProcess/WebURLSchemeHandler.h:46 > > +typedef CompletionHandler<void(const WebCore::ResourceResponse&, const WebCore::ResourceError&, const IPC::DataReference&)> SyncLoadCompletionHandler; > > I think we like "using SyncLoadCompletionHandler = > CompletionHandler<void(const WebCore::ResourceResponse&, const > WebCore::ResourceError&, const IPC::DataReference&)>;" better in new code. Sure. > > > Source/WebKit/UIProcess/WebURLSchemeTask.h:51 > > +typedef CompletionHandler<void(const WebCore::ResourceResponse&, const WebCore::ResourceError&, const IPC::DataReference&)> SyncLoadCompletionHandler; > > using ? Sure. > > > Source/WebKit/UIProcess/WebURLSchemeTask.h:82 > > + bool isSync() const { return !!m_syncCompletionHandler; } > > It may not matter but note that isSync() will start returning false after > the m_syncCompletionHandler has been called. So it will only be sync until > it is completed. Known and by design.
Created attachment 343378 [details] Patch
Attachment 343378 [details] did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:482: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:488: This { should be at the end of the previous line [whitespace/braces] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:492: Extra space before ( in function call [whitespace/parens] [4] ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:498: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 343378 [details] Patch Clearing flags on attachment: 343378 Committed r233113: <https://trac.webkit.org/changeset/233113>