Last Comment Bug 722154 - Review custom quickstubs' necessity
: Review custom quickstubs' necessity
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on: 732704
Blocks: 711843 721230
  Show dependency treegraph
 
Reported: 2012-01-29 05:40 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-03-16 06:37 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part a: Remove custom quickstub for bufferData (10.48 KB, patch)
2012-03-03 08:33 PST, :Ms2ger (⌚ UTC+1/+2)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Part b: Remove custom quickstub for bufferSubData (8.45 KB, patch)
2012-03-03 08:34 PST, :Ms2ger (⌚ UTC+1/+2)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Part c: Remove custom quickstubs for compressedTexImage2D and compressedTexSubImage2D (10.17 KB, patch)
2012-03-03 08:34 PST, :Ms2ger (⌚ UTC+1/+2)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Part d: Remove custom quickstub for readPixels (9.10 KB, patch)
2012-03-03 08:35 PST, :Ms2ger (⌚ UTC+1/+2)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Part e: Remove custom quickstubs for uniform[1-4][i,f]v and uniformMatrix[2-4]fv (27.38 KB, patch)
2012-03-03 08:35 PST, :Ms2ger (⌚ UTC+1/+2)
jacob.benoit.1: review+
Details | Diff | Splinter Review
Part f: Remove custom quickstubs for vertexAttrib[1-4]fv (12.78 KB, patch)
2012-03-03 08:36 PST, :Ms2ger (⌚ UTC+1/+2)
jacob.benoit.1: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-01-29 05:40:03 PST
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.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-02-17 09:28:12 PST
They weren't; I've got patches to move everything but Tex{,Sub}Image2D to normal C++. I'll probably attach them next week.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:33:58 PST
Created attachment 602619 [details] [diff] [review]
Part a: Remove custom quickstub for bufferData
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:34:23 PST
Created attachment 602620 [details] [diff] [review]
Part b: Remove custom quickstub for bufferSubData
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:34:51 PST
Created attachment 602621 [details] [diff] [review]
Part c: Remove custom quickstubs for compressedTexImage2D and compressedTexSubImage2D
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:35:16 PST
Created attachment 602622 [details] [diff] [review]
Part d: Remove custom quickstub for readPixels
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:35:43 PST
Created attachment 602623 [details] [diff] [review]
Part e: Remove custom quickstubs for uniform[1-4][i,f]v and uniformMatrix[2-4]fv
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:36:05 PST
Created attachment 602624 [details] [diff] [review]
Part f: Remove custom quickstubs for vertexAttrib[1-4]fv
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-03-03 08:42:02 PST
(CustomQS_WebGL.h down to ~300 lines from ~1000 on trunk)
Comment 9 Benoit Jacob [:bjacob] (mostly away) 2012-03-04 20:15:52 PST
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?
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-03-05 01:16:51 PST
(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 11 Benoit Jacob [:bjacob] (mostly away) 2012-03-05 06:09:53 PST
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).

Note You need to log in before you can comment on or make changes to this bug.