[webkit-reviews] review granted: [Bug 185152] Web Inspector: Canvas tab: allow recording processing to be stopped midway : [Attachment 347468] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 185152: Web Inspector: Canvas tab: allow recording processing to be stopped
midway
https://bugs.webkit.org/show_bug.cgi?id=185152

Attachment 347468: Patch

https://bugs.webkit.org/attachment.cgi?id=347468&action=review




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 347468
  --> https://bugs.webkit.org/attachment.cgi?id=347468
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Models/Recording.js:173
> +	   console.assert(!this.ready, "Cannot stop a completed process().");

Should this message be: "Cannot start an already started process()."

> Source/WebInspectorUI/UserInterface/Models/Recording.js:414
> +	   if (!this._actions[0].ready) {
> +	       this._actions[0].process(this, this._processContext);
> +	      
this.dispatchEventToListeners(WI.Recording.Event.ProcessedAction, {action:
this._actions[0], index: 0});
> +	   }

This deserves a comment, since its a special action without a frame.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:428
> +	   let cumulativeActionIndex = 0;
> +	   for (let frameIndex = 0; frameIndex < this._frames.length;
++frameIndex) {
> +	       let frame = this._frames[frameIndex];
> +
> +	       for (let actionIndex = 0; actionIndex < frame.actions.length;
++actionIndex) {
> +		   ++cumulativeActionIndex;
> +
> +		   let action = frame.actions[actionIndex];
> +		   if (action.ready)
> +		       continue;

The one sorta bad thing about this for pause + resume is that when you resume
it loops through all of the previously processed frames and actions.

You can short-circuit that, and skip frames that have been processed already:
(I'm assuming a Frame's actions is never an empty list)

	let frame = this._frames[frameIndex];
	if (frame.actions.lastValue.ready)
	    continue;

This might save some time resuming a super long recording of a canvas with many
actions (I'm imagining 3D canvases fall into this bucket).

> Source/WebInspectorUI/UserInterface/Models/Recording.js:438
> +		   if (!actionIndex)
> +		      
this.dispatchEventToListeners(WI.Recording.Event.ProcessedFrame, {frame, index:
frameIndex});

This seems backwards to me. The name indicates to me that the frame has been
processed, but here it seems to happen at index 0, so it seems to be when the
frame has started to be processed. Yes the ProcessedAction event happens after
the action has been processed. So these names appear to be inconsistent.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:443
> +		       await Promise.delay();

This is where the magic happens but its not entirely obvious. It may deserve a
comment. // yield

> Source/WebInspectorUI/UserInterface/Views/GeneralTreeElement.js:55
> +	   return this._statusElement;

Note that the other paths typically create the sub-elements if needed. Is that
needed here too, or is it expected that this may return undefined?