Bug 108004 – [chromium] WebConsoleMessage is missing LevelDebug (chromium bug 172416)

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kevin Day]

http://code.google.com/p/chromium/issues/detail?id=172416

console.debug() causes a NOTREACHED() assert in content::RenderViewImpl::didAddMessageToConsole. 

WebCore::Console:debug calls Webcore::internalAddMessage with a MessageLevel of DebugMessageLevel. This makes it through to Webkit::ChromeClientImpl::addMessageToConsole, which casts it from WebCore::MessageLevel to WebConsoleMessage::Level.

WebCore::MessageLevel (third_party/WebKit/Source/WebCore/page/ConsoleTypes.h) is an enum of 5 levels:

enum MessageLevel {
    TipMessageLevel,
    LogMessageLevel,
    WarningMessageLevel,
    ErrorMessageLevel,
    DebugMessageLevel
};

WebConsoleMessage::Level (third_party/WebKit/Source/WebKit/chromium/public/WebConsoleMessage.h) is an enum of only 4 levels:

    enum Level {
        LevelTip,
        LevelLog,
        LevelWarning,
        LevelError
    };

[Kevin Day]

Created attachment 184858 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

Comment on attachment 184858 [details]
Patch

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

> Source/WebKit/chromium/public/WebConsoleMessage.h:44
> -        LevelError
> +        LevelError,
> +        LevelDebug

Do we have entries in AssertMachingEnums for these?

[Mike West]

We do not. We should.

Kevin, would you mind adding the necessary assertions to Source/WebKit/chromium/src/AssertMatchingEnums.cpp (and, if you're feeling generous, the other ports as well?) as part of this patch?

[Kevin Day]

(In reply to comment #3)
> (From update of attachment 184858 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184858&action=review
> 
> > Source/WebKit/chromium/public/WebConsoleMessage.h:44
> > -        LevelError
> > +        LevelError,
> > +        LevelDebug
> 
> Do we have entries in AssertMachingEnums for these?

Source/WebKit/chromium/src/AssertMatchingEnums.cpp makes no mention of message levels, or anything to do with the console.

It's also probably safe to add to this enum without ensuring it matches, since out-of-bounds enum values are already getting passed into the console renderer every time console.debug is being called. Without debug assertions it's getting silently converted to logging::LOG_VERBOSE, so this ends up being a no-op and just documenting better what's actually happening.

I'm happy to add a test if you guys want though?

[Kevin Day]

(In reply to comment #4)
> We do not. We should.
> 
> Kevin, would you mind adding the necessary assertions to Source/WebKit/chromium/src/AssertMatchingEnums.cpp (and, if you're feeling generous, the other ports as well?) as part of this patch?

Sure thing, I will update the patch in the morning after I have a chance to test it.

[Mike West]

(In reply to comment #6)
> (In reply to comment #4)
> > We do not. We should.
> > 
> > Kevin, would you mind adding the necessary assertions to Source/WebKit/chromium/src/AssertMatchingEnums.cpp (and, if you're feeling generous, the other ports as well?) as part of this patch?
> 
> Sure thing, I will update the patch in the morning after I have a chance to test it.

If dealing with the other ports ends up being more work than you signed up for (especially if they also don't have the debug-level enum), I'll take care of them. I do think that adding asserts to the Chromium port should be part of this patch, however, so I appreciate you looking into it.

Thank you!

[Kevin Day]

Created attachment 184872 [details]
Patch

[Kevin Day]

I checked for similar problems in other platforms:


blackberry:

The only place MessageLevel is referenced is in ChromeClientBlackBerry::addMessageToConsole, where that parameter is completely ignored. There is no blackberry specific enum for message levels. Blackberry also doesn't have an AssertMatchingEnums.

chromium:

Updated AssertMatchingEnums.cpp to test these enums.

efl:

The only place MessageLevel is referenced is in ChromeClientEfl::addMessageToConsole, where that parameter is completely ignored. There is no efl specific enum for message levels. efl does have an AssertMatchingEnums, but there is no webkit levels enum to add to it.

gtk:

exactly the same as efl

mac:

The only place MessageLevel is referenced is in WebChromeClient::addMessageToConsole. It is actually used here, but there is no matching enum - the only thing that's done with it is to call stringForMessageLevel.

qt:

exactly the same as efl, except that it has no AssertMatchingEnums

win:

exactly the same as efl, except that it has no AssertMatchingEnums

wince:

exactly the same as efl, except that it has no AssertMatchingEnums

wx:

MessageLevel is used in ChromeClientWx::addMessageToConsole, and is cast to static_cast<WebViewConsoleMessageLevel>(level) similar to how Chromium handles this. WebViewConsoleMessageLevel is also missing a DebugMessageLevel, which is an easy fix. I don't have a wx development environment set up though, so I'm not sure if anything higher up would need to be patched to allow for this. Nothing in WebKit/wx actually ever uses the level. It's stuffed into an m_level variable inside WebViewConsoleMessageEvent, but nothing inside WebKit uses that. I don't know if this is safe to change or not.



TL;DR: Everything except wx looks okay, but I don't have enough familiarity with wx to know if it's safe to change.

I updated my patch to include an AssertMatchingEnums test for chromium.

Comment on attachment 184872 [details]
Patch

do you have a chromium patch ready as well?

*** Bug 108039 has been marked as a duplicate of this bug. ***

(In reply to comment #10)
> (From update of attachment 184872 [details])
> do you have a chromium patch ready as well?

(if not, I have https://codereview.chromium.org/12091015)

[Adam Barth]

Comment on attachment 184872 [details]
Patch

API change LGTM

[Kevin Day]

(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 184872 [details] [details])
> > do you have a chromium patch ready as well?
> 
> (if not, I have https://codereview.chromium.org/12091015)

That is basically identical to my unsubmitted patch, so feel free to use that.

[WebKit Review Bot]

Comment on attachment 184872 [details]
Patch

Clearing flags on attachment: 184872

Committed r141006: <http://trac.webkit.org/changeset/141006>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 108123

http://trac.webkit.org/changeset/141234