871094 - Add warning for gl.clear(0)

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Calling `gl.clear(0)` is never cheaper than not calling it at all.

The real danger is that `gl.clear(gl.GARBAGE)` is coerced to `gl.clear(0)`, which is an otherwise valid call.

We should warn about this, so (totally hypothetically) someone doesn't waste time trying to figure out why `gl.clear(gl.CLEAR_BUFFER_BIT)` doesn't work, when they mean `gl.clear(gl.COLOR_BUFFER_BIT)`.

Comment 1

[Guillaume Abadie]

Attachment: patch for landing

patch adding a warning message when calling gl.clear(0)

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 766061 [details] [diff] [review]
patch for landing

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +592,5 @@
>      if (mask != m)
>          return ErrorInvalidValue("clear: invalid mask bits");
>  
> +    if (mask == 0) {
> +        GenerateWarning("useless call gl.clear(0)");

I would reword this to something like 'Calling gl.clear(0) has no effect.'.

Comment 3

[Guillaume Abadie]

Attachment: patch revision 1

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 766114 [details] [diff] [review]
patch revision 1

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

That should be good enough. It should at least make people ask themselves if they really meant to call it with 0.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This doesn't apply to inbound. Please rebase.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: clear-zero.patch

Actually we shouldn't bail at this warning. Clear() should still trigger any GL errors it's supposed to, even if won't be doing anything.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a638ac362cb

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4a638ac362cb