Bug 1242628 - Add ability to remove a single snapshot from the list. r=fitzgen

--HG--
extra : rebase_source : 59021ac5bb294442180a064eaa4ad0ee7a65a3ec

devtools/client/locales/en-US/memory.properties

@@ -27,6 +27,10 @@ memory.tooltip=Memory
 # snapshot to disk.
 snapshot.io.save=Save
 
+# LOCALIZATION NOTE (snapshot.io.delete): The label for the link that deletes
+# a snapshot
+snapshot.io.delete=Delete
+
 # LOCALIZATION NOTE (snapshot.io.save.window): The title for the window
 # displayed when saving a snapshot to disk.
 snapshot.io.save.window=Save Heap Snapshot

devtools/client/memory/actions/snapshot.js

@@ -482,6 +482,9 @@ const refreshSelectedCensus = exports.refreshSelectedCensus = function (heapWork
 const refreshSelectedTreeMap = exports.refreshSelectedTreeMap = function (heapWorker) {
   return function*(dispatch, getState) {
     let snapshot = getState().snapshots.find(s => s.selected);
+    if (!snapshot || snapshot.state !== states.READ) {
+      return;
+    }
 
     // Intermediate snapshot states will get handled by the task action that is
     // orchestrating them. For example, if the snapshot census's state is
@@ -489,8 +492,7 @@ const refreshSelectedTreeMap = exports.refreshSelectedTreeMap = function (heapWo
     // the inverted property matches the inverted state. If the snapshot is
     // still in the process of being saved or read, the takeSnapshotAndCensus
     // task action will follow through and ensure that a census is taken.
-    if (snapshot &&
-        (snapshot.treeMap && snapshot.treeMap.state === treeMapState.SAVED) ||
+    if ((snapshot.treeMap && snapshot.treeMap.state === treeMapState.SAVED) ||
         !snapshot.treeMap) {
       yield dispatch(takeTreeMap(heapWorker, snapshot.id));
     }
@@ -761,6 +763,27 @@ const clearSnapshots = exports.clearSnapshots = function (heapWorker) {
   };
 };
 
+/**
+ * Delete a snapshot
+ *
+ * @param {HeapAnalysesClient} heapWorker
+ * @param {snapshotModel} snapshot
+ */
+const deleteSnapshot = exports.deleteSnapshot = function (heapWorker, snapshot) {
+  return function*(dispatch, getState) {
+    dispatch({ type: actions.DELETE_SNAPSHOTS_START, ids: [snapshot.id] });
+
+    try {
+      yield heapWorker.deleteHeapSnapshot(snapshot.path);
+    } catch (error) {
+      reportException("deleteSnapshot", error);
+      dispatch({ type: actions.SNAPSHOT_ERROR, id: snapshot.id, error });
+    }
+
+    dispatch({ type: actions.DELETE_SNAPSHOTS_END, ids: [snapshot.id] });
+  };
+};
+
 /**
  * Expand the given node in the snapshot's census report.
  *

devtools/client/memory/app.js

@@ -30,6 +30,7 @@ const {
   selectSnapshotAndRefresh,
   takeSnapshotAndCensus,
   clearSnapshots,
+  deleteSnapshot,
   fetchImmediatelyDominated,
   expandCensusNode,
   collapseCensusNode,
@@ -210,6 +211,7 @@ const MemoryApp = createClass({
             itemComponent: SnapshotListItem,
             items: snapshots,
             onSave: snapshot => dispatch(pickFileAndExportSnapshot(snapshot)),
+            onDelete: snapshot => dispatch(deleteSnapshot(heapWorker, snapshot)),
             onClick: onClickSnapshotListItem,
             diffing,
           }),

devtools/client/memory/components/snapshot-list-item.js

@@ -26,12 +26,13 @@ const SnapshotListItem = module.exports = createClass({
   propTypes: {
     onClick: PropTypes.func.isRequired,
     onSave: PropTypes.func.isRequired,
+    onDelete: PropTypes.func.isRequired,
     item: snapshotModel.isRequired,
     index: PropTypes.number.isRequired,
   },
 
   render() {
-    let { index, item: snapshot, onClick, onSave, diffing } = this.props;
+    let { index, item: snapshot, onClick, onSave, onDelete, diffing } = this.props;
     let className = `snapshot-list-item ${snapshot.selected ? " selected" : ""}`;
     let statusText = getStatusText(snapshot.state);
     let wantThrobber = !!statusText;
@@ -88,13 +89,20 @@ const SnapshotListItem = module.exports = createClass({
     let saveLink = !snapshot.path ? void 0 : dom.a({
       onClick: () => onSave(snapshot),
       className: "save",
-    }, L10N.getFormatStr("snapshot.io.save"));
+    }, L10N.getStr("snapshot.io.save"));
+
+    let deleteButton = !snapshot.path ? void 0 : dom.button({
+      onClick: () => onDelete(snapshot),
+      className: "devtools-button delete",
+      title: L10N.getStr("snapshot.io.delete")
+    });
 
     return (
       dom.li({ className, onClick },
         dom.span({ className: `snapshot-title ${wantThrobber ? " devtools-throbber" : ""}` },
           checkbox,
-          title
+          title,
+          deleteButton
         ),
         dom.span({ className: "snapshot-info" },
           details,

devtools/client/memory/test/chrome/chrome.ini

@@ -12,7 +12,9 @@ support-files =
 [test_Heap_03.html]
 [test_Heap_04.html]
 [test_Heap_05.html]
+[test_List_01.html]
 [test_ShortestPaths_01.html]
 [test_ShortestPaths_02.html]
+[test_SnapshotListItem_01.html]
 [test_Toolbar_01.html]
 [test_TreeMap_01.html]

devtools/client/memory/test/chrome/head.js

@@ -53,6 +53,8 @@ var DominatorTreeComponent = React.createFactory(require("devtools/client/memory
 var DominatorTreeItem = React.createFactory(require("devtools/client/memory/components/dominator-tree-item"));
 var ShortestPaths = React.createFactory(require("devtools/client/memory/components/shortest-paths"));
 var TreeMap = React.createFactory(require("devtools/client/memory/components/tree-map"));
+var SnapshotListItem = React.createFactory(require("devtools/client/memory/components/snapshot-list-item"));
+var List = React.createFactory(require("devtools/client/memory/components/list"));
 var Toolbar = React.createFactory(require("devtools/client/memory/components/toolbar"));
 
 // All tests are asynchronous.
@@ -163,6 +165,43 @@ var TEST_SHORTEST_PATHS_PROPS = Object.freeze({
   }),
 });
 
+var TEST_SNAPSHOT = Object.freeze({
+  id: 1337,
+  selected: true,
+  path: "/fake/path/to/snapshot",
+  census: Object.freeze({
+    report: Object.freeze({
+      objects: Object.freeze({ count: 4, bytes: 400 }),
+      scripts: Object.freeze({ count: 3, bytes: 300 }),
+      strings: Object.freeze({ count: 2, bytes: 200 }),
+      other: Object.freeze({ count: 1, bytes: 100 }),
+    }),
+    display: Object.freeze({
+      displayName: "Test Display",
+      tooltip: "Test display tooltup",
+      inverted: false,
+      breakdown: Object.freeze({
+        by: "coarseType",
+        objects: Object.freeze({ by: "count", count: true, bytes: true }),
+        scripts: Object.freeze({ by: "count", count: true, bytes: true }),
+        strings: Object.freeze({ by: "count", count: true, bytes: true }),
+        other: Object.freeze({ by: "count", count: true, bytes: true }),
+      }),
+    }),
+    state: censusState.SAVED,
+    inverted: false,
+    filter: null,
+    expanded: new Set(),
+    focused: null,
+    parentMap: Object.freeze(Object.create(null))
+  }),
+  dominatorTree: TEST_DOMINATOR_TREE,
+  error: null,
+  imported: false,
+  creationTime: 0,
+  state: snapshotState.READ,
+});
+
 var TEST_HEAP_PROPS = Object.freeze({
   onSnapshotClick: noop,
   onLoadMoreSiblings: noop,
@@ -175,42 +214,7 @@ var TEST_HEAP_PROPS = Object.freeze({
   onViewSourceInDebugger: noop,
   diffing: null,
   view: { state: viewState.CENSUS, },
-  snapshot: Object.freeze({
-    id: 1337,
-    selected: true,
-    path: "/fake/path/to/snapshot",
-    census: Object.freeze({
-      report: Object.freeze({
-        objects: Object.freeze({ count: 4, bytes: 400 }),
-        scripts: Object.freeze({ count: 3, bytes: 300 }),
-        strings: Object.freeze({ count: 2, bytes: 200 }),
-        other: Object.freeze({ count: 1, bytes: 100 }),
-      }),
-      display: Object.freeze({
-        displayName: "Test Display",
-        tooltip: "Test display tooltup",
-        inverted: false,
-        breakdown: Object.freeze({
-          by: "coarseType",
-          objects: Object.freeze({ by: "count", count: true, bytes: true }),
-          scripts: Object.freeze({ by: "count", count: true, bytes: true }),
-          strings: Object.freeze({ by: "count", count: true, bytes: true }),
-          other: Object.freeze({ by: "count", count: true, bytes: true }),
-        }),
-      }),
-      state: censusState.SAVED,
-      inverted: false,
-      filter: null,
-      expanded: new Set(),
-      focused: null,
-      parentMap: Object.freeze(Object.create(null))
-    }),
-    dominatorTree: TEST_DOMINATOR_TREE,
-    error: null,
-    imported: false,
-    creationTime: 0,
-    state: snapshotState.READ,
-  }),
+  snapshot: TEST_SNAPSHOT,
   sizes: Object.freeze({ shortestPathsSize: .5 }),
   onShortestPathsResize: noop,
 });
@@ -285,6 +289,14 @@ var TEST_TREE_MAP_PROPS = Object.freeze({
   })
 });
 
+var TEST_SNAPSHOT_LIST_ITEM_PROPS = Object.freeze({
+  onClick: noop,
+  onSave: noop,
+  onDelete: noop,
+  item: TEST_SNAPSHOT,
+  index: 1234,
+});
+
 function onNextAnimationFrame(fn) {
   return () =>
     requestAnimationFrame(() =>

devtools/client/memory/test/chrome/test_List_01.html

@@ -0,0 +1,74 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+Test to verify the delete button calls the onDelete handler for an item
+-->
+<head>
+    <meta charset="utf-8">
+    <title>Tree component test</title>
+    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+    <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+</head>
+<body>
+    <div id="container"></div>
+
+    <pre id="test">
+        <script src="head.js" type="application/javascript;version=1.8"></script>
+        <script type="application/javascript;version=1.8">
+         window.onload = Task.async(function* () {
+          try {
+            const container = document.getElementById("container");
+
+            let deletedSnapshots = [];
+
+            let snapshots = [ TEST_SNAPSHOT, TEST_SNAPSHOT, TEST_SNAPSHOT ]
+              .map((snapshot, index) => immutableUpdate(snapshot, {
+                index: snapshot.index + index
+              }));
+
+            yield renderComponent(
+              List({
+                itemComponent: SnapshotListItem,
+                onClick: noop,
+                onDelete: (item) => deletedSnapshots.push(item),
+                items: snapshots
+              }),
+              container
+            );
+
+            let deleteButtons = container.querySelectorAll('.delete');
+
+            is(container.querySelectorAll('.snapshot-list-item').length, 3,
+              "There are 3 list items\n");
+            is(deletedSnapshots.length, 0,
+              "Not snapshots have been deleted\n");
+
+            deleteButtons[1].click();
+
+            is(deletedSnapshots.length, 1, "One snapshot was deleted\n");
+            is(deletedSnapshots[0], snapshots[1],
+              "Deleted snapshot was added to the deleted list\n");
+
+            deleteButtons[0].click();
+
+            is(deletedSnapshots.length, 2, "Two snapshots were deleted\n");
+            is(deletedSnapshots[1], snapshots[0],
+              "Deleted snapshot was added to the deleted list\n");
+
+            deleteButtons[2].click();
+
+            is(deletedSnapshots.length, 3, "Three snapshots were deleted\n");
+            is(deletedSnapshots[2], snapshots[2],
+              "Deleted snapshot was added to the deleted list\n");
+
+
+          } catch(e) {
+            ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
+          } finally {
+            SimpleTest.finish();
+          }
+         });
+        </script>
+    </pre>
+</body>
+</html>

devtools/client/memory/test/chrome/test_SnapshotListItem_01.html

@@ -0,0 +1,53 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+Test to verify that the delete button only shows up for a snapshot when it has a
+path.
+-->
+<head>
+    <meta charset="utf-8">
+    <title>Tree component test</title>
+    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
+    <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css">
+</head>
+<body>
+    <div id="container"></div>
+
+    <pre id="test">
+        <script src="head.js" type="application/javascript;version=1.8"></script>
+        <script type="application/javascript;version=1.8">
+         window.onload = Task.async(function* () {
+          try {
+            const container = document.getElementById("container");
+
+            yield renderComponent(
+              SnapshotListItem(TEST_SNAPSHOT_LIST_ITEM_PROPS),
+              container
+            );
+
+            ok(container.querySelector('.delete'),
+               "Should have delete button when there is a path");
+
+            const pathlessProps = immutableUpdate(
+                TEST_SNAPSHOT_LIST_ITEM_PROPS,
+                {item: immutableUpdate(TEST_SNAPSHOT, {path: null})}
+            );
+
+            yield renderComponent(
+              SnapshotListItem(pathlessProps),
+              container
+            );
+
+            ok(!container.querySelector('.delete'),
+               "No delete button should be found if there is no path\n");
+
+          } catch(e) {
+            ok(false, "Got an error: " + DevToolsUtils.safeErrorString(e));
+          } finally {
+            SimpleTest.finish();
+          }
+         });
+        </script>
+    </pre>
+</body>
+</html>

devtools/client/themes/memory.css

@@ -197,11 +197,32 @@ html, body, #app, #memory-tool {
   font-size: 90%;
 }
 
+.snapshot-list-item .snapshot-title {
+  display: flex;
+  justify-content: space-between;
+}
+
 .snapshot-list-item .save {
   text-decoration: underline;
   cursor: pointer;
 }
 
+.snapshot-list-item .delete {
+  cursor: pointer;
+  position: relative;
+  min-height: 1em;
+  min-width: 1.3em;
+}
+
+.theme-light .snapshot-list-item.selected .delete {
+  filter: invert(100%);
+}
+
+.snapshot-list-item .delete::before {
+  background-image: url("chrome://devtools/skin/images/close.svg");
+  background-position: 0.2em 0;
+}
+
 .snapshot-list-item > .snapshot-title {
   margin-bottom: 14px;
 }