Bug 178744

<rdar://problem/35374379>

Created attachment 326356 [details] [Image] Canvas sidebar - recordings

Created attachment 326357 [details] [Image] Canvas sidebar - shaders

Created attachment 326359 [details] [Image] with shader selected

This looks awesome! Considering that any Canvas could have both recordings and shader programs, is it necessary to split this into two different panels? Could we just have them both be part of the same TreeOutline?

(In reply to Devin Rousso from comment #5) > This looks awesome! Considering that any Canvas could have both recordings > and shader programs, is it necessary to split this into two different > panels? Could we just have them both be part of the same TreeOutline? I raised the same point in person with Matt. I am somewhat ambivalent as to how we do this right now, given the limited number of WebGL things that we try to show. We'll need to do something more elegant once we start showing textures, VAOs, renderbuffers, etc. A lot of this state doesn't really make sense to see outside the context of a recording, but a shader seems especially important to see even if it's not "live" on the page.

(In reply to Brian Burg from comment #6) > (In reply to Devin Rousso from comment #5) > > This looks awesome! Considering that any Canvas could have both recordings > > and shader programs, is it necessary to split this into two different > > panels? Could we just have them both be part of the same TreeOutline? > > I raised the same point in person with Matt. I am somewhat ambivalent as to > how we do this right now, given the limited number of WebGL things that we > try to show. > > We'll need to do something more elegant once we start showing textures, > VAOs, renderbuffers, etc. A lot of this state doesn't really make sense to > see outside the context of a recording, but a shader seems especially > important to see even if it's not "live" on the page. After some in person discussion with Greg as well, it may make sense to just combine the two into a single outline, and move the "+ Import Recording" button into the space occupied by the scope bar. As Brian mentioned, our WebGL story is somewhat limited currently. It would be easy to add buttons for showing only recordings/shaders to the filter bar at the bottom. This is also more consistent with the rest of the UI.

Created attachment 326522 [details] [Image] Navigation sidebar UI mockup Rough sketch of the design I've settled on, which incorporates feedback from comments and offline discussion. Overview sidebar: - Combined Recordings and Shaders in single tree - Removed Recordings/Shaders scope bar Recording sidebar: - Navigation bar with Close (X) and Export buttons. Closing the sidebar returns to the canvas overview and overview sidebar. - Child of overview navigation sidebar - Animate sidebar transition (horizontal wipe could visually reinforce the navigation hierarchy, and help to orient the user)

(In reply to Matt Baker from comment #8) > Created attachment 326522 [details] > [Image] Navigation sidebar UI mockup > > Rough sketch of the design I've settled on, which incorporates feedback from > comments and offline discussion. > > Overview sidebar: > - Combined Recordings and Shaders in single tree > - Removed Recordings/Shaders scope bar > > Recording sidebar: > - Navigation bar with Close (X) and Export buttons. Closing the sidebar > returns to the canvas overview and overview sidebar. > - Child of overview navigation sidebar > - Animate sidebar transition (horizontal wipe could visually reinforce the > navigation hierarchy, and help to orient the user) This looks great. Let's do it! :)

*** Bug 178797 has been marked as a duplicate of this bug. ***

I think this bugs covers the usability direction / improvements I'd like to see made. Overview: - No Sidebars - Click <img> preview to go into a CanvasContentView - Quick actions can stay - Labels can stay - Import CanvasContentView: - Ability to record - Canvas / Program in navigation sidebar - Recordings / Actions in navigation sidebar in a section below - Ability to Export - Detail sidebars

Created attachment 331554 [details] [IMAGE] Overview Idea

Created attachment 331555 [details] [IMAGE] CanvasContentView Idea

Created attachment 333135 [details] Patch

(In reply to Matt Baker from comment #14) > Created attachment 333135 [details] > Patch WIP patch, which makes the layout/behavior changes described by Joe's mockups. Needs some cleanup as well as change log comments, but feedback is welcome. Notes: - Replaces RecordingNavigationSidebarPanel with CanvasSidebsarPanel. - Canvases now have a detailed view, reached by clicking the canvas in the overview. - Canvases are no longer selectable from the overview. Recording can still be started from the overview, since it's super convenient, but upon starting the recording the view changes to the detailed canvas view. - Shaders are now visible in the per-Canvas sidebar. Outstanding Issues: - Imported recordings don't stick around after navigating away from the view. Joe suggested showing a "card" for each imported recording, as there aren't likely to be many of them. - Canvas tree element (CanvasSidebarPanel) should show a spinner status element when recording.

Comment on attachment 333135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333135&action=review I really like this! I think there is a lot of clean-up that could be done (I understand that this is a WIP), but I like where it's going :) A few bugs I noticed while testing: - for imported recordings, all the path components disappear, so I got "stuck" on the imported recording view with no way of getting back to the overview - when starting a recording on a canvas that already has a recording, the first recording in the canvas' ContentView is selected automatically instead of the canvas itself - along these lines, if you start a recording using the sidebar panel button in the CanvasContentView (not the overview), should we switch selection to the Canvas instead of a particular recording? That way, it is guaranteed to make the frame count and buffer usage visible. Just a thought - switching the selected recording in the sidebar causes the view to scroll so that the "Initial State" action is scrolled to the top, hiding the CanvasTreeElement and the recording picker. I think that it might be worth experimenting with having the recording view be on a separate scroll area than the rest of the sidebar, so that the CanvasTreeElement and recording picker are always visible. I could go either way on this though. - TreeOutline's virtualization logic relies upon the idea that the TreeOutline is the only element in it's parent container, so with very large recordings it might not work correctly. You would need to provide a different container element to `registerScrollVirtualizer` or add logic to ensure that scrolling elements outside of the TreeOutline are not counted. - should we add a "Select Recording" option to the recording picker that is disabled and selected by default so that it doesn't appear that a Recording is always selected? If I just want to preview the content of the Canvas, I don't think I'd want to see all the recording actions listed since I haven't selected a recording yet. - the State and Trace sidebars don't update when recording actions are selected in the NavigationSidebar - these were previously hooked up via `actionCompletedCallback`, which was passed in as part of `options` to `updateActionIndex`. The reason for this was that the State sidebar needs to be able to access the canvas preview's context _before_ any `restore` calls. I think that I designed this badly originally, and it might've just been easier to have `updateActionIndex` return a promise with any information needed by the sidebar(s) and have them update once that promise resolves. - is the plan to have ShaderProgram's be listed underneath the CanvasTreeElement as children, and to have a ShaderProgramContentView be shown when one is selected? I'm recovering from food poisoning right now, so I might've missed some obvious things. I will try to do a more in-depth review this week. > Source/WebInspectorUI/ChangeLog:112 > + (WI.CanvasSidebarPanel): I personally like adding "Navigation" or "Recording" to the SidebarPanel's class name, e.g. CanvasNavigationSidebarPanel. > Source/WebInspectorUI/UserInterface/Base/Setting.js:121 > + showImageGrid: new WI.Setting("show-image-grid", true), I'm not sure that this change should be batched into this patch. Is there a reason you want to switch this? > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.css:40 > + display: flex; Switching to `display: flex` from `display: block` seems potentially bug-prone to me. I'd rather make the element always have `display: flex` so that if we decide to add things in the future, we don't really need to worry as much about it not fitting in nicely when the canvas is recording. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:69 > + return [this._refreshButtonNavigationItem, this._showGridButtonNavigationItem]; I think some comment should be listed here explaining that in the Canvas view, we move the recording button to the sidebar, which is why it isn't here (even though it's added to the manually created NavigationBar below). > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:376 > + _updateProgressView(frameCount, bufferUsed) Since `frameCount` and `bufferUsed` are both "optional", I'd personally put them inside an `options = {}`. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:387 > + this._progressView = new WI.ProgressView; One thing I personally find a bit "jarring" is that when a recording is started, the canvas preview shifts down to accommodate the ProgressView. I think it might look better to have the ProgressView overlay (possibly with some semi-transparent background) on top of the canvas. > Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:-56 > - this.canvas = objects.find((object) => object instanceof WI.Canvas); What's the purpose of this change? I'd think that it is fine to set `this.canvas` since we wouldn't even show the sidebar if it's falsy. > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewSidebarPanel.js:26 > +WI.CanvasOverviewSidebarPanel = class CanvasOverviewSidebarPanel extends WI.HierarchicalSidebarPanel I think you forgot to add HierarchicalSidebarPanel to the diff :( > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewSidebarPanel.js:63 > + for (let object of objects) { > + if (object instanceof WI.Canvas || object instanceof WI.CanvasCollection || object instanceof WI.ShaderProgram) > + return true; > + } > + return false; You could simplify this as return objects.some((object) => object instanceof WI.Canvas || object instanceof WI.CanvasCollection || object instanceof WI.ShaderProgram); > Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:116 > + // this.updateEmptyContentPlaceholder(WI.UIString("No Recording Data")); Should this be uncommented? > Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:121 > + debugger; :) > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:164 > + // const subtitle = null; > + // this._canvasTreeOutline.appendChild(new WI.GeneralTreeElement(["recording"], recording.displayName, subtitle, recording)); Do we not want to keep recordings for canvases that no longer exist? (I realize this was probably for testing, but I'm asking more generally)

Comment on attachment 333135 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333135&action=review >> Source/WebInspectorUI/UserInterface/Base/Setting.js:121 >> + showImageGrid: new WI.Setting("show-image-grid", true), > > I'm not sure that this change should be batched into this patch. Is there a reason you want to switch this? I do think this should be the default, but you're right, it has no business in this patch. Will remove. >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:376 >> + _updateProgressView(frameCount, bufferUsed) > > Since `frameCount` and `bufferUsed` are both "optional", I'd personally put them inside an `options = {}`. I prefer this style since they're always specified together, or not at all. >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:387 >> + this._progressView = new WI.ProgressView; > > One thing I personally find a bit "jarring" is that when a recording is started, the canvas preview shifts down to accommodate the ProgressView. I think it might look better to have the ProgressView overlay (possibly with some semi-transparent background) on top of the canvas. I think you're right, but let's do that as follow-up/polish. >> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:-56 >> - this.canvas = objects.find((object) => object instanceof WI.Canvas); > > What's the purpose of this change? I'd think that it is fine to set `this.canvas` since we wouldn't even show the sidebar if it's falsy. Hmm, I think we actually want this: this.canvas = objects.find((object) => object instanceof WI.Canvas); return !!this.canvas; >> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewSidebarPanel.js:26 >> +WI.CanvasOverviewSidebarPanel = class CanvasOverviewSidebarPanel extends WI.HierarchicalSidebarPanel > > I think you forgot to add HierarchicalSidebarPanel to the diff :( This file shouldn't even be in here. My mistake. >> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:164 >> + // this._canvasTreeOutline.appendChild(new WI.GeneralTreeElement(["recording"], recording.displayName, subtitle, recording)); > > Do we not want to keep recordings for canvases that no longer exist? (I realize this was probably for testing, but I'm asking more generally) I'm not sure that we do, but I've added a FIXME comment in its place for now. We can always add it back in.

Created attachment 333254 [details] Patch

(In reply to Devin Rousso from comment #16) > Comment on attachment 333135 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333135&action=review > > I really like this! I think there is a lot of clean-up that could be done > (I understand that this is a WIP), but I like where it's going :) A few > bugs I noticed while testing: > - for imported recordings, all the path components disappear, so I got > "stuck" on the imported recording view with no way of getting back to the > overview Added an "Imported Recordings" folder tree element to the tab's outline. The folder is hidden until a recording is imported, so it won't be shown in the path component select menu. > - when starting a recording on a canvas that already has a recording, the > first recording in the canvas' ContentView is selected automatically instead > of the canvas itself > - along these lines, if you start a recording using the sidebar panel > button in the CanvasContentView (not the overview), should we switch > selection to the Canvas instead of a particular recording? That way, it is > guaranteed to make the frame count and buffer usage visible. Just a thought > - switching the selected recording in the sidebar causes the view to scroll > so that the "Initial State" action is scrolled to the top, hiding the > CanvasTreeElement and the recording picker. I think that it might be worth > experimenting with having the recording view be on a separate scroll area > than the rest of the sidebar, so that the CanvasTreeElement and recording > picker are always visible. I could go either way on this though. I think we can iterate on the sidebar UI. > - TreeOutline's virtualization logic relies upon the idea that the > TreeOutline is the only element in it's parent container, so with very large > recordings it might not work correctly. You would need to provide a > different container element to `registerScrollVirtualizer` or add logic to > ensure that scrolling elements outside of the TreeOutline are not counted. > - should we add a "Select Recording" option to the recording picker that is > disabled and selected by default so that it doesn't appear that a Recording > is always selected? If I just want to preview the content of the Canvas, I > don't think I'd want to see all the recording actions listed since I haven't > selected a recording yet. > - the State and Trace sidebars don't update when recording actions are > selected in the NavigationSidebar Fixed. I plumbed this through their `inspect()` methods, since the current RecordingAction is now included as a supplemental represented object by RecordingContentView. > - these were previously hooked up via `actionCompletedCallback`, which > was passed in as part of `options` to `updateActionIndex`. The reason for > this was that the State sidebar needs to be able to access the canvas > preview's context _before_ any `restore` calls. I think that I designed > this badly originally, and it might've just been easier to have > `updateActionIndex` return a promise with any information needed by the > sidebar(s) and have them update once that promise resolves. I think most of this is sorted out now, and it should be a bit cleaner. > - is the plan to have ShaderProgram's be listed underneath the > CanvasTreeElement as children, and to have a ShaderProgramContentView be > shown when one is selected? Yep. I'll add a screenshot.

Comment on attachment 333254 [details] Patch Attachment 333254 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6393713 New failing tests: http/wpt/resource-timing/rt-initiatorType-media.html

Created attachment 333267 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Created attachment 333335 [details] Patch

Created attachment 333338 [details] [Image] Canvas sidebar - shaders (updated)

Created attachment 333339 [details] [Image] with shader selected (updated)

Comment on attachment 333335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333335&action=review r=me. There are a few things I'd change, but in the interest of time I think we can separate them into followups. - I don't think we should default select a recording when looking at a canvas - we should hide the CanvasSidebarPanel when we are looking at the CanvasOverviewContentView, as it doesn't really add anything - an alternative to hiding it would be to show the entire tree of all Canvas (and related ShaderProgram and Recording), but this might be confusing since we "narrow" down onto a specific Canvas when it's selected - if you click on the "Overview" path component after importing a recording, all of the RecordingAction are still visible in the CanvasSidebarPanel, which doesn't make much sense - the filter bar and "Only show visual actions" button are missing for imported recordings - based on what I could gleam from setting a few breakpoints, we seem to be going through `CanvasSidebarPanel.prototype.set action` multiple times when we change actions > Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:95 > get snapshot() { return this._snapshot; } > + set snapshot(snapshot) { this._snapshot = snapshot; } Style: this should be in a separate code "block" > Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:101 > + transition: opacity 200ms ease-in-out; I'm not sure if we have an official style guide on this, but I don't think we use animations/transitions like this very often. Personally, I think it's unnecessary. > Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.css:40 > + content: url(../Images/Canvas3D.svg); This doesn't show for WebGL2, but I think that's fine for now. You could add `.webgl2` if you want, but we can do that as a followup. > Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:103 > + console.assert(treeElement, "Missing tree element for recording action.", action); I'm seeing this assertion get hit a lot. I think the reason is that `set recording` waits for `Recording.prototype.actions` to resolve, whereas this doesn't, meaning that the TreeElement most likely isn't created till the next frame. > Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:282 > + this.recording = defaultSelectedRecording; I personally think we shouldn't select a recording by default when looking at a canvas, but we can discuss that in a followup. > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:55 > + this._recordShortcut = new WI.KeyboardShortcut(null, WI.KeyboardShortcut.Key.Space, this._handleSpace.bind(this)); > + this._recordShortcut.implicitlyPreventsDefault = false; > + > + this._recordSingleFrameShortcut = new WI.KeyboardShortcut(WI.KeyboardShortcut.Modifier.Shift, WI.KeyboardShortcut.Key.Space, this._handleSpace.bind(this)); > + this._recordSingleFrameShortcut.implicitlyPreventsDefault = false; These don't look like they do anything, because there is no `CanvasSidebarPanel.prototype.get canvas` (used in `_handleSpace`). > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:228 > + let canvas = this.navigationSidebarPanel.canvas; See above (51-55). > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:43 > + this._showRecordings = showRecordings; I'm not sure that this is doing anything. No matter what I try, I am always shown the list of recordings for a given canvas. > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:458 > + this._action.snapshot = snapshot; This isn't really the intended usage of `snapshot`, but I suppose its OK. The original intention of `snapshot` was to hold an image of the canvas' content so we don't have to regenerate the actions for WebGL contexts in the frontend. Since we do regenerate each action for 2D contexts, this is never used, so I guess it's fine (additionally, WebGL contexts don't have the same concept of "state" as 2D, so it isn't really needed in the same way). Since we already use supplementalRepresentedObjects, we might just want to attach a new model object there that represents the context's state.

Comment on attachment 333335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333335&action=review >> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:101 >> + transition: opacity 200ms ease-in-out; > > I'm not sure if we have an official style guide on this, but I don't think we use animations/transitions like this very often. Personally, I think it's unnecessary. I found it better than not having it at all. FWIW, the duration is typical of other transitions in the Inspector. >> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:103 >> + console.assert(treeElement, "Missing tree element for recording action.", action); > > I'm seeing this assertion get hit a lot. I think the reason is that `set recording` waits for `Recording.prototype.actions` to resolve, whereas this doesn't, meaning that the TreeElement most likely isn't created till the next frame. Will fix as a follow-up. >> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:282 >> + this.recording = defaultSelectedRecording; > > I personally think we shouldn't select a recording by default when looking at a canvas, but we can discuss that in a followup. I agree, we should iterate on this. >> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:55 >> + this._recordSingleFrameShortcut.implicitlyPreventsDefault = false; > > These don't look like they do anything, because there is no `CanvasSidebarPanel.prototype.get canvas` (used in `_handleSpace`). Fixed. >> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:43 >> + this._showRecordings = showRecordings; > > I'm not sure that this is doing anything. No matter what I try, I am always shown the list of recordings for a given canvas. This just prevents recording TreeElements from being added to the CanvasTreeElement in the sidebar. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:458 >> + this._action.snapshot = snapshot; > > This isn't really the intended usage of `snapshot`, but I suppose its OK. The original intention of `snapshot` was to hold an image of the canvas' content so we don't have to regenerate the actions for WebGL contexts in the frontend. Since we do regenerate each action for 2D contexts, this is never used, so I guess it's fine (additionally, WebGL contexts don't have the same concept of "state" as 2D, so it isn't really needed in the same way). > > Since we already use supplementalRepresentedObjects, we might just want to attach a new model object there that represents the context's state. I agree that its overloading the snapshot property. However, it is a bit ambiguous since we're using this term elsewhere in the code when talking about 2D canvas recordings. But I think adding another supplemental represented object is a great idea!

Created attachment 333358 [details] Patch

Comment on attachment 333358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333358&action=review > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:458 > + this._action.snapshot = snapshot; One more thing I just realized, this doesn't actually capture the right information for what we want. The `snapshot` variable here is created to hold the state of the context before we apply actions to it, not after those actions have been applied. This is why we used to pass the `actionCompletedCallback`, so that the RecordingStateDetailsSidebarPanel could query the various state items on the CanvasRenderingContext2D directly. If we want to go with this approach (or a supplemental represented object), we really should get all the state values from `snapshot.context` (or pass it into the callback of `applyActions`) so that when `snapshot.context.restore()` is called we don't lose the state of the context after the actions are applied.

Comment on attachment 333358 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333358&action=review >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:458 >> + this._action.snapshot = snapshot; > > One more thing I just realized, this doesn't actually capture the right information for what we want. The `snapshot` variable here is created to hold the state of the context before we apply actions to it, not after those actions have been applied. This is why we used to pass the `actionCompletedCallback`, so that the RecordingStateDetailsSidebarPanel could query the various state items on the CanvasRenderingContext2D directly. If we want to go with this approach (or a supplemental represented object), we really should get all the state values from `snapshot.context` (or pass it into the callback of `applyActions`) so that when `snapshot.context.restore()` is called we don't lose the state of the context after the actions are applied. Oh dang! I think I've got a nice solution for this. We can have `applyActions` return the context state, captured just before the context is restored, and drop the callback. The state can hang off RecordingAction for now. Eventually it would be nice to separate out the 2D canvas rendering code in RecordingContentView into a separate class or some utility functions.

Created attachment 333406 [details] Patch

Comment on attachment 333406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333406&action=review r=me. Super awesome job! Looks great =D I really like the change to `RecordingAction` with the saved `state`. I think it's a LOT cleaner. Only thing I see that's "new" is that you're missing a ChangeLog entry for the changes to RecordingAction. > Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:158 > + this._previewImageElement.addEventListener("click", this._handlePreviewImageClick.bind(this)); Now that I think about it, is it necessary to have this listener? Doesn't CollectionContentView already attempt to show the representedObject when the selected item changes? The reason I mention this is because when I click on one of the navigation items when in Overview, it drills down into that particular canvas, meaning that it can be somewhat annoying to try to start multiple recordings at the same time. We should address that in a followup. > Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:108 > + console.assert(treeElement, "Missing tree element for recording action.", action); I'm not sure if you addressed this issue, so just in case, I am still seeing this assertion hit a lot with the new patch. > Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:159 > + // FIXME: Create tree elements/cards for recordings belonging to the removed canvas. We also need a card for imported recordings :) > Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:463 > + this._action.state = applyActions(snapshot.index, this._index); I don't think we want to reset this each time we reapply an action. It should never be different. Maybe a `console.assert` with `Object.shallowEquals` would suffice.

Created attachment 333429 [details] Patch for landing

Comment on attachment 333406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333406&action=review >> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:158 >> + this._previewImageElement.addEventListener("click", this._handlePreviewImageClick.bind(this)); > > Now that I think about it, is it necessary to have this listener? Doesn't CollectionContentView already attempt to show the representedObject when the selected item changes? The reason I mention this is because when I click on one of the navigation items when in Overview, it drills down into that particular canvas, meaning that it can be somewhat annoying to try to start multiple recordings at the same time. We should address that in a followup. Yes! I'd forgotten that with item selection turned off, this is the default CollectionContentView behavior. This doesn't address the "drilling down" issue, but it's trivial to fix. >> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:463 >> + this._action.state = applyActions(snapshot.index, this._index); > > I don't think we want to reset this each time we reapply an action. It should never be different. Maybe a `console.assert` with `Object.shallowEquals` would suffice. Good catch. Will change to: this._action = actions[this._index]; let state = applyActions(snapshot.index, this._index); console.assert(!this._action.state || Object.shallowEquals(this._action.state, state)); if (!this._action.state) this._action.state = state;

Created attachment 333430 [details] Patch for landing

Created attachment 333432 [details] Patch for landing

Comment on attachment 333432 [details] Patch for landing Clearing flags on attachment: 333432 Committed r228301: <https://trac.webkit.org/changeset/228301>

All reviewed patches have been landed. Closing bug.

*** Bug 178802 has been marked as a duplicate of this bug. ***