[webkit-reviews] review denied: [Bug 191478] Web Inspector: Include styled target identifier in protocol logging : [Attachment 354363] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 191478: Web Inspector: Include styled target identifier in protocol logging
https://bugs.webkit.org/show_bug.cgi?id=191478

Attachment 354363: [PATCH] Proposed Fix

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




--- Comment #4 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 354363
  --> https://bugs.webkit.org/attachment.cgi?id=354363
[PATCH] Proposed Fix

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

r-, as discussed offline, you didn't include any of the changes to the
call-sites of these functions

> Source/WebInspectorUI/ChangeLog:13
> +	   (WI.LoggingProtocolTracer.prototype.set filterMultiplexingBackend):
> +	   (WI.LoggingProtocolTracer.prototype.get filterMultiplexingBackend):

These aren't part of this patch.

> Source/WebInspectorUI/UserInterface/Protocol/LoggingProtocolTracer.js:112
> +	       let connection = entry.connection;

NIT: how about destructuring these variables inside the parameters list?

> Source/WebInspectorUI/UserInterface/Protocol/LoggingProtocolTracer.js:114
> +	       this._logToConsole(`${entry.type} (${targetId}):
${JSON.stringify(entry.message)}`);

How about adding colorization when we are showing all targets?	We'd do the
default behavior of no color when only showing a single connection.