Bug 138941 – Web Inspector: create canvas content view and details sidebar panel

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

It would be nice to have a more descriptive name (where possible) for canvases in the Resources panel. Instead of "Canvas 1", "Canvas 2", etc, maybe we could use "Canvas #foo" for <canvas id="foo">, or "CSS Canvas 'foo'" for -webkit-canvas(foo). Very long IDs could pose a problem, but the details sidebar could display additional information about the canvas:

-Element ID (if it exists)
-Is a CSS canvas? (-webkit-canvas)
-Is an offscreen canvas?
-Rendering context creation parameters (antialias, depth, etc.)

[Radar WebKit Bug Importer]

<rdar://problem/19051672>

[Matt Baker]

It might be useful (especially for offscreen canvases) to show a preview, with the ability to refresh, of the last frame rendered by the selected canvas. For now content view will simply show "Preview not available" as a placeholder while the details sidebar panel is hooked up.

[Devin Rousso]

Created attachment 312385 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 312386 [details]
[Image] After Patch is applied (Experimental Settings)

[Jon Lee]

Could you show the boundaries of the canvas? Either with a border or gray/black/subtle-checkboard background

Source panel should also include device pixel ratio, possibly also layer info.

(In reply to Matt Baker from comment #2)
> It might be useful (especially for offscreen canvases) to show a preview,
> with the ability to refresh, of the last frame rendered by the selected
> canvas.

Live update would be really useful in the case where they are triggered by an event, whether it be user input, or onscroll.

[Devin Rousso]

Created attachment 312502 [details]
[Image] After Patch is applied

(In reply to Jon Lee from comment #5)
> Could you show the boundaries of the canvas? Either with a border or
> gray/black/subtle-checkboard background

https://webkit.org/b/173184

> Source panel should also include device pixel ratio, possibly also layer
> info.

Is this information per <canvas>, or is it per device?  I am trying to keep information specifically related to the <canvas>/context in the details sidebar.

> (In reply to Matt Baker from comment #2)
> > It might be useful (especially for offscreen canvases) to show a preview,
> > with the ability to refresh, of the last frame rendered by the selected
> > canvas.
> 
> Live update would be really useful in the case where they are triggered by
> an event, whether it be user input, or onscroll.

I tried a very simple version of this and saw the page start to stutter (although the page I used is quite intense <http://cocausc.org>).  I think it may be necessary to do some frame-rate limiting, such as only showing 30fps instead of every frame.

[Devin Rousso]

Created attachment 312743 [details]
[Patch] WIP

[Devin Rousso]

Created attachment 312744 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 312743 [details]
[Patch] WIP

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

> Source/WebInspectorUI/UserInterface/Base/Setting.js:126
> +    showCanvasContextsInResources: new WebInspector.Setting("show-canvas-contexts-in-resources", false),

Given its experimental lets throw a "experimental-" prefix on it so we don't use up this nice name (if we wanted this name to ever be non-experiemental).

> Source/WebInspectorUI/UserInterface/Images/Canvas.svg:6
> +    <path d="M 0.5 4.29857047 L 7.09714095 7.59714095 L 7.09714095 15.0189245 L 0.5 11.720354 L 0.5 4.29857047 Z" fill="rgb(92, 140, 229)" stroke="white" stroke-width="0.5" troke-linecap="square" stroke-linejoin="round"/>
> +    <path d="M 13.697 4.299 L 7.097 7.597 L 7.097 15.019 L 13.697 11.72 L 13.697 4.299 Z" fill="rgb(242, 97, 97)" stroke="white" stroke-width="0.5" stroke-linecap="square" troke-linejoin="round"/>
> +    <path d="M 0.5 4.29857047 L 7.09714095 1 L 13.6908901 4.29857047 L 7.09714095 7.59714095 L 0.5 4.29857047 Z" fill="rgb(97, 242, 97)" stroke="white" stroke-width="0.5" troke-linejoin="round"/>

What is `troke-linejoin`? It seems to be in all 3 of these.

Style: Put the `d` attribute last, so the colors and properties are all up front and the long tail is at the end.

[Devin Rousso]

Created attachment 313083 [details]
Patch

[Devin Rousso]

Created attachment 313186 [details]
Patch

[Sam Weinig]

This seems great, but it's a little weird for this to be in the Resources. All the other items in that list are actual resources that got fetched.

[Joseph Pecoraro]

Comment on attachment 313186 [details]
Patch

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

My first pass this looks good. I will try to take a 2nd pass by the end of the day.

> LayoutTests/inspector/canvas/requestContent.html:36
> +                    let keys = Object.keys(results);
> +                    if (keys.length >= WebInspector.canvasManager.canvases.length) {
> +                        // Ensure that the test runs properly even if the canvas events are
> +                        // sent in a different order than the above.
> +                        keys.sort();

What is this? We do canvases.forEach how can we end up with more results than canvases? In fact they might clash on display name and the final condition would never be hit.

Maybe something like this would read better? Up to you.

    let canvases = WebInspector.canvasManager.canvases;
    let contents = new Map;
    let expected = canvases.length;
    canvases.forEach((canvas) => {
        CanvasAgent.requestContent(canvas.identifier, (error, content) => {
            ...
            contents.set(canvas, content);
            if (contents.size === expected)
                finish();
        });
    });

    function finish() {
        let results = {};
        for (let [canvas, content] of contents)
            results[canvas.displayName] = content;
        let keys = Object.keys(result);
        InspectorTest.assert(contents.size === keys.length, "No display name collisions");
        keys.sort();
        ...
    }

> LayoutTests/inspector/canvas/requestNode.html:51
> +                        if (results.length >= WebInspector.canvasManager.canvases.length) {

Ditto.

> LayoutTests/inspector/canvas/requestNode.html:56
> +                            for (let displayName of results)
> +                                InspectorTest.log(`Canvas "${displayName}" has node with valid id.`);

You say "with valid id" but you haven't validated it.

You can get the DOMNode via:

    let domNode = WebInspector.domTreeManager.nodeForId(nodeId);

And you should assert it is non-null, and maybe even verify it has a "canvas" tagName.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:129
> +        // FIXME: Support other canvas context types (e.g. WebGL, WebGL2, WebGPU).

Lets make a bugzilla bug number for this before landing.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:28
> -    constructor(identifier, contextType, frame, cssCanvasName)
> +    constructor(identifier, contextType, frame, optionals = {})

You could just make this:

    constructor(identifier, contextType, frame, {domNode, cssCanvasName})

I'm fine with this style.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:64
> +        let optionals = {
> +            domNode: payload.nodeId ? WebInspector.domTreeManager.nodeForId(payload.nodeId) : null,
> +            cssCanvasName: payload.cssCanvasName,
> +        };
> +        return new WebInspector.Canvas(payload.canvasId, contextType, frame, optionals);

Nit: inline the optionals object.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:128
> +        WebInspector.domTreeManager.requestDocument(documentAvailable.bind(this));

Style: Just inline these callbacks. Explicit functions is legacy style.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:142
> +        CanvasAgent.requestContent(this._identifier, contentAvailable.bind(this));

Style: Just inline these callbacks. Explicit functions is legacy style.

[Joseph Pecoraro]

(In reply to Sam Weinig from comment #12)
> This seems great, but it's a little weird for this to be in the Resources.
> All the other items in that list are actual resources that got fetched.

Yeah, I agree.

We've already got some weird situations right now in Resources:

  - Frames show up and go away if the <iframe> is removed
  - Web Sockets show up and stay when the WebSocket is closed
  - Workers show up and go away when the Worker is terminated / collected

However in these cases they are all backed by some kind of network loaded resource. Canvases would be the first time we'd be showing something not backed by some kind of a resource.

We do plan on showing Canvas Shader Programs as children of Canvas tree element. Showing shader programs in Resources feels less out of place, since we would show them in a text editor like these other resources. However, it still feels out of place.

I think it make senses to make a new Canvas tab. A dedicated tab would work well when a developer intends to debug a canvas. It would also be nicer to have canvas snapshot debugging live in a tab that isn't Resources. It would allow for other UI (controls / space) dedicated to canvas debugging. Especially as we grow to debug a 3D canvas. We had already kind of talked about heading in this direction for snapshots.

@Devin, what do you think?

[Joseph Pecoraro]

Comment on attachment 313186 [details]
Patch

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

> LayoutTests/inspector/canvas/requestContent-expected.txt:10
> +data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAASwAAACWCAYAAABkW7XSAAAAAXNSR0IArs4c6QAAAylJREFUeAHt0DEBAAAAwqD1T20IX4hAYcCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYMCAAQMGDBgwYOAdGL/UAAEPpnR6AAAAAElFTkSuQmCC

Maybe create some content that isn't an empty block? What is this, just a transparent 300x150 box? How about a green box?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:122
> +        ExceptionOr<String> result = canvasEntry->element->toDataURL("image/png");

Nit: ASCIILiteral("image/png")

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:264
> -    if (!canvasEntry.cssCanvasName.isEmpty())
> +    if (canvasEntry.cssCanvasName.isEmpty()) {

The other order seems more readable:

    if ( has css name )
        set canvas name
    else {
        find node id
        set node id
    }

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:150
> +localizedStrings["Canvas #%s"] = "Canvas #%s";

I worry that Localizers will treat this "#" as a Number sign, but its actually there for an CSS ID selector. I think we should add the "#" ourselves and just use: "Canvas %s".

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:916
> +localizedStrings["WebGL"] = "WebGL";

Hmm, not sure we want to localize this.

> Source/WebInspectorUI/UserInterface/Base/Main.js:1135
> +        || (representedObject instanceof WebInspector.Canvas && WebInspector.settings.experimentalShowCanvasContextsInResources.value))

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:100
> +        if (this._domNode) {
> +            let id = this._domNode.getAttribute("id");
> +            if (id)
> +                return WebInspector.UIString("Canvas #%s").format(id);
> +        }

Yeah, I think this should be:

    if (this._domNode) {
        let idSelector = this._domNode.escapedIdSelector;
        if (idSelector)
            return WebInspector.UIString("Canvas %s").format(idSelector);
    }

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:113
> +    _toggleImageGrid()

This is very confusingly named as it doesn't toggle anything! How about _updateImageGrid.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:50
> +    get canvas() { return this._canvas; }

Nit: Don't oneline this given related code.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:54
> +        this._canvas = canvas || null;

If this._canvas is not changing we should bail.
If this._canvas is changing I think we will have to clear our this._node and remove its listeners, otherwise we hold onto _node longer than necessary (for example if we navigated the inspected page).

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:170
> +            if (!this._canvas.cssCanvasName && !this._node.parentNode)
> +                this._offscreenRow.value = WebInspector.UIString("Yes");

I'm not sure this will meet everyones expectations. It looks we are saying Yes if its a CSS -webkit-canvas() or a <canvas> element not immediately in the DOM Tree.

Someone could interpret Offscreen to mean its just scrolled offscreen.

Likewise someone could have a <canvas> that is not detached from the document but has a parent node.

I think this could use some refinement, and maybe a tooltip.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:40
> +        switch (representedObject.contextType) {
> +        case WebInspector.Canvas.ContextType.Canvas2D:
> +            classNames.push("canvas-2d");
> +            break;
> +        case WebInspector.Canvas.ContextType.WebGL:
> +            classNames.push("webgl");
> +            break;
> +        }

If you converted Canvas.ContextType to be a string instead of a symbol you could just do:

    let classNames = ["canvas", representedObject.contextType];

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:60
> +        if (representedObject instanceof WebInspector.Canvas && WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:245
> +        if (representedObject instanceof WebInspector.Canvas && WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:56
> +        if (WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/ResourceSidebarPanel.js:456
> +            || (treeElement instanceof WebInspector.CanvasTreeElement && WebInspector.settings.experimentalShowCanvasContextsInResources.value)) {

I don't think you need the experimental check here.

> Source/WebInspectorUI/UserInterface/Views/ResourcesTabContentView.js:72
> +            || (representedObject instanceof WebInspector.Canvas && WebInspector.settings.experimentalShowCanvasContextsInResources.value)

I don't think you need the experimental check here.

[Devin Rousso]

Created attachment 313351 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 313351 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Images/Canvas.svg:2
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- Copyright © 2017 Apple Inc. All rights reserved. -->

File a bug on GTK to add an Image.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:143
> +        if (this._domNode)
> +            cookie[WebInspector.Canvas.NodePathCookieKey] = this._domNode.path;

Should this be an `else` of CSSCanvasName? Because that DOMNode won't have a path.

> Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:65
> +        if (this._node) {
> +            this._node.removeEventListener(WebInspector.DOMNode.Event.AttributeModified, this._refreshSourceSection, this);
> +            this._node.removeEventListener(WebInspector.DOMNode.Event.AttributeRemoved, this._refreshSourceSection, this);
> +        }

Probably want to assign this._node = null after removing event listeners.

[Devin Rousso]

Created attachment 313359 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 313359 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 313359 [details]
Patch

Clearing flags on attachment: 313359

Committed r218544: <http://trac.webkit.org/changeset/218544>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.