Closed Bug 989325 Opened 6 years ago Closed 6 years ago

Make WebGLExtensionID a typed enum, and WebGLContext::mExtensions an enumerated array

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch WebGLExtensionID (obsolete) — Splinter Review
This is mostly automated changes, with some manual fixes that are quite safe, especially as the added typing means that typos there would be caught by the compiler.

The nontrivial pieces, that need more careful reviewing, are in WebGLContextExtensions.cpp. More specifically:

1. WebGLContext::GetExtensionString is refactored.

The static array of string pointers is moved inside it, which is always better: the previous code caused not just literal strings but also pointers to be stored, and computed at library load time; now these costs only kick in when this function is first called.

The static assertion on the array size is removed because the compiler now implicitly performs this check for us when constructing the EnumeratedArray.

Likewise, the assert that the index is in range is now provided by EnumeratedArray::operator[].

It is slightly unfortunate to be copying these strings in the EnumeratedArray, which wastes some space and takes some cycles, but again this doesn't happen before this function is first called, and this is only temporary: as the comment explains, once we can rely on variadic templates, which is not too far away, we can have a better EnumeratedArray constructor that takes the list of strings directly, and remove this overhead. So, please consider this a step towards a rosy future.

2. WebGLContext::IsExtensionEnabled is no longer safe wrt out-of-bounds accesses. Instead, it asserts in DEBUG builds (implicitly in EnumeratedArray::operator[]). The reason for that is that JS never controls the argument passed to it. Not even in GetExtension. Indeed, GetExtension takes a string, and matches it to find the enum value; it should never produce a bad enum value; if it does, then that's a coding mistake, so we want to assert.
Attachment #8398499 - Flags: review?(jgilbert)
Comment on attachment 8398499 [details] [diff] [review]
WebGLExtensionID

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

::: content/canvas/src/WebGLContext.cpp
@@ +56,5 @@
>  #include "mozilla/dom/WebGLRenderingContextBinding.h"
>  #include "mozilla/dom/BindingUtils.h"
>  #include "mozilla/dom/ImageData.h"
>  #include "mozilla/ProcessPriorityManager.h"
> +#include "mozilla/EnumeratedArrayCycleCollection.h"

Why is this include added here?

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +48,3 @@
>  
> +    static const EnumeratedArray<WebGLExtensionID, WebGLExtensionID::Max, const char*>
> +      sExtensionNamesEnumeratedArray(sExtensionNames);

This might honestly be better as:
typedef EnumeratedArray<WebGLExtensionID, WebGLExtensionID::Max, const char*> ExtNameArrT;
static const ExtNameArrT sExtensionNamesEnumeratedArray(sExtensionNames);
(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Comment on attachment 8398499 [details] [diff] [review]
> WebGLExtensionID
> 
> Review of attachment 8398499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContext.cpp
> @@ +56,5 @@
> >  #include "mozilla/dom/WebGLRenderingContextBinding.h"
> >  #include "mozilla/dom/BindingUtils.h"
> >  #include "mozilla/dom/ImageData.h"
> >  #include "mozilla/ProcessPriorityManager.h"
> > +#include "mozilla/EnumeratedArrayCycleCollection.h"
> 
> Why is this include added here?

Because we need to declare an EnumeratedArray of RefPtr's to the cycle collector, namely mExtensions, see in WebGLContext.cpp.

> 
> ::: content/canvas/src/WebGLContextExtensions.cpp
> @@ +48,3 @@
> >  
> > +    static const EnumeratedArray<WebGLExtensionID, WebGLExtensionID::Max, const char*>
> > +      sExtensionNamesEnumeratedArray(sExtensionNames);
> 
> This might honestly be better as:
> typedef EnumeratedArray<WebGLExtensionID, WebGLExtensionID::Max, const
> char*> ExtNameArrT;
> static const ExtNameArrT sExtensionNamesEnumeratedArray(sExtensionNames);

OK, new patch coming.
Attached patch WebGLExtensionIDSplinter Review
Attachment #8398499 - Attachment is obsolete: true
Attachment #8398499 - Flags: review?(jgilbert)
Attachment #8409133 - Flags: review?(jgilbert)
Comment on attachment 8409133 [details] [diff] [review]
WebGLExtensionID

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

::: content/canvas/src/WebGLContext.h
@@ +894,5 @@
>      // -------------------------------------------------------------------------
>      // WebGL extensions (implemented in WebGLContextExtensions.cpp)
> +    typedef
> +      EnumeratedArray<WebGLExtensionID, WebGLExtensionID::Max, nsRefPtr<WebGLExtensionBase>>
> +      ExtensionsArrayType;

Probably better as:
EnumeratedArray<WebGLExtensionID,
                WebGLExtensionID::Max,
                nsRefPtr<WebGLExtensionBase>> ExtensionsArrayType;

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +48,4 @@
>  
> +    typedef EnumeratedArray<WebGLExtensionID, WebGLExtensionID::Max, const char*>
> +            names_array_t;
> +    static const names_array_t sExtensionNamesEnumeratedArray(sExtensionNames);

'static const' prefix should be 'k', since there's no functional difference between 'static const' and 'const', so marking it as static isn't helpful, and almost misleading. (Initializing static arrays' length with a non-const static would be wrong)

::: content/canvas/src/WebGLTypes.h
@@ +164,5 @@
> +    WEBGL_debug_shaders,
> +    WEBGL_depth_texture,
> +    WEBGL_lose_context,
> +    WEBGL_draw_buffers,
> +    ANGLE_instanced_arrays,

It would be nice to alphabetize these.
Attachment #8409133 - Flags: review?(jgilbert) → review+
(Applied review comments)
https://hg.mozilla.org/mozilla-central/rev/610d7ccaaa9c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.