748266 - Paris bindings for WebGL canvas context

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

First part is bug 745897, but there are various other bits this depends on.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Switch the WebGL canvas context to new DOM bindings.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 626295 [details] [diff] [review]
Switch the WebGL canvas context to new DOM bindings.

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

::: dom/bindings/Bindings.conf
@@ +124,5 @@
>  
> +'HTMLCanvasElement': {
> +     # WebGL only cares about the fact that this is an element, and
> +     # we have fast-ish unwrapping to nsGenericElement already.
> +    'nativeType': 'nsGenericElement',

I can't say I understand the implication here, but it seems scary that we would pass through a non-HTMLCanvasElement if the WebIDL file doesn't allow that... Would it make more sense to just put `Element` in the IDL for now?

::: dom/webidl/WebGLRenderingContext.webidl
@@ +8,5 @@
> + *
> + * Copyright © 2012 Khronos Group
> + */
> +
> + // AUTOGENERATED FILE -- DO NOT EDIT -- SEE Makefile

This confused me, fwiw.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> but it seems scary that we would pass through a non-HTMLCanvasElement if the WebIDL file
> doesn't allow that

Well, the callee would just get an nsGenericElement*.  So it would be pretty obvious that it could be any element.

> Would it make more sense to just put `Element` in the IDL for now?

We could do that, but when we start converting individual elements we'll end up back at this behavior, I suspect, in the short term.

> This confused me, fwiw.

I suppose I could remove it I was trying to minimize the edits to the official IDL.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 626295 [details] [diff] [review]
Switch the WebGL canvas context to new DOM bindings.

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

::: content/canvas/src/WebGLContext.h
@@ +556,5 @@
>          return HTMLCanvasElement();
>      }
>  
> +    virtual JSObject* WrapObject(JSContext *cx, JSObject *scope,
> +                                 bool *triedToWrap)

Because these are virtual anyway I've generally defined them in the cpp.

::: dom/bindings/Bindings.conf
@@ +128,5 @@
> +    'nativeType': 'nsGenericElement',
> +    'prefable': True,
> +    'castable': False
> +},
> +

I've started using

+# These are temporary, until they've been converted to use new DOM bindings
+def addExternalIface(iface, native=None, headerFile=None):
+    if native is None:
+        native = 'nsIDOM' + iface
+    domInterface = {
+        'nativeType': native,
+        'concrete': False,
+        'castable': False
+    }
+    if not headerFile is None:
+        domInterface['headerFile'] = headerFile
+    DOMInterfaces[iface] = domInterface

and

+# If you add one of these, you need to make sure nsDOMQS.h has the relevant
+# macros added for it
+def addExternalHTMLElement(element):
+   nativeElement = 'ns' + element
+   addExternalIface(element, native=nativeElement,
+                   headerFile=nativeElement + '.h')

(with the patch for bug 759275 applied) and just put define these at the end of the file. I realize all these will eventually be converted to new bindings, but I find it confusing to have both new and old in the same list.

@@ +259,5 @@
> +  'resultNotAddRefed': [ 'canvas', 'getContextAttributes', 'getExtension',
> +                         'getAttachedShaders' ],
> +  'implicitJSContext': [ 'getUniform', 'texImage2D', 'texSubImage2D',
> +                         'getParameter', 'getFramebufferAttachmentParameter',
> +                         'getVertexAttrib' ],

Might make sense to start passing cx for typed arrays and |any| arguments and return types?

@@ +303,5 @@
> +                  'vertexAttrib1f', 'vertexAttrib1fv', 'vertexAttrib2f',
> +                  'vertexAttrib2fv', 'vertexAttrib3f', 'vertexAttrib3fv',
> +                  'vertexAttrib4f', 'vertexAttrib4fv', 'vertexAttribPointer',
> +                  'viewport'
> +                  ]

Yikes :-).

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Because these are virtual anyway I've generally defined them in the cpp.

OK, will do.

> I've started using

That's a good idea.  Are addExternalIface and addExternalHTMLElement something you plan to land soon, or should I land them as part of this bug?

> Might make sense to start passing cx for typed arrays and |any| arguments and return
> types?

It's not generally needed for typed arrays; the canvas context code only needed it because it wanted to do weird stuff with them.

Agreed about "any" args and retvals.  Do you want me to roll that into this patch?

> Yikes :-).

Hence bug 757164!

Comment 6

[Boris Zbarsky [:bzbarsky]]

To be clear, I'm waiting on answers to comment 5 before landing.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So we're going to end up with a bit of a behavior change here.  Right now, texImage2D and texSubImage2D accept not only <img>, <canvas>, and <video> but also anything else that's an image loading content showing an image (<svg:image>, <input type="image">, <object> showing an image, <svg:feImage>).  But if I enforce unwrapping to the right HTML element types, those all go away.

That's what the WebIDL spec calls for right now, but I'm not sure whether that's correct (and in particular, excluding <svg:image> is pretty weird).  Benoit, roc, thoughts?

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

The current behavior, that we accept arbitrary DOM elements, is a bug, and something that's been worrying me for a while (I wasn't sure that that was the case). 

The new behavior that you describe (accept only img, video, canvas) is very much wanted! As you note, it is what the spec requires.

I have no clue about SVG but feel free to have that discussion on the WebGL mailing list.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Address review comments

Benoit, could you take a look at the WebGL changes?  Peter, everything else.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 628369 [details] [diff] [review]
Address review comments

Er, and actually setting the review requests.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 628369 [details] [diff] [review]
Address review comments

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

Just this remark, the rest looks OK:

::: content/canvas/src/WebGLContextGL.cpp
@@ +4310,5 @@
>      MakeContextCurrent();
>      gl->fStencilOpSeparate(face, sfail, dpfail, dppass);
>  }
>  
> +template<class ElementType>

It's not great to templatize a relatively large function just for a small part of it: it's a way to get a bloated executable. In this case though, it's not too, too bad.

But why is this needed at all? As far as I can see, |imageOrCanvas| is only passed to SurfaceFromElement, which takes a dom::Element*. So why not just leave this a dom::Element* parameter?

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Also: suggest renaming |imageOrCanvas| to |element|.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> SurfaceFromElement, which takes a dom::Element*.

I'm planning to change that, in a followup bug.  I'll see if I can reduce the amount of templatized code.

> Also: suggest renaming |imageOrCanvas| to |element|.

Can do!

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 628369 [details] [diff] [review]
Address review comments

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

OK, so with this explanation, this makes sense.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  That templating stuff might not work at all, actually, since the declarations are in the header but the impl is in the .cpp and the compiler won't know which instantiations to create.  Doing a full build to check now.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

If needed, you could always move it to WebGLContext.h which is already a kludge anyway (yes, I plan to fix that soon and properly divide the WebGL code into files) or to a new header.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Now actually compiling and all

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 628533 [details] [diff] [review]
Now actually compiling and all

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

::: content/canvas/src/WebGLContext.h
@@ +1235,5 @@
>                        WebGLTexelFormat dstFormat, bool dstPremultiplied,
>                        size_t dstTexelSize);
>  
> +    template<class ElementType>
> +    nsLayoutUtils::SurfaceFromElementResult SurfaceFromElement(ElementType* aElement) {

I don't get it. Why templatize this if you don't add nsLayoutUtils::SurfaceFromElement overloads?

::: content/html/content/src/nsHTMLImageElement.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsHTMLImageElement_h
> +#define nsHTMLImageElement_h

I'd prefer if you exported this.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Why templatize this if you don't add nsLayoutUtils::SurfaceFromElement overloads?

See bug 759997.

> I'd prefer if you exported this.

Yeah, I considered that.  nsGenericHTMLElement is not exported, though, so exporting just nsHTMLImageElement doesn't buy much afaict.  Unless I'm missing something?

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 628533 [details] [diff] [review]
Now actually compiling and all

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

::: content/html/content/public/nsHTMLCanvasElement.h
@@ +178,5 @@
>    bool                     mWriteOnly;
>  };
>  
> +inline nsISupports*
> +GetISupports(nsHTMLCanvasElement* p)

nsDOMQS.h and qsObjectHelper.h already have ToSupports.

::: dom/bindings/BindingUtils.h
@@ +501,5 @@
>             const nsIID* iid, JS::Value* vp)
>  {
>    if (xpc_FastGetCachedWrapper(cache, scope, vp))
>      return true;
> +  qsObjectHelper helper(GetISupports(p), cache);

This shouldn't be needed if you add a ToSupports taking nsHTMLCanvasElement* (or maybe even nsINode*).

::: dom/bindings/Codegen.py
@@ +2166,5 @@
>          if not needsCx:
> +            needsCx = reduce(lambda b, a: (b or
> +                                           a.type.isObject() or
> +                                           a.type.isAny() or
> +                                           a.type.isCallback()), arguments,

I wonder if we should have a |typeNeedsCx(type)|, drop the second return value of getRetvalDeclarationForType and make this:

        needsCx = typeNeedsCx(returnType) or any(typeNeedsCx(a.type) for a in arguments)

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

And backed out for two reasons:

1)  M1 orange because of a bug in the WebGL test suite that I'll fix as I reland.
2)  Crashtest orange (crash) for some reason I need to look into.

Comment 23

[Boris Zbarsky [:bzbarsky]]

The crashtest was due to a bug in the fix for bug 759275.  Peter landed a fix for that, and I repushed this as https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd9299a5ac3

Comment 24

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/5dd9299a5ac3

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can someone raise a spec issue for the types of elements we're allowing?  In particular, I think <svg:image> should almost certainly be allowed.

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

Boris already started a thread about that on the public_webgl mailing list, a couple days ago.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I guess we do the same for drawImage on the 2D context... Can someone file that?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=17427

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 626295 [details] [diff] [review]
Switch the WebGL canvas context to new DOM bindings.

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

::: js/xpconnect/src/nsDOMQS.h
@@ +13,5 @@
> +#include "nsIDocument.h"
> +#include "nsINodeList.h"
> +#include "nsICSSDeclaration.h"
> +#include "nsDocument.h"
> +#include "nsGenericHTMLElement.h"

This line cause build break for configs without MOZ_MEDIA enabled. Error messages:

In file included from ../../../dist/include/nsHTMLVideoElement.h:10:0,
                 from /home/vicamo/WorkSpace/mozilla/b2g/x86_64-unknown-linux-gnu/js/xpconnect/src/nsDOMQS.h:13,
                 from /home/vicamo/WorkSpace/mozilla/b2g/x86_64-unknown-linux-gnu/obj-x86_64-unknown-linux-gnu/js/xpconnect/src/dom_quickstubs.cpp:151:
../../../dist/include/nsHTMLMediaElement.h:11:28: fatal error: nsMediaDecoder.h: No such file or directory
compilation terminated.
make[6]: *** [dom_quickstubs.o] Error 1

I'll file a bug for this.

Comment 30

[Boris Zbarsky [:bzbarsky]]

There is already a bug, and building without MOZ_MEDIA is not supported last I checked....  In particular, it's not compatible with pretty explicit WebIDL requirements for the canvas contexts.

Comment 31

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #30)
> There is already a bug, and building without MOZ_MEDIA is not supported last
> I checked....  In particular, it's not compatible with pretty explicit
> WebIDL requirements for the canvas contexts.

Hmm, I think if you do --disable-webm --disable-ogg, you'll get to build without MOZ_MEDIA.  If that is not supported, this is something that the build system should warn against, right?

Comment 32

[(not currently active) Ted Mielczarek]

Whether we support building with an option or not is up to the module owners whose code the option impacts. I don't really care, except that if you can flip an option using configure arguments, then we should either be able to build in that configuration or we should remove the option.