[webkit-reviews] review denied: [Bug 177027] Web Inspector: Enable WebKit logging configuration and display : [Attachment 321333] Rebased patch.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 177027: Web Inspector: Enable WebKit logging configuration and display
https://bugs.webkit.org/show_bug.cgi?id=177027

Attachment 321333: Rebased patch.

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




--- Comment #28 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 321333
  --> https://bugs.webkit.org/attachment.cgi?id=321333
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();
	    });
	}
    });