Bug 1242628 - Add ability to remove a single snapshot from the list -r=fitzgen
☠☠ backed out by 5ff1aed1b7ca ☠ ☠
authorGreg Tatum <tatum.creative@gmail.com>
Wed, 17 Feb 2016 06:19:00 +0100
changeset 310994 b4a09e08d866e0793347dfaf7aaf54c75ee5a349
parent 310993 05f74b15d9cf3b53d2295f17f52524f81b8fc72b
child 310995 0d9a089756b5c5497ab6936baec6b3182cdd35b9
push idunknown
push userunknown
push dateunknown
bugs1242628
milestone47.0a1
Bug 1242628 - Add ability to remove a single snapshot from the list -r=fitzgen
devtools/client/locales/en-US/memory.properties
devtools/client/memory/actions/snapshot.js
devtools/client/memory/app.js
devtools/client/memory/components/snapshot-list-item.js
devtools/client/memory/test/chrome/chrome.ini
devtools/client/memory/test/chrome/head.js
devtools/client/memory/test/chrome/test_List_01.html
devtools/client/memory/test/chrome/test_SnapshotListItem_01.html
devtools/client/themes/memory.css
--- a/devtools/client/locales/en-US/memory.properties
+++ b/devtools/client/locales/en-US/memory.properties
@@ -22,16 +22,20 @@ memory.panelLabel=Memory Panel
 # LOCALIZATION NOTE (memory.tooltip): This string is displayed in the tooltip of
 # the tab when the memory tool is displayed inside the developer tools window.
 memory.tooltip=Memory
 
 # LOCALIZATION NOTE (snapshot.io.save): The label for the link that saves a
 # 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
 
 # LOCALIZATION NOTE (snapshot.io.import.window): The title for the window
 # displayed when importing a snapshot form disk.
 snapshot.io.import.window=Import Heap Snapshot
 
--- a/devtools/client/memory/actions/snapshot.js
+++ b/devtools/client/memory/actions/snapshot.js
@@ -436,16 +436,37 @@ const clearSnapshots = exports.clearSnap
       });
     }));
 
     dispatch({ type: actions.DELETE_SNAPSHOTS_END, ids });
   };
 };
 
 /**
+ * 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.
  *
  * @param {CensusTreeNode} node
  */
 const expandCensusNode = exports.expandCensusNode = function (id, node) {
   return {
     type: actions.EXPAND_CENSUS_NODE,
     id,
--- a/devtools/client/memory/app.js
+++ b/devtools/client/memory/app.js
@@ -19,16 +19,17 @@ const {
 } = require("./actions/diffing");
 const { toggleInvertedAndRefresh } = require("./actions/inverted");
 const { setFilterStringAndRefresh } = require("./actions/filter");
 const { pickFileAndExportSnapshot, pickFileAndImportSnapshotAndCensus } = require("./actions/io");
 const {
   selectSnapshotAndRefresh,
   takeSnapshotAndCensus,
   clearSnapshots,
+  deleteSnapshot,
   fetchImmediatelyDominated,
   expandCensusNode,
   collapseCensusNode,
   focusCensusNode,
   expandDominatorTreeNode,
   collapseDominatorTreeNode,
   focusDominatorTreeNode,
 } = require("./actions/snapshot");
@@ -161,16 +162,17 @@ const MemoryApp = createClass({
           {
             id: "memory-tool-container"
           },
 
           List({
             itemComponent: SnapshotListItem,
             items: snapshots,
             onSave: snapshot => dispatch(pickFileAndExportSnapshot(snapshot)),
+            onDelete: snapshot => dispatch(deleteSnapshot(heapWorker, snapshot)),
             onClick: onClickSnapshotListItem,
             diffing,
           }),
 
           Heap({
             snapshot: selectedSnapshot,
             diffing,
             onViewSourceInDebugger: frame => toolbox.viewSourceInDebugger(frame.source, frame.line),
--- a/devtools/client/memory/components/snapshot-list-item.js
+++ b/devtools/client/memory/components/snapshot-list-item.js
@@ -15,22 +15,23 @@ const { snapshotState: states, diffingSt
 const { snapshot: snapshotModel } = require("../models");
 
 const SnapshotListItem = module.exports = createClass({
   displayName: "SnapshotListItem",
 
   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;
     let title = getSnapshotTitle(snapshot);
 
     const selectedForDiffing = diffing
           && (diffing.firstSnapshotId === snapshot.id
               || diffing.secondSnapshotId === snapshot.id);
@@ -72,21 +73,28 @@ const SnapshotListItem = module.exports 
       details = dom.span({ className: "snapshot-state" }, statusText);
     }
 
     let saveLink = !snapshot.path ? void 0 : dom.a({
       onClick: () => onSave(snapshot),
       className: "save",
     }, L10N.getFormatStr("snapshot.io.save"));
 
+    let deleteButton = !snapshot.path ? void 0 : dom.button({
+      onClick: () => onDelete(snapshot),
+      className: "devtools-button delete",
+      title: L10N.getFormatStr("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,
           saveLink
         )
       )
     );
   }
--- a/devtools/client/memory/test/chrome/chrome.ini
+++ b/devtools/client/memory/test/chrome/chrome.ini
@@ -5,10 +5,12 @@ support-files =
 [test_DominatorTree_01.html]
 [test_DominatorTree_02.html]
 [test_DominatorTree_03.html]
 [test_DominatorTreeItem_01.html]
 [test_Heap_01.html]
 [test_Heap_02.html]
 [test_Heap_03.html]
 [test_Heap_04.html]
+[test_List_01.html]
+[test_SnapshotListItem_01.html]
 [test_Toolbar_01.html]
 [test_Toolbar_02.html]
--- a/devtools/client/memory/test/chrome/head.js
+++ b/devtools/client/memory/test/chrome/head.js
@@ -32,16 +32,18 @@ const {
 
 var models = require("devtools/client/memory/models");
 
 var React = require("devtools/client/shared/vendor/react");
 var ReactDOM = require("devtools/client/shared/vendor/react-dom");
 var Heap = React.createFactory(require("devtools/client/memory/components/heap"));
 var DominatorTreeComponent = React.createFactory(require("devtools/client/memory/components/dominator-tree"));
 var DominatorTreeItem = React.createFactory(require("devtools/client/memory/components/dominator-tree-item"));
+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.
 SimpleTest.waitForExplicitFinish();
 
 var noop = () => {};
 
 // Counter for mock DominatorTreeNode ids.
@@ -100,57 +102,59 @@ var TEST_DOMINATOR_TREE = Object.freeze(
 var TEST_DOMINATOR_TREE_PROPS = Object.freeze({
   dominatorTree: TEST_DOMINATOR_TREE,
   onLoadMoreSiblings: noop,
   onViewSourceInDebugger: noop,
   onExpand: noop,
   onCollapse: noop,
 });
 
+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 }),
+    }),
+    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 }),
+    }),
+    inverted: false,
+    filter: null,
+    expanded: new Set(),
+    focused: null,
+  }),
+  dominatorTree: TEST_DOMINATOR_TREE,
+  error: null,
+  imported: false,
+  creationTime: 0,
+  state: snapshotState.SAVED_CENSUS,
+});
+
 var TEST_HEAP_PROPS = Object.freeze({
   onSnapshotClick: noop,
   onLoadMoreSiblings: noop,
   onCensusExpand: noop,
   onCensusCollapse: noop,
   onDominatorTreeExpand: noop,
   onDominatorTreeCollapse: noop,
   onCensusFocus: noop,
   onDominatorTreeFocus: noop,
   onViewSourceInDebugger: noop,
   diffing: null,
   view: 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 }),
-      }),
-      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 }),
-      }),
-      inverted: false,
-      filter: null,
-      expanded: new Set(),
-      focused: null,
-    }),
-    dominatorTree: TEST_DOMINATOR_TREE,
-    error: null,
-    imported: false,
-    creationTime: 0,
-    state: snapshotState.SAVED_CENSUS,
-  }),
+  snapshot: TEST_SNAPSHOT
 });
 
 var TEST_TOOLBAR_PROPS = Object.freeze({
   breakdowns: getBreakdownDisplayData(),
   onTakeSnapshotClick: noop,
   onImportClick: noop,
   onBreakdownChange: noop,
   onToggleRecordAllocationStacks: noop,
@@ -163,16 +167,24 @@ var TEST_TOOLBAR_PROPS = Object.freeze({
   onToggleDiffing: noop,
   view: viewState.CENSUS,
   onViewChange: noop,
   dominatorTreeBreakdowns: getDominatorTreeBreakdownDisplayData(),
   onDominatorTreeBreakdownChange: noop,
   snapshots: [],
 });
 
+var TEST_SNAPSHOT_LIST_ITEM_PROPS = Object.freeze({
+  onClick: noop,
+  onSave: noop,
+  onDelete: noop,
+  item: TEST_SNAPSHOT,
+  index: 1234,
+});
+
 function onNextAnimationFrame(fn) {
   return () =>
     requestAnimationFrame(() =>
       requestAnimationFrame(fn));
 }
 
 /**
  * Render the provided ReactElement in the provided HTML container.
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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>
--- a/devtools/client/themes/memory.css
+++ b/devtools/client/themes/memory.css
@@ -189,21 +189,48 @@ html, body, #app, #memory-tool {
 }
 
 .snapshot-list-item .snapshot-info {
   display: flex;
   justify-content: space-between;
   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;
+  min-width: 20px;
+  position: relative;
+  top:-2px;
+  left:4px;
+}
+
+.theme-light .snapshot-list-item.selected .delete {
+  filter: invert(100%);
+}
+
+.snapshot-list-item .delete::before {
+  background-image: url(images/close.png);
+}
+
+@media (min-resolution: 1.1dppx) {
+  .snapshot-list-item .delete::before {
+    background-image: url(images/close@2x.png);
+  }
+}
+
 .snapshot-list-item > .snapshot-title {
   margin-bottom: 14px;
 }
 
 .snapshot-list-item > .snapshot-title > input[type=checkbox] {
   margin: 0;
   margin-inline-end: 5px;
 }