Bug 177606

<rdar://problem/34715819>

Created attachment 324326 [details] Patch

Comment on attachment 324326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324326&action=review r-, because there are bugs with the scrubber. Also, in the previous scrubber, the user was still able to scrub to the Initial State or the final action that was recorded. I think we should still provide this functionality, especially allowing the user to go to the Initial State, to help understand what happens before the first visual action or after the last one. Should we remove WI.RecordingTabContentView in its entirety? With this patch, you can still reach it via the New Tab tab. Along those lines, should we remove Canvas from Resources and simplify any related logic? > Source/WebInspectorUI/UserInterface/Models/Recording.js:36 > + this._visualActionIndices = []; I personally prefer "indexes" over "indices", but that's just me ¯\_(ツ)_/¯ > Source/WebInspectorUI/UserInterface/Models/Recording.js:178 > + get visualActionIndices() { return this._visualActionIndices; } NIT: you can move this right below `get data()`. The only reason I added an extra newline between `get data()` and `get actions()` is because `get actions()` returns a promise, but I don't think that was warranted. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:48 > + this._popover = null; > + > + this._recordingTreeOutline = new WI.TreeOutline; > + this._recordingTreeOutline.disclosureButtons = false; > + this._recordingTreeOutline.addEventListener(WI.TreeOutline.Event.SelectionDidChange, this._recordingTreeOutlineSelectionDidChange, this); Instead of using a popover, I feel like it would be simpler to just have a <select> with the different recording names. They all seem to use the same icon anyways, so the only differentiating factor is the name. Doing it with a <select> would also allow the user to select a recording with a single mouse click, instead of two. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:89 > + console.error("Canvas content not available.", this.representedObject); I'd switch this with the if statement and early bail in a negation. this._pendingContent = content; if (!this._pendingContent) { console.error("Canvas content not available.", this.representedObject); return; } this.needsLayout(); > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:260 > + if (!recording) Can you combine this with the previous if statement, or would you rather have it separate for readability? Either is fine by me. > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:138 > + position: relative; > + width: 16px; > + top: 4px; > + display: inline-block; > + content: url(../Images/Recording.svg); > + -webkit-padding-end: 4px; NIT: I'd reorder these as such. display: inline-block; position: relative; top: 4px; width: 16px; -webkit-padding-end: 4px; content: url(../Images/Recording.svg); > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:156 > + border-radius: 3px; > + background-color: var(--selected-background-color); NIT: I'd switch the order of these two properties. background-color: var(--selected-background-color); border-radius: 3px; > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:247 > + let extensionStart = filename.lastIndexOf("."); > + if (extensionStart !== -1) > + filename = filename.substring(0, extensionStart); We should create a utility for this. > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:259 > + let selectedTreeElement = this.navigationSidebarPanel.contentTreeOutline.selectedTreeElement; Is this different from `event.data.selectedElement`? If not, I think it's easier to read to use that instead. > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:276 > + console.assert(canvasTreeElement, "Missing tree element for canvas.", canvas); This variable doesn't exist. I think you mean `recording.source` instead of `canvas`. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:36 > + color: var(--text-color-gray-dark); > + margin-bottom: 10px; NIT: I'd reorder these. margin-bottom: 10px; color: var(--text-color-gray-dark); > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:50 > + max-width: 400px; > + width: 100%; NIT: I'd reorder these. width: 100%; max-width: 400px; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:55 > + border: 1px solid var(--border-color); > + border-radius: 4px; > + background-color: white; NIT: I'd reorder these. background-color: white; border: 1px solid var(--border-color); border-radius: 4px; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css:67 > + border-radius: 1px; > + background-color: var(--border-color); NIT: I'd reorder these. background-color: var(--border-color); border-radius: 1px; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:109 > if (index < 0 || index >= actions.length || index === this._index) You can remove the last check here since you add it above (104). if (index < 0 || index >= actions.length) > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:168 > + titleElement.textContent = WI.UIString("Imported Recording"); Instead of "Imported Recording", do you want to just display the name of the file that was imported? > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:178 > + this._sliderContainer.classList.add("hidden"); Merge this with line (171). this._sliderContainer.className = "slider-container hidden"; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:191 > + this._sliderElement.max = this.representedObject.visualActionIndices.length; If you only want to let the user scrub visual actions, you either need to subtract one from the length or set the min to 1. this._sliderElement.max = this.representedObject.visualActionIndices.length - 1; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:192 > + this._updateSliderValue(0); This function takes no arguments. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:521 > + let visualActionIndex = 0; This should be the index of the first visual action. let visualActionIndex = visualActionIndices[0]; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:525 > + visualActionIndex = 0; Instead of setting this value back to 0, I would expect the "end" value to be X/X (e.g. 18/18), not 0/X (e.g 0/18). visualActionIndex = visualActionIndices.lastValue; > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:529 > + this._sliderElement.value = visualActionIndex; > + this._sliderValueElement.textContent = WI.UIString("%d of %d").format(visualActionIndex + 1, visualActionIndices.length + 1); You should find the recording index of the action in the list of visual indexes. this._sliderElement.value = visualActionIndices.indexOf(visualActionIndex); this._sliderValueElement.textContent = WI.UIString("%d of %d").format(visualActionIndices.indexOf(visualActionIndex) + 1, visualActionIndices.length - 1); > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:549 > + console.assert(!isNaN(newIndex)); I think this check is unnecessary, since we are using a range input. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:550 > + this.updateActionIndex(newIndex); This value is a visual action index, not a recording action index. let indexForVisualAction = this.representedObject.visualActionIndices[newIndex]; this.updateActionIndex();

Created attachment 324443 [details] [Image] Incorrect Scrubber Text

Created attachment 324444 [details] [Recording] Test Case

Comment on attachment 324326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324326&action=review >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:48 >> + this._recordingTreeOutline.addEventListener(WI.TreeOutline.Event.SelectionDidChange, this._recordingTreeOutlineSelectionDidChange, this); > > Instead of using a popover, I feel like it would be simpler to just have a <select> with the different recording names. They all seem to use the same icon anyways, so the only differentiating factor is the name. Doing it with a <select> would also allow the user to select a recording with a single mouse click, instead of two. I like this suggestion. I had misgivings about the popover approach. This method of navigating to recording is difficult to discover. We may want to reintroduce a navigation sidebar panel for the overview, which would include a [Recordings | Shaders] scope bar. When viewing Recordings, we could have two details sections: Canvas Recordings, and Imported Recordings. When empty, the latter could show the Import button. This would be in addition to a "normal" location for the Import button, such as in the content browser's navigation bar. I think this could work. The problem is that our current navigation sidebar design doesn't support switching out panels, which would be necessary when navigating from the overview to a recording (and eventually, to a shader). I'll explore this in a follow-up. >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:89 >> + console.error("Canvas content not available.", this.representedObject); > > I'd switch this with the if statement and early bail in a negation. > > this._pendingContent = content; > if (!this._pendingContent) { > console.error("Canvas content not available.", this.representedObject); > return; > } > > this.needsLayout(); Cleaner. Will change. >> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:138 >> + -webkit-padding-end: 4px; > > NIT: I'd reorder these as such. > > display: inline-block; > position: relative; > top: 4px; > width: 16px; > -webkit-padding-end: 4px; > content: url(../Images/Recording.svg); Sure. I agree that display and position should always come first. I like putting -webkit-* properties at the end (for some reason), but don't feel too strongly about it. >> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:276 >> + console.assert(canvasTreeElement, "Missing tree element for canvas.", canvas); > > This variable doesn't exist. I think you mean `recording.source` instead of `canvas`. Oops, copypasta. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:168 >> + titleElement.textContent = WI.UIString("Imported Recording"); > > Instead of "Imported Recording", do you want to just display the name of the file that was imported? The title is the canvas display name, so I used "Imported Recording" for recordings without a canvas. That said, both canvas and recording display names appear in the navigation bar path components, so it should just be removed. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:191 >> + this._sliderElement.max = this.representedObject.visualActionIndices.length; > > If you only want to let the user scrub visual actions, you either need to subtract one from the length or set the min to 1. > > this._sliderElement.max = this.representedObject.visualActionIndices.length - 1; Isn't the existing behavior to scrub [initial state, ...visual actions]? That's the behavior in STP at least. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:521 >> + let visualActionIndex = 0; > > This should be the index of the first visual action. > > let visualActionIndex = visualActionIndices[0]; The intent was to set the scrubber to the initial state, if no visual action index is found (for whatever reason). >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:525 >> + visualActionIndex = 0; > > Instead of setting this value back to 0, I would expect the "end" value to be X/X (e.g. 18/18), not 0/X (e.g 0/18). > > visualActionIndex = visualActionIndices.lastValue; Yes, good catch!

Comment on attachment 324326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324326&action=review >>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:525 >>> + visualActionIndex = 0; >> >> Instead of setting this value back to 0, I would expect the "end" value to be X/X (e.g. 18/18), not 0/X (e.g 0/18). >> >> visualActionIndex = visualActionIndices.lastValue; > > Yes, good catch! In this case the slider value needs to be set to this._sliderElement.max, since the value is the visual action ordinal, not the underlying action index.

Created attachment 324640 [details] Patch

Created attachment 324641 [details] [Image] After Patch is applied

Comment on attachment 324640 [details] Patch General comments below. I'll file bugs about these later today. I'd prefer to just get this patch landed and address these as followups since further discussion is warranted and this patch need not be delayed any longer. We need to iterate on the navigation aspects of this tab a bit more after this lands. - It's not obvious that clicking on the text "Recordings (2)" would spawn a drop-down. Maybe it could have a dummy option of "View Recording... (2)" that is shown as text. I guess part of my confusion is that the other controls for a canvas square are in the title bar, not along the bottom. I thought this text was similar to the size info in the lower right corner. - Getting back to the overview is also not obvious. I would prefer a back button, as the ChangeLog claims that this patch adds (it doesn't afaict). An "Canvases Overview" button might also work, but is a bit out of style in our UI convention. - As you pointed out, the navigation sidebar for the overview is pretty useless. Ideally it would show all the contexts in the collection view, and currently attached resources in the 3D case (shaders, textures, render buffers, etc). Perhaps the available recordings could also be listed there as sub-items of the canvas. - The jump bar item(s) corresponding to the overview are buggy. Sometimes there are multiple, sometimes one, sometimes none. I've seen it not do anything when clicked. - I'd prefer there always be a top level "Canvases" jump bar item that's even shown when looking at the collection view. The jump bar item corresponding to the overview should say "Canvases" or "All Canvases". Right now it seems to inherit the name of the first canvas in the collection view. - Jump links in the Trace sidebar don't work, they just take me straight to the new Network Tab without selecting anything. - There's no way to collapse all / expand all for the frames. - No feedback while a recording is in progress (I know there is a bug about fixing this already). - Uncaught exception is thrown if you reload the inspected page while viewing a recording.

(In reply to Brian Burg from comment #10) > Comment on attachment 324640 [details] > Patch > > General comments below. I'll file bugs about these later today. I'd prefer > to just get this patch landed and address these as followups since further > discussion is warranted and this patch need not be delayed any longer. > > We need to iterate on the navigation aspects of this tab a bit more after > this lands. > - It's not obvious that clicking on the text "Recordings (2)" would spawn a > drop-down. Maybe it could have a dummy option of "View Recording... (2)" > that is shown as text. I guess part of my confusion is that the other > controls for a canvas square are in the title bar, not along the bottom. I > thought this text was similar to the size info in the lower right corner. Agreed. > - Getting back to the overview is also not obvious. I would prefer a back > button, as the ChangeLog claims that this patch adds (it doesn't afaict). An > "Canvases Overview" button might also work, but is a bit out of style in our > UI convention. I talked with Matt offline and we both agreed that having a back arrow in the path component doesn't make a lot of sense, especially since it is to the right of the Sidebar path component item and has a left arrow glyph in it's content. I figured that it is more along our style to allow the user to select the path component icon and have it switch canvases, but I also see that that is not ideal. I'll add it back in the meantime. You can see what was mentioned in this screenshot <https://bug-177606-attachments.webkit.org/attachment.cgi?id=324443>. > - As you pointed out, the navigation sidebar for the overview is pretty > useless. Ideally it would show all the contexts in the collection view, and > currently attached resources in the 3D case (shaders, textures, render > buffers, etc). Perhaps the available recordings could also be listed there > as sub-items of the canvas. That's the plan :) > - The jump bar item(s) corresponding to the overview are buggy. Sometimes > there are multiple, sometimes one, sometimes none. I've seen it not do > anything when clicked. Interesting. Do you have any semi-reliable reproductions? > - I'd prefer there always be a top level "Canvases" jump bar item that's > even shown when looking at the collection view. The jump bar item > corresponding to the overview should say "Canvases" or "All Canvases". Right > now it seems to inherit the name of the first canvas in the collection view. Before this patch, we had "Canvas Overview" as the first item. Perhaps we shouldn't remove that. > - Jump links in the Trace sidebar don't work, they just take me straight to > the new Network Tab without selecting anything. I just tried this and it worked just fine. Maybe this is a bug with the new Network tab? The Trace links are instances of WI.CallFrameView, so that might have a regression in it. > - There's no way to collapse all / expand all for the frames. Do you mean for all frames, or just for a particular frame? For the former, we don't have an "Expand All", but that could be added. For the latter, it should follow with the regular TreeOutline functionality in that the right/left arrows will expand/collapse the folder. Is this what you are seeing? > - No feedback while a recording is in progress (I know there is a bug about > fixing this already). > - Uncaught exception is thrown if you reload the inspected page while > viewing a recording. Hmm. I think I ran into this with RecordingTabContentView. I'll investigate.

Comment on attachment 324640 [details] Patch r=me

Created attachment 324711 [details] Patch Additional changes: - Re-added path component for returning to the Canvas Overview when viewing a recording. - Fixed uncaught exception.

Comment on attachment 324711 [details] Patch Oops. Forgot something.

Comment on attachment 324711 [details] Patch Wait, no I didn't. Misread my diff. Sorry!

Comment on attachment 324711 [details] Patch Will land manually.

Committed r223918: <https://trac.webkit.org/changeset/223918>