Closed
Bug 1304352
Opened 8 years ago
Closed 8 years ago
Add a rep for Map
Categories
(DevTools :: Shared Components, defect, P2)
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Firefox 52
| Tracking | Status | |
|---|---|---|
| firefox52 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: nchevobbe)
Details
Attachments
(2 files)
We don't support Map with the current Rep system. We should add a rep to handle them.
In the current console, Maps are displayed like object :
```
Map { a: 1}
```
|
Assignee | |
Comment 2•8 years ago
|
||
(In reply to Dalimil Hajek [:dalimil] from comment #1) > So how should a Map be displayed? Like I said in Comment 0 : ``` Map { a: 1} ``` Currently, when we pass a Map to the Rep, it uses the GripRep object. But it looks for `object.preview.ownProperties` which are undefined for maps grip. Thus, it would display a map has `Map`, without any of its entries. Instead, we should get `object.preview.entries` and build the Rep with that in mind. The Map grip should handle WeakMap too. Since WeakMap keys are object ( actually, object grip ), we will need to modify PropRep, which only works with string properties for now. I should be able to submit a patch for review in the next days.
Flags: needinfo?(chevobbe.nicolas)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 5•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8795610 [details] Bug 1304352 - Fix title issue in the ObjectRep. ; https://reviewboard.mozilla.org/r/81600/#review80680 LGTM! Honza
Attachment #8795610 -
Flags: review?(odvarko) → review+
|
|
||
Comment 6•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8795611 [details] Bug 1304352 - Add a Rep for Map and WeakMap. ; https://reviewboard.mozilla.org/r/81602/#review80686 Nice new addition to the Rep library! Some nits commented inline, but it's already R+ Honza ::: devtools/client/shared/components/reps/prop-rep.js:13 (Diff revision 1) > // Make this available to both AMD and CJS environments > define(function (require, exports, module) { > const React = require("devtools/client/shared/vendor/react"); > const { createFactories } = require("./rep-utils"); > > + const { Grip } = createFactories(require("./prop-rep")); Did you want to require './grip' module? ::: devtools/client/shared/components/reps/prop-rep.js:19 (Diff revision 1) > const { span } = React.DOM; > > /** > * Property for Obj (local JS objects) and Grip (remote JS objects) > * reps. It's used to render object properties. > */ We should update the comment (PropRep is now used in GripMap too). There doesn't have to be list of places where it's used, but it should be clear that it isn't only Obj and Grip ::: devtools/client/shared/components/test/mochitest/test_reps_grip-map.html:5 (Diff revision 1) > + > +<!DOCTYPE HTML> > +<html> > +<!-- > +Test grip rep Should be 'Test GripMap rep'
Attachment #8795611 -
Flags: review?(odvarko) → review+
|
|
Assignee | |
Comment 7•8 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8795611 [details] Bug 1304352 - Add a Rep for Map and WeakMap. ; https://reviewboard.mozilla.org/r/81602/#review80686 > Did you want to require './grip' module? yes ! > We should update the comment (PropRep is now used in GripMap too). There doesn't have to be list of places where it's used, but it should be clear that it isn't only Obj and Grip okay BTW, do you know you can add comment on multiple line in MozReview ( click the first line and then drag to the last one )
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 10•8 years ago
|
||
Fixed the nits (https://reviewboard.mozilla.org/r/81602/diff/1-2/) and pushed to TRY (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a30d21d0eac0)
|
|
||
Comment 11•8 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #7) > BTW, do you know you can add comment on multiple line in MozReview ( click > the first line and then drag to the last one ) Ah, nice! Honza
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 13•8 years ago
|
||
The last TRY does not show any problem https://treeherder.mozilla.org/#/jobs?repo=try&revision=07f7152816c0&selectedJob=28311961
|
||
Comment 14•8 years ago
|
||
Pushed by chevobbe.nicolas@gmail.com: https://hg.mozilla.org/integration/autoland/rev/dd0fc5a3c52f Fix title issue in the ObjectRep. r=Honza; https://hg.mozilla.org/integration/autoland/rev/a85ec3655e36 Add a Rep for Map and WeakMap. r=Honza;
|
||
Comment 15•8 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/dd0fc5a3c52f https://hg.mozilla.org/mozilla-central/rev/a85ec3655e36
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•