Bug 186902

Summary: WKURLSchemeHandler doesn't handle sync XHR
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez, commit-queue, ews-watchlist
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
cdumez: review+
Patch none

Description Brady Eidson 2018-06-21 17:10:13 PDT 
WKURLSchemeHandler doesn't handle sync XHR

<rdar://problem/40955884>
Comment 1 Brady Eidson 2018-06-21 17:15:07 PDT 
Created attachment 343295 [details]
Patch

Have ideas for test enhancements I plan to get to tomorrow, but the code itself is reviewable.
Comment 2 EWS Watchlist 2018-06-21 17:18:02 PDT 
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.
Comment 3 Brady Eidson 2018-06-21 21:38:32 PDT 
Created attachment 343308 [details]
Patch
Comment 4 EWS Watchlist 2018-06-21 21:39:41 PDT 
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.
Comment 5 Brady Eidson 2018-06-22 14:03:14 PDT 
Created attachment 343363 [details]
Patch
Comment 6 EWS Watchlist 2018-06-22 14:05:12 PDT 
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 7 Chris Dumez 2018-06-22 14:43:06 PDT 
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.
Comment 8 Brady Eidson 2018-06-22 15:42:08 PDT 
(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.
Comment 9 Brady Eidson 2018-06-22 15:42:21 PDT 
Created attachment 343378 [details]
Patch
Comment 10 EWS Watchlist 2018-06-22 15:45:27 PDT 
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 11 WebKit Commit Bot 2018-06-22 17:34:55 PDT 
Comment on attachment 343378 [details]
Patch

Clearing flags on attachment: 343378

Committed r233113: <https://trac.webkit.org/changeset/233113>