Closed
Bug 1242628
Opened 8 years ago
Closed 8 years ago
Add ability to remove a single snapshot from the list
Categories
(DevTools :: Memory, defect, P2)
DevTools
Memory
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: fitzgen, Assigned: gregtatum, Mentored)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 10 obsolete files)
168.89 KB,
image/png
|
Details | |
18.94 KB,
patch
|
Details | Diff | Splinter Review |
A little "X" button/link that is visible on hover and selection? Technically, very easy. Look at the code for clearing all heap snapshots but just do less work: https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/actions/snapshot.js#414 https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/app.js#102
Reporter | ||
Comment 1•8 years ago
|
||
Greg, is this a bug you might be interested in?
Flags: needinfo?(gtatum)
Reporter | ||
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•8 years ago
|
||
Sure! I haven't gotten to the UI side of things at all yet, so this would probably be good. I can work on this after my first bug.
Flags: needinfo?(gtatum)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gtatum
Reporter | ||
Updated•8 years ago
|
Has STR: --- → irrelevant
Assignee | ||
Comment 3•8 years ago
|
||
Beyond the code itself, I'm going to post an attachment to get UX feedback for the button location.
Attachment #8715028 -
Flags: review?(jsantell)
Assignee | ||
Comment 4•8 years ago
|
||
Helen: Do you have any opinions on delete button location? I added it next to the save button since there was space for it, and it was easy :)
Flags: needinfo?(hholmes)
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8715028 [details] [diff] [review] Bug1242628.patch Review of attachment 8715028 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review, hope no one minds! On the right path, but I have a couple comments to be addressed below. Additionally, I'd like to see a component test (so devtools/client/memory/test/chrome/test_SnapshotListItem_01.html) for SnapshotListItem that tests that (a) we do not show the "delete" button when the snapshot does not have a path, and (b) that we do show it when the snapshot does have a path. See devtools/client/memory/test/chrome/test_DominatorTreeItem_01.html as a similar example to crib from. ::: devtools/client/memory/actions/io.js @@ +90,5 @@ > + return function* (dispatch, getState) { > + dispatch({ > + type: actions.DELETE_SNAPSHOTS_START, > + ids: [snapshot.id] > + }); This needs to also tell the HeapAnalysesWorker delete the snapshot via the HeapAnalysesClient. HeapAnalysesClient.prototype.deleteHeapSnapshot: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/heapsnapshot/HeapAnalysesClient.js#54-63 Example usage: https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/actions/snapshot.js#424 Finally, this function should be in the devtools/client/memory/actions/snapshot.js file. ::: devtools/client/memory/test/browser/browser_memory-clear-one-snapshot.js @@ +32,5 @@ > + > + info("The remaining snapshots get cleared"); > + yield clearSingleSnapshot(panel.panelWin, 1); > + yield clearSingleSnapshot(panel.panelWin, 0); > + is(getState().snapshots.length, 0, "Two snapshots left"); "No snapshots left" ::: devtools/client/memory/test/browser/head.js @@ +122,5 @@ > (snapshot) => snapshot.state !== states.SAVED_CENSUS) > ); > } > > +function clearSingleSnapshot (window, i) { Please add a comment describing this function and its parameters, for example: /** * Removes a single snapshot from the snapshot list by clicking * on the "delete" button. * * @param {Window} window * The memory panel's window. * @param {Number} i * The index of the snapshot to delete. */ @@ +127,5 @@ > + let { gStore, document } = window; > + let snapshotCount = gStore.getState().snapshots.length; > + let buttons = document.querySelectorAll("#memory-tool-container .snapshot-list-item a.delete"); > + buttons[i].click(); > + return waitUntilState(gStore, () => gStore.getState().snapshots.length === snapshotCount - 1); Nit: This line is getting long, please wrap to ~80 characters. ::: devtools/client/themes/memory.css @@ +189,5 @@ > justify-content: space-between; > font-size: 90%; > } > > +.snapshot-list-item .save, .snapshot-list-item .delete { Nit: please put each selector on its own line, eg: .snapshot-list-item .save, .snapshot-list-item .delete { /* ... */ }
Attachment #8715028 -
Flags: review?(jsantell)
Comment 6•8 years ago
|
||
(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #4) > Created attachment 8715033 [details] > Screenshot of delete button > > Helen: Do you have any opinions on delete button location? I added it next > to the save button since there was space for it, and it was easy :) I'm a fan of Nick's initial suggestion—an 'x' that appears on hover on the right side of the snapshot, vertical aligned to be in the center, with some margin on the right.
Flags: needinfo?(hholmes)
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Comment 8•8 years ago
|
||
Not sure that's the correct patch... double checking. And the review was still set to jsantell. I'm trying out the moz-git-tools for the first time.
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8717141 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8717147 -
Flags: review?(nfitzgerald)
Assignee | ||
Comment 11•8 years ago
|
||
Ok, the latest one should be correct. Sorry for the noise.
Reporter | ||
Comment 12•8 years ago
|
||
Comment on attachment 8717147 [details] [diff] [review] Add ability to remove a single snapshot from the list -r=fitzgen Review of attachment 8717147 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments below addressed Thanks! ::: devtools/client/memory/actions/snapshot.js @@ +453,5 @@ > + heapWorker.deleteHeapSnapshot(snapshot.path) > + .catch(error => { > + reportException("deleteSnapshot", error); > + dispatch({ type: actions.SNAPSHOT_ERROR, id: snapshot.id, error }); > + }); This should be: try { yield heapWorker.deleteHeapSnapshot(snapshot.path); } catch (error) { reportException("deleteSnapshot", error); dispatch({ type: actions.SNAPSHOT_ERROR, id: snapshot.id, error }); } We want to yield because we don't want to dispatch the end action before the delete operation actually ends. ::: devtools/client/memory/test/browser/browser_memory_clear_one_snapshot.js @@ +17,5 @@ > + > + info("Take three snapshots"); > + yield takeSnapshot(panel.panelWin); > + yield takeSnapshot(panel.panelWin); > + yield takeSnapshot(panel.panelWin); Nit: no need to yield on each snapshot here, since we are waiting for them all to complete below. Removing the yields will speed up the test a little bit. @@ +18,5 @@ > + info("Take three snapshots"); > + yield takeSnapshot(panel.panelWin); > + yield takeSnapshot(panel.panelWin); > + yield takeSnapshot(panel.panelWin); > + yield waitUntilSnapshotState(gStore, [states.SAVED_CENSUS, states.SAVED_CENSUS, states.SAVED_CENSUS]); The `waitUntilSnapshotState` helper is gone now because it was causing leaks (no idea how/why) so you need to write this out explicitly (which doesn't leak for whatever reason): yield waitUntilState(gStore, state => { return state.snapshots.length === 3 && state.snapshots[0].state === states.SAVED_CENSUS && state.snapshots[1].state === states.SAVED_CENSUS && state.snapshots[2].state === states.SAVED_CENSUS; }); ::: devtools/client/memory/test/chrome/head.js @@ +171,5 @@ > snapshots: [], > }); > > +var TEST_SNAPSHOT_LIST_ITEM_PROPS = Object.freeze({ > + onClick : noop, Small style nit: no space between the property and the ":" ::: devtools/client/memory/test/chrome/test_SnapshotListItem_01.html @@ +1,5 @@ > +<!DOCTYPE HTML> > +<html> > +<!-- > +Test to verify that the delete button only shows up for a snapshot when it has a > +path. Thanks for writing this test! @@ +29,5 @@ > + "Should have delete button when there is a path"); > + > + const pathlessProps = immutableUpdate( > + TEST_SNAPSHOT_LIST_ITEM_PROPS, > + {item: immutableUpdate(TEST_SNAPSHOT, {path: false})} Nit: the path should be set to null or undefined, which are the values we use as a lack of a path.
Attachment #8717147 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8717463 -
Flags: review?(nfitzgerald)
Reporter | ||
Comment 14•8 years ago
|
||
Comment on attachment 8717463 [details] [diff] [review] Add ability to remove a single snapshot from the list -r=fitzgen Review of attachment 8717463 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Greg! In the future, when a reviewer says something like "r=me with comments below adressed" and marks the review r+, you don't need to reflag them for review, just fix up the comments and land the patch :)
Attachment #8717463 -
Flags: review?(nfitzgerald) → review+
Reporter | ||
Comment 15•8 years ago
|
||
Comment on attachment 8717141 [details] [diff] [review] Add ability to remove a single snapshot from the list -r=fitzgen Also, you can mark old attachments as obsolete via details > edit details > obsolete checkbox, or in your editor at the bottom when running `git bz attach -e <new commit>`.
Attachment #8717141 -
Attachment is obsolete: true
Attachment #8717141 -
Flags: review?(nfitzgerald)
Assignee | ||
Updated•8 years ago
|
Attachment #8715028 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8717131 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8717147 -
Attachment is obsolete: true
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8717463 -
Attachment is obsolete: true
Assignee | ||
Comment 17•8 years ago
|
||
I re-ran the broken tests on Linux debug, but with the tests reordered, and I changed it from taking 3 snapshots to 2 snapshots. This time it passed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fe67b260d0e&selectedJob=16617482
Comment 18•8 years ago
|
||
I've been working on test timeouts lately as well, as I have a similar issue with Bug 1243131. A few things I find helpful to know when investigating: - the mochitest timeout is set at 45 seconds - the logs contain the tests execution times - jobs can be retriggered from Treeherder UI (select a job, such as dt3, and press "R") Usually to check if you have an intermittent, you just retrigger a job several times. But simply looking at the logs can give you a good hint already. In the logs of your try push for dt3 [1], we can see : - INFO TEST-OK | devtools/client/memory/test/browser/browser_memory_clear_one_snapshot.js | took 31198ms - INFO TEST-OK | devtools/client/memory/test/browser/browser_memory_clear_snapshots.js | took 38708ms From what I've seen so far, if a test takes around 40 seconds to run, there's a high chance for intermittent failures. Here browser_memory_clear_snapshots.js is almost at 40 seconds. I guess it would be better to retrigger dt3 a few times, we might see a few failures. [1] http://archive.mozilla.org/pub/firefox/try-builds/gtatum@mozilla.com-6fe67b260d0eaee85cc38747b90529f97807b550/try-linux-debug/try_ubuntu32_vm-debug_test-mochitest-devtools-chrome-2-bm02-tests1-linux32-build2837.txt.gz
Assignee | ||
Comment 19•8 years ago
|
||
Per the discussion on IRC I will rewrite the browser test to be a component test for SnapshotList and check that the delete button works and the correct handler gets called.
Assignee | ||
Comment 20•8 years ago
|
||
This strips out the previous browser integration test and adds in a chrome test for the list element. I'm getting a fail in the windows 7 builds but it seems unrelated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddd43a58e2df&selectedJob=16789088
Attachment #8720285 -
Flags: review?(nfitzgerald)
Assignee | ||
Updated•8 years ago
|
Attachment #8717611 -
Attachment is obsolete: true
Reporter | ||
Comment 21•8 years ago
|
||
Comment on attachment 8720285 [details] [diff] [review] Add ability to remove a single snapshot from the list -r=fitzgen Review of attachment 8720285 [details] [diff] [review]: ----------------------------------------------------------------- Ship it! ::: devtools/client/memory/test/chrome/test_List_01.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<!-- > +Test to verify the delete button calls the onDelete handler for an item Great test -- thanks!
Attachment #8720285 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b4a09e08d866
Keywords: checkin-needed
Comment 23•8 years ago
|
||
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7378364&repo=fx-team
Flags: needinfo?(gtatum)
Comment 24•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/fx-team/rev/5ff1aed1b7ca
Assignee | ||
Comment 25•8 years ago
|
||
Re-based and trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b543c52dd02
Flags: needinfo?(gtatum)
Assignee | ||
Comment 26•8 years ago
|
||
The latest tests appear to be passing now in my latest try tests. I'm re-adding the checkin-needed keyword.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 27•8 years ago
|
||
Apparently I didn't test the correct thing with my try push. I will look into this more.
Assignee | ||
Comment 28•8 years ago
|
||
> backed out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=7378364&repo=fx-team
Ok, I triggered the appropriate test and it's passing now. Re-adding the checkin-needed keyword.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 29•8 years ago
|
||
has problems to apply: patching file devtools/client/memory/app.js Hunk #1 succeeded at 24 with fuzz 2 (offset 4 lines). patching file devtools/client/memory/test/chrome/chrome.ini Hunk #1 succeeded at 8 with fuzz 2 (offset 1 lines). patching file devtools/client/memory/test/chrome/head.js Hunk #1 FAILED at 31 Hunk #2 FAILED at 99 Hunk #3 FAILED at 162 3 out of 3 hunks FAILED -- saving rejects to file devtools/client/memory/test/chrome/head.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Bug-1242628---Add-ability-to-remove-a-single-snaps.patch
Flags: needinfo?(gtatum)
Keywords: checkin-needed
Assignee | ||
Comment 30•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=014e53c1e75d
Assignee | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc017a2aac63
Assignee | ||
Comment 32•8 years ago
|
||
Fixed the tests that were broken on debug.
Assignee | ||
Updated•8 years ago
|
Attachment #8720285 -
Attachment is obsolete: true
Assignee | ||
Comment 33•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8737915 -
Attachment is obsolete: true
Assignee | ||
Comment 34•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ee51e7871c
Assignee | ||
Comment 35•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8738627 -
Attachment is obsolete: true
Assignee | ||
Comment 36•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73c9fe352df8
Assignee | ||
Comment 37•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=509f6b0f24bb
Assignee | ||
Comment 38•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=07fd8a9b1c4c
Assignee | ||
Comment 39•8 years ago
|
||
This latest patch fixes the failing test with no snapshot. There was a small logic error in the if statement in refreshSelectedTreeMap in `devtools/client/memory/actions/snapshot.js`.
Assignee | ||
Updated•8 years ago
|
Attachment #8739186 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(gtatum)
Keywords: checkin-needed
Comment 40•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/0144ac4c8068
Keywords: checkin-needed
Comment 41•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0144ac4c8068
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•8 years ago
|
Keywords: dev-doc-needed
Updated•8 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 42•8 years ago
|
||
-> https://developer.mozilla.org/en-US/docs/Tools/Memory/Basic_operations#Clearing_a_snapshot
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•