Closed Bug 817785 Opened 12 years ago Closed 12 years ago

Split out WebGLProgram into separate files

Categories

(Core :: Graphics: CanvasWebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bjacob, Assigned: sawrubh)

References

Details

(Whiteboard: [mentor=bjacob][lang=c++] webgl-next)

Attachments

(2 files, 4 obsolete files)

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.
Whiteboard: [mentor=bjacob][lang=c++] webgl-next
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #687944 - Flags: review?(bjacob)
Blocks: 817816
Attached patch Patch v2 (obsolete) — Splinter Review
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)
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-
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #687996 - Attachment is obsolete: true
Attachment #688318 - Flags: review?(bjacob)
Saurabh, do update the 'headerFile' descriptor field in dom/bindings/Bindings.conf as well to reflect your changes.
Filed bug 818258 to take care of it.
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-
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #688318 - Attachment is obsolete: true
Attachment #688909 - Flags: review?(bjacob)
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+
Attached patch Patch v5Splinter Review
Attachment #688909 - Attachment is obsolete: true
Attachment #688924 - Flags: review?(bjacob)
Attachment #688924 - Flags: review?(bjacob) → review+
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.
Attached patch Patch v6Splinter Review
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)
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+
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.

Attachment

General

Created:
Updated:
Size: