[webkit-reviews] review granted: [Bug 177604] Web Inspector: Add Canvas tab and CanvasOverviewContentView : [Attachment 323001] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 6 14:45:10 PDT 2017


Devin Rousso <webkit at devinrousso.com> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 177604: Web Inspector: Add Canvas tab and CanvasOverviewContentView
https://bugs.webkit.org/show_bug.cgi?id=177604

Attachment 323001: Patch

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




--- Comment #22 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 323001
  --> https://bugs.webkit.org/attachment.cgi?id=323001
Patch

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

r=me.  I think that this is ready to go.  Other than the comments below, I have
one stylistic issue in that you seem to add .then()/.catch() calls to promises
in very different ways.  As an example, WI.Canvas.prototype.requestNode has the
.then() on the same line and the .catch() on a newline, whereas
WI.Canvas.prototype.requestContent has both the .then() and .catch() on
separate lines, both of which are indented.  I personally prefer the style of
WI.Canvas.prototype.requestContent, but I think we should at least be
consistent.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:240
> +		   Promise.all([getPropertyValue(object, "width"),
getPropertyValue(object, "height")]);

You need to return this value, or inline it without the braces.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:246
> +		   object.release();

This will throw an error, as `object` is defined in the previous promise.  It
seems like this could be a good place to use async-await:

    return WI.RemoteObject.resolveNode(domNode).then(async (object) => {
	let [widthObject, heightObject] = await Promise.all([
	    getPropertyValue(object, "width"),
	    getPropertyValue(object, "height"),
	]);
	let width = widthObject.value;
	let height = heightObject.value;

	widthObject.release();
	heightObject.release();
	object.release();

	return {width, height};
    });

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:107
> +	   navigationBar.addNavigationItem(new WI.DividerNavigationItem);

We should only add the WI.DividerNavigationItem if we have another
NavigationItem before the refresh button.

    if (this._recordButtonNavigationItem) {
	navigationBar.addNavigationItem(this._recordButtonNavigationItem);
	navigationBar.addNavigationItem(new WI.DividerNavigationItem);
    }

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:123
> +    layout()

Missing a `super.layout()` call.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:166
> +	       this._canvasNode = node;

This logic should only happen if the node has changed (which it shouldn't).

    this.representedObject.requestNode().then((node) => {
	console.assert(!this._canvasNode || node === this._canvasNode);
	if (node === this._canvasNode)
	    return;

	...
    });

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:144
> +	       this.setSelectedItem(pathComponent.representedObject);

NIT: you might want to also call scrollIntoViewIfNeeded on the
ContentView.element (either here or in CollectionContentView).	I would expect
that if the user manually changes the selected pathComponent, then it would
scroll accordingly.


More information about the webkit-reviews mailing list