Bug 178185 – Web Inspector: Canvas tab: show detailed status during canvas recording

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
Show detailed status during canvas recording.

Right now it's impossible to tell what's going on during a recording, or whether any data is being recorded. We should add a progress event to fire after every frame, throttled to a maximum rate (maybe 1/sec):

This will require adding an event to Canvas.json:

{
    "name": "recordingProgress",
    "parameters": [
        { "name": "canvasId", "$ref": "CanvasId" },
        { "name": "frameCount", "type": "integer" },
        { "name": "bufferUsed", "type": "number" }
    ]
}

See attached mockups for proposed UI.

[Radar WebKit Bug Importer]

<rdar://problem/34939862>

[Matt Baker]

Created attachment 323451 [details]
[Image] Waiting for frames

[Matt Baker]

Created attachment 323452 [details]
[Image] Live frame count/buffer size

[Blaze Burg]

Looks cool.

[Devin Rousso]

One thing I think we may want to do here is send the Recording::Frame protocol objects with each progress event.  This way, we can distribute the work of parsing the entire JSON structure, instead of all at once at the very end.

    {
        "name": "recordingProgress",
        "parameters": [
            { "name": "canvasId", "$ref": "CanvasId" },
            { "name": "frames", "type": "array", "items": { "$ref": "Recording.Frame" }},
            { "name": "bufferUsed", "type": "number" }
        ]
    }

Then in the frontend all we have to do is concatenate each of these calls together to pass into `_frames` on WI.Recording.  We'd also modify Recording::Recording so that it doesn't have a `frames` member.

[Devin Rousso]

Created attachment 326262 [details]
Patch

[Devin Rousso]

Created attachment 326266 [details]
Patch

Minor style and string adjustments

[Build Bot]

Comment on attachment 326266 [details]
Patch

Attachment 326266 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5141800

New failing tests:
imported/w3c/web-platform-tests/streams/readable-byte-streams/properties.serviceworker.https.html

[Build Bot]

Created attachment 326287 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Blaze Burg]

Comment on attachment 326266 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326266&action=review

r- because backend changes don't seem to be included. That would also imply new tests or improving existing ones to log the progress events. =)

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1016
> +localizedStrings["Waiting for frames..."] = "Waiting for frames...";

We typically use '…' such as in "Edit Breakpoint…" Please fix that.

> Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:50
> +    recordingProgress(canvasId, frames, bufferUsed)

You need to include the backend and protocol changes for this in the patch. And add tests...

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:271
> +        this._recordingProgressElement.textContent = WI.UIString("Waiting for frames...");

Nit: …

[Devin Rousso]

Created attachment 326378 [details]
Patch

Oops.  Ran webkit-patch from the wrong directory :P

[Blaze Burg]

Comment on attachment 326378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326378&action=review

r- for now but overall it looks good. I'd like some clarity on what bufferUsed means, and see that it's exercised in the updated tests if it is deterministic.

> Source/WebCore/ChangeLog:13
> +        After each recorded frame, fire a progress event with the recorded frames as data. This will

If this event fires for every frame, why does it include an array of frames? I prefer the array approach since it lets us use a different coalescing strategy without changing the protocol or frontend. But maybe we should adjust this comment and say something like this in the protocol documentation.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:191
> +                { "name": "bufferUsed", "type": "integer" }

This needs explanation. What is the unit of measure? is it the amount used by the reported frames, the amount used by the backend before/after releasing the frames, ??

> Source/WebCore/inspector/InspectorCanvas.cpp:118
> +    return m_frames;

Please write this as !!m_frames for clarity

> Source/WebCore/inspector/InspectorCanvas.h:65
> +    RefPtr<Inspector::Protocol::Recording::InitialState>&& releaseInitialState() WARN_UNUSED_RETURN;

What does this do?

> Source/WebCore/inspector/InspectorCanvas.h:74
> +    long bufferUsed() const { return m_bufferUsed; }

Ditto the above comment in the change log. This could use a better name, like bufferCapacityInBytes or whatever the semantics are.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:163
> +            bufferUsed,

Ditto.

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:124
> +            if (context.getImageData)

Pardon the ignorance but are these different cases intended for 2d vs webgl? Would it be clearer to switch on the context type? And then what is the last fallback for.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:139
> +    height: calc(var(--preview-height) - 1em - (2 * var(--progress-padding)));

Wow. Yo might want to translate this into English.

> LayoutTests/inspector/canvas/resources/recording-utilities.js:109
> +        let frameCount = 0;

Shouldn't we be checking the size of the frames somewhere (bufferUsed)? Or is this not deterministic enough to put in a test?

[Devin Rousso]

Comment on attachment 326378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=326378&action=review

>> Source/JavaScriptCore/inspector/protocol/Canvas.json:191
>> +                { "name": "bufferUsed", "type": "integer" }
> 
> This needs explanation. What is the unit of measure? is it the amount used by the reported frames, the amount used by the backend before/after releasing the frames, ??

This represents the total number of bytes used by the recording so far.  We default this to 100MB, but the protocol does allow the user to specify a smaller amount.

>> Source/WebCore/inspector/InspectorCanvas.h:65
>> +    RefPtr<Inspector::Protocol::Recording::InitialState>&& releaseInitialState() WARN_UNUSED_RETURN;
> 
> What does this do?

It throws a compile error if callers of this function don't save the return value =D

>> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:124
>> +            if (context.getImageData)
> 
> Pardon the ignorance but are these different cases intended for 2d vs webgl? Would it be clearer to switch on the context type? And then what is the last fallback for.

I thought about switching on the context type, but I felt that this was somewhat "safer".  I can switch it.

The last case is for if we have a WebGPU canvas, or if/when we support Bitmap contexts, so that we won't have to change this much.  It should have an assertion though, so I'll add it.

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:139
>> +    height: calc(var(--preview-height) - 1em - (2 * var(--progress-padding)));
> 
> Wow. Yo might want to translate this into English.

I wanted to keep the overall size of the CanvasContentView the same after adding the progress info so that the flexbox still looks nice.  This just subtracts from the previously set height (280px) the height of the text (1em) and the padding of the progress element (2 * --progress-padding, for top and bottom).  I'll add a comment inline.

>> LayoutTests/inspector/canvas/resources/recording-utilities.js:109
>> +        let frameCount = 0;
> 
> Shouldn't we be checking the size of the frames somewhere (bufferUsed)? Or is this not deterministic enough to put in a test?

It should be consistent, but I think it's more important that we confirm that it increases with each event rather than check the exact value.  I'll add something to the tests.

[Devin Rousso]

Created attachment 326523 [details]
Patch

[Blaze Burg]

Comment on attachment 326523 [details]
Patch

r=me. We can move forward with this. I think we might want to revisit or remove the recording size limit, since we don't actually retain everything in the backend. Let's get some user feedback, if people want longer recordings I think that it would be reasonable to remove the restriction.

[WebKit Commit Bot]

Comment on attachment 326523 [details]
Patch

Clearing flags on attachment: 326523

Committed r224726: <https://trac.webkit.org/changeset/224726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.