Bug 180736 – Web Inspector: add instrumentation for ImageBitmapRenderingContext

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

This includes:
 - Modifying the Inspector Protocol to support ImageBitmapRenderingContext canvases
 - Showing ImageBitmapRenderingContext instances in the Canvas tab
    - Being able to get the content of an ImageBitmapRenderingContext
 - Listing associated information (such as context attributes) when an ImageBitmapRenderingContext is selected
 - Supporting ImageBitmap as a swizzle type for recordings

[Devin Rousso]

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

[Devin Rousso]

Created attachment 329204 [details]
Patch

Attempt fixing non-mac builds

[Joseph Pecoraro]

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?

[Devin Rousso]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Comment on attachment 329204 [details]
Patch

r=me

[Devin Rousso]

Created attachment 329274 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329274 [details]
Patch

Clearing flags on attachment: 329274

Committed r225884: <https://trac.webkit.org/changeset/225884>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36036462>