Bug 177604 – Web Inspector: Add Canvas tab and CanvasOverviewContentView

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
Add Canvas tab and CanvasOverviewContentView.

This patch:
- Adds an experimental setting for showing the Canvas tab
- Adds CanvasTabContentView, which manages a CanvasCollection and overview
- Adds CanvasOverviewContentView, which shows a grid of canvas "cards"

Notes:
Although recording controls are enabled, recordings will continue to be shown in the RecordingTabContentView. Support for browsing recordings from within the canvas tab will be added in a subsequent patch.

[Radar WebKit Bug Importer]

<rdar://problem/34714650>

[Matt Baker]

Created attachment 322089 [details]
[Image] Experimental setting

[Matt Baker]

Created attachment 322090 [details]
[Image] New Tab - Canvas

[Matt Baker]

Created attachment 322092 [details]
[Image] Canvas Overview UI

Canvas overview, with a selection.

Notes:
- The selected canvas is the supplemental represented object for the view. This handled generically by CollectionContentView.
- The selected canvas is indicated by a darker border (this is very subtle and should be improved).
- The selected canvas gets a path component in the overview's navigation bar. This aids navigation when not all canvases can fit in the overview at once. It will also help to reinforces the view context when browsing of recordings is added.
- The selected canvas is the represented object for purposes of showing the details sidebar.

[Matt Baker]

Created attachment 322097 [details]
Patch

[Devin Rousso]

Comment on attachment 322097 [details]
Patch

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

I'm about to head to a class, so I haven't had a chance to run this locally.  Is there a reason you are switching many of these functions to use promises?  Otherwise, mostly NITs.

> Source/WebInspectorUI/ChangeLog:21
> +        Fix `ensureDocument`, which is asynchronous but has never provided a way

This functionality already exists inside of `requestDocument`.  The idea behind `ensureDocument` is that we want to make sure the document node is tracked by the frontend, but we don't explicitly need the document DOMNode itself.  Since messages are queued to/from the backend, `requestDocument` is going to be evaluated before any other `requestNode` call, meaning that the frontend will receive the information about the document DOMNode before the callback to `requestNode` is executed.

> Source/WebInspectorUI/ChangeLog:168
> +        to give represented objects a change to load.

Typo: "change" -> "chance"

> Source/WebInspectorUI/ChangeLog:172
> +        Make sure ContentView.prototype.shown/hidden are called.

NIT: i'd add a newline after this to make it easier to read.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:154
> +                CanvasAgent.requestNode(this._identifier).then(({nodeId}) => {

I personally prefer to use the callback approach in situations where we aren't chaining protocol functions.  I think it's much clearer to read.

    CanvasAgent.requestNode(this._identifier, (error, nodeId) => {
        this._domNode = WI.domTreeManager.nodeForId(nodeId);
        resolve(this._domNode);
    });

Also, in the case that there is an error, should we return `null`?  I realize that all canvas contexts must have an associated Node, but (thinking forward) I don't think things like OffscreenCanvas have a node, so they would have an error.

<https://html.spec.whatwg.org/multipage/scripting.html#the-offscreencanvas-interface>

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:164
> +        return CanvasAgent.requestContent(this._identifier).then((result) => {

Style: you can inline this callback

    return CanvasAgent.requestContent(this._identifier)
        .then((result) => result.content)
        .catch((error) => console.error(error));

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:184
> +            CanvasAgent.requestCSSCanvasClientNodes(this._identifier).then((clientNodeIds) => {

Ditto (154).

    CanvasAgent.requestCSSCanvasClientNodes(this._identifier, (error, clientNodeIds) => {
        if (error) {
            callback(null);
            return;
        }

        clientNodeIds = Array.isArray(clientNodeIds) ? clientNodeIds : [];
        this._cssCanvasClientNodes = clientNodeIds.map((clientNodeId) => WI.domTreeManager.nodeForId(clientNodeId));
        callback(this._cssCanvasClientNodes);
    });

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:206
> +            let width = getAttributeValue("width");
> +            let height = getAttributeValue("height");
> +            return {width, height};

Style: I'd just inline these inside the object:

    return {
        width: getAttributeValue("width"),
        height: getAttributeValue("height"),
    };

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:222
> +            }).then((results) => {

Is it really necessary to have this additional `then()`?  Can you merge the two `then()` into one?

> Source/WebInspectorUI/UserInterface/Models/CollectionTypes.js:30
> +        super((item) => item instanceof WI.Canvas);

This already exists as `WI.Collection.TypeVerifier.Canvas`.

> Source/WebInspectorUI/UserInterface/Models/CollectionTypes.js:32
> +        canvases.forEach(this.add.bind(this));

I think our style is to use for...of for these cases:

    for (let canvas of canvases)
        this.add(canvas);

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:91
> +        titles.className = "titles";

Use `classList.add` instead of `className`.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:94
> +        title.className = "title";

Ditto (91).

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:98
> +        subtitle.className = "subtitle";

Ditto (91).

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:109
> +        this._previewContainerElement.className = "preview";

Ditto (91).

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:114
> +        this._pixelSizeElement.className = "pixel-size";

Ditto (91).

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:116
> +        this._memoryCostElement.className = "memory-cost";

Ditto (91).

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:128
> +        if (!this._pendingContent)

Do we want to show a spinner/loading-indicator?  Also, for WebGPU contexts, we currently are unable to retrieve the content.  I realize that WebGPU is still "experimental", but we might want to do at least do something simple.

<https://webkit.org/b/173621> Web Inspector: Support getting the content of WebGPU contexts

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:271
> +            this._pixelSizeElement.textContent = `${this._pixelSize.width} Ã ${this._pixelSize.height}`;

Do we want to use the `multiplicationSign`?

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:38
> +    flex-grow: 0 !important;

Is this "!important" required?

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:85
> +    content: " â ";

I think it's easier to read if you put the hex code instead of the character itself.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:38
> +        this._refreshButtonNavigationItem = new WI.ButtonNavigationItem("refresh", WI.UIString("Refresh all"), "Images/ReloadFull.svg", 13, 13);

Should the identifier match the title?  "refresh" -> "refresh-all"

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:69
> +    get supportsSplitContentBrowser() { return true; }

Style: this should be on separate lines.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:102
> +    --canvas-preview-card-width: 400px;
> +    --canvas-preview-card-height: 300px;

These aren't used.

[Devin Rousso]

(In reply to Matt Baker from comment #4)
> Created attachment 322092 [details]
> [Image] Canvas Overview UI
> 
> Canvas overview, with a selection.
> 
> Notes:
> - The selected canvas is the supplemental represented object for the view.
> This handled generically by CollectionContentView.
> - The selected canvas is indicated by a darker border (this is very subtle
> and should be improved).
Could we use the "selection" color we normally use as a border color?

> - The selected canvas gets a path component in the overview's navigation
> bar. This aids navigation when not all canvases can fit in the overview at
> once. It will also help to reinforces the view context when browsing of
> recordings is added.
> - The selected canvas is the represented object for purposes of showing the
> details sidebar.

[Matt Baker]

Comment on attachment 322097 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:222
>> +            }).then((results) => {
> 
> Is it really necessary to have this additional `then()`?  Can you merge the two `then()` into one?

This probably went through some churn. Will combine.

>> Source/WebInspectorUI/UserInterface/Models/CollectionTypes.js:30
>> +        super((item) => item instanceof WI.Canvas);
> 
> This already exists as `WI.Collection.TypeVerifier.Canvas`.

Then I propose removing it from WI.Collection.TypeVerifier, and moving away from that pattern in the future.

1) Ideally as little code as possible should ever need to know the collection's type.
2) If the type really needs to be known at runtime, we should rely on dynamic type-checking (instanceof).
3) If a central place to look up all collection types is desirable (which I've heard is a nice property of other base classes with factory-like behavior, i.e. ContentView), then one need look no further than CollectionTypes.js.

>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:91
>> +        titles.className = "titles";
> 
> Use `classList.add` instead of `className`.

Recently the team discussed this, and we concluded that producers of DOM elements can do this, since it's safe, but consumers should not.

>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:128
>> +        if (!this._pendingContent)
> 
> Do we want to show a spinner/loading-indicator?  Also, for WebGPU contexts, we currently are unable to retrieve the content.  I realize that WebGPU is still "experimental", but we might want to do at least do something simple.
> 
> <https://webkit.org/b/173621> Web Inspector: Support getting the content of WebGPU contexts

Content loads so quickly that I didn't consider a loading indicator. I think its a good idea, and I can add it in a follow-up.

>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:271
>> +            this._pixelSizeElement.textContent = `${this._pixelSize.width} Ã ${this._pixelSize.height}`;
> 
> Do we want to use the `multiplicationSign`?

Yes!

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:38
>> +    flex-grow: 0 !important;
> 
> Is this "!important" required?

It's not, good catch!

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:38
>> +        this._refreshButtonNavigationItem = new WI.ButtonNavigationItem("refresh", WI.UIString("Refresh all"), "Images/ReloadFull.svg", 13, 13);
> 
> Should the identifier match the title?  "refresh" -> "refresh-all"

Will change to be consistent.

>> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:69
>> +    get supportsSplitContentBrowser() { return true; }
> 
> Style: this should be on separate lines.

I don't have a problem with inlining getters that return literals, but I will defer to our code style.

[Devin Rousso]

Comment on attachment 322097 [details]
Patch

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

OK I had a chance to apply it and have some general feedback:
 - Is it necessary have the DetailsSidebar for the Canvas tab, seeing as much of the data is shown in the CanvasContentView itself?
 - Will we add back the DOM highlighting when CanvasContentView is hovered (especially useful for CSS canvases)?
    - Along these lines, are we going to show any indication that the canvas is onscreen/attached or offscreen/detached?
 - Should we also have individual "show-grid" navigation items for each CanvasContentView?
 - You mentioned this in <https://webkit.org/b/177606>, but just so I understand the plan forwards, how are we planning on tying in ShaderProgram items to CanvasContentView?

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:167
> +            console.error(error);

This will always error for WebGPU contexts.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:219
> +                let result = Promise.all([object.getProperty("width"), object.getProperty("height")]);

RemoteObject.prototype.getProperty doesn't support Promise operations.  Either modify it to support promises or this logic to use callbacks.  Using WI.WrappedPromise might make the latter option easier.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:64
>      get navigationItems()

Just so I understand, this will be removed once Canvases are no longer listed in Resources?

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:102
> +        navigationBar.addNavigationItem(this._recordButtonNavigationItem);

This should only be added if `_recordButtonNavigationItem` exists (2D and WebGL).

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:120
> +            node.addEventListener(WI.DOMNode.Event.AttributeModified, this._updateSize, this);
> +            node.addEventListener(WI.DOMNode.Event.AttributeRemoved, this._updateSize, this);

I think you meant `_refreshPixelSize`?

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:40
> +    width: 400px;

Instead of having this be a specific width, could we use some relative value?

    width: calc(50% - 20px);
    min-width: 200px;

Removing the `flex-grow: 0 !important;` would also allow each CanvasContentView to expand horizontally as it needs so that all the content space is used (not constrained to exactly the width).

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:130
> +        console.log(pathComponent);

I think you meant to change this?

[Devin Rousso]

Comment on attachment 322097 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/CollectionTypes.js:30
>>> +        super((item) => item instanceof WI.Canvas);
>> 
>> This already exists as `WI.Collection.TypeVerifier.Canvas`.
> 
> Then I propose removing it from WI.Collection.TypeVerifier, and moving away from that pattern in the future.
> 
> 1) Ideally as little code as possible should ever need to know the collection's type.
> 2) If the type really needs to be known at runtime, we should rely on dynamic type-checking (instanceof).
> 3) If a central place to look up all collection types is desirable (which I've heard is a nice property of other base classes with factory-like behavior, i.e. ContentView), then one need look no further than CollectionTypes.js.

My only issue with #3 is that this introduces some unnecessary code and potential maintenance cost.  If we decide to change the constructor of WI.Collection in the future, we'd also have to change the constructor of each of these subclasses.  We already have to do this throughout WebInspector, but I feel like this is a situation where it isn't really worth it.

If we want to move away from using TypeVerifier, then we should consider dropping that system entirely and replacing it with a protected function that subclasses override, instead of passing it in as part of the constructor of Wi.Collection.  Then, in WI.CollectionContentView, we can do `instanceof` checks for the passed in `collection` and determine the default title that way.  I am also fine with this approach.

[Matt Baker]

Comment on attachment 322097 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:154
>> +                CanvasAgent.requestNode(this._identifier).then(({nodeId}) => {
> 
> I personally prefer to use the callback approach in situations where we aren't chaining protocol functions.  I think it's much clearer to read.
> 
>     CanvasAgent.requestNode(this._identifier, (error, nodeId) => {
>         this._domNode = WI.domTreeManager.nodeForId(nodeId);
>         resolve(this._domNode);
>     });
> 
> Also, in the case that there is an error, should we return `null`?  I realize that all canvas contexts must have an associated Node, but (thinking forward) I don't think things like OffscreenCanvas have a node, so they would have an error.
> 
> <https://html.spec.whatwg.org/multipage/scripting.html#the-offscreencanvas-interface>

I was thinking of going 100% promises and never looking back (hah), but I think the point about readability is valid. I'll adopt this style here, and going forward when it makes sense.

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:167
>> +            console.error(error);
> 
> This will always error for WebGPU contexts.

Since WebGPU support is currently a unicorn, I'm not going to worry too much about it.

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:219
>> +                let result = Promise.all([object.getProperty("width"), object.getProperty("height")]);
> 
> RemoteObject.prototype.getProperty doesn't support Promise operations.  Either modify it to support promises or this logic to use callbacks.  Using WI.WrappedPromise might make the latter option easier.

Oops, this is from a version where I added promise support for these. Will fix.

>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:64
>>      get navigationItems()
> 
> Just so I understand, this will be removed once Canvases are no longer listed in Resources?

In the future we may want to use CanvasContentView the way it's been used up until now, as an actual "full size" content view. We might want to do this to show more things, like shaders, etc. That said, it's pretty up I the air, and even if we did that it would probably make more sense to add a subclass (PreviewCanvasContentView or something) that adds the "card" stuff (header, footer, etc).

Keeping the hybrid forms makes it a bit easier during the transition.

>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:102
>> +        navigationBar.addNavigationItem(this._recordButtonNavigationItem);
> 
> This should only be added if `_recordButtonNavigationItem` exists (2D and WebGL).

Good catch!

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:40
>> +    width: 400px;
> 
> Instead of having this be a specific width, could we use some relative value?
> 
>     width: calc(50% - 20px);
>     min-width: 200px;
> 
> Removing the `flex-grow: 0 !important;` would also allow each CanvasContentView to expand horizontally as it needs so that all the content space is used (not constrained to exactly the width).

I agree there is definitely room for improvement here. I played around with a few sizing strategies while working on this, including media queries to better utilities the space at wide/narrow widths. I decided to keep it dumb initially, and attempt this as a polish step later on.

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:130
>> +        console.log(pathComponent);
> 
> I think you meant to change this?

Doh!

[Matt Baker]

(In reply to Devin Rousso from comment #9)
> Comment on attachment 322097 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322097&action=review
> 
> OK I had a chance to apply it and have some general feedback:
>  - Is it necessary have the DetailsSidebar for the Canvas tab, seeing as
> much of the data is shown in the CanvasContentView itself?

Probably not, although some information (like the context attributes) aren't available anywhere else.

>  - Will we add back the DOM highlighting when CanvasContentView is hovered
> (especially useful for CSS canvases)?
>     - Along these lines, are we going to show any indication that the canvas
> is onscreen/attached or offscreen/detached?

The CSS canvas information should in the sidebar is the only place we show this stuff.

>  - Should we also have individual "show-grid" navigation items for each
> CanvasContentView?

I omitted this because I felt it was overkill. If we back "dedicated" canvas views then it would make sense to add it again. This design choice was motivated in part by the fact that this button is backed by a single setting that effects all canvases.

>  - You mentioned this in <https://webkit.org/b/177606>, but just so I
> understand the plan forwards, how are we planning on tying in ShaderProgram
> items to CanvasContentView?

The plan is to eventually incorporate shader support, but what form that would take hasn't been decided.

[Matt Baker]

Created attachment 322949 [details]
Patch

[Build Bot]

Comment on attachment 322949 [details]
Patch

Attachment 322949 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4774949

New failing tests:
inspector/view/basics.html

[Build Bot]

Created attachment 322958 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322949 [details]
Patch

Attachment 322949 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4774933

New failing tests:
inspector/view/basics.html

[Build Bot]

Created attachment 322959 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322949 [details]
Patch

Attachment 322949 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4778212

New failing tests:
inspector/view/basics.html

[Build Bot]

Created attachment 322995 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Devin Rousso]

Comment on attachment 322949 [details]
Patch

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

I haven't had a chance to apply this locally, but I will do that tomorrow.  Nice update :)

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:-154
> -        WI.domTreeManager.ensureDocument();

This doesn't need to be removed, and it could simplify the logic for the rest of the function.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:152
> +        return new Promise((resolve, reject) => {

If multiple calls to `requestNode` occur before it has a chance to resolve, we might be creating multiple promises that all do the same thing.  I think our typical approach is to save the promise to a member variable in the constructor, like `_requestNodePromise`.

    if (!this._requestNodePromise) {
        WI.domTreeManager.ensureDocument();

        this._requestNodePromise = CanvasAgent.requestNode(this._identifier)
            .then((result) => {
                this._domNode = WI.domTreeManager.nodeForId(result.nodeId);
                if (!this._domNode) {
                    reject(`No DOM node for identifier: ${result.nodeId}.`);
                    return;
                }

                resolve(this._domNode);
            }
            .catch(reject);
    }

    return this._requestNodePromise;

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:195
> +                callback(null);

The callers of `requestCSSCanvasClientNodes` expect the result to be an array (CanvasOverviewContentView.js:187), so I don't think you want to do this.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:221
> +        function awaitPropertyValue(remoteObject, name) {

This name is slightly confusing, since it doesn't actually use the `await` keyword.  Perhaps just `getPropertyValue` is enough, considering it's a local function.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:222
> +            return new WI.WrappedPromise((resolve, reject) => {

I realize I said this in a previous review, but I don't think WI.WrappedPromise is necessary here.  I think I misunderstood how you wanted to use this function.  A regular Promise should be fine.  My apologies.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:46
> +            const altToolTip = WI.UIString("Stop recording");

Should we match the text of `toolTip` and make this "Stop recording canvas actions"?

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:-70
> -        super.initialLayout();

Is there a reason to remove this?

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:121
> +            node.addEventListener(WI.DOMNode.Event.AttributeModified, this._refreshPixelSize, this);
> +            node.addEventListener(WI.DOMNode.Event.AttributeRemoved, this._refreshPixelSize, this);

These don't appear to be removed anywhere.  Also, seeing as `_refreshPixelSize` is called with every `layout()` anyways, you could probably move the add/remove event listener to `attached()`/`detached()`.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:124
> +        this._refreshPixelSize();

This is called in `layout()` so it can probably be removed.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:276
> +            this._pixelSizeElement.textContent = `${this._pixelSize.width} ${multiplicationSign} ${this._pixelSize.height}`;

Nice!

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:97
> +        contentView.element.addEventListener("mouseenter", this._contentViewMouseEnter);
> +        contentView.element.addEventListener("mouseleave", this._contentViewMouseLeave);

Awesome!

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:119
> +        this._canvasCollection = new WI.CanvasCollection(WI.canvasManager.canvases);

Should this be in an `initialLayout()`, or even the constructor?

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:129
> +        super.attached();

This should be `super.detached();`.

> LayoutTests/ChangeLog:11
> +        * inspector/view/basics.html:

The test failures seem to be because you didn't update the expected result.

[Matt Baker]

Created attachment 323001 [details]
Patch

[Devin Rousso]

Comment on attachment 323001 [details]
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.

[Matt Baker]

Comment on attachment 323001 [details]
Patch

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

>> 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;
> 
>         ...
>     });

Will add the check. Could this assertion ever fail?

>> 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.

Great suggestion. Will add.

[Matt Baker]

Created attachment 323058 [details]
Patch for landing

[Matt Baker]

Created attachment 323059 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 323059 [details]
Patch for landing

Clearing flags on attachment: 323059

Committed r223011: <http://trac.webkit.org/changeset/223011>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.