Bug 180736

Created attachment 329199 [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`)

Created attachment 329204 [details] Patch Attempt fixing non-mac builds

Comment on attachment 329204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329204&action=review Patch looks great. One question below. > Source/WebCore/html/HTMLCanvasElement.cpp:289 > + auto scope = DECLARE_THROW_SCOPE(state.vm()); > + auto attributes = convert<IDLDictionary<ImageBitmapRenderingContextSettings>>(state, !arguments.isEmpty() ? arguments[0].get() : JSC::jsUndefined()); > + RETURN_IF_EXCEPTION(scope, Exception { ExistingExceptionError }); > + > + auto context = createContextBitmapRenderer(contextId, WTFMove(attributes)); Is this a web exposed change? It seems like you can now do: canvas.getContext("bitmaprender", {alpha: true}); And the attributes will be passed along now where they weren't before? If that is true then this needs a test for the alpha behavior. And could potentially be broken out into its own patch. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:96 > + return WI.unlocalizedString("Bitmap Renderer"); Unlocalized?

Comment on attachment 329204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329204&action=review >> Source/WebCore/html/HTMLCanvasElement.cpp:289 >> + auto context = createContextBitmapRenderer(contextId, WTFMove(attributes)); > > Is this a web exposed change? It seems like you can now do: > > canvas.getContext("bitmaprender", {alpha: true}); > > And the attributes will be passed along now where they weren't before? > > If that is true then this needs a test for the alpha behavior. And could potentially be broken out into its own patch. Yes, it's exposed. I was following the spec <https://html.spec.whatwg.org/multipage/canvas.html#the-imagebitmaprenderingcontext-interface>. From what I can tell, right now the ImageBitmapRenderingContext doesn't actually do anything with it's settings, so I don't think there's a risk of a regression. As far as tests, I've added ImageBitmapRenderingContext to the "inspector/canvas/context-attributes.html" test, so it is confirmed that this is working. >> Source/WebInspectorUI/UserInterface/Models/Canvas.js:96 >> + return WI.unlocalizedString("Bitmap Renderer"); > > Unlocalized? I think this should follow what we are doing with WebGL and WebGPU, as Bitmap Renderer isn't something I think we should localize. It's more of a object name (especially since you use "bitmaprenderer" with getContext) than a description.

Comment on attachment 329204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329204&action=review >>> Source/WebCore/html/HTMLCanvasElement.cpp:289 >>> + auto context = createContextBitmapRenderer(contextId, WTFMove(attributes)); >> >> Is this a web exposed change? It seems like you can now do: >> >> canvas.getContext("bitmaprender", {alpha: true}); >> >> And the attributes will be passed along now where they weren't before? >> >> If that is true then this needs a test for the alpha behavior. And could potentially be broken out into its own patch. > > Yes, it's exposed. I was following the spec <https://html.spec.whatwg.org/multipage/canvas.html#the-imagebitmaprenderingcontext-interface>. > > From what I can tell, right now the ImageBitmapRenderingContext doesn't actually do anything with it's settings, so I don't think there's a risk of a regression. > > As far as tests, I've added ImageBitmapRenderingContext to the "inspector/canvas/context-attributes.html" test, so it is confirmed that this is working. Okay, since it is unsupported then there is no observable difference we don't need a test until we support it. Hopefully we have a bug about supporting ImageBitmapRenderingContextSettings like alpha. > Source/WebCore/html/canvas/ImageBitmapRenderingContextSettings.idl:27 > + JSGenerateToJSObject, I don't see why this is needed. Do we actually toJS() this anywhere? I only saw the convert<> path below but I think we generate that without this attribute.

Comment on attachment 329204 [details] Patch r=me

Created attachment 329274 [details] Patch

Comment on attachment 329274 [details] Patch Clearing flags on attachment: 329274 Committed r225884: <https://trac.webkit.org/changeset/225884>

All reviewed patches have been landed. Closing bug.

<rdar://problem/36036462>