Extend error reporting for GL enums.

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kamidphish, Assigned: kamidphish)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 8524361 [details] [diff] [review]
Extend error reporting
Attachment #8524361 - Flags: review?(jgilbert)
Comment on attachment 8524361 [details] [diff] [review]
Extend error reporting

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

::: dom/canvas/WebGLContextUtils.cpp
@@ +505,5 @@
>  void
>  WebGLContext::ErrorInvalidEnumInfo(const char *info, GLenum enumvalue)
>  {
> +    const char* name = EnumName(enumvalue);
> +    if (!name || name[0] == '[')

We should really have two functions:
Infallible `const char* EnumName(GLenum val)`
Fallible `bool EnumName(GLenum val, const char* const out_name)`

In theory, the fallible version could just return null, but I think it's more clear which is fallible this way.

Use the fallible version here.

@@ +508,5 @@
> +    const char* name = EnumName(enumvalue);
> +    if (!name || name[0] == '[')
> +        return ErrorInvalidEnum("%s: invalid enum value 0x%04x", info, enumvalue);
> +
> +    return ErrorInvalidEnum("%s: invalid enum value %s", info, name);

So here's the problem with this. It assumes (GLenum val)->(const char* name) is one-to-one. This isn't universally true, though. 

The proper construction here for all GLenums is map<GLenum val, set<const char*> names>. 

Maybe the question is whether this matters. Restrict `GLenum val` to >= 0x10 and we should be one-to-one again.
Attachment #8524361 - Flags: review?(jgilbert) → review+
Yeah, I know it's not one-to-one, but how do you select the result you want? An option to the function call, I suppose.
Created attachment 8526465 [details] [diff] [review]
Extend error reporting

Updated with fallible and non-fallible versions of EnumName.
Attachment #8526465 - Flags: review?(jgilbert)
Attachment #8524361 - Attachment is obsolete: true
Attachment #8526465 - Flags: review?(jgilbert) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=5db03e87ca06

*sigh* Programming is hard.
https://hg.mozilla.org/mozilla-central/rev/5fdc1acbc53f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.