999687 - Some arguments are treated as enums when they shouldn't be

Status: RESOLVED FIXED

(DevTools Graveyard :: Canvas Debugger, defect)

Description

[Victor Porof [:vporof][:vp]]

Like, in the case of vertexAttribPointer or disable/enableVertexAttribArray. The arg is a simple int, not an enum.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Canvas (all chrome-only):
* asyncDrawXULElement[1], 7th arg
* drawWindow [2], 7th arg

3D Context (lots):
https://developer.mozilla.org/en-US/docs/Web/API/WebGLRenderingContext

[1] https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D#asyncDrawXULElement%28%29
[2] https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D#drawWindow%28%29

Comment 2

[Victor Porof [:vporof][:vp]]

Good picks for the 2D context Jordan. Here are the ones for WebGL, the enum params being named `enum`.

* activeTexture(enum);
* bindBuffer(enum, WebGLBuffer);
* bindFramebuffer(enum, WebGLFramebuffer);
* bindRenderbuffer(enum, WebGLRenderbuffer);
* bindTexture(enum, WebGLTexture);
* blendEquation(enum);
* blendEquationSeparate(enum, enum);
* blendFunc(enum, enum);
* blendFuncSeparate(enum, enum, enum, enum);
* bufferData(enum, enum, enum);
* bufferData(enum, ArrayBufferView, enum);
* bufferData(enum, ArrayBuffer, enum);
* bufferSubData(enum, enum, ArrayBufferView);
* bufferSubData(enum, enum, ArrayBuffer);
* checkFramebufferStatus(enum);
* clear(unsigned enum);
* compressedTexImage2D(enum, int, enum, int, int, int, ArrayBufferView);
* compressedTexSubImage2D(enum, int, int, int, int, int, enum, ArrayBufferView);
* copyTexImage2D(enum, int, enum, int, int, int, int, int);
* copyTexSubImage2D(enum, int, int, int, int, int, int, int);
* createShader(enum);
* cullFace(enum);
* depthFunc(enum);
* disable(enum);
* drawArrays(enum, int, int);
* drawElements(enum, int, enum, int);
* enable(enum);
* framebufferRenderbuffer(enum, enum, enum, WebGLRenderbuffer);
* framebufferTexture2D(enum, enum, enum, WebGLTexture, int);
* frontFace(enum);
* generateMipmap(enum);
* getBufferParameter(enum, enum);
* getParameter(enum);
* getError();
* getFramebufferAttachmentParameter(enum, enum, enum);
* getProgramParameter(WebGLProgram, enum);
* getRenderbufferParameter(enum, enum);
* getShaderParameter(WebGLShader, enum);
* getShaderPrecisionFormat(enum, enum);
* getTexParameter(enum, enum);
* getVertexAttrib(uint, enum);
* getVertexAttribOffset(uint, enum);
* hint(enum, enum);
* isEnabled(enum);
* pixelStorei(enum, int);
* readPixels(int, int, int, int, enum, enum, ArrayBufferView);
* renderbufferStorage(enum, enum, int, int);
* stencilFunc(enum, int, uint);
* stencilFuncSeparate(enum, enum, int, uint);
* stencilMaskSeparate(enum, uint);
* stencilOp(enum, enum, enum);
* stencilOpSeparate(enum, enum, enum, enum);
* texImage2D(enum, int, enum, int, int, int, enum, enum, ArrayBufferView);
* texImage2D(enum, int, enum, enum, enum, ImageData);
* texImage2D(enum, int, enum, enum, enum, HTMLImageElement);
* texImage2D(enum, int, enum, enum, enum, HTMLCanvasElement);
* texImage2D(enum, int, enum, enum, enum, HTMLVideoElement);
* texParameterf(enum, enum, float);
* texParameteri(enum, enum, int);
* texSubImage2D(enum, int, int, int, int, int, enum, enum, ArrayBufferView);
* texSubImage2D(enum, int, int, int, enum, enum, ImageData);
* texSubImage2D(enum, int, int, int, enum, enum, WebGLUniformLocation, float);
* vertexAttribPointer(uint, int, enum, bool, int, int);

Comment 3

[Victor Porof [:vporof][:vp]]

Clarification for the above list:

* clear(unsigned enum); // takes a single enum parameter
* getError(); // actually returns an enum, so we don't really need to handle this one

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

dang.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 999687-enum-canvas-debugger.patch

A working idea. How do you like this? Anything else it needs?

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 8446907 [details] [diff] [review]
999687-enum-canvas-debugger.patch

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

I am thoroughly satisfied.

A better test would've been one one with some WebGL calls and actual enums, but we have DRAWWINDOW_DRAW_VIEW == 4 which is ok. Maybe you should assert this in the test, so if this enum's value ever changes (it's non-standard after all) then this test gets updated.

::: toolkit/devtools/server/actors/call-watcher.js
@@ +220,5 @@
>          return "Object";
>        }
> +      // If this argument matches the method's signature
> +      // and is an enum, change it to its constant name.
> +      if (enumArgs && ~enumArgs.indexOf(i)) {

Fun, but is == -1 really that ugly? :P

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 999687-enum-canvas-debugger.patch

Fine, fine, we'll settle for == -1

Comment 8

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/bb5dbe7a3378

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb5dbe7a3378