The default bug view has changed. See this FAQ.

Paris bindings for WebGL canvas context

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

First part is bug 745897, but there are various other bits this depends on.
Depends on: 742144
Depends on: 748267
Depends on: 742145
Depends on: 749864
Depends on: 750264
Created attachment 626295 [details] [diff] [review]
Switch the WebGL canvas context to new DOM bindings.
Attachment #626295 - Flags: review?(peterv)
Whiteboard: [need review]
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 :-).
Attachment #626295 - Flags: review?(peterv) → review+
> 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.
Created attachment 628369 [details] [diff] [review]
Address review comments

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.
Attachment #628369 - Flags: review?(peterv)
Attachment #628369 - Flags: review?(bjacob)
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.
Attachment #628369 - Flags: review?(bjacob) → review+
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.
Created attachment 628533 [details] [diff] [review]
Now actually compiling and all
Attachment #628533 - Flags: review?(peterv)
Attachment #628533 - Flags: review?(bjacob)
Attachment #628369 - Attachment is obsolete: true
Attachment #628369 - Flags: review?(peterv)
Attachment #628533 - Flags: review?(bjacob) → review+
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?
Blocks: 759997
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)
Attachment #628533 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f96e0f078b49
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla15
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.
Target Milestone: mozilla15 → ---
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
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/5dd9299a5ac3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.

Updated

5 years ago
Depends on: 764166

Updated

5 years ago
No longer depends on: 764166

Updated

5 years ago
Depends on: 765198

Updated

5 years ago
Depends on: 776243
Depends on: 775852
You need to log in before you can comment on or make changes to this bug.