Currently, all WebGL classes are declared in WebGLContext.h. That makes it huge. To make things even worse, many of its functions are defined inline inside of the class, making this file even more huge. We should split WebGLBuffer away from WebGLContext.h, into a new WebGLBuffer.h file. You shouldn't have to declare it at all in the Makefile, as it would be purely internal to the WebGL implementation, it wouldn't have to be exported. Unless I'm missing something --- we'll see if you get compile errors. The methods in WebGLBuffer aren't very performance-sensitive to call, so there is no need to aggressively inline them. This means that you should be able to comfortably move their implementations to the WebGLBuffer.cpp file (which already exists). Use discretion to decide which trivial methods make sense to keep defined in the header.

Hi Benoit! I can't assign this to myself. I need some help :)

Automatically unassigning mentored bugs that haven't seen action for two weeks. The idea is to maximize the number of mentored bugs that newcomers can pick from.

I'd like to ask for some advice on how to best tackle this bug. This patch *will* compile. (see https://tbpl.mozilla.org/?tree=Try&rev=a9b418d35a76 ) However, it includes the new WebGLBuffer.h in the middle of WebGLContext.h . I'm not sure whether this is acceptable in terms of good coding practice. If we want to move the |include "WebGLBuffer.h"| to the top of the file, we might need to split WebGLContext.h into two files. WebGLBuffer inherits from WebGLRefCountedObject and WebGLContextBoundObject, we need to have these two classes defined before WebGLBuffer is defined. Do you have any advice for how to approach this from here? Should we keep the |#include "WebGLBuffer.h"| in the middle of WebGLContext.h, or would it make more sense to split WebGLContext.h and make a common header that defines the necessary Base classes? Or are there other alternatives? Thank you very much!

(In reply to Erick Dransch [:edransch] from comment #3) > Created attachment 678093 [details] [diff] [review] > Patch for feedback > > I'd like to ask for some advice on how to best tackle this bug. > > This patch *will* compile. (see > https://tbpl.mozilla.org/?tree=Try&rev=a9b418d35a76 ) However, it includes > the new WebGLBuffer.h in the middle of WebGLContext.h . I'm not sure whether > this is acceptable in terms of good coding practice. That is not acceptable :-) > > If we want to move the |include "WebGLBuffer.h"| to the top of the file, we > might need to split WebGLContext.h into two files. WebGLBuffer inherits from > WebGLRefCountedObject and WebGLContextBoundObject, we need to have these two > classes defined before WebGLBuffer is defined. Yep, that is very acceptable. You could call it WebGLObjectModel.h or something like that. Notice that the present bug is just the start of a big reorganization of the WebGL code into more, smaller files. So this splitting into more files is a desirable property :-) Thanks for working on this!

I would like to ask for more advice regarding this patch. We have the following situation: WebGLBuffer inherits from WebGLRefCountedObject and WebGLContextBoundObject WebGLContextBoundObject requires WebGLContext to be defined (http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/WebGLContext.h#1471). Therefore, as seen in the patch, we would need to move WebGLContext to our new file (WebGLObjectModel.h). I don't like this solution, I think this is confusing. The WebGLContext class definition will no longer be in WebGLContext.h. I was wondering how you would suggest working around this issue. Some ideas I have are: 1) Rename WebGLContext.h to something different. It would make it less confusing, but it would require changing the makefile and all |#include "WebGLContext.h"| across our codebase. 2) Keep the name WebGLContext.h, but split the files further, creating a new file called something like WebGLContextDefinition.h which would contain the definition of WebGLContext, and anything it inherits from or requires. Neither of these solutions seems elegant to me, do you have any further suggestions? Thank you very much!

(In reply to Erick Dransch [:edransch] from comment #5) > Created attachment 680403 [details] [diff] [review] > Patch for discussion > > I would like to ask for more advice regarding this patch. > We have the following situation: > > WebGLBuffer inherits from WebGLRefCountedObject and WebGLContextBoundObject > > WebGLContextBoundObject requires WebGLContext to be defined > (http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/ > WebGLContext.h#1471). Good question! This is only _inside the body_ of a function (a constructor). So you could add a new .cpp file, WebGLObjectModel.cpp, and move the implementation of that function there. That file would #include "WebGLContext.h". Likewise, in class WebGLBuffer you have functions whose bodies require WebGLContext to be defined. You can solve that problem in the same way by moving them to a WebGLBuffer.cpp file. None of these functions is performance-critical. WebGL objects are very expensive to create/destroy anyway (because of the combined cost of DOM bindings and OpenGL object creation/destruction). So function inlining is not important here.

Also note that since 2 days ago we don't export any WebGL headers anymore. That means that the only files that can #include our headers are right here in this directory.

As discussed, we have WebGLObjectModel.h and WebGLObjectModel.cpp which implement the classes that WebGLBuffer depends on. Try run for this patch is at https://tbpl.mozilla.org/?tree=Try&rev=d5e57f2a521a

Comment on attachment 682360 [details] [diff] [review] Patch for review Review of attachment 682360 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks good from a quick glance, will do proper review tomorrow. Already one remark: please move WebGLRefPtr to WebGLObjectModel.h as well. This will require you to also move the corresponding ImplCycleCollection{Unlink,Traverse} currently found in WebGLContext.h (since yesterday).

Comment on attachment 682360 [details] [diff] [review] Patch for review Actually, please address the above comment, then I'll review.

This patch also moves WebGLRefPtr, and ImplCycleCollection{Unlink,Traverse}(mozilla:WebGLRefPtr<T>&) to WebGLObjectModel.h.

Comment on attachment 682700 [details] [diff] [review] Move WebGLRefPtr to WebGLObjectModel Review of attachment 682700 [details] [diff] [review]: ----------------------------------------------------------------- Some minor nits, just enough to require another round, so r-, but this is looking very good! Thanks so much for working on this, and if you are still interested, we have all the other WebGL classes to do as well! ::: content/canvas/src/WebGLBuffer.h @@ +5,5 @@ > + > +#ifndef WEBGLBUFFER_H_ > +#define WEBGLBUFFER_H_ > +#include <stdarg.h> > +#include <vector> A header should only include other headers that it directly needs. What is the need for these headers anyway? @@ +10,5 @@ > + > +#include "nsWrapperCache.h" > + > +#include "mozilla/LinkedList.h" > +#include "mozilla/CheckedInt.h" Same comment about CheckedInt.h. ::: content/canvas/src/WebGLContext.h @@ -71,5 @@ > -typedef int64_t WebGLintptr; > -typedef uint32_t WebGLuint; > -typedef float WebGLfloat; > -typedef float WebGLclampf; > -typedef bool WebGLboolean; This should stay in this file! ::: content/canvas/src/WebGLObjectModel.h @@ +5,5 @@ > + > +#ifndef WEBGLOBJECTMODEL_H_ > +#define WEBGLOBJECTMODEL_H_ > +#include <stdarg.h> > +#include <vector> Again, why include these headers here? One header that you should need to include here is "nsCycleCollectionNoteChild.h". The ImplCycleCollection* functions definitely need it; it seems that just by accident it compiled because this file was only being included after other headers.

(In reply to Benoit Jacob [:bjacob] from comment #12) > ::: content/canvas/src/WebGLContext.h > @@ -71,5 @@ > > -typedef int64_t WebGLintptr; > > -typedef uint32_t WebGLuint; > > -typedef float WebGLfloat; > > -typedef float WebGLclampf; > > -typedef bool WebGLboolean; > > This should stay in this file! Ah, no, sorry, I get it now. You needed to move this to another header so that headers such as WebGLBuffer could use these typedefs without including WebGLContext.h. Fine, that works then.

(In reply to Benoit Jacob [:bjacob] from comment #12) > One header that you should need to include here is > "nsCycleCollectionNoteChild.h". The ImplCycleCollection* functions > definitely need it; it seems that just by accident it compiled because this > file was only being included after other headers. In another project that I worked on in the past (KDE, Qt), we had a rule: always include your own local headers first, before including more general headers. In this way you ensure that if your local headers forgot to include what they needed, you will get a compile error.

The unnecessary includes are removed (good catch, thanks!). I also moved the local includes to the beginning, as you mentioned. Try run for this patch is at https://tbpl.mozilla.org/?tree=Try&rev=e77ce409e377

Comment on attachment 683475 [details] [diff] [review] Remove unnecessary includes Review of attachment 683475 [details] [diff] [review]: ----------------------------------------------------------------- Fantastic!

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b94284158d4 ^ I edited it so you are the author; next time don't forget about that! I could have forgotten myself.