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

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.

> 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 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 :-).

> 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!

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

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?

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.

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

Comment on attachment 628369 [details] [diff] [review] Address review comments Er, and actually setting the review requests.

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?

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

> 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 on attachment 628369 [details] [diff] [review] Address review comments Review of attachment 628369 [details] [diff] [review]: ----------------------------------------------------------------- OK, so with this explanation, this makes sense.

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.

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 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.

> 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 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)

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.

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

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.

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

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

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

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.

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.

(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?

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.