Bug 175283

Summary: Web Inspector: show save/restore stack for recorded 2D Canvases
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, drousso, 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 Flags
Patch
none
[Image] After Patch is applied
none
Patch
none
[Image] After Patch is applied
none
Patch
none
[Image] "Current State" shown under saved state
none
Patch none

Description Devin Rousso 2017-08-07 13:18:18 PDT
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.
Comment 1 Devin Rousso 2017-08-08 11:54:18 PDT
Created attachment 317589 [details]
Patch
Comment 2 Devin Rousso 2017-08-08 11:54:29 PDT
Created attachment 317590 [details]
[Image] After Patch is applied
Comment 3 Matt Baker 2017-08-08 12:22:29 PDT
What is the use case for this enhancement?
Comment 4 Radar WebKit Bug Importer 2017-08-23 12:11:51 PDT
<rdar://problem/34040756>
Comment 5 Brian Burg 2017-09-01 13:30:32 PDT
(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.
Comment 6 Joseph Pecoraro 2017-09-01 16:18:47 PDT
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 7 Brian Burg 2017-10-23 16:32:33 PDT
Comment on attachment 317589 [details]
Patch

r- based on Joe's design feedback.
Comment 8 Devin Rousso 2018-10-25 13:49:16 PDT
Created attachment 353105 [details]
Patch
Comment 9 Devin Rousso 2018-10-25 13:50:49 PDT
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 10 Brian Burg 2018-11-01 09:24:04 PDT
Comment on attachment 353105 [details]
Patch

This doesn't apply to TOT any more. If you rebase it, I'll review it.
Comment 11 Devin Rousso 2018-11-01 09:44:02 PDT
Created attachment 353608 [details]
Patch

Rebase
Comment 12 Matt Baker 2018-11-03 16:11:42 PDT
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.
Comment 13 Matt Baker 2018-11-03 16:12:56 PDT
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?
Comment 14 Devin Rousso 2018-11-04 11:44:10 PST
(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 😛
Comment 15 Devin Rousso 2018-11-04 12:06:06 PST
Created attachment 353805 [details]
Patch
Comment 16 Matt Baker 2018-11-04 19:34:04 PST
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 17 WebKit Commit Bot 2018-11-05 09:34:56 PST
Comment on attachment 353805 [details]
Patch

Clearing flags on attachment: 353805

Committed r237808: <https://trac.webkit.org/changeset/237808>
Comment 18 WebKit Commit Bot 2018-11-05 09:34:58 PST
All reviewed patches have been landed.  Closing bug.