Closed
Bug 748266
Opened 13 years ago
Closed 13 years ago
Paris bindings for WebGL canvas context
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
54.91 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
53.36 KB,
patch
|
bjacob
:
review+
peterv
:
review+
|
Details | Diff | Splinter Review |
First part is bug 745897, but there are various other bits this depends on.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #626295 -
Flags: review?(peterv)
Assignee | ||
Updated•13 years ago
|
Whiteboard: [need review]
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
> 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•13 years ago
|
||
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+
Assignee | ||
Comment 5•13 years ago
|
||
> 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!
Assignee | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
Benoit, could you take a look at the WebGL changes? Peter, everything else.
Assignee | ||
Comment 10•13 years ago
|
||
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 11•13 years ago
|
||
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•13 years ago
|
||
Also: suggest renaming |imageOrCanvas| to |element|.
Assignee | ||
Comment 13•13 years ago
|
||
> 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•13 years ago
|
||
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+
Assignee | ||
Comment 15•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
Attachment #628533 -
Flags: review?(peterv)
Attachment #628533 -
Flags: review?(bjacob)
Assignee | ||
Updated•13 years ago
|
Attachment #628369 -
Attachment is obsolete: true
Attachment #628369 -
Flags: review?(peterv)
Updated•13 years ago
|
Attachment #628533 -
Flags: review?(bjacob) → review+
Comment 18•13 years ago
|
||
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.
Assignee | ||
Comment 19•13 years ago
|
||
> 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 20•13 years ago
|
||
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+
Assignee | ||
Comment 21•13 years ago
|
||
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla15
Assignee | ||
Comment 22•13 years ago
|
||
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 → ---
Assignee | ||
Comment 23•13 years ago
|
||
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
Comment 24•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 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.
Comment 26•13 years ago
|
||
Boris already started a thread about that on the public_webgl mailing list, a couple days ago.
Comment 27•13 years ago
|
||
I guess we do the same for drawImage on the 2D context... Can someone file that?
Assignee | ||
Comment 28•13 years ago
|
||
Comment 29•12 years ago
|
||
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.
Assignee | ||
Comment 30•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Depends on: CVE-2012-3968
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•