Last Comment Bug 748266 - Paris bindings for WebGL canvas context
: Paris bindings for WebGL canvas context
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 742144 742145 745897 748267 749864 750264 765198 CVE-2012-3968 776243
Blocks: ParisBindings 759997
  Show dependency treegraph
 
Reported: 2012-04-24 01:13 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-07-25 19:05 PDT (History)
7 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Switch the WebGL canvas context to new DOM bindings. (54.91 KB, patch)
2012-05-22 19:51 PDT, Boris Zbarsky [:bz] (still a bit busy)
peterv: review+
Details | Diff | Splinter Review
Address review comments (46.31 KB, patch)
2012-05-30 10:06 PDT, Boris Zbarsky [:bz] (still a bit busy)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Now actually compiling and all (53.36 KB, patch)
2012-05-30 16:33 PDT, Boris Zbarsky [:bz] (still a bit busy)
jacob.benoit.1: review+
peterv: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-04-24 01:13:48 PDT
First part is bug 745897, but there are various other bits this depends on.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2012-05-22 19:51:45 PDT
Created attachment 626295 [details] [diff] [review]
Switch the WebGL canvas context to new DOM bindings.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-05-23 07:18:13 PDT
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 [:bz] (still a bit busy) 2012-05-23 07:26:07 PDT
> 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] 2012-05-29 08:16:35 PDT
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 [:bz] (still a bit busy) 2012-05-29 21:15:15 PDT
> 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 [:bz] (still a bit busy) 2012-05-30 07:48:57 PDT
To be clear, I'm waiting on answers to comment 5 before landing.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-05-30 09:32:35 PDT
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) 2012-05-30 09:46:36 PDT
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 [:bz] (still a bit busy) 2012-05-30 10:06:15 PDT
Created attachment 628369 [details] [diff] [review]
Address review comments

Benoit, could you take a look at the WebGL changes?  Peter, everything else.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-05-30 10:28:16 PDT
Comment on attachment 628369 [details] [diff] [review]
Address review comments

Er, and actually setting the review requests.
Comment 11 Benoit Jacob [:bjacob] (mostly away) 2012-05-30 12:41:42 PDT
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) 2012-05-30 12:42:26 PDT
Also: suggest renaming |imageOrCanvas| to |element|.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-05-30 12:47:03 PDT
> 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) 2012-05-30 12:48:26 PDT
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 [:bz] (still a bit busy) 2012-05-30 13:33:08 PDT
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) 2012-05-30 13:34:45 PDT
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 [:bz] (still a bit busy) 2012-05-30 16:33:00 PDT
Created attachment 628533 [details] [diff] [review]
Now actually compiling and all
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-05-31 02:06:59 PDT
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 [:bz] (still a bit busy) 2012-05-31 06:50:53 PDT
> 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] 2012-05-31 08:13:37 PDT
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 [:bz] (still a bit busy) 2012-05-31 11:36:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f96e0f078b49
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-05-31 14:14:52 PDT
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 [:bz] (still a bit busy) 2012-06-01 09:48:14 PDT
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 25 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-02 12:02:09 PDT
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) 2012-06-02 12:11:29 PDT
Boris already started a thread about that on the public_webgl mailing list, a couple days ago.
Comment 27 :Ms2ger (⌚ UTC+1/+2) 2012-06-02 12:43:09 PDT
I guess we do the same for drawImage on the 2D context... Can someone file that?
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2012-06-05 13:26:13 PDT
Filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=17427
Comment 29 Vicamo Yang [:vicamo][:vyang] 2012-06-06 12:18:09 PDT
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 [:bz] (still a bit busy) 2012-06-06 12:35:17 PDT
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 :Ehsan Akhgari 2012-06-06 12:45:07 PDT
(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 Ted Mielczarek [:ted.mielczarek] 2012-06-07 04:55:40 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.