Add Paris binding APIs to WebGLContext

RESOLVED FIXED in mozilla15

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: webgl-next)

Attachments

(1 attachment, 2 obsolete attachments)

Tracker for work needed to get WebGL into the new faster setup.
Depends on: 745899
Whiteboard: webgl-next
Blocks: 745840
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 745673
Blocks: 746733
Blocks: 746738
Depends on: 711843
Blocks: 747598
Depends on: 747825
Depends on: 747826
Depends on: 747827
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.
Summary: Paris bindings for WebGL → Add Paris binding APIs to WebGLContext
Blocks: 748211
Depends on: 748237
Created attachment 617817 [details] [diff] [review]
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.
Attachment #617817 - Flags: review?(bjacob)
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.
No longer blocks: 580070
Blocks: 748266
Created attachment 618464 [details] [diff] [review]
Add some more methods the spec IDL used to be missing.
Attachment #618464 - Flags: review?(bjacob)
Attachment #617817 - Attachment is obsolete: true
Attachment #617817 - Flags: review?(bjacob)
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?
Attachment #618464 - Flags: review?(bjacob) → review+
(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.
Blocks: 749485
Created attachment 618935 [details] [diff] [review]
Updated to comments
Attachment #618464 - Attachment is obsolete: true
Whiteboard: webgl-next → webgl-next [need blocking bugs fixed]
Blocks: 749539
https://hg.mozilla.org/integration/mozilla-inbound/rev/838abb8c06d9
Flags: in-testsuite+
Whiteboard: webgl-next [need blocking bugs fixed] → webgl-next
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/838abb8c06d9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 722082
You need to log in before you can comment on or make changes to this bug.