[webkit-reviews] review denied: [Bug 54926] All Console Messages should be passed to ChromeClients. : [Attachment 83605] [PATCH] Addressed Comments

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at apple.com> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 54926: All Console Messages should be passed to ChromeClients.
https://bugs.webkit.org/show_bug.cgi?id=54926

Attachment 83605: [PATCH] Addressed Comments
https://bugs.webkit.org/attachment.cgi?id=83605&action=review

------- Additional Comments from Timothy Hatcher <timothy at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83605&action=review

Looking good. Still some things that should be fixed.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:104
> +NSString * const ConsoleMessageHTMLMessageSource  = @"HTMLMessageSource";
> +NSString * const ConsoleMessageWMLMessageSource   = @"WMLMessageSource";
> +NSString * const ConsoleMessageXMLMessageSource   = @"XMLMessageSource";
> +NSString * const ConsoleMessageJSMessageSource    = @"JSMessageSource";
> +NSString * const ConsoleMessageCSSMessageSource   = @"CSSMessageSource";
> +NSString * const ConsoleMessageOtherMessageSource = @"OtherMessageSource";
> +
> +NSString * const ConsoleMessageLogMessageType		 =
@"LogMessageType";
> +NSString * const ConsoleMessageObjectMessageType		 =
@"ObjectMessageType";
> +NSString * const ConsoleMessageTraceMessageType		 =
@"TraceMessageType";
> +NSString * const ConsoleMessageStartGroupMessageType 	 =
@"StartGroupMessageType";
> +NSString * const ConsoleMessageStartGroupCollapsedMessageType =
@"StartGroupCollapsedMessageType";
> +NSString * const ConsoleMessageEndGroupMessageType		 =
@"EndGroupMessageType";
> +NSString * const ConsoleMessageAssertMessageType		 =
@"AssertMessageType";
> +NSString * const ConsoleMessageUncaughtExceptionMessageType	 =
@"UncaughtExceptionMessageType";
> +NSString * const ConsoleMessageNetworkErrorMessageType	 =
@"NetworkErrorMessageType";
> +
> +NSString * const ConsoleMessageTipMessageLevel     = @"TipMessageLevel";
> +NSString * const ConsoleMessageLogMessageLevel     = @"LogMessageLevel";
> +NSString * const ConsoleMessageWarningMessageLevel = @"WarningMessageLevel";

> +NSString * const ConsoleMessageErrorMessageLevel   = @"ErrorMessageLevel";
> +NSString * const ConsoleMessageDebugMessageLevel   = @"DebugMessageLevel";

While const is more correct, none of the other extern NSStrings in WebKit have
const. Just drop the const.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:433
> +	   selector = @selector(webView:addMessageToConsole:);
> +	   if (![delegate respondsToSelector:selector])
> +	       return;
> +	   // The old selector only takes JSMessageSource messages.
> +	   if (source != JSMessageSource)
> +	       return;

You should flip the order here. You shouldn't ask if it responds to the
selector unless you plan to call it. That will match the old behavior.
(Checking the source first is faster too.)

> Source/WebKit/mac/WebView/WebUIDelegatePrivate.h:119
> +// Message Sources.
> +extern NSString * const ConsoleMessageHTMLMessageSource;
> +extern NSString * const ConsoleMessageWMLMessageSource;
> +extern NSString * const ConsoleMessageXMLMessageSource;
> +extern NSString * const ConsoleMessageJSMessageSource;
> +extern NSString * const ConsoleMessageCSSMessageSource;
> +extern NSString * const ConsoleMessageOtherMessageSource;
> +
> +// Message Types.
> +extern NSString * const ConsoleMessageLogMessageType;
> +extern NSString * const ConsoleMessageObjectMessageType;
> +extern NSString * const ConsoleMessageTraceMessageType;
> +extern NSString * const ConsoleMessageStartGroupMessageType;
> +extern NSString * const ConsoleMessageStartGroupCollapsedMessageType;
> +extern NSString * const ConsoleMessageEndGroupMessageType;
> +extern NSString * const ConsoleMessageAssertMessageType;
> +extern NSString * const ConsoleMessageUncaughtExceptionMessageType;
> +extern NSString * const ConsoleMessageNetworkErrorMessageType;
> +
> +// Message Levels.
> +extern NSString * const ConsoleMessageTipMessageLevel;
> +extern NSString * const ConsoleMessageLogMessageLevel;
> +extern NSString * const ConsoleMessageWarningMessageLevel;
> +extern NSString * const ConsoleMessageErrorMessageLevel;
> +extern NSString * const ConsoleMessageDebugMessageLevel;

Same as above, just drop the const.

> Source/WebKit/mac/WebView/WebUIDelegatePrivate.h:142
> +    @method webView:addMessageToConsole

This is the wrong method name.