Bug 54926

Summary: All Console Messages should be passed to ChromeClients.
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebCore Misc.Assignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, bweinstein, commit-queue, ddkilzer, eric, joepeck, keishi, loislo, pfeldman, pmuellr, priyajeet.hora, rik, rniwa, svillar, timothy, tonyg, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 53707    
Attachments:
Description Flags
[PATCH] Improve Console Messages
timothy: review-, joepeck: commit-queue-
[PATCH] Addressed Comments
timothy: review-, joepeck: commit-queue-
[PATCH] Addressed Comments
timothy: review+, joepeck: commit-queue-
[PATCH] Update Viewport Tests Expected Results With New Console Messages
none
[PATCH] Update Viewport + Qt Tests Expected Results With New Console Messages none

Description Joseph Pecoraro 2011-02-21 19:12:28 PST
Currently only JSMessageSource messages are passed to ChromeClients.
The addMessageToConsole client method already takes in the MessageSource
type, so it should be able to handle it; but currently the only message
types sent are JS messages.

This means clients are not getting notified of HTML, XML, CSS, and other
message types that may be useful.
Comment 1 Joseph Pecoraro 2011-02-21 19:26:20 PST
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)
Comment 2 Joseph Pecoraro 2011-02-21 19:38:16 PST
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 3 Kenneth Rohde Christiansen 2011-02-21 23:15:49 PST
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.
Comment 4 Joseph Pecoraro 2011-02-22 09:46:51 PST
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 5 Timothy Hatcher 2011-02-22 10:00:32 PST
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.
Comment 6 Timothy Hatcher 2011-02-22 10:04:29 PST
Actually Safari 2 (3?) was a client too, but we don't care about that anymore.
Comment 7 Joseph Pecoraro 2011-02-23 19:30:36 PST
(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.
Comment 8 Joseph Pecoraro 2011-02-23 21:49:09 PST
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 9 Eric Seidel (no email) 2011-02-24 02:33:14 PST
Comment on attachment 83605 [details]
[PATCH] Addressed Comments

Very cool.
Comment 10 David Kilzer (:ddkilzer) 2011-02-24 09:22:44 PST
(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.
Comment 11 David Kilzer (:ddkilzer) 2011-02-24 09:25:57 PST
(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 12 David Kilzer (:ddkilzer) 2011-02-24 09:30:47 PST
Comment on attachment 83605 [details]
[PATCH] Addressed Comments

Looks good to me.  I'll let Tim have the final review.
Comment 13 Timothy Hatcher 2011-02-24 09:40:24 PST
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.
Comment 14 Timothy Hatcher 2011-02-24 09:41:46 PST
Oh, and don't line up the strings in the .mm file.
Comment 15 Joseph Pecoraro 2011-02-24 10:10:15 PST
> 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.
Comment 16 Joseph Pecoraro 2011-02-24 10:13:47 PST
Created attachment 83676 [details]
[PATCH] Addressed Comments
Comment 17 Timothy Hatcher 2011-03-01 10:27:45 PST
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.
Comment 18 Joseph Pecoraro 2011-03-01 16:22:05 PST
> r+ if you add the Web prefix to the strings.

Done.
Comment 19 Joseph Pecoraro 2011-03-01 16:22:23 PST
Landed r80061:
http://trac.webkit.org/changeset/80061
Comment 20 Joseph Pecoraro 2011-03-01 16:55:21 PST
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.
Comment 21 Tony Gentilcore 2011-03-01 17:18:26 PST
This broke http/tests/xmlviewer/dumpAsText/wml.xml. Looks like it got the same console message that was added to other tests. Expected?
Comment 22 Joseph Pecoraro 2011-03-01 18:51:09 PST
(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.
Comment 23 Joseph Pecoraro 2011-03-01 19:23:33 PST
(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.
Comment 24 Joseph Pecoraro 2011-03-01 19:44:05 PST
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.
Comment 25 Joseph Pecoraro 2011-03-01 20:22:08 PST
Checked in Leopard platform specific differences in r80088:
http://trac.webkit.org/changeset/80088
Comment 26 Joseph Pecoraro 2011-03-01 20:41:00 PST
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 27 Sergio Villar Senin 2011-03-02 03:43:16 PST
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>
Comment 28 Sergio Villar Senin 2011-03-02 03:45:35 PST
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.
Comment 29 Ryosuke Niwa 2011-03-02 04:42:15 PST
Also see http://trac.webkit.org/changeset/80114.
Comment 30 Sergio Villar Senin 2011-03-02 05:05:02 PST
(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
Comment 31 Joseph Pecoraro 2011-03-02 11:44:23 PST
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.