Closed Bug 722154 Opened 8 years ago Closed 8 years ago

Review custom quickstubs' necessity

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(6 files)

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.
Status: NEW → ASSIGNED
Assignee: nobody → Ms2ger
Depends on: 732704
(CustomQS_WebGL.h down to ~300 lines from ~1000 on trunk)
Attachment #602619 - Flags: review?(bjacob) → review+
Attachment #602620 - Flags: review?(bjacob) → review+
Attachment #602621 - Flags: review?(bjacob) → review+
Attachment #602622 - Flags: review?(bjacob) → review+
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?
Attachment #602623 - Flags: review?(bjacob) → review-
Attachment #602624 - Flags: review?(bjacob) → review+
(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.
Attachment #602623 - Flags: review- → review?(bjacob)
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).
Attachment #602623 - Flags: review?(bjacob) → review+
Blocks: 711843
You need to log in before you can comment on or make changes to this bug.