It seems to me that the functions introduced in bug 721230 don't actually need to be custom quickstubs, but could get there by taking jsval arguments and using [optional_argc] in IDL. These probably aren't alone.

They weren't; I've got patches to move everything but Tex{,Sub}Image2D to normal C++. I'll probably attach them next week.

(CustomQS_WebGL.h down to ~300 lines from ~1000 on trunk)

Comment on attachment 602623 [details] [diff] [review] Part e: Remove custom quickstubs for uniform[1-4][i,f]v and uniformMatrix[2-4]fv Review of attachment 602623 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextGL.cpp @@ +4051,5 @@ > } > > +template<size_t type> > +static JSObject* > +GetTypedArray(JSContext* aCx, const JS::Value& aValue) Although there is a large legacy of code that uses 'a' prefixes for function arguments, many developers seem to agree that this isn't very useful. How about not using this practice in new code? @@ +4057,5 @@ > + if (!aValue.isObject()) { > + return NULL; > + } > + > + JSObject& value = aValue.toObject(); I find the name |value| confusing here. Why not |object| ? Also, I think you want a const reference here: anyway GetObjectClass takes a pointer to const. Also, since all the uses below seem to want a pointer, why not make |value| a pointer? All in all, replace JSObject& value by const JSObject* object sounds good?

(In reply to Benoit Jacob [:bjacob] from comment #9) > Comment on attachment 602623 [details] [diff] [review] > Part e: Remove custom quickstubs for uniform[1-4][i,f]v and > uniformMatrix[2-4]fv > > Review of attachment 602623 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/canvas/src/WebGLContextGL.cpp > @@ +4051,5 @@ > > } > > > > +template<size_t type> > > +static JSObject* > > +GetTypedArray(JSContext* aCx, const JS::Value& aValue) > > Although there is a large legacy of code that uses 'a' prefixes for function > arguments, many developers seem to agree that this isn't very useful. How > about not using this practice in new code? How about fixing the legacy code to use a prefixes to match the style guide? Many developers seem to agree they're useful. > @@ +4057,5 @@ > > + if (!aValue.isObject()) { > > + return NULL; > > + } > > + > > + JSObject& value = aValue.toObject(); > > I find the name |value| confusing here. Why not |object| ? > > Also, I think you want a const reference here: anyway GetObjectClass takes a > pointer to const. > > Also, since all the uses below seem to want a pointer, why not make |value| > a pointer? > > All in all, replace > > JSObject& value > > by > > const JSObject* object > > sounds good? JS_IsArrayObject takes a non-const pointer. Can do JSObject* object if you prefer, though I like references to make it clear the object can't be null.

Comment on attachment 602623 [details] [diff] [review] Part e: Remove custom quickstubs for uniform[1-4][i,f]v and uniformMatrix[2-4]fv Review of attachment 602623 [details] [diff] [review]: ----------------------------------------------------------------- Alright, I didn't realize about IsArrayObject (why the heck does it take ptr to nonconst????) and I didn't want to block this review on an argument about the 'a' prefixes (we need to have that conversation elsewhere, maybe on dev-platform).

https://hg.mozilla.org/mozilla-central/rev/df19889893e6 https://hg.mozilla.org/mozilla-central/rev/5a4131e4b7da https://hg.mozilla.org/mozilla-central/rev/b0c120ba1da1 https://hg.mozilla.org/mozilla-central/rev/d7a496a81f66 https://hg.mozilla.org/mozilla-central/rev/265fe1dacc91 https://hg.mozilla.org/mozilla-central/rev/ee56787a20fb