Bug 169422 – Add WKURLSchemeHandler API for handling custom protocols

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Add WKURLSchemeHandler API for handling custom protocols

[Brady Eidson]

<rdar://problem/17190141>

[Brady Eidson]

Created attachment 303942 [details]
Patch

[WebKit Commit Bot]

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…

[Tim Horton]

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

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 303946 [details]
Patch

[WebKit Commit Bot]

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.

[Brady Eidson]

Created attachment 303949 [details]
Patch

[WebKit Commit Bot]

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.

[Tim Horton]

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?

[Brady Eidson]

Created attachment 303962 [details]
Patch

[Brady Eidson]

FWIW, I have a growing set of tests that I will be submitting in https://bugs.webkit.org/show_bug.cgi?id=169434

[WebKit Commit Bot]

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.

[Tim Horton]

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?

[Brady Eidson]

(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)

[Tim Horton]

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?

[Brady Eidson]

(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!

[Brady Eidson]

Created attachment 303981 [details]
Patch

[WebKit Commit Bot]

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.

[Brady Eidson]

Created attachment 303999 [details]
Patch

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 303999 [details]
Patch

Clearing flags on attachment: 303999

Committed r213686: <http://trac.webkit.org/changeset/213686>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

32-bit debug build fix in https://trac.webkit.org/changeset/213693