765137 - cleanup around WebGL extensions

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

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)]

Attachment: 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)]

Attachment: clean up IsExtensionSupported

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: 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)]

https://tbpl.mozilla.org/?tree=Try&rev=7311b632208d

Comment 5

[Kelsey Gilbert [:jgilbert]]

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)]

Attachment: 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)]

Attachment: more cleanup; rename mEnabledExtensions to mExtensions, but keep IsExtensionEnabled for user code

compile fix

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e3c16c670291
http://hg.mozilla.org/integration/mozilla-inbound/rev/b4155b0db568
http://hg.mozilla.org/integration/mozilla-inbound/rev/45fc4aa0941e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e3c16c670291
https://hg.mozilla.org/mozilla-central/rev/b4155b0db568
https://hg.mozilla.org/mozilla-central/rev/45fc4aa0941e