Would be useful for JSON debugging. Possibly "Log Frame" and "Log Frame as JSON Object" context menu items.
Created attachment 305144 [details] Patch
Created attachment 305145 [details] [Image] After Patch is applied
Comment on attachment 305144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305144&action=review > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:62 > + const options = { > + objectGroup: WebInspector.RuntimeManager.ConsoleObjectGroup, > + generatePreview: true, > + }; I'm not really sure how these work, but I think I understand what these two do. Suggestions would be appreciated here as to whether any other keys (or the ones already here) are needed. objectGroup includeCommandLineAPI doNotPauseOnExceptionsAndMuteConsole returnByValue generatePreview saveResult generatePreview sourceURLAppender > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75 > + try { > + if (JSON.parse(this._data.data)) { > + contextMenu.appendItem(WebInspector.UIString("Log Frame as JSON"), () => { > + WebInspector.runtimeManager.evaluateInInspectedWindow(`JSON.parse(\`${this._data.data}\`);`, options, logResult); > + }); > + } > + } catch (error) { } This is done so that the "Log Frame as JSON" item is only visible when the data in the frame is actually JSON parsable.
Comment on attachment 305144 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305144&action=review This is great, but a few suggestions above to really tighten this up. > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:52 > + let logResult = (result, wasThrown, savedResultIndex) => { This should be handling wasThrown or guarantee it is not possible to throw. With my suggestions below, we should be able to `console.assert(!wasThrown)`. >> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:62 >> + }; > > I'm not really sure how these work, but I think I understand what these two do. Suggestions would be appreciated here as to whether any other keys (or the ones already here) are needed. > > objectGroup > includeCommandLineAPI > doNotPauseOnExceptionsAndMuteConsole > returnByValue > generatePreview > saveResult > generatePreview > sourceURLAppender • objectGroup - this is used to group RemoteObject values retained inside of the InjectedScript. The group is used to manage the lifetime of these values. If they don't get released they are essentially leaked - ConsoleObjectGroup is correct here, since you're logging to the Console its lifetime can match the console. • includeCommandLineAPI - this tells InjectedScript whether or not to include the CommandLineAPI ($$, $x, dir(), values(), $_, $0, $1, ...) for use by the expression being evaluted - You do not want it here because you are not using any of the CommandLineAPI • doNotPauseOnExceptionsAndMuteConsole - I don't really know for certain. You could pass either but since you don't expect an exception it doesn't matter • returnByValue - this specifies whether you want a RemoteObject or a JSON value back - Console expects remote objects, so that probably makes now even though you know it will be JSON. • generatePreview - this specifies whether the RemoteObject you get back previews its properties - You will want this here, we want it pretty much everywhere • saveResult - this specifies whether or not the result will get a $n ($1, $2, ...) saved result index - You will want this here (I'm surprised your screenshots show $n but this code doesn't have this?!) • sourceURLAppender - this is used by the frontend, if it wanted to inject a sourceURL into the expression (e.g. to hide it in inspector, or treat it specially) - You will not want this here. By default you will get the `appendWebInspectorSourceURL` appender which is what you want for any non-user-typed evaluation we perform in the page So I'd expect: options = { objectGroup: WebInspector.RuntimeManager.ConsoleObjectGroup, generatePreview: true, saveResult: true, } Maybe the doNotPauseOnExceptionsAndMuteConsole, because you are evaluating `JSON.parse(...)` and the Page may have replaced JSON.parse. > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:67 > + contextMenu.appendItem(WebInspector.UIString("Log Frame"), () => { > + WebInspector.runtimeManager.evaluateInInspectedWindow(`\"${this._data.data.escapeCharacters("\"")}\"`, options, logResult); > + }); Heh, this is more complex than it needs to be: 1. You know its a String and you have the string. Trying to evaluate the string is error prone and unnecessary. Primitives are the same everywhere so just having the string is enough. Evaluating in the page and getting back the same string is not particularly useful. 2. You want a $n value for this to be useful in the console. You can use Runtime.saveResult to get it without doing evaluateInInspectedWindow. 3. escapeCharacters is hacky. JSON.stringify would get you what you want and be faster. So, I'd suggest: let remoteObject = WebInspector.RemoteObject.fromPrimitiveValue(this._data.data); contextMenu.appendItem(WebInspector.UIString("Log Frame"), () => { WebInspector.runtimeManager.saveResult(remoteObject, (savedResultIndex) => { logResult(remoteObject, false, savedResultIndex); }); }); This is more direct (you are getting the $n for the value), less traffic (sends the string one way instead of two ways), and simpler (you aren't trying to escape the string on your own). > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:74 > + if (JSON.parse(this._data.data)) { > + contextMenu.appendItem(WebInspector.UIString("Log Frame as JSON"), () => { > + WebInspector.runtimeManager.evaluateInInspectedWindow(`JSON.parse(\`${this._data.data}\`);`, options, logResult); > + }); > + } Clever! I like this. A couple problems: 1. The page may have overridden JSON.parse and would throw an exception: `JSON.parse = () => { throw "hahaha"; }` and right now Web Inspector would get a "wasThrown = true" result which is not handled above. 2. This is not escaping any backpacks in the data. So if the data was '{"message": "The ` character is crazy"}' you would get a "wasThrown = true" result which is not handled above: >>> JSON.parse(`{"message": "The ` character is crazy"}`) Unexpected identifier 'character'. Expected ')' to end an argument list.:1 I think you can avoid both of these problems with another clever trick: try { if (JSON.parse(this._data.data)) { contextMenu.appendItem(WebInspector.UIString("Log Frame as JSON"), () => { let expression = "(" + this._data.data + ")"; WebInspector.runtimeManager.evaluateInInspectedWindow(expression, options, logResult); }); } } catch (error) { } For the message above, we are essentially doing this: >>> ({"message": "The ` character is crazy"}) [object Object] This works because: 1. We have already done JSON.parse in a trusted context (inside the frontend page) and verified that the result is a valid JSON value. So we know whatever the string is, its contents are some JSON literal and not unsafe to evaluate. 2. By wrapping in ()s we force the JavaScript parser to treat this as an expression + value and not a program. This avoids the `{a:1}` being parsed as a program with a label "a" and statement "1". - Here we are using the language built-ins without opportunity for the page to override anything (like JSON.parse) to construct our object. So we've avoided all of the problems above. NOTE: In fact, evaluateInInspectedWindow would have done this for us without modifying the string due to Console Conveniences but we should be explicit here and do the ()s ourselves. >> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75 >> + } catch (error) { } > > This is done so that the "Log Frame as JSON" item is only visible when the data in the frame is actually JSON parsable. Some suggestions: • "Log as String", "Log as Object" • "Log Frame Text", "Log Value" • "Log Frame Text", "Log Number|String|Object..." based on the typeof JSON.parse(...)
Created attachment 305263 [details] Patch
Comment on attachment 305263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305263&action=review r=me. I'm still iffy on the strings. I think we may want to discuss them. But the patch looks great otherwise. > Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:127 > - node = new WebInspector.WebSocketDataGridNode({data, time, isOutgoing}); > + node = new WebInspector.WebSocketDataGridNode(Object.shallowMerge({data, time}, attributes)); > else > - node = new WebInspector.WebSocketDataGridNode({data, isOutgoing}); > + node = new WebInspector.WebSocketDataGridNode(Object.shallowMerge({data}, attributes)); I think we can use Object.assign instead of Object.shallowMerge. I think using builtins might be more natural here. What do you think? > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:66 > + const wasThrown = false; > + logResult(remoteObject, wasThrown, savedResultIndex); Style: I don't actually think the const variable here, since we are calling a local function so close by that its easy to check. > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:72 > + let json = JSON.parse(this._data.data); > + if (json) { This doesn't allow for valid falsey values (undefined, 0, null, ""). Maybe we don't care about those, but it would be odd to allow `true` but not `false`. > Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75 > + if (Array.isArray(json)) > + title = WebInspector.UIString("Log Frame as Array"); Could handle type "boolean". The JSON site has a super simple at a glance diagram: <http://www.json.org> How do all of these customizations feel? Is just `Log Value` enough or do these customizations make it feel nicer?
Comment on attachment 305263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305263&action=review >> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:127 >> + node = new WebInspector.WebSocketDataGridNode(Object.shallowMerge({data}, attributes)); > > I think we can use Object.assign instead of Object.shallowMerge. I think using builtins might be more natural here. What do you think? Personally, I prefer Object.shallowMerge for two reasons: 1. It doesn't modify either of the objects being passed in. I realized that this doesn't matter here, but I tend to try to avoid side effects of parameters. 2. It will assert when both objects have the same key with different values. Object.assign will just use the key from the last object. Again, I realize that this probably won't happen here, but it might in the future and could present a debugging annoyance. >> Source/WebInspectorUI/UserInterface/Views/WebSocketDataGridNode.js:75 >> + title = WebInspector.UIString("Log Frame as Array"); > > Could handle type "boolean". The JSON site has a super simple at a glance diagram: <http://www.json.org> > > How do all of these customizations feel? Is just `Log Value` enough or do these customizations make it feel nicer? I think just "Log Value" is enough. It could get confusing otherwise.
Created attachment 305310 [details] Patch
Comment on attachment 305310 [details] Patch Attachment 305310 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3405332 New failing tests: http/tests/preload/single_download_preload.html
Created attachment 305319 [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
Comment on attachment 305310 [details] Patch Clearing flags on attachment: 305310 Committed r214371: <http://trac.webkit.org/changeset/214371>
All reviewed patches have been landed. Closing bug.