Bug 173397 – Web Inspector: Allow users to log any tracked canvas context

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

[Devin Rousso]

Since contexts can sometimes be hard to retrieve, we should make it possible for users to log the context object to the console (the <canvas> element can be accessed via the context's `.canvas` getter).

[Devin Rousso]

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

[Devin Rousso]

Created attachment 312955 [details]
[Patch] WIP

[Joseph Pecoraro]

Comment on attachment 312955 [details]
[Patch] WIP

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

Looking good. Some simple comments for the next iteration.

> LayoutTests/inspector/canvas/resolveCanvasContext.html:10
> +    window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("2d");
> +    window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("webgl");
> +    window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("webgl2");
> +    window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("webgpu");

Whats up here? Shouldn't they be distinct variables?

> LayoutTests/inspector/canvas/resolveCanvasContext.html:21
> +    suite.addTestCase({
> +        name: "Canvas.resolveCanvasContext.validIdentifier.Canvas2D",
> +        description: "Should return a valid object for the given canvas identifier.",

Most of these tests are near direct copies. You can create a helper and simplify the tests:

    function addTestCase({name, description, type, className}) {
        suite.addTestCase({
            name, description,
            test(resolve, reject) {
                let canvas = WebInspector.canvasManager.canvases.find((canvas) => canvas.contextType === type);
                ...
                CanvasAgent.resolveCanvasContext(canvas.ientifier, objectGroup, (error, object) => {
                    ...
                    InspectorTest.expectEqual(object.className, className, `Payload should have className "${className}".`);
                });
            }
        });
    }

So with all the boilerplate packaged up the repeated tests can look very simple.

    addTestCase({
        name: "Canvas.resolveCanvasContext.Canvas2D",
        description: "...",
        type: WebInspector.Canvas.ContextType.Canvas2D,
        className: "CanvasRenderingContext2D",
    });

    addTestCase({
        name: "Canvas.resolveCanvasContext.WebGPU",
        description: "...",
        type: WebInspector.Canvas.ContextType.WebGPU,
        className: "WebGPURenderingContext",
    });

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:46
> +#if ENABLE(WEBGL)
> +#include "JSWebGLRenderingContext.h"
> +#include "JSWebGL2RenderingContext.h"
> +#endif
> +
> +#if ENABLE(WEBGPU)
> +#include "JSWebGPURenderingContext.h"
> +#endif

The usual style for includes would look like:

    #inculde "config.h"
    #include "Foo.h"

    #include "A.h"
    #include "B.h"
    #include "C.h"
    #include <wtf/AA.h>
    #include <wtf/BB.h>

    #if ENABLE(FOO)
    #include "FooA.h"
    #endif

    #if ENABLE(BAR)
    #include "FooB.h"
    #endif

However in this case I don't think you even need the ENABLE blocks. We always generate these files and inside the files are enable guards, so it tends to be easier and clearer to just include them unconditionally.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:188
> +    String objectGroupName = objectGroup ? *objectGroup : String();

This could be pushed down to right before the wrapObject line since its not used before then.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:54
> +    populateContextMenu(contextMenu, event)

Should this be calling super.populateContextMenu?

[Radar WebKit Bug Importer]

<rdar://problem/33111581>

[Devin Rousso]

Created attachment 314512 [details]
Patch

[Blaze Burg]

<rdar://problem/33109380>

[Joseph Pecoraro]

Comment on attachment 314512 [details]
Patch

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

Nice! r=me

> LayoutTests/platform/mac/TestExpectations:1145
> +webkit.org/b/174066 inspector/canvas/resolveCanvasContext-webgl2.html [ Pass Timeout ]
> +webkit.org/b/174066 inspector/canvas/resolveCanvasContext-webgpu.html [ Pass Timeout ]

Do these really need to be pass timeout as well?

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-494
> -localizedStrings["Layer"] = "Layer";

Whoa, when did this go away?! Weird.

[Devin Rousso]

Created attachment 314614 [details]
Patch

[WebKit Commit Bot]

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

The commit-queue is continuing to process your patch.

[Build Bot]

Comment on attachment 314614 [details]
Patch

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

New failing tests:
inspector/canvas/resolveCanvasContext-webgpu.html

[Build Bot]

Created attachment 314626 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 314614 [details]
Patch

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

New failing tests:
inspector/canvas/resolveCanvasContext-webgpu.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 314614 [details]
Patch

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

New failing tests:
inspector/canvas/resolveCanvasContext-webgpu.html

[Build Bot]

Created attachment 314634 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

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

[Devin Rousso]

Created attachment 314643 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 314643 [details]
Patch

Clearing flags on attachment: 314643

Committed r219150: <http://trac.webkit.org/changeset/219150>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.