Summary: | Web Inspector: show save/restore stack for recorded 2D Canvases | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | bburg, commit-queue, hi, inspector-bugzilla-changes, joepeck, mattbaker, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 174484, 190473 | ||||||||||||||||||
Bug Blocks: | 173807, 175485, 193830 | ||||||||||||||||||
Attachments: |
|
Description
Devin Rousso
2017-08-07 13:18:18 PDT
Created attachment 317589 [details]
Patch
Created attachment 317590 [details]
[Image] After Patch is applied
What is the use case for this enhancement? (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. |