[webkit-reviews] review denied: [Bug 73176] Add 'isProtocolHandlerRegistered' and 'unregisterProtocolHandler'. : [Attachment 144730] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jun 1 00:58:43 PDT 2012
Adam Barth <abarth at webkit.org> has denied Dongwoo Joshua Im (dwim)
<dw.im at samsung.com>'s request for review:
Bug 73176: Add 'isProtocolHandlerRegistered' and 'unregisterProtocolHandler'.
https://bugs.webkit.org/show_bug.cgi?id=73176
Attachment 144730: Patch
https://bugs.webkit.org/attachment.cgi?id=144730&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144730&action=review
This patch looks like it's in pretty good shape. I've left you a couple minor
comments below. Once those are addressed, we should be all set. Thanks!
> Source/WebCore/page/CustomHandlersState.cpp:7
> + version 2.1 of the License, or (at your option) any later version.
Typically new files are licensed using BSD, but it's up to you.
> Source/WebCore/page/CustomHandlersState.cpp:27
> +String customHandlersStateString(CustomHandlersState state)
Rather than creating a whole new file for this rather simple function, you can
declare the CustomHandlersState enum in ChromeClient.h and have the client
method return an enum value rather than a string. Then you can do the
translation between enum values and strings in a static function in
NavigatorRegisterProtocolHandler.cpp.
> Source/WebCore/page/NavigatorRegisterProtocolHandler.cpp:157
> + if (!document)
> + return declined;
document can never be 0 here.
> Source/WebCore/page/NavigatorRegisterProtocolHandler.cpp:182
> + if (!document)
> + return;
ditto
> Source/WebCore/page/NavigatorRegisterProtocolHandler.idl:27
> +#if defined(ENABLE_REGISTER_PROTOCOL_HANDLER) &&
ENABLE_REGISTER_PROTOCOL_HANDLER
You can just use the [Conditional] attribute on the method.
More information about the webkit-reviews
mailing list