Bug 1532319 - Log Points should have context. r=davidwalsh

This has some fun wins

- colored prompt
- multiline textarea
- default value for log points

Differential Revision: https://phabricator.services.mozilla.com/D26585

--HG--
extra : moz-landing-system : lando

devtools/client/debugger/src/actions/breakpoints/index.js

@@ -244,7 +244,11 @@ export function toggleBreakpointAtLine(cx: Context, line: number) {
   };
 }
 
-export function addBreakpointAtLine(cx: Context, line: number) {
+export function addBreakpointAtLine(
+  cx: Context,
+  line: number,
+  shouldLog: ?boolean = false
+) {
   return ({ dispatch, getState, client, sourceMaps }: ThunkArgs) => {
     const state = getState();
     const source = getSelectedSource(state);
@@ -252,15 +256,19 @@ export function addBreakpointAtLine(cx: Context, line: number) {
     if (!source || isEmptyLineInSource(state, line, source.id)) {
       return;
     }
+    const breakpointLocation = {
+      sourceId: source.id,
+      sourceUrl: source.url,
+      column: undefined,
+      line
+    };
 
-    return dispatch(
-      addBreakpoint(cx, {
-        sourceId: source.id,
-        sourceUrl: source.url,
-        column: undefined,
-        line
-      })
-    );
+    const options = {};
+    if (shouldLog) {
+      options.logValue = "displayName";
+    }
+
+    return dispatch(addBreakpoint(cx, breakpointLocation, options));
   };
 }
 

devtools/client/debugger/src/components/Editor/ConditionalPanel.css

@@ -15,12 +15,18 @@
 
 .conditional-breakpoint-panel .prompt {
   font-size: 1.8em;
-  color: var(--theme-conditional-breakpoint-color);
+  color: var(--theme-graphs-orange);
   padding-left: 3px;
   padding-right: 3px;
   padding-bottom: 3px;
   text-align: right;
   width: 30px;
+  align-self: baseline;
+  margin-top: 3px;
+}
+
+.conditional-breakpoint-panel.log-point .prompt {
+  color: var(--purple-60);
 }
 
 .conditional-breakpoint-panel .CodeMirror {

devtools/client/debugger/src/components/Editor/ConditionalPanel.js

@@ -20,6 +20,12 @@ import {
 
 import type { SourceLocation, Context } from "../../types";
 
+function addNewLine(doc: Object) {
+  const cursor = doc.getCursor();
+  const pos = { line: cursor.line, ch: cursor.ch };
+  doc.replaceRange("\n", pos);
+}
+
 type Props = {
   cx: Context,
   breakpoint: ?Object,
@@ -33,7 +39,8 @@ type Props = {
 
 export class ConditionalPanel extends PureComponent<Props> {
   cbPanel: null | Object;
-  input: ?HTMLInputElement;
+  input: ?HTMLTextAreaElement;
+  codeMirror: ?Object;
   panelNode: ?HTMLDivElement;
   scrollParent: ?HTMLElement;
 
@@ -50,15 +57,19 @@ export class ConditionalPanel extends PureComponent<Props> {
 
   saveAndClose = () => {
     if (this.input) {
-      this.setBreakpoint(this.input.value);
+      this.setBreakpoint(this.input.value.trim());
     }
 
     this.props.closeConditionalPanel();
   };
 
-  onKey = (e: SyntheticKeyboardEvent<HTMLInputElement>) => {
+  onKey = (e: SyntheticKeyboardEvent<HTMLTextAreaElement>) => {
     if (e.key === "Enter") {
-      this.saveAndClose();
+      if (this.codeMirror && e.altKey) {
+        addNewLine(this.codeMirror.doc);
+      } else {
+        this.saveAndClose();
+      }
     } else if (e.key === "Escape") {
       this.props.closeConditionalPanel();
     }
@@ -146,10 +157,40 @@ export class ConditionalPanel extends PureComponent<Props> {
     }
   }
 
-  renderConditionalPanel(props: Props) {
-    const { breakpoint, log, editor } = props;
+  createEditor = (input: ?HTMLTextAreaElement) => {
+    const { log, editor } = this.props;
+
+    const codeMirror = editor.CodeMirror.fromTextArea(input, {
+      mode: "javascript",
+      theme: "mozilla",
+      placeholder: L10N.getStr(
+        log
+          ? "editor.conditionalPanel.logPoint.placeholder2"
+          : "editor.conditionalPanel.placeholder2"
+      )
+    });
+    const codeMirrorWrapper = codeMirror.getWrapperElement();
+
+    codeMirrorWrapper.addEventListener("keydown", e => {
+      codeMirror.save();
+      this.onKey(e);
+    });
+
+    this.input = input;
+    this.codeMirror = codeMirror;
+    codeMirror.focus();
+    codeMirror.setCursor(codeMirror.lineCount(), 0);
+  };
+
+  getDefaultValue() {
+    const { breakpoint, log } = this.props;
     const options = (breakpoint && breakpoint.options) || {};
-    const condition = log ? options.logValue : options.condition;
+    return log ? options.logValue : options.condition;
+  }
+
+  renderConditionalPanel(props: Props) {
+    const { log } = props;
+    const defaultValue = this.getDefaultValue();
 
     const panel = document.createElement("div");
     ReactDOM.render(
@@ -158,33 +199,13 @@ export class ConditionalPanel extends PureComponent<Props> {
           "log-point": log
         })}
         onClick={() => this.keepFocusOnInput()}
-        onBlur={this.props.closeConditionalPanel}
+        // onBlur={this.props.closeConditionalPanel}
         ref={node => (this.panelNode = node)}
       >
         <div className="prompt">»</div>
-        <input
-          defaultValue={condition}
-          ref={input => {
-            const codeMirror = editor.CodeMirror.fromTextArea(input, {
-              mode: "javascript",
-              theme: "mozilla",
-              placeholder: L10N.getStr(
-                log
-                  ? "editor.conditionalPanel.logPoint.placeholder"
-                  : "editor.conditionalPanel.placeholder"
-              )
-            });
-            const codeMirrorWrapper = codeMirror.getWrapperElement();
-
-            codeMirrorWrapper.addEventListener("keydown", e => {
-              codeMirror.save();
-              this.onKey(e);
-            });
-
-            this.input = input;
-            codeMirror.focus();
-            codeMirror.setCursor(codeMirror.lineCount(), 0);
-          }}
+        <textarea
+          defaultValue={defaultValue}
+          ref={input => this.createEditor(input)}
         />
       </div>,
       panel

devtools/client/debugger/src/components/Editor/Editor.css

@@ -30,14 +30,6 @@
   overflow-anchor: none;
 }
 
-.theme-dark {
-  --theme-conditional-breakpoint-color: #9fa4a9;
-}
-
-.theme-light {
-  --theme-conditional-breakpoint-color: var(--theme-body-color);
-}
-
 /**
  * There's a known codemirror flex issue with chrome that this addresses.
  * BUG https://github.com/firefox-devtools/debugger/issues/63

devtools/client/debugger/src/components/Editor/index.js

@@ -418,7 +418,7 @@ class Editor extends PureComponent<Props, State> {
       return continueToHere(cx, sourceLine);
     }
 
-    return addBreakpointAtLine(cx, sourceLine);
+    return addBreakpointAtLine(cx, sourceLine, ev.altKey);
   };
 
   onGutterContextMenu = (event: MouseEvent) => {

devtools/client/debugger/test/mochitest/.eslintrc

@@ -35,9 +35,13 @@
     "selectSource": false,
     "prettyPrint": false,
     "closeTab": false,
+    "pushPref": false,
 
     // Globals introduced in debugger-specific head.js
     "getCM": false,
+    "getDebuggerSplitConsole": false,
+    "hasConsoleMessage": false,
+    "findConsoleMessage": false,
     "promise": false,
     "BrowserToolboxProcess": false,
     "OS": false,
@@ -53,6 +57,7 @@
     "waitForSource": false,
     "waitForLoadedSource": false,
     "waitForSelectedSource": false,
+    "waitForBreakpoint": false,
     "waitForBreakpointCount": false,
     "isPaused": false,
     "assertSourceCount": false,
@@ -85,6 +90,7 @@
     "clickElement": false,
     "clickElementWithSelector": false,
     "clickDOMElement": false,
+    "altClickElement": false,
     "rightClickElement": false,
     "clickGutter": false,
     "selectMenuItem": false,

devtools/client/debugger/test/mochitest/browser_dbg-breakpoints-cond.js

@@ -36,10 +36,6 @@ function waitForElementFocus(dbg, el) {
   return waitFor(() => doc.activeElement == el && doc.hasFocus());
 }
 
-function waitForBreakpoint(dbg, url, line) {
-  return waitForState(dbg, () => findBreakpoint(dbg, url, line));
-}
-
 function waitForBreakpointWithCondition(dbg, url, line, cond) {
   return waitForState(dbg, () => {
     const bp = findBreakpoint(dbg, url, line);
@@ -63,11 +59,6 @@ function waitForBreakpointWithoutCondition(dbg, url, line) {
   });
 }
 
-async function assertConditionalBreakpointIsFocused(dbg) {
-  const input = findElement(dbg, "conditionalPanelInput");
-  await waitForElementFocus(dbg, input);
-}
-
 async function setConditionalBreakpoint(dbg, index, condition) {
   const {
     addConditionalBreakpoint,
@@ -79,7 +70,6 @@ async function setConditionalBreakpoint(dbg, index, condition) {
   rightClickElement(dbg, "gutter", index);
   selectContextMenuItem(dbg, selector);
   await waitForElement(dbg, "conditionalPanelInput");
-  await assertConditionalBreakpointIsFocused(dbg);
 
   // Position cursor reliably at the end of the text.
   pressKey(dbg, "End");
@@ -96,7 +86,6 @@ async function setLogPoint(dbg, index, value) {
   rightClickElement(dbg, "gutter", index);
   selectContextMenuItem(dbg, selector);
   await waitForElement(dbg, "conditionalPanelInput");
-  await assertConditionalBreakpointIsFocused(dbg);
 
   // Position cursor reliably at the end of the text.
   pressKey(dbg, "End");
@@ -158,4 +147,8 @@ add_task(async function() {
 
   bp = findBreakpoint(dbg, "simple2", 5);
   is(bp.options.logValue, "44", "breakpoint condition removed");
+
+  await altClickElement(dbg, "gutter", 6);
+  bp = await waitForBreakpoint(dbg, "simple2", 6);
+  is(bp.options.logValue, "displayName", "logPoint has default value");
 });

devtools/client/debugger/test/mochitest/browser_dbg-log-points.js

@@ -10,21 +10,27 @@ add_task(async function() {
   Services.prefs.setBoolPref("devtools.toolbox.splitconsoleEnabled", true);
   const dbg = await initDebugger("doc-script-switching.html", "switching-01");
 
-  const source = findSource(dbg, "switching-01")
+  const source = findSource(dbg, "switching-01");
   await selectSource(dbg, "switching-01");
 
   await getDebuggerSplitConsole(dbg);
 
+  await altClickElement(dbg, "gutter", 7);
+  await waitForBreakpoint(dbg, "switching-01", 7);
+
   await dbg.actions.addBreakpoint(
     getContext(dbg),
-    { line: 5, sourceId: source.id },
+    { line: 8, sourceId: source.id },
     { logValue: "'a', 'b', 'c'" }
   );
+
   invokeInTab("firstCall");
   await waitForPaused(dbg);
 
   await hasConsoleMessage(dbg, "a b c");
+  await hasConsoleMessage(dbg, "firstCall");
+
   const { link, value } = await findConsoleMessage(dbg, "a b c");
-  is(link, "script-switching-01.js:5:2", "logs should have the relevant link");
+  is(link, "script-switching-01.js:8:2", "logs should have the relevant link");
   is(value, "a b c", "logs should have multiple values");
 });

devtools/client/debugger/test/mochitest/examples/script-switching-01.js

@@ -1,6 +1,9 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
+function a() {}
 function firstCall() {
+  a();
   secondCall();
 }

devtools/client/debugger/test/mochitest/helpers.js

@@ -209,14 +209,11 @@ async function waitForElementWithSelector(dbg, selector) {
   return findElementWithSelector(dbg, selector);
 }
 
-function waitForSelectedLocation(dbg, line ) {
-  return waitForState(
-    dbg,
-    state => {
-      const location = dbg.selectors.getSelectedLocation(state)
-      return location && location.line == line
-    }
-  );
+function waitForSelectedLocation(dbg, line) {
+  return waitForState(dbg, state => {
+    const location = dbg.selectors.getSelectedLocation(state);
+    return location && location.line == line;
+  });
 }
 
 function waitForSelectedSource(dbg, url) {
@@ -465,6 +462,10 @@ function waitForBreakpointCount(dbg, count) {
   );
 }
 
+function waitForBreakpoint(dbg, url, line) {
+  return waitForState(dbg, () => findBreakpoint(dbg, url, line));
+}
+
 /**
  * Waits for the debugger to be fully paused.
  *
@@ -796,7 +797,11 @@ async function addBreakpoint(dbg, source, line, column, options) {
   source = findSource(dbg, source);
   const sourceId = source.id;
   const bpCount = dbg.selectors.getBreakpointCount(dbg.getState());
-  await dbg.actions.addBreakpoint(getContext(dbg), { sourceId, line, column }, options);
+  await dbg.actions.addBreakpoint(
+    getContext(dbg),
+    { sourceId, line, column },
+    options
+  );
   is(
     dbg.selectors.getBreakpointCount(dbg.getState()),
     bpCount + 1,
@@ -815,8 +820,12 @@ function disableBreakpoint(dbg, source, line, column) {
 function setBreakpointOptions(dbg, source, line, column, options) {
   source = findSource(dbg, source);
   const sourceId = source.id;
-  column = column || getFirstBreakpointColumn(dbg, {line, sourceId});
-  return dbg.actions.setBreakpointOptions(getContext(dbg), { sourceId, line, column }, options);
+  column = column || getFirstBreakpointColumn(dbg, { line, sourceId });
+  return dbg.actions.setBreakpointOptions(
+    getContext(dbg),
+    { sourceId, line, column },
+    options
+  );
 }
 
 function findBreakpoint(dbg, url, line) {
@@ -1319,6 +1328,14 @@ function dblClickElement(dbg, elementName, ...args) {
   );
 }
 
+function altClickElement(dbg, elementName, ...args) {
+  const selector = getSelector(elementName, ...args);
+  const el = findElementWithSelector(dbg, selector);
+  el.scrollIntoView();
+
+  return EventUtils.synthesizeMouseAtCenter(el, { altKey: true }, dbg.win);
+}
+
 function rightClickElement(dbg, elementName, ...args) {
   const selector = getSelector(elementName, ...args);
   const doc = dbg.win.document;

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

@@ -555,13 +555,13 @@ editor.editLogPoint.accesskey=E
 editor.removeLogPoint.label=Remove log
 editor.removeLogPoint.accesskey=V
 
-# LOCALIZATION NOTE (editor.conditionalPanel.placeholder): Placeholder text for
+# LOCALIZATION NOTE (editor.conditionalPanel.placeholder2): Placeholder text for
 # input element inside ConditionalPanel component
-editor.conditionalPanel.placeholder=This breakpoint will pause when the expression is true
+editor.conditionalPanel.placeholder2=Breakpoint condition, e.g. items.length > 0
 
-# LOCALIZATION NOTE (editor.conditionalPanel.logPoint.placeholder): Placeholder text for
+# LOCALIZATION NOTE (editor.conditionalPanel.logPoint.placeholder2): Placeholder text for
 # input element inside ConditionalPanel component when a log point is set
-editor.conditionalPanel.logPoint.placeholder=This breakpoint will log the result of the expression
+editor.conditionalPanel.logPoint.placeholder2=Log message, e.g. displayName
 
 # LOCALIZATION NOTE (editor.conditionalPanel.close): Tooltip text for
 # close button inside ConditionalPanel component

devtools/server/actors/breakpoint.js

@@ -8,6 +8,8 @@
 
 "use strict";
 
+const { formatDisplayName } = require("devtools/server/actors/frame");
+
 /**
  * Set breakpoints on all the given entry points with the given
  * BreakpointActor as the handler.
@@ -95,13 +97,16 @@ BreakpointActor.prototype = {
       for (const offset of offsets) {
         const { lineNumber, columnNumber } = script.getOffsetLocation(offset);
         script.replayVirtualConsoleLog(
-          offset, options.logValue, options.condition, (executionPoint, rv) => {
+          offset,
+          options.logValue,
+          options.condition,
+          (executionPoint, rv) => {
             const message = {
               filename: script.url,
               lineNumber,
               columnNumber,
               executionPoint,
-              "arguments": ["return" in rv ? rv.return : rv.throw],
+              arguments: ["return" in rv ? rv.return : rv.throw],
               logpointId: options.logGroupId,
             };
             this.threadActor._parent._consoleActor.onConsoleAPICall(message);
@@ -174,22 +179,26 @@ BreakpointActor.prototype = {
     } = this.threadActor.sources.getFrameLocation(frame);
     const url = generatedSourceActor.url;
 
-    if (this.threadActor.sources.isBlackBoxed(url, generatedLine, generatedColumn)
-        || this.threadActor.skipBreakpoints
-        || frame.onStep) {
+    if (
+      this.threadActor.sources.isBlackBoxed(url, generatedLine, generatedColumn) ||
+      this.threadActor.skipBreakpoints ||
+      frame.onStep
+    ) {
       return undefined;
     }
 
     // If we're trying to pop this frame, and we see a breakpoint at
     // the spot at which popping started, ignore it.  See bug 970469.
     const locationAtFinish = frame.onPop && frame.onPop.generatedLocation;
-    if (locationAtFinish &&
-        locationAtFinish.generatedLine === generatedLine &&
-        locationAtFinish.generatedColumn === generatedColumn) {
+    if (
+      locationAtFinish &&
+      locationAtFinish.generatedLine === generatedLine &&
+      locationAtFinish.generatedColumn === generatedColumn
+    ) {
       return undefined;
     }
 
-    const reason = { type: "breakpoint", actors: [ this.actorID ] };
+    const reason = { type: "breakpoint", actors: [this.actorID] };
     const { condition, logValue } = this.options || {};
 
     // When replaying, breakpoints with log values are handled via
@@ -212,7 +221,8 @@ BreakpointActor.prototype = {
     }
 
     if (logValue) {
-      const completion = frame.eval(`[${logValue}]`);
+      const displayName = formatDisplayName(frame);
+      const completion = frame.evalWithBindings(`[${logValue}]`, { displayName });
       let value;
       if (!completion) {
         // The evaluation was killed (possibly by the slow script dialog).
@@ -231,7 +241,8 @@ BreakpointActor.prototype = {
         filename: url,
         lineNumber: generatedLine,
         columnNumber: generatedColumn,
-        "arguments": value,
+        level: "logPoint",
+        arguments: value,
       };
       this.threadActor._parent._consoleActor.onConsoleAPICall(message);
 

devtools/server/actors/frame.js

@@ -83,21 +83,20 @@ const FrameActor = ActorClassWithSpec(frameSpec, {
    */
   form: function() {
     const threadActor = this.threadActor;
-    const form = { actor: this.actorID,
-                   type: this.frame.type };
+    const form = { actor: this.actorID, type: this.frame.type };
 
     // NOTE: ignoreFrameEnvironment lets the client explicitly avoid
     // populating form environments on pause.
-    if (
-      !this.threadActor._options.ignoreFrameEnvironment &&
-      this.frame.environment
-    ) {
+    if (!this.threadActor._options.ignoreFrameEnvironment && this.frame.environment) {
       form.environment = this.getEnvironment();
     }
 
     if (this.frame.type != "wasmcall") {
-      form.this = createValueGrip(this.frame.this, threadActor._pausePool,
-        threadActor.objectGrip);
+      form.this = createValueGrip(
+        this.frame.this,
+        threadActor._pausePool,
+        threadActor.objectGrip
+      );
     }
 
     form.displayName = formatDisplayName(this.frame);
@@ -123,9 +122,11 @@ const FrameActor = ActorClassWithSpec(frameSpec, {
       return [];
     }
 
-    return this.frame.arguments.map(arg => createValueGrip(arg,
-      this.threadActor._pausePool, this.threadActor.objectGrip));
+    return this.frame.arguments.map(arg =>
+      createValueGrip(arg, this.threadActor._pausePool, this.threadActor.objectGrip)
+    );
   },
 });
 
 exports.FrameActor = FrameActor;
+exports.formatDisplayName = formatDisplayName;