Summary: | Add WKURLSchemeHandler API for handling custom protocols | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||
Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, ggaren, mitz, thorton | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=169434 | ||||||||||||||||
Attachments: |
|
Description
Brady Eidson
2017-03-09 11:29:06 PST
Created attachment 303942 [details]
Patch
Attachment 303942 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:36: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:410: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:431: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:452: More than one command on the same line [whitespace/newline] [4]
Total errors found: 4 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 303942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303942&action=review > Source/WebKit2/Shared/API/APIObject.h:123 > + URLSchemeHandlerTask, Feels out of place here between the K and the M… Comment on attachment 303942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303942&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKURLSchemeHandler.h:44 > + represented by the url scheme handler task. should URL in all these comments be capitalized? > Source/WebKit2/WebProcess/Network/WebLoaderStrategy.cpp:203 > + RELEASE_LOG_IF_ALLOWED(resourceLoader, "scheduleLoad: URL will be handled by a UIProcess url scheme handler (frame = %p, resourceID = %" PRIu64 ")", resourceLoader.frame(), identifier); URL url URL url URL (In reply to comment #5) > Comment on attachment 303942 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303942&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKURLSchemeHandler.h:44 > > + represented by the url scheme handler task. > > should URL in all these comments be capitalized? Probably. > > > Source/WebKit2/WebProcess/Network/WebLoaderStrategy.cpp:203 > > + RELEASE_LOG_IF_ALLOWED(resourceLoader, "scheduleLoad: URL will be handled by a UIProcess url scheme handler (frame = %p, resourceID = %" PRIu64 ")", resourceLoader.frame(), identifier); > > URL url URL url URL See above. Created attachment 303946 [details]
Patch
Attachment 303946 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:36: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:410: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:431: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:452: More than one command on the same line [whitespace/newline] [4]
Total errors found: 4 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 303949 [details]
Patch
Attachment 303949 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:36: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:410: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:431: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:452: More than one command on the same line [whitespace/newline] [4]
Total errors found: 4 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 303949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303949&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:193 > +/* @abstract Returns the currently registered url scheme handler object for the given URL scheme. tiny URL again. > Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:334 > + configuration->_urlSchemeHandlers.set([self._urlSchemeHandlers mutableCopyWithZone:zone]); Isn't this a leak? Created attachment 303962 [details]
Patch
FWIW, I have a growing set of tests that I will be submitting in https://bugs.webkit.org/show_bug.cgi?id=169434 Attachment 303962 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:36: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:410: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:431: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:452: More than one command on the same line [whitespace/newline] [4]
Total errors found: 4 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 303962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303962&action=review > Source/WebKit2/UIProcess/WebURLSchemeHandler.h:57 > + uint64_t m_identifier { 1 }; why? (In reply to comment #15) > Comment on attachment 303962 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303962&action=review > > > Source/WebKit2/UIProcess/WebURLSchemeHandler.h:57 > > + uint64_t m_identifier { 1 }; > > why? Because if you try to put 0 in a hash table you're gonna have a bad time. (But if your comment is that since we always initialize it in the c'tor with the next identifier so why bother with the default initializer.... good call. I just put the default in there very early on while testing) Comment on attachment 303962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303962&action=review > Source/WebKit2/Shared/API/APIObject.h:123 > + URLSchemeHandlerTask, As previously noted, this seems out of order in an otherwise sorted list. > Source/WebKit2/UIProcess/API/Cocoa/WKURLSchemeHandlerTask.mm:40 > + if (exceptionType == WebKit::WebURLSchemeHandlerTask::ExceptionType::TaskAlreadyStopped) Should this be a switch so you catch new enum values? > Source/WebKit2/WebProcess/WebPage/WebURLSchemeHandlerTaskProxy.h:40 > +class WebURLSchemeHandlerTaskProxy { Do all of your C++ classes want any MAKE_NONCOPYABLE or whatever? (In reply to comment #17) > Comment on attachment 303962 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303962&action=review > > > Source/WebKit2/Shared/API/APIObject.h:123 > > + URLSchemeHandlerTask, > > As previously noted, this seems out of order in an otherwise sorted list. Yup, leftover from the rename. Moved. > > > Source/WebKit2/UIProcess/API/Cocoa/WKURLSchemeHandlerTask.mm:40 > > + if (exceptionType == WebKit::WebURLSchemeHandlerTask::ExceptionType::TaskAlreadyStopped) > > Should this be a switch so you catch new enum values? Yup. > > > Source/WebKit2/WebProcess/WebPage/WebURLSchemeHandlerTaskProxy.h:40 > > +class WebURLSchemeHandlerTaskProxy { > > Do all of your C++ classes want any MAKE_NONCOPYABLE or whatever? Prolly. Thx! Created attachment 303981 [details]
Patch
Attachment 303981 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:36: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:410: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:431: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:452: More than one command on the same line [whitespace/newline] [4]
Total errors found: 4 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 303999 [details]
Patch
Attachment 303999 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:36: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:413: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:434: More than one command on the same line [whitespace/newline] [4]
ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:455: More than one command on the same line [whitespace/newline] [4]
Total errors found: 4 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 303999 [details] Patch Clearing flags on attachment: 303999 Committed r213686: <http://trac.webkit.org/changeset/213686> All reviewed patches have been landed. Closing bug. 32-bit debug build fix in https://trac.webkit.org/changeset/213693 |