Summary: | Web Inspector: Pausing with a deep call stack can be very slow, avoid eagerly generating object previews | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | buildbot, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, mattbaker, msaboff, saam | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Joseph Pecoraro
2017-06-22 00:23:12 PDT
Created attachment 313593 [details]
[PATCH] Proposed Fix
Attachment 313593 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.h:65: The parameter name "preview" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 313593 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review > LayoutTests/ChangeLog:10 > + Test the new protocol commnand `Runtime.getPreview` as well as the frontend commnand -> command > LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:12 > + Remove whitespace. Comment on attachment 313593 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review I've gotten through the tests, will continue with backend/frontend. > LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:51 > + if (callFrame.thisObject.canLoadPreview()) { Should we also assert that callFrame.thisObject.canLoadPreview()? Later it's assumed that every call frame was mapped to a Promise, and access callFrame.thisObject.preview for every frame. > LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:70 > + InspectorTest.expectThat(expectedFrame.thisValue[0] === prop.name, `'this' value should have expected property: ${expectedFrame.thisValue[0]}`); Could use InspectorTest.expectEqual here. > LayoutTests/inspector/runtime/getPreview-expected.txt:28 > +PASS: RemoteObject.updatePreview should return null for a object RemoteObject. This last result seemed strange at first, until reading the test code and seeing that it's the fake remote object. > LayoutTests/inspector/runtime/getPreview-expected.txt:46 > +Error: Could not find InjectedScript for objectId Nit: include a period at end of sentence, to match the lines above. > LayoutTests/inspector/runtime/getPreview.html:8 > + InspectorTest.debug(); Remove InspectorTest.debug(); > LayoutTests/inspector/runtime/getPreview.html:60 > + function testRemoteObject(remoteObject, type) { `type` isn't used. Was it meant to be included in test output instead of ${remoteObject.type}? Comment on attachment 313593 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review >> LayoutTests/inspector/debugger/tail-deleted-frames-this-value.html:51 >> + if (callFrame.thisObject.canLoadPreview()) { > > Should we also assert that callFrame.thisObject.canLoadPreview()? Later it's assumed that every call frame was mapped to a Promise, and access callFrame.thisObject.preview for every frame. Nope. The `thisObject` of a CallFrame could be null, or a primitive value, and I believe that is the case in this test. I'm not sure what you mean by call frame's being mapped to promises. Here we are calling `updatePreview` only on a few call frames, and wait for all of those requests to finish (having updated the RemoteObjects in place) before proceeding to loop over all call frames. >> LayoutTests/inspector/runtime/getPreview-expected.txt:28 >> +PASS: RemoteObject.updatePreview should return null for a object RemoteObject. > > This last result seemed strange at first, until reading the test code and seeing that it's the fake remote object. Ahh yea, I'll use that extra `type` argument to clarify this case! >> LayoutTests/inspector/runtime/getPreview-expected.txt:46 >> +Error: Could not find InjectedScript for objectId > > Nit: include a period at end of sentence, to match the lines above. This is just logging the exact protocol `error` value. Comment on attachment 313593 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=313593&action=review Nice work, r=me with a few nits/comments. > Source/JavaScriptCore/ChangeLog:10 > + the `thisObject` of each of the call frame's. In some cases, this could be more No apostrophe in frames. > Source/JavaScriptCore/inspector/InjectedScriptSource.js:240 > + var object = this._objectForId(parsedObjectId); Nit: The rest of the file still uses var, but why not use let for new code? > Source/JavaScriptCore/inspector/InjectedScriptSource.js:475 > + var result = evalFunction.call(object, expression, commandLineAPI); Nit: let > Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:170 > + muteConsole(); This is such a common pattern in InspectorRuntimeAgent, I wonder if it would be worth making into a RAII-style guard. |