1242628 - Add ability to remove a single snapshot from the list

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P2)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Greg, is this a bug you might be interested in?

Comment 2

[Greg Tatum [:gregtatum]]

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.

Comment 3

[Greg Tatum [:gregtatum]]

Attachment: Bug1242628.patch

Beyond the code itself, I'm going to post an attachment to get UX feedback for the button location.

Comment 4

[Greg Tatum [:gregtatum]]

Attachment: 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 :)

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 {
  /* ... */
}

Comment 6

[Helen V. Holmes (:helenvholmes) (:✨)]

(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.

Comment 7

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=jsantell

Comment 8

[Greg Tatum [:gregtatum]]

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.

Comment 9

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

Comment 10

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

Comment 11

[Greg Tatum [:gregtatum]]

Ok, the latest one should be correct. Sorry for the noise.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 13

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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 :)

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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>`.

Comment 16

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

Comment 17

[Greg Tatum [:gregtatum]]

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

[Julian Descottes [:jdescottes]]

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/[e-mail]-6fe67b260d0eaee85cc38747b90529f97807b550/try-linux-debug/try_ubuntu32_vm-debug_test-mochitest-devtools-chrome-2-bm02-tests1-linux32-build2837.txt.gz

Comment 19

[Greg Tatum [:gregtatum]]

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.

Comment 20

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

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

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b4a09e08d866

Comment 23

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7378364&repo=fx-team

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/5ff1aed1b7ca

Comment 25

[Greg Tatum [:gregtatum]]

Re-based and trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b543c52dd02

Comment 26

[Greg Tatum [:gregtatum]]

The latest tests appear to be passing now in my latest try tests. I'm re-adding the checkin-needed keyword.

Comment 27

[Greg Tatum [:gregtatum]]

Apparently I didn't test the correct thing with my try push. I will look into this more.

Comment 28

[Greg Tatum [:gregtatum]]

> 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.

Comment 29

[Carsten Book [:Tomcat]]

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

Comment 30

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=014e53c1e75d

Comment 31

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc017a2aac63

Comment 32

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

Fixed the tests that were broken on debug.

Comment 33

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

Comment 34

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31ee51e7871c

Comment 35

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

Comment 36

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73c9fe352df8

Comment 37

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=509f6b0f24bb

Comment 38

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07fd8a9b1c4c

Comment 39

[Greg Tatum [:gregtatum]]

Attachment: Add ability to remove a single snapshot from the list -r=fitzgen

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`.

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0144ac4c8068

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0144ac4c8068

Comment 42

[Will Bamberg [:wbamberg]]

-> https://developer.mozilla.org/en-US/docs/Tools/Memory/Basic_operations#Clearing_a_snapshot