Closed Bug 1242628 Opened 4 years ago Closed 4 years ago

Add ability to remove a single snapshot from the list

Categories

(DevTools :: Memory, defect, P2)

defect

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
Greg, is this a bug you might be interested in?
Flags: needinfo?(gtatum)
Priority: -- → P2
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: nobody → gtatum
Has STR: --- → irrelevant
Attached patch Bug1242628.patch (obsolete) — Splinter Review
Beyond the code itself, I'm going to post an attachment to get UX feedback for the button location.
Attachment #8715028 - Flags: review?(jsantell)
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)
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)
(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)
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.
Attachment #8717141 - Flags: review?(nfitzgerald)
Ok, the latest one should be correct. Sorry for the noise.
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+
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+
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)
Attachment #8715028 - Attachment is obsolete: true
Attachment #8717131 - Attachment is obsolete: true
Attachment #8717147 - Attachment is obsolete: true
Attachment #8717463 - Attachment is obsolete: true
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
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
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.
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)
Attachment #8717611 - Attachment is obsolete: true
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+
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7378364&repo=fx-team
Flags: needinfo?(gtatum)
Re-based and trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b543c52dd02
Flags: needinfo?(gtatum)
The latest tests appear to be passing now in my latest try tests. I'm re-adding the checkin-needed keyword.
Keywords: checkin-needed
Keywords: checkin-needed
Apparently I didn't test the correct thing with my try push. I will look into this more.
> 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.
Keywords: checkin-needed
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
Fixed the tests that were broken on debug.
Attachment #8720285 - Attachment is obsolete: true
Attachment #8737915 - Attachment is obsolete: true
Attachment #8738627 - Attachment is obsolete: true
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`.
Attachment #8739186 - Attachment is obsolete: true
Flags: needinfo?(gtatum)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0144ac4c8068
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
Depends on: 1476289
You need to log in before you can comment on or make changes to this bug.