Closed
Bug 817785
Opened 12 years ago
Closed 12 years ago
Split out WebGLProgram into separate files
Categories
(Core :: Graphics: CanvasWebGL, defect)
Core
Graphics: CanvasWebGL
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: bjacob, Assigned: sawrubh)
References
Details
(Whiteboard: [mentor=bjacob][lang=c++] webgl-next)
Attachments
(2 files, 4 obsolete files)
26.92 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
28.99 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
This is a class living almost entirely inside WebGLContext.h. Split it into a new separate header, WebGLProgram.h, and try to move most (or all) of its actual code to WebGLProgram.cpp. Typically WebGLProgram.h would only include WebGLShader.h and not WebGLContext.h. The point is to make it so that we can #include WebGLProgram.h without dragging in too many dependencies, and to bring WebGLContext.h down to a reasonable size. When moving its code into WebGLProgram.cpp, use discretion to decide where to stop. If a method is very small, does not require dragging in any dependency, is performance-critical so that the cost of the function call matters, and is used outside of WebGLProgram.cpp, then it can make sense for it to stay in WebGLProgram.h so that the compiler can still inline it without relying on link-time optimization. If you get something that builds and passes 1.0.1 conformance tests, https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html you'll know that you're on the right track.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [mentor=bjacob][lang=c++] webgl-next
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #687944 -
Flags: review?(bjacob)
Assignee | ||
Comment 3•12 years ago
|
||
Seems I had forgotten to |hg add| the new WebGLProgram.h that I had created.
Attachment #687944 -
Attachment is obsolete: true
Attachment #687944 -
Flags: review?(bjacob)
Attachment #687996 -
Flags: review?(bjacob)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 687996 [details] [diff] [review] Patch v2 Review of attachment 687996 [details] [diff] [review]: ----------------------------------------------------------------- Just a nit and a suggestion to make the patch even moar awesome: ::: content/canvas/src/WebGLContext.h @@ +14,4 @@ > #include "WebGLVertexAttribData.h" > #include "WebGLShaderPrecisionFormat.h" > #include <stdarg.h> > #include <vector> Please remove this #include <vector> now, as it does not seem to be needed anymore. ::: content/canvas/src/WebGLProgram.h @@ +116,5 @@ > + nsTArray<WebGLRefPtr<WebGLShader> > mAttachedShaders; > + CheckedUint32 mGeneration; > + > + // post-link data > + std::vector<bool> mAttribsInUse; Oh, so that's why we are #including <vector> here. That's a bit stupid (not your fault!). That's not a requirement, but do you feel like doing the bit of extra work to use nsTArray instead, which we are using here anyways? So you will be able to remove the #include <vector> which is a relatively nasty thing to have in a header file. Or if you don't have time, please file a follow-up bug for that.
Attachment #687996 -
Flags: review?(bjacob) → review-
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #687996 -
Attachment is obsolete: true
Attachment #688318 -
Flags: review?(bjacob)
Comment 6•12 years ago
|
||
Saurabh, do update the 'headerFile' descriptor field in dom/bindings/Bindings.conf as well to reflect your changes.
Assignee | ||
Comment 7•12 years ago
|
||
Filed bug 818258 to take care of it.
Reporter | ||
Comment 8•12 years ago
|
||
Comment on attachment 688318 [details] [diff] [review] Patch v3 Review of attachment 688318 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLContextValidate.cpp @@ +37,5 @@ > > GLint attribCount; > mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTES, &attribCount); > > + if (!mAttribsInUse.SetCapacity(mContext->mGLMaxVertexAttribs)) I am very surprised if you tell me that this works, because SetCapacity, as far as I know, only sets storage capacity and does not set the actual array length. So the subsequent array indexing should assert. No? Use SetLength instead.
Attachment #688318 -
Flags: review?(bjacob) → review-
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #688318 -
Attachment is obsolete: true
Attachment #688909 -
Flags: review?(bjacob)
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 688909 [details] [diff] [review] Patch v4 Review of attachment 688909 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but here is one way that it could look even better: ::: content/canvas/src/WebGLContextValidate.cpp @@ +38,5 @@ > GLint attribCount; > mContext->gl->fGetProgramiv(mGLName, LOCAL_GL_ACTIVE_ATTRIBUTES, &attribCount); > > + if (!mAttribsInUse.SetLength(mContext->mGLMaxVertexAttribs)) > + return false; before returning false, please also call ErrorOutOfMemory giving an error message about the fact that it failed to allocate enough memory to handle %d attribs and print that value.
Attachment #688909 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #688909 -
Attachment is obsolete: true
Attachment #688924 -
Flags: review?(bjacob)
Reporter | ||
Updated•12 years ago
|
Attachment #688924 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5725e73685e9
Assignee | ||
Comment 13•12 years ago
|
||
Backed out : https://hg.mozilla.org/integration/mozilla-inbound/rev/143acb93b300
Assignee | ||
Comment 14•12 years ago
|
||
Don't land this, I think there might be a problem with Patch v5 (it should be mContext->ErrorOutOfMemory rather than just ErrorOutOfMemory). I am testing it currently on the try server and will post for review the new patch accordingly. Thanks.
Assignee | ||
Comment 15•12 years ago
|
||
The problems were as follows : a) Had to replace ErrorOutOfMemory with mContext->ErrorOutOfMemory. b) Had to move SplitLastSquareBracket to WebGLProgram.cpp since it's a static function and that's where it's called. https://tbpl.mozilla.org/?tree=Try&rev=3416c13e27a0 is the try run with this patch. I hope it's enough for an r+. Bonus : This is no longer bitrotted and can be landed soon afterwards. And sorry Benoit for all the confusion.
Attachment #690124 -
Flags: review?(bjacob)
Reporter | ||
Comment 16•12 years ago
|
||
Comment on attachment 690124 [details] [diff] [review] Patch v6 Review of attachment 690124 [details] [diff] [review]: ----------------------------------------------------------------- Good! Moving SplitLastSquareBracket to WebGLProgram.cpp is very good, it's the whole point of these patches to take code out of WebGLContext.h.
Attachment #690124 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 17•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eddb7e1fc9d
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4eddb7e1fc9d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•