Bug 138941 - Web Inspector: create canvas content view and details sidebar panel
Summary: Web Inspector: create canvas content view and details sidebar panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 172623 173184
Blocks: WebInspectorCanvasRecording 140901 173087 173327 173396 173397 173569 173580 173621 173965 174209
  Show dependency treegraph
 
Reported: 2014-11-20 16:18 PST by Matt Baker
Modified: 2017-07-07 14:41 PDT (History)
15 users (show)

See Also:


Attachments
[Image] After Patch is applied (570.33 KB, image/png)
2017-06-08 22:43 PDT, Devin Rousso
no flags Details
[Image] After Patch is applied (Experimental Settings) (286.54 KB, image/png)
2017-06-08 22:44 PDT, Devin Rousso
no flags Details
[Image] After Patch is applied (723.66 KB, image/png)
2017-06-09 15:35 PDT, Devin Rousso
no flags Details
[Patch] WIP (66.25 KB, patch)
2017-06-12 19:48 PDT, Devin Rousso
drousso: commit-queue-
Details | Formatted Diff | Diff
[Image] After Patch is applied (664.79 KB, image/png)
2017-06-12 19:49 PDT, Devin Rousso
no flags Details
Patch (75.25 KB, patch)
2017-06-16 09:52 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (71.99 KB, patch)
2017-06-16 22:24 PDT, Devin Rousso
joepeck: review-
Details | Formatted Diff | Diff
Patch (74.29 KB, patch)
2017-06-19 19:08 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (74.53 KB, patch)
2017-06-19 21:18 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2014-11-20 16:18:14 PST
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.)
Comment 1 Radar WebKit Bug Importer 2014-11-20 16:18:25 PST
<rdar://problem/19051672>
Comment 2 Matt Baker 2015-01-26 13:11:22 PST
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.
Comment 3 Devin Rousso 2017-06-08 22:43:40 PDT
Created attachment 312385 [details]
[Image] After Patch is applied
Comment 4 Devin Rousso 2017-06-08 22:44:04 PDT
Created attachment 312386 [details]
[Image] After Patch is applied (Experimental Settings)
Comment 5 Jon Lee 2017-06-09 08:58:08 PDT
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.
Comment 6 Devin Rousso 2017-06-09 15:35:45 PDT
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.
Comment 7 Devin Rousso 2017-06-12 19:48:47 PDT
Created attachment 312743 [details]
[Patch] WIP
Comment 8 Devin Rousso 2017-06-12 19:49:01 PDT
Created attachment 312744 [details]
[Image] After Patch is applied
Comment 9 Joseph Pecoraro 2017-06-14 11:28:59 PDT
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.
Comment 10 Devin Rousso 2017-06-16 09:52:35 PDT
Created attachment 313083 [details]
Patch
Comment 11 Devin Rousso 2017-06-16 22:24:46 PDT
Created attachment 313186 [details]
Patch
Comment 12 Sam Weinig 2017-06-17 16:46:27 PDT
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.
Comment 13 Joseph Pecoraro 2017-06-19 13:06:08 PDT
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.
Comment 14 Joseph Pecoraro 2017-06-19 16:07:56 PDT
(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?
Comment 15 Joseph Pecoraro 2017-06-19 17:08:54 PDT
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.
Comment 16 Devin Rousso 2017-06-19 19:08:59 PDT
Created attachment 313351 [details]
Patch
Comment 17 Joseph Pecoraro 2017-06-19 19:34:14 PDT
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.
Comment 18 Devin Rousso 2017-06-19 21:18:03 PDT
Created attachment 313359 [details]
Patch
Comment 19 WebKit Commit Bot 2017-06-19 22:07:41 PDT
The commit-queue encountered the following flaky tests while processing attachment 313359 [details]:

The commit-queue is continuing to process your patch.
Comment 20 WebKit Commit Bot 2017-06-19 23:48:14 PDT
Comment on attachment 313359 [details]
Patch

Clearing flags on attachment: 313359

Committed r218544: <http://trac.webkit.org/changeset/218544>
Comment 21 WebKit Commit Bot 2017-06-19 23:48:17 PDT
All reviewed patches have been landed.  Closing bug.