Closed
Bug 1454103
Opened 6 years ago
Closed 6 years ago
Correct display of local and sessionStorage in DevTools
Categories
(DevTools :: General, enhancement, P2)
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: miker, Assigned: miker)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
The web console and debugger both make use of reps, which doesn't take account that local and sessionStorage are special objects. Adding a "property" to localStorage doesn't add a property but does add a new entry to the storage type. This means that we shouldn't show any properties except for length. Also, we currently fail to display keys that have the same name as anything on the prototype. We also neglect to show an <entries> node even though storage types are collections of name, value entries.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
The attached images show the web console and debugger after this fix.
Comment 6•6 years ago
|
||
tried it and works like a charm :) it will be even better when we'll autoexpand <entries> node in the object inspector. Would you mind creating a PR in devtools-core instead of modifying bundles here ? I'm planning a new reps bundle release this week so we won't have to wait long. Also, can we add a mochitest to make sure we can inspect localStorage and sessionStorage as expected in the console ?
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6) > tried it and works like a charm :) > it will be even better when we'll autoexpand <entries> node in the object > inspector. > > Would you mind creating a PR in devtools-core instead of modifying bundles > here ? I'm planning a new reps bundle release this week so we won't have to > wait long. > Okay, so why not do both? I have created a pull request at https://github.com/devtools-html/devtools-core/pull/1046 which updates devtools-connection and devtools-reps... it is a harmless patch that will do nothing until the object inspector sends the new type. > Also, can we add a mochitest to make sure we can inspect localStorage and > sessionStorage as expected in the console ? Test coming soon.
Assignee | ||
Comment 8•6 years ago
|
||
By "why not do both?" I mean why not keep the changes in to devtools-reps and devtools-connection in this patch? I just means we get the changes as soon as this patch lands.
Comment 9•6 years ago
|
||
Thanks for the PR on Github Mike, I'll review it shortly. (In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #8) > By "why not do both?" I mean why not keep the changes in to devtools-reps > and devtools-connection in this patch? I just means we get the changes as > soon as this patch lands. That's a solution indeed, but I fear the changes gets overridden in the next reps/debugger release. For reps, that's fine, we can wait for the PR to land and include it in the next bundle. For debugger, it's a bit more tricky since they do multiple release per week, and we need to push a new version of devtools-reps to npm so it can be added as a dependency in the debugger. I think modifying the debugger bundle will add complexity the release process for the debugger team. So maybe here we could: keep the change to reps.js, add a mochitest for console only. And then, when I update the debugger to use the new reps version, I'll add a test for the debugger as well. How does that sound to you Mike ?
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9) > Thanks for the PR on Github Mike, I'll review it shortly. > > (In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from > comment #8) > > By "why not do both?" I mean why not keep the changes in to devtools-reps > > and devtools-connection in this patch? I just means we get the changes as > > soon as this patch lands. > > That's a solution indeed, but I fear the changes gets overridden in the next > reps/debugger release. > For reps, that's fine, we can wait for the PR to land and include it in the > next bundle. > > For debugger, it's a bit more tricky since they do multiple release per > week, and we need to push > a new version of devtools-reps to npm so it can be added as a dependency in > the debugger. > I think modifying the debugger bundle will add complexity the release > process for the debugger team. > > So maybe here we could: keep the change to reps.js, add a mochitest for > console only. > And then, when I update the debugger to use the new reps version, I'll add a > test for the debugger as well. > > How does that sound to you Mike ? Sounds like a plan.
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8967906 [details] Bug 1454103 - Correct display of local and sessionStorage in DevTools https://reviewboard.mozilla.org/r/236600/#review243682 Looks good to me, thanks Mike. I only have a comment in the test where there could be a race condition. For the server side test, we can file a follow up so this doesn't block this patch (which I think is fien since we do have the client-side test). ::: devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_local_session_storage.js:14 (Diff revision 2) > +const TEST_URI = "http://example.com/browser/devtools/client/webconsole/" + > + "test/mochitest/test-local-session-storage.html"; > + > +add_task(async function() { > + const hud = await openNewTabAndConsole(TEST_URI); > + const nodes = await waitFor(() => findMessages(hud, "Storage")); there could be a race here, where the first log is displayed but not the second one yet. Maybe we could do `console.log(localStorage, sessionStorage)` in test-local-session-storage.html so we don't have any risks of such thing ::: devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_local_session_storage.js:50 (Diff revision 2) > + > + entriesNode.querySelector(".arrow").click(); > + await onMapOiMutation; > + > + nodes = oi.querySelectorAll(".node"); > + // There are now 24 nodes, the 4 original ones, and the 20 entries. nit: erroneous comment (there's 6 nodes :) ) ::: devtools/server/actors/object.js:566 (Diff revision 2) > + if (isStorage(this.obj)) { > + if (name === "length") { > + return undefined; > + } > + return { > + configurable: true, > + writable: true, > + enumerable: true, > + value: name > + }; > + } could we pull this out of the if and keep it as it was before ? ::: devtools/server/actors/object/property-iterator.js:139 (Diff revision 2) > } > > function enumObjectProperties(objectActor, options) { > let names = []; > try { > + if (!ObjectUtils.isStorage(objectActor.obj)) { can this happen since we have enumStorageEntries ? ::: devtools/server/actors/object/property-iterator.js:291 (Diff revision 2) > +function enumStorageEntries(objectActor) { > + // Iterating over local / sessionStorage entries goes through various > + // intermediate objects - an Iterator object, then a 2-element Array object, > + // then the actual values we care about. We don't have Xrays to Iterator > + // objects, so we get Opaque wrappers for them. > + let raw = objectActor.obj.unsafeDereference(); > + let keys = []; > + for (let i = 0; i < raw.length; i++) { > + keys.push(raw.key(i)); > + } > + return { > + [Symbol.iterator]: function* () { > + for (let key of keys) { > + let value = raw.getItem(key); > + yield [ key, value ].map(val => gripFromEntry(objectActor, val)); > + } > + }, > + size: keys.length, > + propertyName(index) { > + return index; > + }, > + propertyDescription(index) { > + let key = keys[index]; > + let val = raw.getItem(key); > + return { > + enumerable: true, > + value: { > + type: "storageEntry", > + preview: { > + key: gripFromEntry(objectActor, key), > + value: gripFromEntry(objectActor, val) > + } > + } > + }; > + } > + }; > +} this looks good. Could we have a unit test for this, with longString keys and longString values (I assume that's the only reason we create a grip for key and value, since there should always be strings) ?
Attachment #8967906 -
Flags: review?(nchevobbe) → review+
Assignee | ||
Comment 13•6 years ago
|
||
mozreview-review |
Comment on attachment 8967906 [details] Bug 1454103 - Correct display of local and sessionStorage in DevTools https://reviewboard.mozilla.org/r/236600/#review246100 ::: devtools/server/actors/object.js:566 (Diff revision 2) > + if (isStorage(this.obj)) { > + if (name === "length") { > + return undefined; > + } > + return { > + configurable: true, > + writable: true, > + enumerable: true, > + value: name > + }; > + } Done
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967906 [details] Bug 1454103 - Correct display of local and sessionStorage in DevTools https://reviewboard.mozilla.org/r/236600/#review243682 > there could be a race here, where the first log is displayed but not the second one yet. Maybe we could do `console.log(localStorage, sessionStorage)` in test-local-session-storage.html so we don't have any risks of such thing I decided to log the messages directly from the test and wait for each message individually because that gives us finer grained control. I think this makes more sense from a webconsole test perspective anyhow. > could we pull this out of the if and keep it as it was before ? Done > can this happen since we have enumStorageEntries ? Nope, removed. > this looks good. Could we have a unit test for this, with longString keys and longString values (I assume that's the only reason we create a grip for key and value, since there should always be strings) ? There are not unit tests for `enumMapEntries, enumWeakMapEntries, enumSetEntries` or `enumWeakSetEntries` so poor old `enumStorageEntries` is feeling a bit singled out... would you like me to log a bug to create tests for all of them?
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8967906 [details] Bug 1454103 - Correct display of local and sessionStorage in DevTools https://reviewboard.mozilla.org/r/236600/#review243682 > There are not unit tests for `enumMapEntries, enumWeakMapEntries, enumSetEntries` or `enumWeakSetEntries` so poor old `enumStorageEntries` is feeling a bit singled out... would you like me to log a bug to create tests for all of them? would be great
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•6 years ago
|
||
Bug 1457445 is the property enumerator test bug.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
Pushed by mratcliffe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b860b811cbb0 Correct display of local and sessionStorage in DevTools r=nchevobbe
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b860b811cbb0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•