Created attachment 83257 [details]
[PATCH] Improve Console Messages

This patch:

  • removes the JSMessageSource restriction
  • improves the dictionary passed to UIDelegate in -webView:addMessageToConsole:
  • fixes a test with a legitimate HTML parse error
  • adds new Console messages output to affected tests (HTML/XML/Other messages)

Nit: Forgot to update the copyright in WebChromeClient.mm to include Apple 2011.

As an added bonus this might help notify developers of HTML parse errors when
writing tests. Although, in all the tests, I only encountered one which had an
HTML console warning.

Comment on attachment 83257 [details]
[PATCH] Improve Console Messages

What about adding a shouldDumpConsoleMessages? or similar to the DRT? I especially do not think we should dump console messages together with the render tree output. Alternatively, it could be part of the text dumping.

I also thought about that. Early returning unless we were in dumpAsText mode.
I don't have any strong opinions about opting in or not.

Comment on attachment 83257 [details]
[PATCH] Improve Console Messages

View in context: https://bugs.webkit.org/attachment.cgi?id=83257&action=review

r- for not having extern strings. Dashcode is also a concern that needs considered/tested.

> Source/WebCore/page/Console.cpp:146
> -    if (source == JSMessageSource)
> -        page->chrome()->client()->addMessageToConsole(source, type, level, message, lineNumber, sourceURL);
> +    page->chrome()->client()->addMessageToConsole(source, type, level, message, lineNumber, sourceURL);

The reason we did this was for Dashcode, they were originally the only client of the WebKit API. And when we enhanced the Inspector logging we didn't want to start spamming Dashcode with messages they didn't expect.

Maybe it will be fine, but another approch would be to make a new delegate call and deprecate the old one. The new delegate call gets everything, but the old one only gets JSMessageSource.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:345
> +    case HTMLMessageSource:
> +        return @"HTMLMessageSource";
> +    case WMLMessageSource:
> +        return @"WMLMessageSource";
> +    case XMLMessageSource:
> +        return @"XMLMessageSource";
> +    case JSMessageSource:
> +        return @"JSMessageSource";
> +    case CSSMessageSource:
> +        return @"CSSMessageSource";
> +    case OtherMessageSource:
> +        return @"OtherMessageSource";

These strings should be defined as "extern NSString *" in the header. And defined in the file with the same symbol.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:371
> +    case LogMessageType:
> +        return @"LogMessageType";
> +    case ObjectMessageType:
> +        return @"ObjectMessageType";
> +    case TraceMessageType:
> +        return @"TraceMessageType";
> +    case StartGroupMessageType:
> +        return @"StartGroupMessageType";
> +    case StartGroupCollapsedMessageType:
> +        return @"StartGroupCollapsedMessageType";
> +    case EndGroupMessageType:
> +        return @"EndGroupMessageType";
> +    case AssertMessageType:
> +        return @"AssertMessageType";
> +    case UncaughtExceptionMessageType:
> +        return @"UncaughtExceptionMessageType";
> +    case NetworkErrorMessageType:
> +        return @"NetworkErrorMessageType";

These strings too.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:389
> +    case TipMessageLevel:
> +        return @"TipMessageLevel";
> +    case LogMessageLevel:
> +        return @"LogMessageLevel";
> +    case WarningMessageLevel:
> +        return @"WarningMessageLevel";
> +    case ErrorMessageLevel:
> +        return @"ErrorMessageLevel";
> +    case DebugMessageLevel:
> +        return @"DebugMessageLevel";

These strings too.

> Source/WebKit/mac/WebView/WebUIDelegatePrivate.h:125
> +      <dt>MessageSource</dt> <dd>Where the message came from. HTML, XML, JavaScript, CSS, etc.</dd>
> +      <dt>MessageType</dt> <dd>Class of message. Start / End of a Group, a Log, Network related, etc.</dd>
> +      <dt>MessageLevel</dt> <dd>Severity level of the message. Tip, Log, Warning, etc.</dd>

The comments here should reference the string constants you are going to make.

Actually Safari 2 (3?) was a client too, but we don't care about that anymore.

(In reply to comment #3)
> (From update of attachment 83257 [details])
> What about adding a shouldDumpConsoleMessages? or similar to the DRT? I especially
> do not think we should dump console messages together with the render tree output.
> Alternatively, it could be part of the text dumping.

I gave this a shot, and it turns out plenty of tests already have console message output
even if they aren't dumpAsText. This includes the inspector/ tests, numerous in
svg/custom and svg/hixie. It was more than 25 tests. I think it'll be best to leave
this aspect alone or file a separate bug on that.

Created attachment 83605 [details]
[PATCH] Addressed Comments

Addressed Timothy Hatcher's comments. I added a new delegate, which doesn't filter.
The old delegate, does filter to receive just JSMessageSource messages. The two
delegates are exclusive; so if the new delegate is implemented, than it will get
called and the old delegate won't.

Comment on attachment 83605 [details]
[PATCH] Addressed Comments

Very cool.

(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 83257 [details] [details])
> > What about adding a shouldDumpConsoleMessages? or similar to the DRT? I especially
> > do not think we should dump console messages together with the render tree output.
> > Alternatively, it could be part of the text dumping.
> 
> I gave this a shot, and it turns out plenty of tests already have console message output
> even if they aren't dumpAsText. This includes the inspector/ tests, numerous in
> svg/custom and svg/hixie. It was more than 25 tests. I think it'll be best to leave
> this aspect alone or file a separate bug on that.

A follow-up bug sounds fine to handle the new test results after adding the delegate method to DRT.

(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #3)
> > > (From update of attachment 83257 [details] [details] [details])
> > > What about adding a shouldDumpConsoleMessages? or similar to the DRT? I especially
> > > do not think we should dump console messages together with the render tree output.
> > > Alternatively, it could be part of the text dumping.
> > 
> > I gave this a shot, and it turns out plenty of tests already have console message output
> > even if they aren't dumpAsText. This includes the inspector/ tests, numerous in
> > svg/custom and svg/hixie. It was more than 25 tests. I think it'll be best to leave
> > this aspect alone or file a separate bug on that.
> 
> A follow-up bug sounds fine to handle the new test results after adding the delegate method to DRT.

I misread the original message.  I guess Kenneth can comment on whether this should be fixed separately.

Comment on attachment 83605 [details]
[PATCH] Addressed Comments

Looks good to me.  I'll let Tim have the final review.

Comment on attachment 83605 [details]
[PATCH] Addressed Comments

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.

Oh, and don't line up the strings in the .mm file.

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

I modelled this after WebViewDidBeginEditingNotification in WebView.{h,mm}
which is extern const, and does line up the strings. But I'm fine with reverting
them to the prevailing style.

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

Good point, Done.

> > Source/WebKit/mac/WebView/WebUIDelegatePrivate.h:142
> > +    @method webView:addMessageToConsole
> 
> This is the wrong method name.

Arg, good catch. Fixed.

Created attachment 83676 [details]
[PATCH] Addressed Comments

Comment on attachment 83676 [details]
[PATCH] Addressed Comments

View in context: https://bugs.webkit.org/attachment.cgi?id=83676&action=review

r+ if you add the Web prefix to the strings.

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

Sorry I didn't catch this earlier, but these need a "Web" prefix.

> r+ if you add the Web prefix to the strings.

Done.

Landed r80061:
http://trac.webkit.org/changeset/80061

As expected, this produced a bunch of errors on the viewport tests. See:
https://bugs.webkit.org/show_bug.cgi?id=53707

I will land that patch now to bring this down to a more reasonable number of issues to wade through.

This broke http/tests/xmlviewer/dumpAsText/wml.xml. Looks like it got the same console message that was added to other tests. Expected?

(In reply to comment #21)
> This broke http/tests/xmlviewer/dumpAsText/wml.xml. Looks like it got the same console message that was added to other tests. Expected?

Yes, expected. I will be updating this, and any others that I can reproduce.
Unfortunately, of the 4 new issues on the Leopard bot, I can only reproduce
1 of them (this one). If all else fails I'll at least get this one updated tonight.

(In reply to comment #22)
> (In reply to comment #21)
> > This broke http/tests/xmlviewer/dumpAsText/wml.xml. 

Added new expected results in r80085:
trac.webkit.org/changeset/80085

Looks like my laptop missed that one when I created the patch.

Created attachment 84359 [details]
[PATCH] Update Viewport Tests Expected Results With New Console Messages

Since I had to back out the other change, it would be best to update
viewport expected results in the meantime.

Checked in Leopard platform specific differences in r80088:
http://trac.webkit.org/changeset/80088

Created attachment 84362 [details]
[PATCH] Update Viewport + Qt Tests Expected Results With New Console Messages

Better still, this:

  • rebaselined viewport expected results
  • produces qt platform specific results for the qt tests that produce xslt related console errors

Comment on attachment 84362 [details]
[PATCH] Update Viewport + Qt Tests Expected Results With New Console Messages

Clearing flags on attachment: 84362

Committed r80107: <http://trac.webkit.org/changeset/80107>

Joseph I hope you don't mind I landed this after talking to Ozzy, kling and rniwa. We did it because a lot of tests were failing on GTK and Qt ports.

Also see http://trac.webkit.org/changeset/80114.

(In reply to comment #29)
> Also see http://trac.webkit.org/changeset/80114.

Also related:
http://trac.webkit.org/changeset/80113
http://trac.webkit.org/changeset/80115

Sergio, Ozzy, kling and rniwa: No problem. Thanks for helping clean this up!

With these changes in, it looks like the bots are happy. I'll close move this
to resolved.