Combined bitmask arguments aren't handled at all in the FunctionCallActor for the Canvas Debugger

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: vporof, Assigned: jsantell)

Tracking

unspecified
Firefox 33
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Suppose you have the following call:
> gl.clear(gl.COLOR_BUFFER_BIT);
The registered argument is named "COLOR_BUFFER_BIT" instead of the actual value.

However, in the case of the following call:
> gl.clear(gl.COLOR_BUFFER_BIT | gl.DEPTH_BUFFER_BIT);
The argument's value is 16640, and registered as such. We should reverse-lookup the actual enum names, because the value isn't very useful.
Depends on: 917226
Component: Developer Tools → Developer Tools: Canvas Debugger
Assignee: nobody → jsantell
Rough patch, but working -- are there tests for this anywhere in canvas or shader editor?
Status: NEW → ASSIGNED
Oops -- now with combined bitmask caching
Attachment #8439717 - Attachment is obsolete: true
Are there tests for this anywhere in the canvas or shader editor?
Flags: needinfo?(vporof)
Yup, see browser_canvas-actor-test-03.js in devtools/canvasdebugger/test
Flags: needinfo?(vporof)
Depends on: 999687
Attachment #8439720 - Attachment is obsolete: true
Attachment #8448226 - Flags: review?(vporof)
Comment on attachment 8448226 [details] [diff] [review]
978964-combined-bitmasks-canvas-dbg.patch

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

Noice!
r+ with comments addressed.

::: toolkit/devtools/server/actors/call-watcher.js
@@ +662,5 @@
> +  }
> +
> +  // Otherwise, attempt to reduce it to the original bit flags:
> +  // `16640` -> "COLOR_BUFFER_BIT | DEPTH_BUFFER_BIT"
> +  else {

Superfluous else.

@@ +664,5 @@
> +  // Otherwise, attempt to reduce it to the original bit flags:
> +  // `16640` -> "COLOR_BUFFER_BIT | DEPTH_BUFFER_BIT"
> +  else {
> +    let flags = [];
> +    for (let key in table) {

I would rename 'key' to 'enum' or 'flag' here, since 'key' usually suggests a string.

@@ +669,5 @@
> +      if (INVALID_ENUMS.indexOf(table[key]) !== -1) {
> +        continue;
> +      }
> +
> +      key = parseInt(key);

Instead of parseInt and isNaN, you could just check if key === (key | 0), which is probably faster. Also, please add a comment here suggesting that we need to make sure the key is an integer first.
Attachment #8448226 - Flags: review?(vporof) → review+
Sounds good, except all the keys are stored as strings, so we'll need to cast to an integer regardless to check the bitwise math
Ah there we go

flag = flag | 0;
if (flag && (arg & flag) === flag) {
Attachment #8448226 - Attachment is obsolete: true
Attachment #8448263 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/cfa338ad3552
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.