Bug 173397 - Web Inspector: Allow users to log any tracked canvas context
Summary: Web Inspector: Allow users to log any tracked canvas context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 138941 173396
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-14 18:36 PDT by Devin Rousso
Modified: 2017-07-05 13:14 PDT (History)
11 users (show)

See Also:


Attachments
[Image] After Patch is applied (490.14 KB, image/png)
2017-06-14 18:38 PDT, Devin Rousso
no flags Details
[Patch] WIP (16.74 KB, patch)
2017-06-14 22:54 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch (28.21 KB, patch)
2017-07-03 13:31 PDT, Devin Rousso
joepeck: review+
Details | Formatted Diff | Diff
Patch (27.35 KB, patch)
2017-07-05 10:32 PDT, Devin Rousso
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (996.02 KB, application/zip)
2017-07-05 11:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.10 MB, application/zip)
2017-07-05 11:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.75 MB, application/zip)
2017-07-05 12:13 PDT, Build Bot
no flags Details
Patch (26.98 KB, patch)
2017-07-05 12:57 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 Devin Rousso 2017-06-14 18:36:38 PDT
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).
Comment 1 Devin Rousso 2017-06-14 18:38:34 PDT
Created attachment 312944 [details]
[Image] After Patch is applied
Comment 2 Devin Rousso 2017-06-14 22:54:18 PDT
Created attachment 312955 [details]
[Patch] WIP
Comment 3 Joseph Pecoraro 2017-06-15 11:42:57 PDT
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?
Comment 4 Radar WebKit Bug Importer 2017-07-03 13:05:06 PDT
<rdar://problem/33111581>
Comment 5 Devin Rousso 2017-07-03 13:31:33 PDT
Created attachment 314512 [details]
Patch
Comment 6 BJ Burg 2017-07-03 13:53:14 PDT
<rdar://problem/33109380>
Comment 7 Joseph Pecoraro 2017-07-03 18:51:04 PDT
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.
Comment 8 Devin Rousso 2017-07-05 10:32:38 PDT
Created attachment 314614 [details]
Patch
Comment 9 WebKit Commit Bot 2017-07-05 11:04:10 PDT
The commit-queue encountered the following flaky tests while processing attachment 314614 [details]:

The commit-queue is continuing to process your patch.
Comment 10 Build Bot 2017-07-05 11:31:27 PDT
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
Comment 11 Build Bot 2017-07-05 11:31:28 PDT
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
Comment 12 Build Bot 2017-07-05 11:37:01 PDT
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
Comment 13 Build Bot 2017-07-05 11:37:02 PDT
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
Comment 14 Build Bot 2017-07-05 12:13:23 PDT
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
Comment 15 Build Bot 2017-07-05 12:13:24 PDT
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
Comment 16 Devin Rousso 2017-07-05 12:57:16 PDT
Created attachment 314643 [details]
Patch
Comment 17 WebKit Commit Bot 2017-07-05 13:13:59 PDT
Comment on attachment 314643 [details]
Patch

Clearing flags on attachment: 314643

Committed r219150: <http://trac.webkit.org/changeset/219150>
Comment 18 WebKit Commit Bot 2017-07-05 13:14:01 PDT
All reviewed patches have been landed.  Closing bug.