Review custom quickstubs' necessity

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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)

Updated

5 years ago
Assignee: nobody → Ms2ger
(Assignee)

Updated

5 years ago
Depends on: 732704
(Assignee)

Comment 2

5 years ago
Created attachment 602619 [details] [diff] [review]
Part a: Remove custom quickstub for bufferData
Attachment #602619 - Flags: review?(bjacob)
(Assignee)

Comment 3

5 years ago
Created attachment 602620 [details] [diff] [review]
Part b: Remove custom quickstub for bufferSubData
Attachment #602620 - Flags: review?(bjacob)
(Assignee)

Comment 4

5 years ago
Created attachment 602621 [details] [diff] [review]
Part c: Remove custom quickstubs for compressedTexImage2D and compressedTexSubImage2D
Attachment #602621 - Flags: review?(bjacob)
(Assignee)

Comment 5

5 years ago
Created attachment 602622 [details] [diff] [review]
Part d: Remove custom quickstub for readPixels
Attachment #602622 - Flags: review?(bjacob)
(Assignee)

Comment 6

5 years ago
Created attachment 602623 [details] [diff] [review]
Part e: Remove custom quickstubs for uniform[1-4][i,f]v and uniformMatrix[2-4]fv
Attachment #602623 - Flags: review?(bjacob)
(Assignee)

Comment 7

5 years ago
Created attachment 602624 [details] [diff] [review]
Part f: Remove custom quickstubs for vertexAttrib[1-4]fv
Attachment #602624 - Flags: review?(bjacob)
(Assignee)

Comment 8

5 years ago
(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+
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 12

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.