Last Comment Bug 765137 - cleanup around WebGL extensions
: cleanup around WebGL extensions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-14 21:39 PDT by Benoit Jacob [:bjacob] (mostly away)
Modified: 2012-06-16 06:50 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use case-insensitive comparisons rather than lower-case versions of extension strings (2.41 KB, patch)
2012-06-14 21:40 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
clean up IsExtensionSupported (2.47 KB, patch)
2012-06-14 21:42 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review
more cleanup; rename mEnabledExtensions to mExtensions (17.16 KB, patch)
2012-06-14 22:00 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review-
Details | Diff | Splinter Review
more cleanup; rename mEnabledExtensions to mExtensions, but keep IsExtensionEnabled for user code (17.46 KB, patch)
2012-06-15 14:19 PDT, Benoit Jacob [:bjacob] (mostly away)
no flags Details | Diff | Splinter Review
more cleanup; rename mEnabledExtensions to mExtensions, but keep IsExtensionEnabled for user code (17.46 KB, patch)
2012-06-15 14:25 PDT, Benoit Jacob [:bjacob] (mostly away)
jgilbert: review+
Details | Diff | Splinter Review

Description Benoit Jacob [:bjacob] (mostly away) 2012-06-14 21:39:33 PDT
Because working on the wrong things to prioritize asserts my free will in an existentialist kind of way.
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-06-14 21:40:51 PDT
Created attachment 633395 [details] [diff] [review]
use case-insensitive comparisons rather than lower-case versions of extension strings

Our API for case-insensitive compare is terrible but at least that's not our fault.
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2012-06-14 21:42:54 PDT
Created attachment 633396 [details] [diff] [review]
clean up IsExtensionSupported
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-06-14 22:00:17 PDT
Created attachment 633400 [details] [diff] [review]
more cleanup; rename mEnabledExtensions to mExtensions

This patch may require some more explanation.

mEnabledExtensions gets renamed to mExtensions. Indeed, it wasn't fully clear that by "enabled extensions" we meant "extensions that have already been created by GetExtension". It could have meant "extensions that are supported". Really what mExtensions is, is an array of all the extension objects we've created and returned in GetExtension. It's the array of all our extension objects in existence in this context. So mExtensions it is.

IsExtensionEnabled goes away because it was just checking if that pointer was non-null. The assertion there became useless as nsTArray (which is a base class of nsAutoTArray in my understanding) started using fatal assertions for out-of-bounds accesses.

WebGLExtensionID enums got reworked a bit: The WebGL_ prefixes were useless, as the only conceivable confusion was with the GLExtension enums in class GLContext and the only way to get them from WebGLContext would be to explicitly write GLContext:: , I don't believe we'd make this confusion. See how we check for OpenGL extensions in WebGLContextGL.cpp: for example gl->IsExtensionSupported(gl::GLContext::OES_depth24).

WebGLExtensionID_Max got renamed to something more appropriate IMO though I had no idea what the coding style for this identifier should be. Notice how Max was wrong as it's 1 + the actual max.

A new _unknown enum value was added, instead of abusing the _Max value.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 11:28:48 PDT
https://tbpl.mozilla.org/?tree=Try&rev=7311b632208d
Comment 5 Jeff Gilbert [:jgilbert] 2012-06-15 14:05:05 PDT
Comment on attachment 633400 [details] [diff] [review]
more cleanup; rename mEnabledExtensions to mExtensions

Review of attachment 633400 [details] [diff] [review]:
-----------------------------------------------------------------

As we discussed, "enabled" is the terminology the spec uses, even if it leaves the implication that extensions can be "disabled". "Activated" might be better, but the spec uses "enabled", so it's probably best to go with that, though adding a comment to this effect.

The other changes are r+, though.
Comment 6 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 14:19:19 PDT
Created attachment 633674 [details] [diff] [review]
more cleanup; rename mEnabledExtensions to mExtensions, but keep IsExtensionEnabled for user code

OK you're right. I kept the renaming of mEnabledExtensions to mExtensions, but restored IsExtensionEnabled for user code. Internal code in GetExtension still uses mExtensions directly to know if an extension has already been created, as it is what it has to manipulate anyway to store newly created extensions.
Comment 7 Benoit Jacob [:bjacob] (mostly away) 2012-06-15 14:25:12 PDT
Created attachment 633676 [details] [diff] [review]
more cleanup; rename mEnabledExtensions to mExtensions, but keep IsExtensionEnabled for user code

compile fix

Note You need to log in before you can comment on or make changes to this bug.