Bug 176988

Summary: Web Inspector: REGRESSION(r222057): recording state doesn't update when changing actions
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 176936    
Bug Blocks: 173807    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Devin Rousso
Reported 2017-09-15 00:55:27 PDT
The changes in <https://webkit.org/b/176936> caused the details sidebars to first request that the recording's actions promise resolved before attempting to pull information from the context/action. In the case of the Trace panel, this was fine since it doesn't need the context to render its content. For the State panel, however, it needs to be able to access the context before `restore()` is called. Since the recording's actions promise will resolve on the next tick, the synchronous `WI.RecordingContentView.prototype._generateContentCanvas2D()` will run to completion before the State panel has a chance to do so, meaning that the State panel will access the context after it has been `restore()`d.
Attachments
Patch (9.75 KB, patch)
2017-09-15 00:57 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (11.22 KB, patch)
2017-09-15 17:43 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (11.21 KB, patch)
2017-09-15 17:44 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2017-09-15 00:57:27 PDT
Created attachment 320878 [details] Patch
Matt Baker
Comment 2 2017-09-15 13:44:12 PDT
Comment on attachment 320878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320878&action=review r=me > Source/WebInspectorUI/UserInterface/Views/RecordingStateDetailsSidebarPanel.js:33 > + this._action = NaN; Should be null instead of NaN. > Source/WebInspectorUI/UserInterface/Views/RecordingStateDetailsSidebarPanel.js:84 > + _generateDetailsCanvas2D(action, context, options = {}) A more descriptive name would be nice. How about `_populateCanvasStateDataGrid`? I think having the name be canvas-type agnostic is fine. > Source/WebInspectorUI/UserInterface/Views/RecordingTraceDetailsSidebarPanel.js:82 > + noTraceDataMessageElement.textContent = WI.UIString("No Trace Data"); What about "Call Stack Unavailable" or "No Call Stack" instead? As an aside, I think we should standardize on referring to traces/stack traces as call stacks in user-facing text. Variables names for things can still be trace, etc.
Devin Rousso
Comment 3 2017-09-15 17:40:52 PDT
Comment on attachment 320878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320878&action=review >> Source/WebInspectorUI/UserInterface/Views/RecordingStateDetailsSidebarPanel.js:84 >> + _generateDetailsCanvas2D(action, context, options = {}) > > A more descriptive name would be nice. How about `_populateCanvasStateDataGrid`? I think having the name be canvas-type agnostic is fine. In the future, we may want to display state information about WebGL contexts (such as which buffer is bound). I think that keeping this name as is makes it easier to distinguish what context type will be used when generating the content. Furthermore, this matches the naming style of RecordingNavigationSidebarPanel.
Devin Rousso
Comment 4 2017-09-15 17:43:23 PDT
Created attachment 320978 [details] Patch
Devin Rousso
Comment 5 2017-09-15 17:44:50 PDT
Created attachment 320979 [details] Patch Forgot to change bug title
WebKit Commit Bot
Comment 6 2017-09-15 19:12:20 PDT
Comment on attachment 320979 [details] Patch Clearing flags on attachment: 320979 Committed r222124: <http://trac.webkit.org/changeset/222124>
WebKit Commit Bot
Comment 7 2017-09-15 19:12:22 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 8 2017-09-27 12:25:54 PDT
<rdar://problem/34693265>
Note You need to log in before you can comment on or make changes to this bug.