Kill these macros: OBTAIN_UNIFORM_LOCATION SIMPLE_ARRAY_METHOD_NO_COUNT SIMPLE_ARRAY_METHOD_UNIFORM SIMPLE_MATRIX_METHOD_UNIFORM etc. Factor out common code in helper functions instead of macros.

Do we want to do this before the new-binding work or after? I haven't gotten to the methods using this stuff yet, I think....

It can wait until after you're done: it's not an emergency, we're busy with other things, and it won't make your work a lot easier by itself.

So, anyone willing to take that bug: the main file to modify here is http://hg.mozilla.org/mozilla-central/file/tip/content/canvas/src/WebGLContextGL.cpp In this file, search for the macro nams listed in comment 0. See how these macros are used to generate several of the WebGLContext methods corresponding to WebGL uniform setters (for example WebGLContext::Uniform4fv implements the javascript webgl uniform4fv function). That's ugly. The goal of this bug is to get rid of these huge macros. If common code needs to be factored in a shared helper, that should be a function rather than a macro.

Hi Benoit, I'll take a look at it and check it out! I'll come and comment here if I'm stuck or anything.

Hi Peter, Saurabh expressed interest earlier today -- please check with him. To both of you: do not forget to assign bugs to yourself once you plan to work on them, as a means of letting others know. It does seem that there is demand for more WebGL mentored bugs :-) Now is an excellent time to start coding on the WebGL implementation, there's lots to be done, but I have fallen behind on filing mentored bugs. Feel free to email me or query me on IRC to get me to file more mentored bugs faster.

Comment on attachment 671450 [details] [diff] [review] Patch v1 Review of attachment 671450 [details] [diff] [review]: ----------------------------------------------------------------- Please configure your mercurial with this in the [diff] section: [diff] git = 1 showfunc = 1 unified = 8 See https://developer.mozilla.org/en-US/docs/Installing_Mercurial#Configuration ::: content/canvas/src/WebGLContext.h @@ -577,4 @@ > if (!mWebGLError) > mWebGLError = *currentGLError; > } > - Please do not make any whitespace changes except in the precise areas that you are otherwise modifying. This makes patches easier to review and rebase and makes hg annotate more usable. ::: content/canvas/src/WebGLContextGL.cpp @@ +23,4 @@ > > #include "WebGLTexelConversions.h" > #include "WebGLValidateStrings.h" > +#include <string> We normally use our own string classes (nsString, nsCString) rather than std strings. You would need a very specific reason to. @@ +3612,5 @@ > } > > +void > +WebGLContext::ObtainUniformLocation(const char* info, WebGLUniformLocation *location_object, > + int& returnFlag) It seems that returnFlag is only assigned 1 or 0 here, so it should probably be a boolean and be named to something more explicit like "success" or "error". And it would be more intuitive if "true" meant "success". Also, since the function doesn't already have a return value, why not return this as its return value, instead of taking an output-parameter? @@ +3626,5 @@ > + if (mCurrentProgram != location_object->Program()) > + return ErrorInvalidOperation("%s: this uniform location doesn't correspond to the current program", info); > + if (mCurrentProgram->Generation() != location_object->ProgramGeneration()) > + return ErrorInvalidOperation("%s: This uniform location is obsolete since the program has been relinked", info); > + returnFlag = 0; Yup, that looks good. Since this function only does validation, it should be renamed to something that starts in Validate... . @@ +3632,5 @@ > > +void > +WebGLContext::SimpleArrayMethodNoCount(const char* name, int cnt, uint32_t arrayLength, int& returnFlag) > +{ > + returnFlag = 1; Same remarks apply here. @@ +3638,5 @@ > + return; > + } > + if (arrayLength < cnt) { > + return ErrorInvalidOperation((std::string(name) + ": array must be >= %d elements").c_str(), > + cnt); You don't need std::string for that, you can do ErrorInvalidOperation("%s: message", name); I'll stop here for now. The general approach looks good otherwise. @@ +3641,5 @@ > + return ErrorInvalidOperation((std::string(name) + ": array must be >= %d elements").c_str(), > + cnt); > + } > + returnFlag = 0; > + MakeContextCurrent(); Don't put the MakeContextCurrent() here. Better keep this function doing only validation. The MakeContextCurrent() call should always be done at the same place, just before, the actual GL call (gl->fFoo()).

Comment on attachment 671537 [details] [diff] [review] Patch v2 Review of attachment 671537 [details] [diff] [review]: ----------------------------------------------------------------- This is looking really good! Let's do one more round to do the following additional improvements: ::: content/canvas/src/WebGLContext.h @@ +1028,5 @@ > WebGLboolean transpose, uint32_t arrayLength, > const float* data); > > void UseProgram(WebGLProgram *prog); > + bool ValidateArrayMethodNoCount(const char* name, int cnt, uint32_t arrayLength); I feel that these function names are bad. Not your fault --- the macro names were bad. But let's take this occasion to make them better. For example, it seems that (but please verify it) ValidateArrayMethodNoCount is only used by Attrib setters. So it's probably better renamed to ValidateAttribArraySetter. @@ +1030,5 @@ > > void UseProgram(WebGLProgram *prog); > + bool ValidateArrayMethodNoCount(const char* name, int cnt, uint32_t arrayLength); > + bool ValidateArrayMethodUniform(const char* name, int expectedElemSize, WebGLUniformLocation *location_object, > + GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength); Likewise: ValidateUniformArraySetter @@ +1033,5 @@ > + bool ValidateArrayMethodUniform(const char* name, int expectedElemSize, WebGLUniformLocation *location_object, > + GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength); > + bool ValidateMatrixMethodUniform(const char* name, int dim, WebGLUniformLocation *location_object, > + GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength, > + WebGLboolean aTranspose); ValidateUniformMatrixArraySetter @@ +1034,5 @@ > + GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength); > + bool ValidateMatrixMethodUniform(const char* name, int dim, WebGLUniformLocation *location_object, > + GLint& location, uint32_t& numElementsToUpload, uint32_t arrayLength, > + WebGLboolean aTranspose); > + bool ValidateMethodUniform(const char* name, WebGLUniformLocation *location_object, GLint& location); ValidateUniformSetter (since this seems to be used specifically by non-array setters)

Comment on attachment 671537 [details] [diff] [review] Patch v2 Review of attachment 671537 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextGL.cpp @@ +3681,5 @@ > + arrayLength != expectedElemSize) { > + ErrorInvalidOperation( > + "%s: expected an array of length exactly" > + " %d (since this uniform is not an array" > + " uniform), got an array of length %d", name, Also, can you refactor this string to use fewer, longer lines --- use at least 80 columns before wrapping.

Surely it should be 'use up to about 80 columns', not that 80 is our minimum. I usually target an 80-char maximum, but exceptionally let some lines grow up to more like 100, generally because of long function names. (Thanks OpenGL! :P)

Guess you're right :) I usually target 150 characters columns, but that does irk people.

Comment on attachment 671585 [details] [diff] [review] Patch v3 Review of attachment 671585 [details] [diff] [review]: ----------------------------------------------------------------- One style nit below -- I believe it's actually quite important for readability. Also, I forgot this before: in our current file organization (which isn't great, but that's a separate issue) these new WebGLContext::Validate* methods should go to the separate WebGLContextValidate.cpp file. ::: content/canvas/src/WebGLContextGL.cpp @@ +3848,5 @@ > + uint32_t numElementsToUpload; > + GLint location; > + if (!ValidateUniformArraySetter("Uniform1iv", 1, location_object, location, > + numElementsToUpload, arrayLength)) > + return; After a multi-line if() condition, curly braces {...} are really mandatory. (In fact I don't remember if our coding style makes them always mandatory and maybe we're just not following it in this file --- but at least for multi-line if() we do follow it.)

Will ask for review after the try run comes back.

The previous run had converted warnings as errors regarding comparing of int to unsigned int. I have made some changes and done another try run : https://tbpl.mozilla.org/?tree=Try&rev=efde7e324278 Will post the patch after the try run is green.

Most probably fixed the build errors on Linux during the try run of the previous patch, by using uint32_t instead of int.

It's green, congratz.

Comment on attachment 671802 [details] [diff] [review] Patch v5 The try run looks green \o/. Waiting for review and then please land it.

http://hg.mozilla.org/integration/mozilla-inbound/rev/8ce5daee0470 Thanks!