Bug 178185

<rdar://problem/34939862>

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

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

Looks cool.

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.

Created attachment 326262 [details] Patch

Created attachment 326266 [details] Patch Minor style and string adjustments

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

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

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: …

Created attachment 326378 [details] Patch Oops. Ran webkit-patch from the wrong directory :P

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?

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.

Created attachment 326523 [details] Patch

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.

Comment on attachment 326523 [details] Patch Clearing flags on attachment: 326523 Committed r224726: <https://trac.webkit.org/changeset/224726>

All reviewed patches have been landed. Closing bug.