[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