Bug 180736 - Web Inspector: add instrumentation for ImageBitmapRenderingContext
Summary: Web Inspector: add instrumentation for ImageBitmapRenderingContext
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 172623
Blocks: WebInspectorCanvasTab 180805 180770
  Show dependency treegraph
 
Reported: 2017-12-12 20:52 PST by Devin Rousso
Modified: 2017-12-14 08:07 PST (History)
14 users (show)

See Also:


Attachments
Patch (147.43 KB, patch)
2017-12-12 21:01 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (148.03 KB, patch)
2017-12-12 21:57 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (147.37 KB, patch)
2017-12-13 15:47 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-12-12 20:52:53 PST
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
Comment 1 Devin Rousso 2017-12-12 21:01:20 PST
Created attachment 329199 [details]
Patch
Comment 2 EWS Watchlist 2017-12-12 21:02:36 PST
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 3 Devin Rousso 2017-12-12 21:57:48 PST
Created attachment 329204 [details]
Patch

Attempt fixing non-mac builds
Comment 4 Joseph Pecoraro 2017-12-13 10:58:10 PST
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 5 Devin Rousso 2017-12-13 11:26:15 PST
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 6 Joseph Pecoraro 2017-12-13 11:41:17 PST
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 7 Joseph Pecoraro 2017-12-13 11:46:22 PST
Comment on attachment 329204 [details]
Patch

r=me
Comment 8 Devin Rousso 2017-12-13 15:47:26 PST
Created attachment 329274 [details]
Patch
Comment 9 WebKit Commit Bot 2017-12-13 16:57:39 PST
Comment on attachment 329274 [details]
Patch

Clearing flags on attachment: 329274

Committed r225884: <https://trac.webkit.org/changeset/225884>
Comment 10 WebKit Commit Bot 2017-12-13 16:57:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2017-12-13 16:58:29 PST
<rdar://problem/36036462>