Bug 175283 - Web Inspector: show save/restore stack for recorded 2D Canvases
Summary: Web Inspector: show save/restore stack for recorded 2D Canvases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 174484 190473
Blocks: WebInspectorCanvasRecording WebInspectorCanvasTab 193830
  Show dependency treegraph
 
Reported: 2017-08-07 13:18 PDT by Devin Rousso
Modified: 2019-01-25 10:38 PST (History)
7 users (show)

See Also:


Attachments
Patch (19.38 KB, patch)
2017-08-08 11:54 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (449.94 KB, image/png)
2017-08-08 11:54 PDT, Devin Rousso
no flags Details
Patch (39.05 KB, patch)
2018-10-25 13:49 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (952.09 KB, image/png)
2018-10-25 13:50 PDT, Devin Rousso
no flags Details
Patch (39.23 KB, patch)
2018-11-01 09:44 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] "Current State" shown under saved state (235.26 KB, image/png)
2018-11-03 16:12 PDT, Matt Baker
no flags Details
Patch (39.06 KB, patch)
2018-11-04 12:06 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.