Bug 108004 - [chromium] WebConsoleMessage is missing LevelDebug (chromium bug 172416)
Summary: [chromium] WebConsoleMessage is missing LevelDebug (chromium bug 172416)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 108039 (view as bug list)
Depends on: 108123
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-26 00:08 PST by Kevin Day
Modified: 2013-01-30 01:40 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.46 KB, patch)
2013-01-26 00:17 PST, Kevin Day
no flags Details | Formatted Diff | Diff
Patch (3.91 KB, patch)
2013-01-26 10:52 PST, Kevin Day
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Day 2013-01-26 00:08:49 PST
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
    };
Comment 1 Kevin Day 2013-01-26 00:17:34 PST
Created attachment 184858 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-26 00:23:55 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Adam Barth 2013-01-26 00:26:24 PST
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?
Comment 4 Mike West 2013-01-26 00:42:43 PST
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?
Comment 5 Kevin Day 2013-01-26 00:44:01 PST
(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?
Comment 6 Kevin Day 2013-01-26 00:45:17 PST
(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.
Comment 7 Mike West 2013-01-26 00:53:25 PST
(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!
Comment 8 Kevin Day 2013-01-26 10:52:52 PST
Created attachment 184872 [details]
Patch
Comment 9 Kevin Day 2013-01-26 10:54:30 PST
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 10 jochen 2013-01-27 12:41:10 PST
Comment on attachment 184872 [details]
Patch

do you have a chromium patch ready as well?
Comment 11 jochen 2013-01-27 12:42:01 PST
*** Bug 108039 has been marked as a duplicate of this bug. ***
Comment 12 jochen 2013-01-27 12:42:41 PST
(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)
Comment 13 Adam Barth 2013-01-27 12:44:22 PST
Comment on attachment 184872 [details]
Patch

API change LGTM
Comment 14 Kevin Day 2013-01-27 13:48:49 PST
(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.
Comment 15 WebKit Review Bot 2013-01-28 13:49:15 PST
Comment on attachment 184872 [details]
Patch

Clearing flags on attachment: 184872

Committed r141006: <http://trac.webkit.org/changeset/141006>
Comment 16 WebKit Review Bot 2013-01-28 13:49:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2013-01-28 15:38:57 PST
Re-opened since this is blocked by bug 108123
Comment 18 jochen 2013-01-30 01:40:22 PST
http://trac.webkit.org/changeset/141234