Web Inspector: Record actions performed on ImageBitmapRenderingContext

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Use the protocol/instrumentation logic created in <https://webkit.org/b/174481> Web Inspector: create protocol functions for recording Canvas contexts

[Devin Rousso]

Created attachment 330577 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[EWS Watchlist]

Comment on attachment 330577 [details]
Patch

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

New failing tests:
inspector/canvas/recording-bitmaprenderer.html

[EWS Watchlist]

Created attachment 330597 [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

[Joseph Pecoraro]

> 73 73  10: "asyncFunctionResume"
> 74 74  11: [10,4,1,11]
> 75     12: [4,4,28,17]
>    75  12: [4,4,24,17]

asyncFunctionResume is a built-in. Built-ins are supposed to get somewhat optimized in release builds (unnecessary whitespace removed, like leading spaces) and not debug builds, but the intent of that optimization was to keep lines the same.

Maybe thats not the case?

---

In my build directory this certainly seems to be the problem.

$build/Release/DerivedSources/JavaScriptCore/JSCBuiltins.cpp

> const char* s_asyncFunctionPrototypeAsyncFunctionResumeCode =
>     "(function (generator, promiseCapability, sentValue, resumeMode)\n" \
>     "{\n" \
>     "    \"use strict\";\n" \
>     "    let state = generator.@generatorState;\n" \
>     "    let value = @undefined;\n" \
>     "\n" \
>     "    if (state === @GeneratorStateCompleted || (resumeMode !== @GeneratorResumeModeNormal && resumeMode !== @GeneratorResumeModeThrow))\n" \
>     "        @throwTypeError(\"Async function illegally resumed\");\n" \
>     "\n" \
>     "    try {\n" \
>     "        generator.@generatorState = @GeneratorStateExecuting;\n" \
>     "        value = generator.@generatorNext.@call(generator.@generatorThis, generator, state, sentValue, resumeMode, generator.@generatorFrame);\n" \
>     "        if (generator.@generatorState === @GeneratorStateExecuting) {\n" \
>     "            generator.@generatorState = @GeneratorStateCompleted;\n" \
>     "            promiseCapability.@resolve(value);\n" \
>     "            return promiseCapability.@promise;\n" \
>     "        }\n" \
>     "    } catch (error) {\n" \
>     "        generator.@generatorState = @GeneratorStateCompleted;\n" \
>     "        promiseCapability.@reject(error);\n" \
>     "        return promiseCapability.@promise;\n" \
>     "    }\n" \
>     "\n" \
>     "    let wrappedValue = @newPromiseCapability(@Promise);\n" \
>     "    wrappedValue.@resolve.@call(@undefined, value);\n" \
>     "\n" \
>     "    wrappedValue.@promise.@then(\n" \
>     "        function(value) { @asyncFunctionResume(generator, promiseCapability, value, @GeneratorResumeModeNormal); },\n" \
>     "        function(error) { @asyncFunctionResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });\n" \
>     "\n" \
>     "    return promiseCapability.@promise;\n" \
>     "})\n" \
> ;


$build/Debug/DerivedSources/JavaScriptCore/JSCBuiltins.cpp

> const char* s_asyncFunctionPrototypeAsyncFunctionResumeCode =
>     "(function (generator, promiseCapability, sentValue, resumeMode)\n" \
>     "{\n" \
>     "    \"use strict\";\n" \
>     "    let state = generator.@generatorState;\n" \
>     "    let value = @undefined;\n" \
>     "    if (state === @GeneratorStateCompleted || (resumeMode !== @GeneratorResumeModeNormal && resumeMode !== @GeneratorResumeModeThrow))\n" \
>     "        @throwTypeError(\"Async function illegally resumed\");\n" \
>     "    try {\n" \
>     "        generator.@generatorState = @GeneratorStateExecuting;\n" \
>     "        value = generator.@generatorNext.@call(generator.@generatorThis, generator, state, sentValue, resumeMode, generator.@generatorFrame);\n" \
>     "        if (generator.@generatorState === @GeneratorStateExecuting) {\n" \
>     "            generator.@generatorState = @GeneratorStateCompleted;\n" \
>     "            promiseCapability.@resolve(value);\n" \
>     "            return promiseCapability.@promise;\n" \
>     "        }\n" \
>     "    } catch (error) {\n" \
>     "        generator.@generatorState = @GeneratorStateCompleted;\n" \
>     "        promiseCapability.@reject(error);\n" \
>     "        return promiseCapability.@promise;\n" \
>     "    }\n" \
>     "    let wrappedValue = @newPromiseCapability(@Promise);\n" \
>     "    wrappedValue.@resolve.@call(@undefined, value);\n" \
>     "    wrappedValue.@promise.@then(\n" \
>     "        function(value) { @asyncFunctionResume(generator, promiseCapability, value, @GeneratorResumeModeNormal); },\n" \
>     "        function(error) { @asyncFunctionResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });\n" \
>     "    return promiseCapability.@promise;\n" \
>     "})\n" \
> ;

[Joseph Pecoraro]

I guess this was always broken:
<https://webkit.org/b/152290> Builtin source should be minified more
http://trac.webkit.org/changeset/194144

> diff --git a/Source/JavaScriptCore/Scripts/builtins/builtins_model.py b/Source/JavaScriptCore/Scripts/builtins/builtins_model.py
> old mode 100644
> new mode 100755
> index d765eca865e..cdd5f900354
> --- a/Source/JavaScriptCore/Scripts/builtins/builtins_model.py
> +++ b/Source/JavaScriptCore/Scripts/builtins/builtins_model.py
> @@ -100,6 +103,11 @@ class BuiltinFunction:
>      @staticmethod
>      def fromString(function_string):
>          function_source = multilineCommentRegExp.sub("", function_string)
> +        if os.getenv("CONFIGURATION", "Debug").startswith("Debug"):
> +            function_source = lineWithOnlySingleLineCommentRegExp.sub("", function_source)
> +            function_source = lineWithTrailingSingleLineCommentRegExp.sub("\n", function_source)
> +            function_source = multipleEmptyLinesRegExp.sub("\n", function_source)

This probably should have been "not os.getenv(...).startsWith("Debug").

But you would have the same problem. Any modification here will result in line or column changes which would break your test.

[Devin Rousso]

Created attachment 331537 [details]
Patch

Simplified the test output

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #6)
> I guess this was always broken:
> <https://webkit.org/b/152290> Builtin source should be minified more
> http://trac.webkit.org/changeset/194144
> 
> ...
>
> This probably should have been "not os.getenv(...).startsWith("Debug").

I'll fix this in:
<https://webkit.org/b/182165> JavaScriptCore builtins should be partially minified in Release builds not Debug builds

[Joseph Pecoraro]

So what is an ImageBitmapRenderingContext and why would we want to record it?

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #9)
> So what is an ImageBitmapRenderingContext and why would we want to record it?
ImageBitmapRenderingContext is part of the OffscreenCanvas spec [1] and from what I understand is designed to be a very basic rendering context for displaying ImageBitmap objects.  These are usually generated either via OffscreenCanvas.prototype.transferToImageBitmap [2] or through the more generic createImageBitmap [3].  Either way, I see the intention of it to be as the replacement for the front buffer in double buffering, as the drawing buffer can be created in a Worker with OffscreenCanvas.  I think we want to be able to record it to see a list of transferFromImageBitmap [4] calls for a single frame, just to make sure we are only drawing what we need to each frame.  Also, it was pretty easy to implement, so I figured why not :P

[1] https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapRenderingContext
[2] https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/transferToImageBitmap
[3] https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/createImageBitmap
[4] https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmapRenderingContext/transferFromImageBitmap

[Joseph Pecoraro]

Do we still want to do this? Does this make the code clearer in some reasonable way?

I don't think developers will be writing directly to ImageBitmapRenderingContext often. Until we have a use case I'm not sure its worth the effort to support it.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #11)
> Do we still want to do this? Does this make the code clearer in some
> reasonable way?
> 
> I don't think developers will be writing directly to
> ImageBitmapRenderingContext often. Until we have a use case I'm not sure its
> worth the effort to support it.
Frankly, I think the amount of work necessary to add this is so small that it might be worth it.  The main benefit I can see here is the ability to figure out when/where ImageBitmap are drawn to the canvas, such as checking for performance issues (e.g. accidentally drawing twice).  I can't claim that this is widely used as of now though, so I can't say that it's absolutely necessary.

[Devin Rousso]

Created attachment 348273 [details]
Patch

Rebase

[Devin Rousso]

Created attachment 348274 [details]
Patch

Forgot new test file

[EWS Watchlist]

Comment on attachment 348274 [details]
Patch

Attachment 348274 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9007978

New failing tests:
inspector/canvas/recording-bitmaprenderer.html

[EWS Watchlist]

Created attachment 348279 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 348274 [details]
Patch

Attachment 348274 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9008075

New failing tests:
inspector/canvas/recording-bitmaprenderer.html

[EWS Watchlist]

Created attachment 348284 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

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

[EWS Watchlist]

Comment on attachment 348274 [details]
Patch

Attachment 348274 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9008056

New failing tests:
inspector/canvas/recording-bitmaprenderer.html

[EWS Watchlist]

Created attachment 348285 [details]
Archive of layout-test-results from ews117 for mac-sierra

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

[Devin Rousso]

Created attachment 348300 [details]
Patch

Forgot to update a few functions

[Joseph Pecoraro]

Comment on attachment 348300 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:448
> -        let snapshotIndex = Math.floor(this._index / WI.RecordingContentView.SnapshotInterval);
> -        if (!isNaN(this._index) && this._snapshots[snapshotIndex])
> -            this._snapshots[snapshotIndex].element.classList.toggle("show-grid", activated);
> +        if (!isNaN(this._index))
> +            this._previewContainer.firstElementChild.classList.toggle("show-grid", activated);

Why this change?

[Devin Rousso]

Comment on attachment 348300 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:448
>> +            this._previewContainer.firstElementChild.classList.toggle("show-grid", activated);
> 
> Why this change?

This is due to the fact that `_generateContentFromSnapshot` doesn't follow the same `WI.RecordingContentView.SnapshotInterval` as `_generateContentCanvas2D`.  The snapshot determines the index based on the last visual action before the currently selected action, meaning that most of the time the expected snapshot element (based on `WI.RecordingContentView.SnapshotInterval`) will be null, as the "current snapshot for the given index" will actually be the last selected visual action's index.  It is expected that the first child of `_previewContainer` is the preview <canvas>/<img>.

[Devin Rousso]

Created attachment 349742 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 349742 [details]
Patch

Clearing flags on attachment: 349742

Committed r236008: <https://trac.webkit.org/changeset/236008>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44459256>