Bug 191785 - Web Inspector: Include default filtering of InspectorBackend.dumpInspectorProtocolMessages with multi target backend
Summary: Web Inspector: Include default filtering of InspectorBackend.dumpInspectorPro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-16 15:35 PST by Joseph Pecoraro
Modified: 2018-11-16 17:30 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (9.78 KB, patch)
2018-11-16 15:57 PST, Joseph Pecoraro
mattbaker: review+
Details | Formatted Diff | Diff
[IMAGE] All Button States (215.57 KB, image/png)
2018-11-16 15:57 PST, Joseph Pecoraro
no flags Details
[IMAGE] Before - overwhelming logs (1.12 MB, image/png)
2018-11-16 15:57 PST, Joseph Pecoraro
no flags Details
[IMAGE] After - typical logs (1.13 MB, image/png)
2018-11-16 15:57 PST, Joseph Pecoraro
no flags Details
[PATCH] For Bots (9.77 KB, patch)
2018-11-16 16:26 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2018-11-16 15:35:20 PST
Include default filtering of InspectorBackend.dumpInspectorProtocolMessages with multi target backend

Dumping all messages will include everything twice:

  1. The agent message per-target
  2. The multi backend message Target agent message containing the above message

Since the multi backend right now really is just a multiplexer, lets just filter its messages by default.
Comment 1 Joseph Pecoraro 2018-11-16 15:57:05 PST
Created attachment 355146 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2018-11-16 15:57:19 PST
Created attachment 355147 [details]
[IMAGE] All Button States
Comment 3 Joseph Pecoraro 2018-11-16 15:57:39 PST
Created attachment 355148 [details]
[IMAGE] Before - overwhelming logs
Comment 4 Joseph Pecoraro 2018-11-16 15:57:56 PST
Created attachment 355149 [details]
[IMAGE] After - typical logs
Comment 5 Matt Baker 2018-11-16 16:23:50 PST
Comment on attachment 355146 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:45
> +    const dumpMessagesToolTip = WI.unlocalizedString("Enable dump inspector messages to console.\nShift-click to dump inspector all messages with no filtering.");

"inspector all messages" -> "all inspector messages"

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:109
> +    }

Style: we usually put the getter above the setter for read/write properties.

> Source/WebInspectorUI/UserInterface/Protocol/LoggingProtocolTracer.js:68
> +    }

Ditto.
Comment 6 Joseph Pecoraro 2018-11-16 16:25:40 PST
Comment on attachment 355146 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:45
>> +    const dumpMessagesToolTip = WI.unlocalizedString("Enable dump inspector messages to console.\nShift-click to dump inspector all messages with no filtering.");
> 
> "inspector all messages" -> "all inspector messages"

oops!

>> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:109
>> +    }
> 
> Style: we usually put the getter above the setter for read/write properties.

Yeah, these two files are oddly different and I didn't want a bunch of red in the patch =(
Comment 7 Joseph Pecoraro 2018-11-16 16:26:59 PST
Created attachment 355160 [details]
[PATCH] For Bots
Comment 8 Devin Rousso 2018-11-16 16:29:28 PST
Comment on attachment 355146 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:44
> +    const DumpMessagesState = {Off: "off", Filtering: "filtering", Everything: "everything"};

Style: this should be on separate lines

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:66
> +        case DumpMessagesState.Filtering:

Rather than "Filtering", what about "Filtered" to keep the same word type as "Off" and "Everything"?

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:76
> +            dumpMessagesToolbarItem.element.style.color = "rgb(164, 41, 154)";

I prefer using the `setProperty` function, as it makes it somewhat easier to search for CSS properties since they aren't mangled (e.g. "background-image" becomes "style.backgroundImage")

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:108
> +        return WI.settings.filterMultiplexingBackendInspectorProtocolMessages.value;

Should we bother asserting that `this._defaultTracer.filterMultiplexingBackend` has the same value as the setting?
Comment 9 Joseph Pecoraro 2018-11-16 17:29:29 PST
https://trac.webkit.org/r238331
Comment 10 Radar WebKit Bug Importer 2018-11-16 17:30:53 PST
<rdar://problem/46144912>