Use the protocol/instrumentation logic created in <https://webkit.org/b/174481> Web Inspector: create protocol functions for recording Canvas contexts
Created attachment 330577 [details] Patch
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`)
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
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
> 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" \ > ;
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.
Created attachment 331537 [details] Patch Simplified the test output
(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
So what is an ImageBitmapRenderingContext and why would we want to record it?
(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
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.
(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.
Created attachment 348273 [details] Patch Rebase
Created attachment 348274 [details] Patch Forgot new test file
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
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
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
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
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
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
Created attachment 348300 [details] Patch Forgot to update a few functions
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?
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>.
Created attachment 349742 [details] Patch
Comment on attachment 349742 [details] Patch Clearing flags on attachment: 349742 Committed r236008: <https://trac.webkit.org/changeset/236008>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44459256>