We already show the current state, so we should also be able to show the state for previous calls to `save`. Additionally, we can also scan to find the corresponding `restore` if one exists.
Created attachment 317589 [details] Patch
Created attachment 317590 [details] [Image] After Patch is applied
What is the use case for this enhancement?
<rdar://problem/34040756>
(In reply to Matt Baker from comment #3) > What is the use case for this enhancement? It's fairly easy to mess up push/pop of transforms on the current context, at least in WebGL. I imagine it's similar for Canvas2d. Showing the actual stack is making more internal state visible to developer.
The Image shows: ▼ Current State [...] ▼ Action 14 [...] The sidebar is suddenly overloaded with data. And most of that data is old pushed stacks that I normally won't care about. How about something like: ▼ Current State [...] ▼ Context Stack ▶︎ Push (Action 99) ▶︎ Push (Action 52) ▶︎ Push (Action 14) Or even showing the pops and their balanced grayed out / strikes through: ▼ Current State [...] ▼ Context Stack ▶︎ <s>Pop (Action 91)</s> ▶︎ <s>Push (Action 77)</s> ▶︎ <s>Pop (Action 58)</s> ▶︎ <s>Push (Action 52)</s> ▶︎ Push (Action 14)
Comment on attachment 317589 [details] Patch r- based on Joe's design feedback.
Created attachment 353105 [details] Patch
Created attachment 353106 [details] [Image] After Patch is applied (In reply to Joseph Pecoraro from comment #6) > How about something like: > > ▼ Current State > [...] > > ▼ Context Stack > ▶︎ Push (Action 99) > ▶︎ Push (Action 52) > ▶︎ Push (Action 14) > I followed this idea, but only when there are saved states. If there are no saved states (e.g. just the current state), the `WI.DataGrid` gets moved outside `WI.DetailsSection` and becomes top-level. IMO, there's no reason to have a `WI.DetailsSection` for one item.
Comment on attachment 353105 [details] Patch This doesn't apply to TOT any more. If you rebase it, I'll review it.
Created attachment 353608 [details] Patch Rebase
Comment on attachment 353608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353608&action=review > Source/WebInspectorUI/UserInterface/Models/RecordingState.js:36 > + static deriveFromContext(type, context, options = {}) Nit: this should just be `fromContext`, to match our other static factory functions. > Source/WebInspectorUI/UserInterface/Models/RecordingState.js:38 > + if (type === WI.Recording.Type.Canvas2D) { Style: until another type is supported, early return to reduce indenting: if (type !== WI.Recording.Type.Canvas2D) return null; > Source/WebInspectorUI/UserInterface/Models/RecordingState.js:84 > + if (recording.type === WI.Recording.Type.Canvas2D) { Ditto. Change to an early return.
Created attachment 353784 [details] [Image] "Current State" shown under saved state On a very simple page, this is what the sidebar shows after selecting the first "save" action. Is this expected?
(In reply to Matt Baker from comment #13) > Created attachment 353784 [details] > [Image] "Current State" shown under saved state > > On a very simple page, this is what the sidebar shows after selecting the > first "save" action. Is this expected? In a way, yes 😅 When calling `save()`, it copies the current state into the list of saved states. So yes, it should be the same as the current state, but probably not with the same name 😛
Created attachment 353805 [details] Patch
Comment on attachment 353805 [details] Patch r=me, and Joe Peck. Tested on the simple demo at https://www.tutorialspoint.com/html5/canvas_states.htm. Pushing/popping saved states works just as expected; very nice!
Comment on attachment 353805 [details] Patch Clearing flags on attachment: 353805 Committed r237808: <https://trac.webkit.org/changeset/237808>
All reviewed patches have been landed. Closing bug.