-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Web Inspector: Canvas: expand OffscreenCanvas support #13845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
EWS run on previous version of this PR (hash 34e99dc) |
EWS run on previous version of this PR (hash c31da8f) |
EWS run on previous version of this PR (hash c1de1ac) |
Thanks! Looks very good, but I'm not 100% confident to review. Waiting for @patrickangle a bit. |
EWS run on previous version of this PR (hash 120ef6a) |
EWS run on previous version of this PR (hash a041570) |
EWS run on previous version of this PR (hash b349d75) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes me wonder if "offscreen" should just be another boolean instead of a context type? (Better to do so know when we only have to shim compatibility for a single existing value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id prefer things be explicit like this as it means we can immediately understand what's going on rather than having to look for the source of truth in two places (especially since the API is not always the same between main page and offscreen)
also, if a context type is added in the future that only exists in one then that would be more annoying to express
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto from Canvas.json:27
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm getting a sense of deja vu, but I still wish these method names were more specific (e.g. getCanvasCurrentX
). But I know there is a reason we didn't that I've since forgotten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we definitely coulda done that, but IMO the argument type should be a good enough clue as to what's going on (at least in C++)
i think you're thinking of Source/WebInspectorUI/UserInterface/Base/IDLExtensions.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't super consistent about this, but should this be 2D @ Canvas Context Type
(same below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's correct that we begin translating these color space names. To my knowledge sRGB and Display P3 aren't typically localized. For example, the product page for Apple Studio Display in France says Internet et Web (sRGB)
and in the US says Internet and Web (sRGB)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id rather leave that up to the localizers, as that may not always be true in every language (e.g. Japanese might prefer to use katakana instead of english)
my approach is usually "unless what we're showing is a code snippet or something from the developer, everything should have the opportunity to be localized"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with the Recording
class could this be isCanvas2D
(same for WebGL ones below)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO that's a bit redundant since this is a WI.Canvas
object (e.g. id find it weird to read canvas.isCanvas2D
)
FWIW the only reason WI.Recording
uses isCanvas2D
instead of just is2D
is because there's technically nothing about WI.Recording
that's specific to <canvas>
. when i first built it many years ago my thinking was that we'd be able to reuse the general purpose data structure for other kinds of recordings (e.g. JS object mutations, DOM tree shenanigans, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is a bug tracking this, or if this is the most aspirational type of FIXME
(the type no one filed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there is <https://webkit.org/b/201856>
not sure why it isn't linked here 🤔
EWS run on previous version of this PR (hash e4cb508) |
EWS run on current version of this PR (hash f78b59e) |
https://bugs.webkit.org/show_bug.cgi?id=256747 Reviewed by Patrick Angle. `OffscreenCanvas` currently supports more than just `"2d"`, so Web Inspector should too. * Source/JavaScriptCore/inspector/protocol/Canvas.json: * Source/JavaScriptCore/inspector/protocol/Recording.json: * Source/JavaScriptCore/inspector/scripts/codegen/generator.py: * Source/WebCore/inspector/InspectorCanvas.cpp: (WebCore::InspectorCanvas::buildObjectForCanvas): (WebCore::InspectorCanvas::releaseObjectForRecording): Add `offscreen-*` variants for `bitmaprenderer`, `webgl`, and `webgl2`. Drive-by: Fix an issue that prevented getting the content of an `OffscreenCanvas`. * Source/WebInspectorUI/UserInterface/Models/Canvas.js: (WI.Canvas.fromPayload): (WI.Canvas.displayNameForContextType): (WI.Canvas.displayNameForColorSpace): (WI.Canvas.prototype.get supportsRecording): Added. (WI.Canvas.prototype.get is2D): Added. (WI.Canvas.prototype.get isBitmapRender): Added. (WI.Canvas.prototype.get isWebGL): Added. (WI.Canvas.prototype.get isWebGL2): Added. (WI.Canvas.prototype.is2D): Deleted. * Source/WebInspectorUI/UserInterface/Models/Recording.js: (WI.Recording): (WI.Recording.fromPayload): (WI.Recording.displayNameForRecordingType): (WI.Recording.prototype.get isCanvas): Added. (WI.Recording.prototype.get isCanvas2D): Added. (WI.Recording.prototype.get isCanvasBitmapRender): Added. (WI.Recording.prototype.get isCanvasWebGL): Added. (WI.Recording.prototype.get isCanvasWebGL2): Added. (WI.Recording.prototype.createContext): (WI.Recording.prototype.async _process): (WI.Recording.is2D): Deleted. Add helpers for identifying the context type regardless of whether it's from `HTMLCanvasElement` or `OffscreenCanvas` (since the API of the rendering context is (almost) identical in most cases). * Source/WebInspectorUI/UserInterface/Models/RecordingAction.js: (WI.RecordingAction._prototypeForType): (WI.RecordingAction.prototype.process): (WI.RecordingAction.prototype.process.getContent): (WI.RecordingAction.prototype.async swizzle): * Source/WebInspectorUI/UserInterface/Models/RecordingState.js: (WI.RecordingState.prototype.fromCanvasContext2D): Renamed from `fromContext`. (WI.RecordingState.async swizzleInitialState): * Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js: (WI.ShaderProgram): (WI.ShaderProgram.contextTypeSupportsProgramType): (WI.ShaderProgram.prototype.get displayName): (WI.ShaderProgram.prototype.set disabled): (WI.ShaderProgram.prototype.showHighlight): (WI.ShaderProgram.prototype.hideHighlight): * Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js: (WI.CanvasContentView.prototype.initialLayout): * Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js: (WI.CanvasSidebarPanel.prototype._canvasChanged): (WI.CanvasSidebarPanel.prototype._updateRecordNavigationItem): * Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js: (WI.RecordingContentView): (WI.RecordingContentView.prototype.get navigationItems): (WI.RecordingContentView.prototype.attached): (WI.RecordingContentView.prototype._export): (WI.RecordingContentView.prototype._updateExportButton): (WI.RecordingContentView.prototype._handleExportNavigationItemClicked): * Source/WebInspectorUI/UserInterface/Views/RecordingStateDetailsSidebarPanel.js: (WI.RecordingStateDetailsSidebarPanel.prototype.inspect): (WI.RecordingStateDetailsSidebarPanel.prototype.set action): * Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js: (WI.ShaderProgramContentView.prototype.get saveData): * Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js: (WI.ShaderProgramTreeElement): (WI.ShaderProgramTreeElement.prototype.onattach): (WI.ShaderProgramTreeElement.prototype.ondetach): (WI.ShaderProgramTreeElement.prototype.populateContextMenu): Use the new helpers instead of directly checking the `contextType`/`type` where able. * Source/WebCore/inspector/InspectorFrontendHost.idl: * Source/WebCore/inspector/InspectorFrontendHost.h: * Source/WebCore/inspector/InspectorFrontendHost.cpp: (WebCore::InspectorFrontendHost::getCurrentX const): (WebCore::InspectorFrontendHost::getCurrentY const): (WebCore::InspectorFrontendHost::getPath const): (WebCore::InspectorFrontendHost::setPath const): * Source/WebInspectorUI/UserInterface/Base/IDLExtensions.js: Add IDL extensions for `OffscreenCanvasRenderingContext2D`. * Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js: * LayoutTests/inspector/canvas/resources/recording-2d.js: (async load): * LayoutTests/inspector/canvas/resources/recording-bitmaprenderer.js: (load): * LayoutTests/inspector/canvas/resources/recording-webgl.js: (load): * LayoutTests/inspector/canvas/resources/recording-webgl2.js: (load): * LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgl.js: (createProgram): * LayoutTests/inspector/canvas/console-record-offscreen-bitmaprenderer.html: Added. * LayoutTests/inspector/canvas/console-record-offscreen-bitmaprenderer-expected.txt: Added. * LayoutTests/inspector/canvas/console-record-offscreen-canvas-2d.html: * LayoutTests/inspector/canvas/console-record-offscreen-canvas-2d-expected.txt: * LayoutTests/inspector/canvas/console-record-offscreen-webgl.html: Added. * LayoutTests/inspector/canvas/console-record-offscreen-webgl-expected.txt: Added. * LayoutTests/inspector/canvas/console-record-offscreen-webgl2.html: Added. * LayoutTests/inspector/canvas/console-record-offscreen-webgl2-expected.txt: Added. * LayoutTests/inspector/canvas/create-context-bitmaprenderer.html: * LayoutTests/inspector/canvas/create-context-bitmaprenderer-expected.txt: * LayoutTests/inspector/canvas/create-context-webgl.html: * LayoutTests/inspector/canvas/create-context-webgl-expected.txt: * LayoutTests/inspector/canvas/create-context-webgl2.html: * LayoutTests/inspector/canvas/create-context-webgl2-expected.txt: * LayoutTests/inspector/canvas/recording-offscreen-bitmaprenderer-frameCount.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-bitmaprenderer-frameCount-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-bitmaprenderer-full.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-bitmaprenderer-full-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-bitmaprenderer-memoryLimit.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-bitmaprenderer-memoryLimit-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-frameCount.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-frameCount-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl-frameCount-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-full.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-full-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl-full-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-memoryLimit.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-memoryLimit-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl-memoryLimit-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-snapshots.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl-snapshots-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl-snapshots-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-frameCount.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-frameCount-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl2-frameCount-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-full.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-full-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl2-full-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-memoryLimit.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-memoryLimit-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl2-memoryLimit-expected.txt: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-snapshots.html: Added. * LayoutTests/inspector/canvas/recording-offscreen-webgl2-snapshots-expected.txt: Added. * LayoutTests/platform/mac-wk1/inspector/canvas/recording-offscreen-webgl2-snapshots-expected.txt: Added. * LayoutTests/platform/gtk/TestExpectations: * LayoutTests/platform/mac-wk1/TestExpectations: * LayoutTests/platform/mac-wk2/TestExpectations: * LayoutTests/platform/mac/TestExpectations: Canonical link: https://commits.webkit.org/271300@main
f78b59e
to
78a4378
Compare
Committed 271300@main (78a4378): https://commits.webkit.org/271300@main Reviewed commits have been landed. Closing PR #13845 and removing active labels. |
78a4378
f78b59e
🧪 wpe-wk2