745897 - Add Paris binding APIs to WebGLContext

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Tracker for work needed to get WebGL into the new faster setup.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I'm going to mutate this bug ato be about just adding the new-binding APIs to WebGLContext and file a new tracker for the whole work.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

This passes try with two exceptions:

1)  Android is red due to bug 748237 and the fatal warnings content/canvas/src uses.
2)  The WebGL conformance test that passes -8000000000 as the offset to vertexAttribPointer still fails.  Waiting for a decision on how we want to handle that.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Review notes:  This is mostly tedious mechanical changes.  The main hard parts are in .h file (the new APIs), the .idl file (all the binaryname stuff to deal with name collisions on void methods that don't take object arguments), and the methods that used to return nsIVariant* and now return JS::Value.

Oh, and the change from GetConcreteObject to ValidateObject*.

Reading the .h changes is probably the best place to start.

I'll obviously make sure the two issues from comment 2 are fixed before landing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Add some more methods the spec IDL used to be missing.

Comment 5

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

Comment on attachment 618464 [details] [diff] [review]
Add some more methods the spec IDL used to be missing.

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

r+ with some nits. I didn't want to delay landing this so r+'d but please have a look at these nits, in particular, I would like us to avoid introducing new macros for generating declarations/definitions. Some questions, too.

This patch only changes WebGLContext. What about the other classes? Are other patches coming?

::: content/canvas/src/WebGLContext.cpp
@@ +786,5 @@
>  WebGLContext::GetContextAttributes(jsval *aResult)
>  {
> +    nsresult rv = NS_OK;
> +    JSObject* obj = GetContextAttributes(rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

I remember seeing people on dev-platform saying that NS_ENSURE_SUCCESS should not be used in new code because it's evil to hide return statements in macros, but, up to you.

@@ +896,5 @@
>  NS_IMETHODIMP
>  WebGLContext::GetExtension(const nsAString& aName, nsIWebGLExtension **retval)
>  {
> +    *retval = GetExtension(aName);
> +    NS_IF_ADDREF(*retval);

I am unclear on where we need to addref and where the bindings do it for us. Just FYI. I'm not able to tell why we addref here but not in the other GetExtension variant.

::: content/canvas/src/WebGLContext.h
@@ +706,5 @@
> +#define GL_SAME_METHOD_4(glname, name, t1, t2, t3, t4)      \
> +    void name(t1 a1, t2 a2, t3 a3, t4 a4) {                 \
> +        if (!IsContextStable()) { return; }                 \
> +        MakeContextCurrent(); gl->f##glname(a1,a2,a3,a4);   \
> +    }

We'll soon get rid of these macros anyway, but that will be done in a follow-up bug.

@@ +772,5 @@
> +    void CopyTexSubImage2D(WebGLenum target, WebGLint level, WebGLint xoffset,
> +                           WebGLint yoffset, WebGLint x, WebGLint y,
> +                           WebGLsizei width, WebGLsizei height);
> +#define DECL_CREATOR(_type) \
> +    already_AddRefed<WebGL##_type> Create##_type();

Please, no macro for this! please type this line four times. We want to free ourselves of our macro addiction.

@@ +1366,5 @@
>      }
>  };
>  
> +// NOTE: When this class is switched to new DOM bindings, update the
> +// (then-slow) WrapObject calls in GetParameter and GetVertexAttrib.

Oh... so this patch is only switching WebGLContext, and moar patches will have to follow to switch other classes? Do you plan to write these patches or should I?

@@ +3178,5 @@
> +IMPL_CREATOR(Buffer)
> +IMPL_CREATOR(Framebuffer)
> +IMPL_CREATOR(Program)
> +IMPL_CREATOR(Renderbuffer)
> +IMPL_CREATOR(Texture)

Again, please don't introduce new such macros.

::: content/canvas/src/WebGLContextGL.cpp
@@ +3359,5 @@
>          }
> +    }
> +
> +    // Else preserving behavior, but I'm not sure this is correct per spec
> +    return JS::UndefinedValue();

Normally, we should never get there: the case of an invalid uniform location has been handled above, so we can only get here in case of a bug in the GL. That's why the WebGL spec can't easily specify this case. Returning undefined seems a decent choice; generating an exception would be another possibility.

::: dom/interfaces/canvas/nsIDOMWebGLRenderingContext.idl
@@ +608,5 @@
>    //  METHODS
>    //
>    jsval getContextAttributes();
>  
> +  [binaryname(MozActiveTexture)]

Do you really have to specify binaryname even for non-overloaded functions?

@@ +615,1 @@
>    void attachShader([optional] in nsIWebGLProgram program, [optional] in nsIWebGLShader shader);

So why did you have to specify binaryname for activeTexture, but not for attachShader?

Comment 6

[Boris Zbarsky [:bzbarsky]]

(In reply to Benoit Jacob [:bjacob] from comment #5)
> I didn't want to delay landing this

It's still blocked on Peter reviewing a bug it depends on, so no worries.  ;)  Thanks a ton for the quick review!

> This patch only changes WebGLContext. What about the other classes? Are
> other patches coming?

At some point, yes.  I can actually switch WebGLContext over to new bindings without doing the other classes (and we can ship that way if desired for risk-minimization), so I figured I'd focus on that first.  But yes, the next order of business is the other classes.

> I remember seeing people on dev-platform saying that NS_ENSURE_SUCCESS
> should not be used in new code

OK.  For what it's worth, these calls actually go away in bug 749485, so if it's OK I'll save myself the merge pain and not touch them here.  ;)

> >  WebGLContext::GetExtension(const nsAString& aName, nsIWebGLExtension **retval)
> 
> I am unclear on where we need to addref and where the bindings do it for us.
> Just FYI. I'm not able to tell why we addref here but not in the other
> GetExtension variant.

The above is the old-binding XPCOM method.  It expects an addref on the thing being returned.

The other GetExtension variant is the new-binding method; it allows returning non-addrefed values from callers tat are returning existing objects as opposed to creating new ones.  There's actually an out-of-band annotation (in a conf file for now, possibly in the IDL later) that tells the binding code generator whether to expect an addrefed value or not.

> Please, no macro for this! please type this line four times. We want to free
> ourselves of our macro addiction.

OK.  Will do.

> > +// NOTE: When this class is switched to new DOM bindings, update the
> > +// (then-slow) WrapObject calls in GetParameter and GetVertexAttrib.
> 
> Oh... so this patch is only switching WebGLContext, and moar patches will
> have to follow to switch other classes?

Yes.

> Do you plan to write these patches or should I?

I was generally planning to, but help is always nice.  ;)  The hardest part of moving the other things is actually adding all the cycle collection bits, I think.  And perhaps creating a new helper method to replace the WrapObject calls...  That's one reason it might make sense for me to do it: this is one of our early guinea pigs for the new binding infrastructure, and there are still rough edges.

> Again, please don't introduce new such macros.

Will fix.

> Normally, we should never get there: the case of an invalid uniform location
> has been handled above, so we can only get here in case of a bug in the GL.
> That's why the WebGL spec can't easily specify this case. Returning
> undefined seems a decent choice; generating an exception would be another
> possibility.

Up to you.  Both are really easy to do.  ;)

> > +  [binaryname(MozActiveTexture)]
> 
> Do you really have to specify binaryname even for non-overloaded functions?

The problem is that it's overloaded.  Since we want to have a kill-switch on the new bindings, initially WebGLContext will support both the new-binding and XPCOM APIs; which one will get used depends on a pref setting.  So without the binaryname bit there, it would have two methods with the following signatures:

  void ActiveTexture(WebGLenum);     // New bindings
  nsresult ActiveTexture(WebGLenum); // Old bindings

which the compiler gets all sorts of unhappy with, obviously.  ;)

Once we decide that new bindings for this class are no longer pref-controlled, we can remove all the old-binding methods as well as the XPIDL file.  That will mean no more XPCOM access to the WebGL context from C++, though, but that might be OK.  As it is, actually _using_ it from C++ is pretty hard...

> So why did you have to specify binaryname for activeTexture, but not for attachShader?

Because for attachShader, the two signatures involved look like this:

  nsresult AttachShader(nsIWebGLProgram *, nsIWebGLShader *); // Old bindings
  void AttachShader(WebGLProgram *, WebGLShader *);           // New bindings

Note the lack of "nsI" on the arguments in the new bindings.  That's enough for the compiler to be happy.  So in general, I only had to add the [binaryname] on things whose signatures did not change: void methods that are infallible and all of whose arguments are numeric types, not objects.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/838abb8c06d9

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/838abb8c06d9