cleanup around WebGL extensions

RESOLVED FIXED in mozilla16

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Because working on the wrong things to prioritize asserts my free will in an existentialist kind of way.
(Assignee)

Comment 1

5 years ago
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.
Attachment #633395 - Flags: review?(jgilbert)
(Assignee)

Comment 2

5 years ago
Created attachment 633396 [details] [diff] [review]
clean up IsExtensionSupported
Attachment #633396 - Flags: review?(jgilbert)
(Assignee)

Comment 3

5 years ago
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.
Attachment #633400 - Flags: review?(jgilbert)
(Assignee)

Comment 4

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=7311b632208d
Attachment #633395 - Flags: review?(jgilbert) → review+
Attachment #633396 - Flags: review?(jgilbert) → review+
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.
Attachment #633400 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 6

5 years ago
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.
Attachment #633400 - Attachment is obsolete: true
Attachment #633674 - Flags: review?(jgilbert)
(Assignee)

Comment 7

5 years ago
Created attachment 633676 [details] [diff] [review]
more cleanup; rename mEnabledExtensions to mExtensions, but keep IsExtensionEnabled for user code

compile fix
Attachment #633674 - Attachment is obsolete: true
Attachment #633674 - Flags: review?(jgilbert)
Attachment #633676 - Flags: review?(jgilbert)
Attachment #633676 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 8

5 years ago
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
Assignee: nobody → bjacob
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/e3c16c670291
https://hg.mozilla.org/mozilla-central/rev/b4155b0db568
https://hg.mozilla.org/mozilla-central/rev/45fc4aa0941e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.