Bug 175283 – Web Inspector: show save/restore stack for recorded 2D Canvases

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 317589 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

What is the use case for this enhancement?

[Radar WebKit Bug Importer]

<rdar://problem/34040756>

[Blaze Burg]

(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.

[Joseph Pecoraro]

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)

[Blaze Burg]

Comment on attachment 317589 [details]
Patch

r- based on Joe's design feedback.

[Devin Rousso]

Created attachment 353105 [details]
Patch

[Devin Rousso]

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.

[Blaze Burg]

Comment on attachment 353105 [details]
Patch

This doesn't apply to TOT any more. If you rebase it, I'll review it.

[Devin Rousso]

Created attachment 353608 [details]
Patch

Rebase

[Matt Baker]

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.

[Matt Baker]

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?

[Devin Rousso]

(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 😛

[Devin Rousso]

Created attachment 353805 [details]
Patch

[Matt Baker]

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!

[WebKit Commit Bot]

Comment on attachment 353805 [details]
Patch

Clearing flags on attachment: 353805

Committed r237808: <https://trac.webkit.org/changeset/237808>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.