Web Inspector: Enable WebKit logging configuration and display

[Eric Carlson]

Make it possible to configure and display WebKit runtime logging.

[Eric Carlson]

<rdar://problem/33964767>

[Eric Carlson]

Created attachment 320970 [details]
Proposed patch.

[Eric Carlson]

Created attachment 320971 [details]
Screen shot of settings panel.

[Eric Carlson]

Created attachment 320972 [details]
Screenshot of log content panel

[Build Bot]

Attachment 320970 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

I think it would be more useful to have the log channel configuration in a Console sidebar panel, but I put them in the settings panel for now.

[Eric Carlson]

Created attachment 320973 [details]
Proposed patch.

[Build Bot]

Comment on attachment 320973 [details]
Proposed patch.

Attachment 320973 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4564479

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320982 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Eric Carlson]

Created attachment 320986 [details]
Updated patch.

[Eric Carlson]

Created attachment 320987 [details]
Updated patch.

[Build Bot]

Comment on attachment 320987 [details]
Updated patch.

Attachment 320987 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4565652

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320989 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320987 [details]
Updated patch.

Attachment 320987 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4565659

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320990 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 320987 [details]
Updated patch.

Attachment 320987 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4565657

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 320992 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

In order to get rid of the blue message appearance of console.debug() messages you can remove this CSS rule from ConsoleMessageView.css:

    .console-debug-level .console-message-text {
        color: blue;
    }

The blue messages seem out of place, as if they are a higher priority, but need not be.

[Eric Carlson]

Created attachment 321314 [details]
Proposed patch.

[Build Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Eric Carlson]

Created attachment 321316 [details]
Patch with rebaselined generator test results.

[Build Bot]

Comment on attachment 321316 [details]
Patch with rebaselined generator test results.

Attachment 321316 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4604095

New failing tests:
inspector/console/webcore-logging.html

[Build Bot]

Created attachment 321321 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 321316 [details]
Patch with rebaselined generator test results.

Attachment 321316 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4604181

New failing tests:
inspector/console/webcore-logging.html

[Build Bot]

Created attachment 321323 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Eric Carlson]

Created attachment 321325 [details]
Skip the new test on Windows, Yosemite and ElCapitan.

[Eric Carlson]

Created attachment 321333 [details]
Rebased patch.

[Joseph Pecoraro]

Comment on attachment 321333 [details]
Rebased patch.

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

I have a few comments I want to see addressed before landing but most are just style nits.

I think we should find some time to articulate a clear logging level story before shipping this. I think the fewer "levels" the better. Likewise we may want to tweak the Console's performance and style for these messages, especially if they will be in bulk.

One final question is what is the persistence story for WTF Log levels? Currently setting them in Web Inspector seems like it will persist it for the Web Process it is inspecting. But every new Web Process will start with its new default log channel levels. I think that is fine. This allows both the ability to turn on logging, close inspector, do things, and open inspector and get some media logs. That said we only store up to 100 messages while inspector is closed. So the most likely scenario is "keep web inspector open and you will get all your logs".

> Source/JavaScriptCore/inspector/protocol/Console.json:10
> +            "description": "The type of rendering context backing the canvas element."

This description is wrong. Lets make it something like "Channels for different types of log messages.".

> Source/JavaScriptCore/inspector/protocol/Console.json:15
> +            "enum": ["off", "log", "error", "warning", "notice", "info", "debug"],

I think realistically we will want less levels then this. I don't think a super fined grained set is needed.

System logs on macOS (os_log) have just a few levels:
https://developer.apple.com/documentation/os/os_log_type_t?language=objc

    OS_LOG_TYPE_DEFAULT
    ... Use this level to capture information about things that might result a failure. Logging a message of this type is equivalent to calling the os_log function.
    OS_LOG_TYPE_INFO
    ... Use this level to capture information that may be helpful, but isn’t essential, for troubleshooting errors. Logging a message of this type is equivalent to calling the os_log_info function.
    OS_LOG_TYPE_DEBUG
    ... Debug logging is intended for use in a development environment and not in shipping software.
    OS_LOG_TYPE_ERROR
    ... Error-level messages are intended for reporting process-level errors. ...
    OS_LOG_TYPE_FAULT
    ... Fault-level messages are intended for capturing system-level or multi-process errors only. ...

Its precursor, syslog, has many more levels:
http://unix.superglobalmegacorp.com/Net2/newsrc/sys/syslog.h.html

    #define	LOG_EMERG	0	/* system is unusable */
    #define	LOG_ALERT	1	/* action must be taken immediately */
    #define	LOG_CRIT	2	/* critical conditions */
    #define	LOG_ERR		3	/* error conditions */
    #define	LOG_WARNING	4	/* warning conditions */
    #define	LOG_NOTICE	5	/* normal but significant condition */
    #define	LOG_INFO	6	/* informational */
    #define	LOG_DEBUG	7	/* debug-level messages */

I think we should be able to reduce the list of levels to something more user friendly set like:

    Off
    Errors (meaning error or higher)
    Warnings (meaning warning or higher)
    Verbose (meaning all)

Inside WebKit they can map to an appropriate WTF Logging Level. I don't think users will know or care to choose between "notice" and "info".

> Source/JavaScriptCore/inspector/scripts/tests/generic/expected/type-declaration-enum-type.json-result:35
> +InspectorBackend.activateDomain("Runtime");

Wow, sadly this means these tests have been broken for a very long time. I guess they aren't being run by any bots =(.

> Source/WebCore/dom/Document.cpp:7383
> +    MessageSource messageSource;
> +    if (equalIgnoringASCIICase(mediaChannel, channel.name))
> +        messageSource = MessageSource::Media;
> +    else if (equalIgnoringASCIICase(webrtcChannel, channel.name))
> +        messageSource = MessageSource::WebRTC;
> +    else {
> +        ASSERT_NOT_REACHED();
> +        return;
> +    }
> +
> +    MessageLevel messageLevel = MessageLevel::Log;
> +    switch (level) {
> +    case WTFLogLevelAlways:
> +        messageLevel = MessageLevel::Log;
> +        break;
> +    case WTFLogLevelError:
> +        messageLevel = MessageLevel::Error;
> +        break;
> +    case WTFLogLevelWarning:
> +        messageLevel = MessageLevel::Warning;
> +        break;
> +    case WTFLogLevelInfo:
> +        messageLevel = MessageLevel::Info;
> +        break;
> +    case WTFLogLevelDebug:
> +        messageLevel = MessageLevel::Debug;
> +        break;
> +    }

I'd like to see these move into static helper functions, so you'd end up with:

    auto messageSource = messageSourceForWTFLogChannel(channel);
    auto messageLevel = messageLevelForWTFLogLevel(level);

We could also fall back to MessageSource::Other in the source case, or just bail.

> Source/WebCore/inspector/WebConsoleAgent.cpp:97
> +            .setSource(channelTable[i].source)

Nit: You may be able to do: ASCIILiteral(channelTable[i].source) to avoid some work.

> Source/WebCore/inspector/WebConsoleAgent.cpp:108
> +    if (!channel)
> +        return;

In this case we should set the errorString and then return.

    if (!channel) {
        errorString = ASCIILiteral("Logging channel not found");
        return;
    }

We will be able to test this as well.

> Source/WebCore/inspector/WebConsoleAgent.cpp:125
> +        else
> +            ASSERT_NOT_REACHED();

We should treat incoming values here as untrusted code. So we it would help to move this to a helper and set an error string if not found:

    static std::optional<WTFLogLevel> logChannelForString(const String& level) { ... }

    auto logLevel = logChannelForString(channelLevel);
    if (!logLevel) {
        errorString = ASCIILiteral("Invalid logging level");
        return;
    }

    channel->level = *logLevel;

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:273
> +localizedStrings["Debugs"] = "Debugs";

Hmm, I don't see this string in the screenshots. It also feels weird.

> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:65
> +

Style: We normally drop this empty line and pack the getters close together.

> Source/WebInspectorUI/UserInterface/Models/LoggingChannel.js:40
> +    // Payload

Style: We normally have a newline after these Payload / Public comments. They delicate sections of the class.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:75
> +            new WI.ScopeBarItem(WI.LogContentView.Scopes.Infos, WI.UIString("Infos"), false, "infos", true),
> +            new WI.ScopeBarItem(WI.LogContentView.Scopes.Debugs, WI.UIString("Debugs"), false, "debugs", true),

I think if we can reduce the levels to Errors / Warnings / Verbose. Then Infos/Debugs could just be a single group named "Other" or "Verbose".

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:81
> +        this._haveLogMessages = false;

Nit: This name can be more descriptive. How about `this._hasNonDefaultLogChannelMessage`

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:407
> +        if (!this._haveLogMessages && WI.logManager.logChannelSources.includes(message._source)) {

Nit: Here use the public getter `message.source`.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:409
> +            this._haveLogMessages = true;
> +            this.dispatchEventToListeners(WI.ContentView.Event.NavigationItemsDidChange);

We can avoid dispatching the event every time we receive a message, and only do it when we have received our first non-custom log message:

    if (!this._hasNonDefaultLogChannelMessage) {
        this. _hasNonDefaultLogChannelMessage = true;
        this.dispatchEventToListeners(WI.ContentView.Event.NavigationItemsDidChange);
    }

That will greatly cut down on the amount of work when receiving a Media / WebRTC message.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:786
> +        var item = this._messageSourceBar.selectedItems[0];

Style: Use `let` instead of var. You may have to rename the other `var item` below to be different.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:1135
> +    WebRTC: "log-webrtc"

Style: We include the trailing semicolon in recent code to make future diffs nicer.

> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:118
> +

Nit: Oops!

> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:43
> +        this._element.classList.toggle("hidden", this._hidden);

Nit: Lets move this down to line 47 where the other class name is set, so they are together.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:234
> +                [ WI.LoggingChannel.Level.Off, WI.UIString("Off")],

Style: Our style for array literals is no spaces. So drop the early space in these.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:242
> +            const editorLabels = {
> +                "media": WI.UIString("Media Logging:"),

Style: Likewise these property names `media` and `webrtc` can drop the quotes.

> LayoutTests/inspector/console/webcore-logging.html:44
> +                InspectorTest.expectThat(WI.LogManager.supportsLogChannels, "Log channels supported.");

Nit: This should be calling `supportsLogChannel()` because it is not a getter!
Style: Our typical test expectation message has the word "should" in it somewhere.

So we would go with:

    InspectorTest.expectThat(WI.LogManager.supportsLogChannels(), "Log channels should be supported.").

Which will produce either:

    PASS: Log channels should be supported.
    FAIL: Log channels should be supported.

This often means if we read the FAIL line it tells us what should have happened and didn't.

> LayoutTests/inspector/console/webcore-logging.html:52
> +                    InspectorTest.expectThat(sources.indexOf(channel.source) != -1, "Log channel has known source.");

Style: expectThat(sources.includes(channel.source, "...");

> LayoutTests/inspector/console/webcore-logging.html:53
> +                    InspectorTest.expectThat(channel.level === WI.LoggingChannel.Level.Off, "Log channel disabled by default.");

Style: expectEqual(channel.level, WI.LoggingChannel.Level.Off, "...");

> LayoutTests/inspector/console/webcore-logging.html:65
> +            ConsoleAgent.clearMessages();

I think you should be able to drop this. Prior messages won't matter.

> LayoutTests/inspector/console/webcore-logging.html:70
> +                InspectorTest.assert(false, "Nothing should be logged to the console.");

Style: The new way to do this is with `InspectorTest.fail`: InspectorTest.fail("Nothing should be logged to the console.");

> LayoutTests/inspector/console/webcore-logging.html:84
> +            InspectorTest.evaluateInPage("play()");
> +            InspectorTest.evaluateInPage("pause()");

Style: We typically use template strings like `play()` instead of "play()" when the string is source code we are evaluating. It makes it easier to spot code embedded in strings and easier to write the code if it contains strings.

> LayoutTests/inspector/console/webcore-logging.html:93
> +            ConsoleAgent.clearMessages();

Ditto, this should be removable.

> LayoutTests/inspector/console/webcore-logging.html:129
> +    });

We should add tests that directly call the ConsoleAgent methods with invalid values. Search around for tests with "DOES_NOT_EXIST". So here we could test:

    suite.addTestCase({
        name: "Console.Logging.InvalidChannel",
        description: "...",
        test(resolve, reject) {
            ConsoleAgent.setLoggingChannelLevel("DOES_NOT_EXIST", WI.LoggingChannel.Level.Off, (error) => {
                if (!error) {
                    InspectorTest.fail("Should have an error with invalid channel.");
                    reject();
                    return;
                }
                InspectorTest.pass(error);
                resolve();
            });
        }
    });

    suite.addTestCase({
        name: "Console.Logging.InvalidLevel",
        description: "...",
        test(resolve, reject) {
            ConsoleAgent.setLoggingChannelLevel(WI.ConsoleMessage.MessageSource.Media, "DOES_NOT_EXIST", (error) => {
                if (!error) {
                    InspectorTest.fail("Should have an error with invalid level.");
                    reject();
                    return;
                }
                InspectorTest.pass(error);
                resolve();
            });
        }
    });

[Eric Carlson]

Comment on attachment 321333 [details]
Rebased patch.

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

>> Source/JavaScriptCore/inspector/protocol/Console.json:10
>> +            "description": "The type of rendering context backing the canvas element."
> 
> This description is wrong. Lets make it something like "Channels for different types of log messages.".

Fixed.

>> Source/JavaScriptCore/inspector/protocol/Console.json:15
>> +            "enum": ["off", "log", "error", "warning", "notice", "info", "debug"],
> 
> I think realistically we will want less levels then this. I don't think a super fined grained set is needed.
> 
> System logs on macOS (os_log) have just a few levels:
> https://developer.apple.com/documentation/os/os_log_type_t?language=objc
> 
>     OS_LOG_TYPE_DEFAULT
>     ... Use this level to capture information about things that might result a failure. Logging a message of this type is equivalent to calling the os_log function.
>     OS_LOG_TYPE_INFO
>     ... Use this level to capture information that may be helpful, but isn’t essential, for troubleshooting errors. Logging a message of this type is equivalent to calling the os_log_info function.
>     OS_LOG_TYPE_DEBUG
>     ... Debug logging is intended for use in a development environment and not in shipping software.
>     OS_LOG_TYPE_ERROR
>     ... Error-level messages are intended for reporting process-level errors. ...
>     OS_LOG_TYPE_FAULT
>     ... Fault-level messages are intended for capturing system-level or multi-process errors only. ...
> 
> Its precursor, syslog, has many more levels:
> http://unix.superglobalmegacorp.com/Net2/newsrc/sys/syslog.h.html
> 
>     #define	LOG_EMERG	0	/* system is unusable */
>     #define	LOG_ALERT	1	/* action must be taken immediately */
>     #define	LOG_CRIT	2	/* critical conditions */
>     #define	LOG_ERR		3	/* error conditions */
>     #define	LOG_WARNING	4	/* warning conditions */
>     #define	LOG_NOTICE	5	/* normal but significant condition */
>     #define	LOG_INFO	6	/* informational */
>     #define	LOG_DEBUG	7	/* debug-level messages */
> 
> I think we should be able to reduce the list of levels to something more user friendly set like:
> 
>     Off
>     Errors (meaning error or higher)
>     Warnings (meaning warning or higher)
>     Verbose (meaning all)
> 
> Inside WebKit they can map to an appropriate WTF Logging Level. I don't think users will know or care to choose between "notice" and "info".

Oops, "notice" shouldn't be there at all - I removed that WebCore logging level a couple of weeks ago.

While we should talk about the number of levels we expose, I think collapsing all non warning/error logs to a single level will make this feature unusable for most people most of the time. I became interested in this because we often wished that we had logging in public seed bug reports this year, but we can't just turn all of our existing webkit media logging into release logging because it is EXTREMELY verbose. For example I just enabled MSE logging and it generated 2200 lines when playing 10 seconds of an YouTube video. This verbosity is absolutely necessary to track down a certain class of problem, but most of the time it just gets in the way.

Here is the current mapping from WTFLogLevel to WI.LoggingChannel.Level

    WTFLogLevelError    => Error
    WTFLogLevelWarning  => Warning
    WTFLogLevelAlways   => Log
    WTFLogLevelInfo     => Info
    WTFLogLevelDebug    => Debug

HTMLMediaElement uses WTFLogLevelAlways for logging that might be useful to web development, e.g. readyState and networkState changes, play, pause, time jumps, etc. It uses WTFLogLevelInfo to log things that will help us track down bugs, for example internal state changes, method entry, etc. It uses WTFLogLevelDebug for really noisy but sometimes helpful logging like event dispatch, AVFoundation KFO notifications, etc.

We could combine Info and Debug, but I think it makes both less useful.

>> Source/JavaScriptCore/inspector/scripts/tests/generic/expected/type-declaration-enum-type.json-result:35
>> +InspectorBackend.activateDomain("Runtime");
> 
> Wow, sadly this means these tests have been broken for a very long time. I guess they aren't being run by any bots =(.

I wondered how my changes could have added "Runtime" :-)

>> Source/WebCore/dom/Document.cpp:7383
>> +    }
> 
> I'd like to see these move into static helper functions, so you'd end up with:
> 
>     auto messageSource = messageSourceForWTFLogChannel(channel);
>     auto messageLevel = messageLevelForWTFLogLevel(level);
> 
> We could also fall back to MessageSource::Other in the source case, or just bail.

Good idea, fixed.

>> Source/WebCore/inspector/WebConsoleAgent.cpp:97
>> +            .setSource(channelTable[i].source)
> 
> Nit: You may be able to do: ASCIILiteral(channelTable[i].source) to avoid some work.

The bindings generator seems to use NeverDestroyed<String> for static strings these days, so I went with:

    static const struct ChannelTable {
        NeverDestroyed<String> name;
        Inspector::Protocol::Console::ChannelSource source;
    } channelTable[] = {
        { MAKE_STATIC_STRING_IMPL("WebRTC"), Inspector::Protocol::Console::ChannelSource::WebRTC },
        { MAKE_STATIC_STRING_IMPL("Media"), Inspector::Protocol::Console::ChannelSource::Media },
    };

>> Source/WebCore/inspector/WebConsoleAgent.cpp:108
>> +        return;
> 
> In this case we should set the errorString and then return.
> 
>     if (!channel) {
>         errorString = ASCIILiteral("Logging channel not found");
>         return;
>     }
> 
> We will be able to test this as well.

Fixed.

>> Source/WebCore/inspector/WebConsoleAgent.cpp:125
>> +            ASSERT_NOT_REACHED();
> 
> We should treat incoming values here as untrusted code. So we it would help to move this to a helper and set an error string if not found:
> 
>     static std::optional<WTFLogLevel> logChannelForString(const String& level) { ... }
> 
>     auto logLevel = logChannelForString(channelLevel);
>     if (!logLevel) {
>         errorString = ASCIILiteral("Invalid logging level");
>         return;
>     }
> 
>     channel->level = *logLevel;

Good idea. We need to set both level and state, so I went with 

  static std::optional<std::pair<WTFLogChannelState, WTFLogLevel>> channelConfigurationForString(const String& levelString) { ... }

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:273
>> +localizedStrings["Debugs"] = "Debugs";
> 
> Hmm, I don't see this string in the screenshots. It also feels weird.

Oops, I pluralized the names of all of the scope buttons after I grabbed the screen shot to match the existing "Errors", "Warnings", and "Logs" buttons. I agree that it is odd, but having some singular and some plural is also a bit strange. What do you suggest?

>> Source/WebInspectorUI/UserInterface/Models/LoggingChannel.js:40
>> +    // Payload
> 
> Style: We normally have a newline after these Payload / Public comments. They delicate sections of the class.

Fixed (and please don't call my class delicate).

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:81
>> +        this._haveLogMessages = false;
> 
> Nit: This name can be more descriptive. How about `this._hasNonDefaultLogChannelMessage`

Good idea, fixed.

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:407
>> +        if (!this._haveLogMessages && WI.logManager.logChannelSources.includes(message._source)) {
> 
> Nit: Here use the public getter `message.source`.

Fixed, here and in _messageShouldBeVisible.

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:409
>> +            this.dispatchEventToListeners(WI.ContentView.Event.NavigationItemsDidChange);
> 
> We can avoid dispatching the event every time we receive a message, and only do it when we have received our first non-custom log message:
> 
>     if (!this._hasNonDefaultLogChannelMessage) {
>         this. _hasNonDefaultLogChannelMessage = true;
>         this.dispatchEventToListeners(WI.ContentView.Event.NavigationItemsDidChange);
>     }
> 
> That will greatly cut down on the amount of work when receiving a Media / WebRTC message.

That is what it does already, modulo the variable name change.

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:786
>> +        var item = this._messageSourceBar.selectedItems[0];
> 
> Style: Use `let` instead of var. You may have to rename the other `var item` below to be different.

Changed this one to selectedItem. I copied this from _scopeBarSelectionDidChange, so I updated that as well.

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:1135
>> +    WebRTC: "log-webrtc"
> 
> Style: We include the trailing semicolon in recent code to make future diffs nicer.

Fixed (assuming you meant trailing comma rather than semicolon).

>> Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:118
>> +
> 
> Nit: Oops!

:-(

>> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:43
>> +        this._element.classList.toggle("hidden", this._hidden);
> 
> Nit: Lets move this down to line 47 where the other class name is set, so they are together.

Fixed.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:234
>> +                [ WI.LoggingChannel.Level.Off, WI.UIString("Off")],
> 
> Style: Our style for array literals is no spaces. So drop the early space in these.

Fixed.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:242
>> +                "media": WI.UIString("Media Logging:"),
> 
> Style: Likewise these property names `media` and `webrtc` can drop the quotes.

Fixed.

>> LayoutTests/inspector/console/webcore-logging.html:44
>> +                InspectorTest.expectThat(WI.LogManager.supportsLogChannels, "Log channels supported.");
> 
> Nit: This should be calling `supportsLogChannel()` because it is not a getter!
> Style: Our typical test expectation message has the word "should" in it somewhere.
> 
> So we would go with:
> 
>     InspectorTest.expectThat(WI.LogManager.supportsLogChannels(), "Log channels should be supported.").
> 
> Which will produce either:
> 
>     PASS: Log channels should be supported.
>     FAIL: Log channels should be supported.
> 
> This often means if we read the FAIL line it tells us what should have happened and didn't.

Fixed.

>> LayoutTests/inspector/console/webcore-logging.html:52
>> +                    InspectorTest.expectThat(sources.indexOf(channel.source) != -1, "Log channel has known source.");
> 
> Style: expectThat(sources.includes(channel.source, "...");

Fixed.

>> LayoutTests/inspector/console/webcore-logging.html:53
>> +                    InspectorTest.expectThat(channel.level === WI.LoggingChannel.Level.Off, "Log channel disabled by default.");
> 
> Style: expectEqual(channel.level, WI.LoggingChannel.Level.Off, "...");

Fixed.

>> LayoutTests/inspector/console/webcore-logging.html:65
>> +            ConsoleAgent.clearMessages();
> 
> I think you should be able to drop this. Prior messages won't matter.

Fixed.

>> LayoutTests/inspector/console/webcore-logging.html:70
>> +                InspectorTest.assert(false, "Nothing should be logged to the console.");
> 
> Style: The new way to do this is with `InspectorTest.fail`: InspectorTest.fail("Nothing should be logged to the console.");

Fixed.

>> LayoutTests/inspector/console/webcore-logging.html:84
>> +            InspectorTest.evaluateInPage("pause()");
> 
> Style: We typically use template strings like `play()` instead of "play()" when the string is source code we are evaluating. It makes it easier to spot code embedded in strings and easier to write the code if it contains strings.

Fixed.

>> LayoutTests/inspector/console/webcore-logging.html:93
>> +            ConsoleAgent.clearMessages();
> 
> Ditto, this should be removable.

Ditto.

>> LayoutTests/inspector/console/webcore-logging.html:129
>> +    });
> 
> We should add tests that directly call the ConsoleAgent methods with invalid values. Search around for tests with "DOES_NOT_EXIST". So here we could test:
> 
>     suite.addTestCase({
>         name: "Console.Logging.InvalidChannel",
>         description: "...",
>         test(resolve, reject) {
>             ConsoleAgent.setLoggingChannelLevel("DOES_NOT_EXIST", WI.LoggingChannel.Level.Off, (error) => {
>                 if (!error) {
>                     InspectorTest.fail("Should have an error with invalid channel.");
>                     reject();
>                     return;
>                 }
>                 InspectorTest.pass(error);
>                 resolve();
>             });
>         }
>     });
> 
>     suite.addTestCase({
>         name: "Console.Logging.InvalidLevel",
>         description: "...",
>         test(resolve, reject) {
>             ConsoleAgent.setLoggingChannelLevel(WI.ConsoleMessage.MessageSource.Media, "DOES_NOT_EXIST", (error) => {
>                 if (!error) {
>                     InspectorTest.fail("Should have an error with invalid level.");
>                     reject();
>                     return;
>                 }
>                 InspectorTest.pass(error);
>                 resolve();
>             });
>         }
>     });

Fixed.

[Eric Carlson]

Created attachment 321515 [details]
Updated patch.

[Eric Carlson]

Created attachment 323668 [details]
Rebased patch.

[Eric Carlson]

Created attachment 323923 [details]
Rebased patch.

[Eric Carlson]

Created attachment 324015 [details]
Rebased patch.

[Joseph Pecoraro]

Comment on attachment 324015 [details]
Rebased patch.

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

r=me

> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:49
> +    'webrtc': 'WebRTC',  # Canvas.ContextType.webrtc

Heh this isn't Canvas. Comment should be Console.ChannelSource.webrtc

> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:42
> +            this._loggingChannelSources = [ WI.ConsoleMessage.MessageSource.Media, WI.ConsoleMessage.MessageSource.WebRTC ];

Style: Avoid leading and trailing padding spaces in the array literal.

> Source/WebInspectorUI/UserInterface/Models/LoggingChannel.js:34
> +        console.assert(level === WI.LoggingChannel.Level.Off || level === WI.LoggingChannel.Level.Always || level === WI.LoggingChannel.Level.Error || level === WI.LoggingChannel.Level.Warning || level === WI.LoggingChannel.Level.Notice || level === WI.LoggingChannel.Level.Info || level === WI.LoggingChannel.Level.Debug);

Style: A compact way to do this is:

    console.assert(Object.values(WI.LoggingChannel.Level).includes(level));

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:399
> +

Style: Extra empty line.

[Eric Carlson]

Created attachment 324586 [details]
Patch for landing.

[Eric Carlson]

Comment on attachment 324015 [details]
Rebased patch.

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

>> Source/JavaScriptCore/inspector/scripts/codegen/generator.py:49
>> +    'webrtc': 'WebRTC',  # Canvas.ContextType.webrtc
> 
> Heh this isn't Canvas. Comment should be Console.ChannelSource.webrtc

Oops, fixed.

>> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:42
>> +            this._loggingChannelSources = [ WI.ConsoleMessage.MessageSource.Media, WI.ConsoleMessage.MessageSource.WebRTC ];
> 
> Style: Avoid leading and trailing padding spaces in the array literal.

Fixed.

>> Source/WebInspectorUI/UserInterface/Models/LoggingChannel.js:34
>> +        console.assert(level === WI.LoggingChannel.Level.Off || level === WI.LoggingChannel.Level.Always || level === WI.LoggingChannel.Level.Error || level === WI.LoggingChannel.Level.Warning || level === WI.LoggingChannel.Level.Notice || level === WI.LoggingChannel.Level.Info || level === WI.LoggingChannel.Level.Debug);
> 
> Style: A compact way to do this is:
> 
>     console.assert(Object.values(WI.LoggingChannel.Level).includes(level));

Nice, fixed.

>> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:399
>> +
> 
> Style: Extra empty line.

Removed.

[WebKit Commit Bot]

Comment on attachment 324586 [details]
Patch for landing.

Rejecting attachment 324586 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 324586, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4961966

[Eric Carlson]

Created attachment 324705 [details]
Patch for landing.

[Eric Carlson]

Created attachment 324707 [details]
Updated patch for landing.

[WebKit Commit Bot]

Comment on attachment 324707 [details]
Updated patch for landing.

Rejecting attachment 324707 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 324707, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
f32b2c1dedcfed7067df10f382c103b1db0 980e46e04dc3f5f1d3226dfc81bbd9ac7fec1269 M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Total errors found: 0 in 2 files

Full output: http://webkit-queues.webkit.org/results/4974040

[WebKit Commit Bot]

Comment on attachment 324707 [details]
Updated patch for landing.

Clearing flags on attachment: 324707

Committed r223929: <https://trac.webkit.org/changeset/223929>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Plus https://trac.webkit.org/r223957 to fix a flakey test.