[webkit-reviews] review granted: [Bug 182995] Web Inspector: Canvas tab: determine hasVisibleEffect for all actions immediately after recording is added : [Attachment 339193] Patch

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 182995: Web Inspector: Canvas tab: determine hasVisibleEffect for all
actions immediately after recording is added
https://bugs.webkit.org/show_bug.cgi?id=182995

Attachment 339193: Patch

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




--- Comment #13 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 339193
  --> https://bugs.webkit.org/attachment.cgi?id=339193
Patch

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

r=me, very nice! Just a couple minor comments.

Also, I'm not sure about having two progress bars, but I'm okay with it for
now. We can revisit later if needed.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:170
> +	       function* createIteratorForActions() {

A generator isn't necessary, you can just use an array:

let items = this._actions.map((action, index) => { return {action, index} });
this._swizzleTask = new WI.YieldableTask(this, items);
this._applyTask = new WI.YieldableTask(this, items);

> Source/WebInspectorUI/UserInterface/Models/Recording.js:362
> +	  
this.dispatchEventToListeners(WI.Recording.Event.ProcessedActionApply, {index:
item.index});

Style: indent

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:283
> +	   let wasValid = this._valid;

The temporary can be eliminated:

markInvalid()
{
    if (!this._valid)
	return;

    this._valid = false;
    this.dispatchEventToListeners(...)
}

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:457
> +	       if (!this._processing)

Combine these checks:

if (this._showPathButtonNavigationItem.activated !== activated &&
!this._processing)
    this._generateContentCanvas2D(this._index);