1212477 - Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Matteo Ferretti [:zer0] [:matteo]]

Currently we don't have a proper way to access to a context in a <canvas> element injected using Anonymous Content API; because we don't have access to the node.

We should have a method that, giving the ID of an element in the Anonymous Content, and the type of the context we want to ("2d", or "webgl"), it returns the context's object of the specified element if it's a <canvas>, or `null` otherwise.

Comment 1

[(no longer active)]

Here is how to do this:

1. Copy the getContext declaration from HTMLCanvasElement.webidl to AnonymousContent.webidl.
2. You will need to add a GetContext method to the C++ AnonymousContext object similar to this: <https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLCanvasElement.h#129>.
3. In the definition of that method, you first need to make sure that you're actually dealing with a canvas element.  You can do that by calling IsHTMLElement(nsGkAtoms::canvas) on your element.  Once that passes, you can static_cast it to an HTMLCanvasElement* and forward the call to GetContext() to that method.
4. One problem is that the CanvasRenderingContext2D and WebGLRenderingContext interfaces both have a canvas property that can give you back the DOM node, which is undesirable here.  In order to solve that, you need to do some surgery on the underlying C++ objects to make them return null from their GetCanvas() methods.  One simple way to do that is to add a new method to nsICanvasRenderingContextInternal which would set a flag on the object telling it to return null from GetCanvas().
5. Add tests!

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Okay, the attachment is the last version of many and I hope I did the things right. The patch adds `getCanvasContext` method to anonymous content API, returns `null` if the element is not a <canvas>.

I handled the point four differently, because it seems to me that using a method to set an internal flag could lead to side effects. What I mean is: if any other methods is asking for the canvas' context, inserted in NAC, and then accessing the `canvas` property to the context, it could have different – unpredictable – result, depends if `getCanvasContext` was called before or not.

Since the issue here is that we do not want to mess with the DOM once we add elements to the NAC, I consistently returns `null` if the canvas element of the context is in the NAC.

I'm going to add the test in a separate, coming patch, in this bug.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

I noticed now I left the WebGL part out; what I noticed is that compared to the 2d context, the webgl context internally uses the `GetCanvas` method: 

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContext.cpp?from=webglcontext.cpp#526
https://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContext.cpp?from=webglcontext.cpp#762

I'm not sure what would be the side effect of having `GetCanvas` returns `null` in case we are in NAC, here.

In the worst scenario, I think we could probably doing what we have done for the events in NAC: we removed the original target, that points to the DOM node, from the javascript API.

Any thoughts?

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8685453 [details] [diff] [review]
Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API

Added some unit test and webgl fix for canvas's property.

Robert, since Ehsan is in vacation till 19th, Andrea mentioned to me that you could be a good reviewer for this patch, and maybe help me with the doubts I exposed above. Thanks!

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8685453 [details] [diff] [review]
Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API

Review of attachment 8685453 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!!! You will probably need DOM peer review too.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8685453 [details] [diff] [review]
Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API

>   HTMLCanvasElement* GetCanvas() const
>   {
>+    if (mCanvasElement->IsInAnonymousSubtree())
>+      return nullptr;
Always {} with if.

And you want to use IsInNativeAnonymousSubtree().
(IsInAnonymousSubtree() returns true also for elements in XBL etc)

> WebGLContext::GetCanvas(Nullable<dom::OwningHTMLCanvasElementOrOffscreenCanvas>& retval)
> {
>     if (mCanvasElement) {
>         MOZ_RELEASE_ASSERT(!mOffscreenCanvas);
>-        retval.SetValue().SetAsHTMLCanvasElement() = mCanvasElement;
>+
>+        if (mCanvasElement->IsInAnonymousSubtree()) {
ditto

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

I run all the linux's test on try; as suggested by Andrea. Here the current patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35c28f677a57

Here the patch with `isInNativeAnonymousSubtree` instead:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d670354afe3f

I don't think the errors are related to this patch, but I'd like to double check with you.

Before attach the new patch and set the checkin-needed flag, do I have to run on Try some specific tests on some other machine?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

those Marionette tests are odd, but indeed aren't probably about this bug.

The code looks cross platform, so there shouldn't be need for running more tests on try.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API; r=roc;r=smaug

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API; r=roc;r=smaug

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8687552 [details] [diff] [review]
Needs a way to access to <canvas>'s context (2d, webgl) from Anonymous Content API; r=roc;r=smaug

Almost forgot the brackets!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ac9ea5105b3

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/febd3461af7f

Comment 16

[Phil Ringnalda (:philor)]

That's a followup that may or may not be the right thing to do, skipping the test on B2G where it fails like https://treeherder.mozilla.org/logviewer.html#?job_id=17301603&repo=mozilla-inbound, "uncaught exception - TypeError: webgl is null". Perhaps the right thing to do is instead to test whether webgl is supported and only run those parts of the test if it is, since I didn't find a whole litter of manifest annotations saying that tests were disabled on b2g because of the lack of webgl support.

Comment 17

[Patrick Brosset <:pbro>]

The AnonymousContent API is documented on the wiki [1] for devtools people who want to create new highlighters.
Can you update the doc to mention getCanvasContext?

[1] https://wiki.mozilla.org/DevTools/Highlighter#The_AnonymousContent_API

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1ac9ea5105b3
https://hg.mozilla.org/mozilla-central/rev/febd3461af7f

Comment 19

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #17)
> The AnonymousContent API is documented on the wiki [1] for devtools people
> who want to create new highlighters.
> Can you update the doc to mention getCanvasContext?
> 
> [1] https://wiki.mozilla.org/DevTools/Highlighter#The_AnonymousContent_API

I added the new method on wiki, but I noticed the document is quite outdated; specifically it mention `tabActor` instead of `HighlighterEnvironment`. I was going to file a bug about it, but I didn't find a documentation component for the devtools / inspector. Any idea under which category I should file it?

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #19)
> (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #17)
> > The AnonymousContent API is documented on the wiki [1] for devtools people
> > who want to create new highlighters.
> > Can you update the doc to mention getCanvasContext?
> > 
> > [1] https://wiki.mozilla.org/DevTools/Highlighter#The_AnonymousContent_API
> 
> I added the new method on wiki, but I noticed the document is quite
> outdated; specifically it mention `tabActor` instead of
> `HighlighterEnvironment`. I was going to file a bug about it, but I didn't
> find a documentation component for the devtools / inspector. Any idea under
> which category I should file it?
Well we don't always file bugs for doc I think, but you can probably file it in the global devtools component.