Show entries for WeakMap and WeakSet in console

RESOLVED FIXED in Firefox 45

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: 446240525, Assigned: tromey)

Tracking

unspecified
Firefox 45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(1 attachment, 8 obsolete attachments)

Just like we did for Map and Set.
And both Chrome and Safari(webkit nightly) already support this.
Whiteboard: [polish-backlog][difficulty=medium]
Bug 1172920 is the same but for non-weak versions.
See Also: → 1172920
Note: For iterating weak map items (it feels gross just saying that), one can use the Cu.getJSTestingFunctions().nondeterministicGetWeakMapKeys function.
I'm making them show up in the console and deferring the variables view change
to bug 1172920.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
This patch works but I think it should probably have more tests for the new
JS API, not just via the webconsole tests.
I added a simple test in xpconnect.
Attachment #8682693 - Attachment is obsolete: true
git add the test
Attachment #8682724 - Attachment is obsolete: true
Comment on attachment 8682729 [details] [diff] [review]
display WeakSet and WeakMap contents in console

This adds display of the contents of WeakSet and WeakMap to the console.

To make this work for WeakSet, I had to add a new API to JS to fetch
the keys of the WeakSet object.  I exposed this in xpconnect where the
equivalent for WeakMap is exposed.
Attachment #8682729 - Flags: review?(nfitzgerald)
Attachment #8682729 - Flags: review?(bzbarsky)
Depends on: 1221292
Comment on attachment 8682729 [details] [diff] [review]
display WeakSet and WeakMap contents in console

Cancelling the review while I work on the precursor bug first.
Attachment #8682729 - Flags: review?(nfitzgerald)
Attachment #8682729 - Flags: review?(bzbarsky)
Comment on attachment 8682729 [details] [diff] [review]
display WeakSet and WeakMap contents in console

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

Would prefer attaching nondeterministicBlahBlahBlah to ThreadSafeChromeUtils.webidl since we can use that in workers and we are trying to get the console + debugger properly supported on workers. My hope is that it wouldn't be too difficult, but if it turns out to be then feel free to push it off as a follow up.

The rest of the changes look A+ to me.

::: devtools/client/webconsole/test/test-console-table.html
@@ +45,5 @@
>        mySet.add(null);
>        mySet.add(undefined);
> +
> +      var bunnies = new String("bunnies");
> +      var lizards = new String("lizards");

And we know these will still be around when we preview the weak set because these are global variables and therefore can't be reclaimed by the GC? Might deserve a quick note.
Attachment #8682729 - Flags: feedback+
Comment on attachment 8682729 [details] [diff] [review]
display WeakSet and WeakMap contents in console

(Sorry to revive from the dead!)
Attachment #8682729 - Attachment is obsolete: true
Updated to follow bug 1221292.
Attachment #8686243 - Attachment is obsolete: true
Comment on attachment 8686254 [details] [diff] [review]
display WeakSet and WeakMap contents in console

Updated per initial review, and to follow bug 1221292.
Attachment #8686254 - Flags: review?(nfitzgerald)
Attachment #8686254 - Flags: review?(bzbarsky)
Comment on attachment 8686254 [details] [diff] [review]
display WeakSet and WeakMap contents in console

r=me on the dom/base and dom/webidl changes.
Attachment #8686254 - Flags: review?(bzbarsky) → review+
Comment on attachment 8686254 [details] [diff] [review]
display WeakSet and WeakMap contents in console

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

Looks great!
Attachment #8686254 - Flags: review?(nfitzgerald) → review+
Add r= to commit message.
Attachment #8686254 - Attachment is obsolete: true
Attachment #8686669 - Flags: review+
Naturally try points out that the test assumes a particular ordering of keys.
Whoops.  I will figure out what to do about the test.
Update tests to allow "out-of-order" results.
Attachment #8686669 - Attachment is obsolete: true
Comment on attachment 8687267 [details] [diff] [review]
display WeakSet and WeakMap contents in console

I changed the tests to allow the results to appear in any order.
I wasn't sure if this was big enough to warrant re-review, so
erring on the side of caution.
Attachment #8687267 - Flags: review?(nfitzgerald)
Attachment #8687267 - Flags: review?(nfitzgerald) → review+
Typo in one of the expected results.
Attachment #8687267 - Attachment is obsolete: true
Attachment #8687408 - Flags: review+
A silly slip of the keys snuck in.
Someday this will make it through.

One thing that occurred to me that is odd about this patch is that if
a user prints a WeakSet on the console, then that will keep the
elements alive.  This may be unexpected.
Attachment #8687408 - Attachment is obsolete: true
Attachment #8687936 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4e0d0bf80b36
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.