Bug 173569

Summary: Web Inspector: Support getting the content of WebGL/WebGL2 contexts
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, buildbot, cdumez, commit-queue, dino, esprehn+autocc, graouts, gyuyoung.kim, inspector-bugzilla-changes, joepeck, kondapallykalyan, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=173621
Bug Depends on: 138941, 173396    
Bug Blocks:    
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Patch
joepeck: review-
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch
none
Patch
joepeck: review+
Patch none

Description Devin Rousso 2017-06-19 16:17:44 PDT
For WebGL/WebGL2/WebGPU canvases that do not have the "preserveDrawingBuffer" attribute set to true, it is not possible to get the content of the canvas via toDataURL.  In order to display a snapshot of the canvas, we need another way to get its content.
Comment 1 Devin Rousso 2017-06-20 18:39:13 PDT
Created attachment 313465 [details]
Patch
Comment 2 Build Bot 2017-06-20 19:46:02 PDT
Comment on attachment 313465 [details]
Patch

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

New failing tests:
inspector/canvas/requestContent.html
Comment 3 Build Bot 2017-06-20 19:46:04 PDT
Created attachment 313474 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-06-20 20:04:08 PDT
Comment on attachment 313465 [details]
Patch

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

New failing tests:
inspector/canvas/requestContent.html
Comment 5 Build Bot 2017-06-20 20:04:09 PDT
Created attachment 313479 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-06-20 20:16:23 PDT
Comment on attachment 313465 [details]
Patch

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

New failing tests:
inspector/canvas/requestContent.html
Comment 7 Build Bot 2017-06-20 20:16:24 PDT
Created attachment 313481 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Devin Rousso 2017-07-03 13:51:44 PDT
Created attachment 314513 [details]
Patch
Comment 9 Radar WebKit Bug Importer 2017-07-03 13:52:46 PDT
<rdar://problem/33112420>
Comment 10 Joseph Pecoraro 2017-07-03 17:01:59 PDT
Comment on attachment 314513 [details]
Patch

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

r- just because of the bunch of comments I'd like to see an updated patch. The approach seems good to me.

> LayoutTests/inspector/canvas/requestContent-webgl2.html:27
> +            CanvasAgent.requestContent(canvas.identifier, (error, content) => {

You should be able to write this like:

    CanvasAgent.requestContent(canvas.identifier)
        .then(({content}) => { InspectorTest.log(content); })
        .then(resolve, reject);

Which looks a little cleaner.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:213
> +    void preventBufferClearForInspector(bool value) { m_preventBufferClearForInspector = value; }

This should be a setter `setPreventBufferClearForInspector(bool)`. And you might as well include a getter even if it is not used.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:841
> +    bool m_preventBufferClearForInspector { false };

The rest of the member variables are in the protected area. For example:

    572    bool m_synthesizedErrorsToConsole { true };
    573    int m_numGLErrorsToConsoleAllowed;

I'd expect this to go near those.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:151
> +            return;

This would return without clearing the PreventBufferClearForInspector state you just added. I'd expect code more like:

    WebGLRenderingContextBase* glContext = downcast<WebGLRenderingContextBase>(context);
    glContext->setPreventBufferClearForInspector(true);
    ExceptionOr<String> result = canvasEntry->element->toDataURL(ASCIILiteral("image/png"));
    glContext->setPreventBufferClearForInspector(false);
    if (result.hasException()) {
        errorString = result.releaseException().releaseMessage();
        return;
    }
Comment 11 Devin Rousso 2017-07-05 10:49:05 PDT
Created attachment 314617 [details]
Patch
Comment 12 Build Bot 2017-07-05 11:24:45 PDT
Comment on attachment 314617 [details]
Patch

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

New failing tests:
inspector/canvas/requestContent-2d.html
Comment 13 Build Bot 2017-07-05 11:24:46 PDT
Created attachment 314624 [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
Comment 14 Build Bot 2017-07-05 11:34:26 PDT
Comment on attachment 314617 [details]
Patch

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

New failing tests:
inspector/canvas/requestContent-2d.html
Comment 15 Build Bot 2017-07-05 11:34:27 PDT
Created attachment 314627 [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
Comment 16 Build Bot 2017-07-05 12:04:12 PDT
Comment on attachment 314617 [details]
Patch

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

New failing tests:
inspector/canvas/requestContent-2d.html
Comment 17 Build Bot 2017-07-05 12:04:13 PDT
Created attachment 314633 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Devin Rousso 2017-07-05 13:12:18 PDT
Created attachment 314645 [details]
Patch
Comment 19 Devin Rousso 2017-07-05 13:20:14 PDT
Created attachment 314647 [details]
Patch
Comment 20 Joseph Pecoraro 2017-07-05 20:04:10 PDT
Comment on attachment 314647 [details]
Patch

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

r=me

> Source/WebCore/html/canvas/WebGLRenderingContextBase.h:213
> +    bool getPreventBufferClearForInspector() const { return m_preventBufferClearForInspector; }

Getter should not have a `get` prefix.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:160
> +        gl->setPreventBufferClearForInspector(true);
> +
> +        ExceptionOr<String> result = canvasEntry->element->toDataURL(ASCIILiteral("image/png"));
> +
> +        gl->setPreventBufferClearForInspector(false);

Can you group these right next to the toDataURL line? That is the significant part to me.
Comment 21 Devin Rousso 2017-07-06 10:13:57 PDT
Created attachment 314732 [details]
Patch
Comment 22 WebKit Commit Bot 2017-07-06 11:36:02 PDT
Comment on attachment 314732 [details]
Patch

Clearing flags on attachment: 314732

Committed r219208: <http://trac.webkit.org/changeset/219208>
Comment 23 WebKit Commit Bot 2017-07-06 11:36:04 PDT
All reviewed patches have been landed.  Closing bug.