Bug 151635 - Web Inspector: control whether to collect and dump protocol messages using a WebInspector.Setting
Summary: Web Inspector: control whether to collect and dump protocol messages using a ...
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: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 137663 InspectorDebug
  Show dependency treegraph
 
Reported: 2015-11-27 17:28 PST by Brian Burg
Modified: 2015-12-17 15:54 PST (History)
8 users (show)

See Also:


Attachments
Proposed Fix (28.95 KB, patch)
2015-11-28 00:02 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (45.58 KB, patch)
2015-11-28 00:13 PST, Brian Burg
timothy: review-
Details | Formatted Diff | Diff
Proposed Fix (33.15 KB, patch)
2015-11-29 17:08 PST, Brian Burg
no flags Details | Formatted Diff | Diff
For Landing (35.51 KB, patch)
2015-12-09 13:46 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Follow-up fix (1.80 KB, patch)
2015-12-17 15:02 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2015-11-27 17:28:28 PST
The first step towards inspector^2 tooling for analyzing protocol messages. See the meta bug for some big picture ideas.
Comment 1 Radar WebKit Bug Importer 2015-11-27 17:28:41 PST
<rdar://problem/23679143>
Comment 2 Brian Burg 2015-11-28 00:02:58 PST
Created attachment 266212 [details]
Proposed Fix

May not apply yet, due to being on top of 5 patches. I don't think it depends on them outright, though.
Comment 3 Brian Burg 2015-11-28 00:13:10 PST
Created attachment 266213 [details]
Proposed Fix
Comment 4 Timothy Hatcher 2015-11-28 13:35:50 PST
Comment on attachment 266213 [details]
Proposed Fix

Wrong patch.
Comment 5 Brian Burg 2015-11-29 17:08:53 PST
Created attachment 266228 [details]
Proposed Fix
Comment 6 Timothy Hatcher 2015-12-09 13:33:11 PST
Comment on attachment 266228 [details]
Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Protocol/InspectorBackend.js:117
> +        if (this.activeTracer)
> +            this.activeTracer.logStarted();
> +        // If the custom tracer was removed and automatic tracing is enabled,
> +        // then create a new automatic tracer and install it in its place.
> +        else

This style is confusing. The comment should move under the else and add {}.
Comment 7 Brian Burg 2015-12-09 13:46:15 PST
Created attachment 267043 [details]
For Landing
Comment 8 WebKit Commit Bot 2015-12-09 14:45:42 PST
Comment on attachment 267043 [details]
For Landing

Clearing flags on attachment: 267043

Committed r193870: <http://trac.webkit.org/changeset/193870>
Comment 9 WebKit Commit Bot 2015-12-09 14:45:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Joseph Pecoraro 2015-12-14 19:08:25 PST
Comment on attachment 267043 [details]
For Landing

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

> Source/WebInspectorUI/UserInterface/Protocol/LoggingProtocolTracer.js:112
> +    logWillHandleEvent(message)
> +    {
> +        console.assert(typeof message === "string", "Must stringify messages to avoid leaking all JSON protocol messages.")
> +
> +        let entry = {type: "event", message};
> +        this._processEntry(entry);
> +    }
> +
> +    logDidHandleEvent(message, timings = null)
> +    {
> +        console.assert(typeof message === "string", "Must stringify messages to avoid leaking all JSON protocol messages.")
> +
> +        let entry = {type: "event", message};
> +        if (timings)
> +            entry.timings = Object.shallowCopy(timings);
> +
> +        this._processEntry(entry);
> +    }

This is processing the entry (logging to console) on both willHandle and didHandle. This means when dumping to the console its happening twice for each event (and response). What is the intended behavior here?
Comment 11 Brian Burg 2015-12-15 09:05:57 PST
Comment on attachment 267043 [details]
For Landing

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

>> Source/WebInspectorUI/UserInterface/Protocol/LoggingProtocolTracer.js:112
>> +    }
> 
> This is processing the entry (logging to console) on both willHandle and didHandle. This means when dumping to the console its happening twice for each event (and response). What is the intended behavior here?

The prior behavior was to log twice only if time-stats are enabled. Now it always logs twice. We could change the outer else if to be (this._dumpMessagesToConsole && !entry.timings).
Comment 12 Brian Burg 2015-12-17 15:02:09 PST
Reopening to attach new patch.
Comment 13 Brian Burg 2015-12-17 15:02:11 PST
Created attachment 267585 [details]
Follow-up fix
Comment 14 Joseph Pecoraro 2015-12-17 15:05:32 PST
Comment on attachment 267585 [details]
Follow-up fix

r=me
Comment 15 WebKit Commit Bot 2015-12-17 15:54:18 PST
Comment on attachment 267585 [details]
Follow-up fix

Clearing flags on attachment: 267585

Committed r194244: <http://trac.webkit.org/changeset/194244>
Comment 16 WebKit Commit Bot 2015-12-17 15:54:22 PST
All reviewed patches have been landed.  Closing bug.