make breakpoints more robust and improve protocol interface #222

Merged
merged 1 commit into from Jun 1, 2016
Jump to file or symbol
Failed to load files and symbols.
+138 −109
Diff settings

Always

Just for now

@@ -12,19 +12,6 @@ const {
const { Task } = require("ff-devtools-libs/sham/task");
const fromJS = require("../util/fromJS");
-// Because breakpoints are just simple data structures, we still need
-// a way to lookup the actual client instance to talk to the server.
-// We keep an internal database of clients based off of actor ID.
-const BREAKPOINT_CLIENT_STORE = new Map();
-
-function setBreakpointClient(actor, client) {
- BREAKPOINT_CLIENT_STORE.set(actor, client);
-}
-
-function getBreakpointClient(actor) {
- return BREAKPOINT_CLIENT_STORE.get(actor);
-}
-
function enableBreakpoint(location) {
// Enabling is exactly the same as adding. It will use the existing
// breakpoint that still stored.
@@ -40,7 +27,7 @@ function _getOrCreateBreakpoint(state, location, condition) {
return getBreakpoint(state, location) || fromJS({ location, condition });
}
-function addBreakpoint(location, condition, snippet) {
+function addBreakpoint(location, { condition, getTextForLine }) {
return ({ dispatch, getState, client }) => {
if (_breakpointExists(getState(), location)) {
return promise.resolve();
@@ -53,24 +40,15 @@ function addBreakpoint(location, condition, snippet) {
breakpoint: bp.toJS(),
condition: condition,
[PROMISE]: Task.spawn(function* () {
- const [response, bpClient] = yield client.setBreakpoint(
+ const { id, actualLocation } = yield client.setBreakpoint(
bp.get("location").toJS(),
bp.get("condition")
);
- const { isPending, actualLocation } = response;
-
- // Save the client instance
- setBreakpointClient(bpClient.id, bpClient);
-
return {
- text: snippet,
-
- // If the breakpoint response has an "actualLocation" attached, then
- // the original requested placement for the breakpoint wasn't
- // accepted.
- actualLocation: isPending ? null : actualLocation,
- id: bpClient.id
+ id: id,
+ actualLocation: actualLocation,
+ text: getTextForLine ? getTextForLine(actualLocation.line) : ""
};
})
});
@@ -86,18 +64,17 @@ function removeBreakpoint(location) {
}
function _removeOrDisableBreakpoint(location, isDisabled) {
- return ({ dispatch, getState }) => {
+ return ({ dispatch, getState, client }) => {
let bp = getBreakpoint(getState(), location);
if (!bp) {
throw new Error("attempt to remove breakpoint that does not exist");
}
- if (bp.loading) {
+ if (bp.get("loading")) {
// TODO(jwl): make this wait until the breakpoint is saved if it
// is still loading
throw new Error("attempt to remove unsaved breakpoint");
}
- const bpClient = getBreakpointClient(bp.get("clientId"));
const action = {
type: constants.REMOVE_BREAKPOINT,
breakpoint: bp.toJS(),
@@ -110,7 +87,7 @@ function _removeOrDisableBreakpoint(location, isDisabled) {
// will be removed completely from the state.
if (!bp.disabled) {
return dispatch(Object.assign({}, action, {
- [PROMISE]: bpClient.remove()
+ [PROMISE]: client.removeBreakpoint(bp.get("id"))
}));
}
return dispatch(Object.assign({}, action, { status: "done" }));
@@ -136,34 +113,28 @@ function removeAllBreakpoints() {
* A promise that will be resolved with the breakpoint client
*/
function setBreakpointCondition(location, condition) {
- return ({ dispatch, getState, client }) => {
- const bp = getBreakpoint(getState(), location);
- if (!bp) {
- throw new Error("Breakpoint does not exist at the specified location");
- }
- if (bp.loading) {
- // TODO(jwl): when this function is called, make sure the action
- // creator waits for the breakpoint to exist
- throw new Error("breakpoint must be saved");
- }
-
- const bpClient = getBreakpointClient(bp.actor);
-
- return dispatch({
- type: constants.SET_BREAKPOINT_CONDITION,
- breakpoint: bp,
- condition: condition,
- [PROMISE]: Task.spawn(function* () {
- const newClient = yield bpClient.setCondition(client, condition);
-
- // Remove the old instance and save the new one
- setBreakpointClient(bpClient.id, null);
- setBreakpointClient(newClient.id, newClient);
-
- return { actor: newClient.actor };
- })
- });
- };
+ throw new Error("not implemented");
+
+ // return ({ dispatch, getState, client }) => {
+ // const bp = getBreakpoint(getState(), location);
+ // if (!bp) {
+ // throw new Error("Breakpoint does not exist at the specified location");
+ // }
+ // if (bp.get("loading")) {
+ // // TODO(jwl): when this function is called, make sure the action
+ // // creator waits for the breakpoint to exist
+ // throw new Error("breakpoint must be saved");
+ // }
+
+ // return dispatch({
+ // type: constants.SET_BREAKPOINT_CONDITION,
+ // breakpoint: bp,
+ // condition: condition,
+ // [PROMISE]: Task.spawn(function* () {
+ // yield client.setBreakpointCondition(bp.get("id"), condition);
+ // })
+ // });
+ // };
}
module.exports = {
@@ -5,7 +5,7 @@ const {
InspectorBackend
} = require("./chrome/api");
-const { Tab, Source, Location, Frame } = require("../types");
+const { Tab, Source, Location, BreakpointResult, Frame } = require("../types");
const { isEnabled } = require("../configs/feature");
const defer = require("../util/defer");
@@ -50,23 +50,36 @@ let APIClient = {
},
setBreakpoint(location, condition) {
- return debuggerAgent.setBreakpoint({
- scriptId: location.sourceId,
- lineNumber: location.line - 1,
- columnNumber: location.column
- }, (_, breakpointId, actualLocation) => ([
- {},
- {
- id: breakpointId,
- remove: () => {
- // TODO: resolve promise when request is completed.
- return new Promise((resolve, reject) => {
- resolve(debuggerAgent.removeBreakpoint(breakpointId));
- });
- },
- setCondition: () => {},
- }
- ]));
+ return new Promise((resolve, reject) => {
+ return debuggerAgent.setBreakpoint({
+ scriptId: location.sourceId,
+ lineNumber: location.line - 1,
+ columnNumber: location.column
+ }, (err, breakpointId, actualLocation) => {
+ if(err) {
+ reject(err);
+ return;
+ }
+
+ actualLocation = actualLocation ? {
+ sourceId: actualLocation.scriptId,
+ line: actualLocation.lineNumber + 1,
+ column: actualLocation.columnNumber
+ } : location;
+
+ resolve(BreakpointResult({
+ id: breakpointId,
+ actualLocation: Location(actualLocation)
+ }));
+ });
+ });
+ },
+
+ removeBreakpoint(breakpointId) {
+ // TODO: resolve promise when request is completed.
+ return new Promise((resolve, reject) => {
+ resolve(debuggerAgent.removeBreakpoint(breakpointId));
+ });
}
};
@@ -129,7 +142,7 @@ function makeDispatcher(actions) {
endLine, endColumn, executionContextId, hash,
isContentScript, isInternalScript, isLiveEdit,
sourceMapURL, hasSourceURL, deprecatedCommentWasUsed) {
- actions.newSource(Source({ id: scriptId, url }));
+ actions.newSource(Source({ id: scriptId, url: url }));
},
scriptFailedToParse() {
@@ -3,7 +3,7 @@
const { DebuggerClient } = require("ff-devtools-libs/shared/client/main");
const { DebuggerTransport } = require("ff-devtools-libs/transport/transport");
const { TargetFactory } = require("ff-devtools-libs/client/framework/target");
-const { Tab, Source, Location, Frame } = require("../types");
+const { Tab, Source, Location, BreakpointResult, Frame } = require("../types");
const defer = require("../util/defer");
let currentClient = null;
@@ -13,6 +13,8 @@ let currentTabTarget = null;
// API implementation
let APIClient = {
+ _bpClients: {},
+
resume() {
return new Promise(resolve => {
currentThreadClient.resume(resolve);
@@ -54,7 +56,29 @@ let APIClient = {
line: location.line,
column: location.column,
condition: condition
+ }).then(([res, bpClient]) => {
+ this._bpClients[bpClient.actor] = bpClient;
+
+ // Firefox only returns `actualLocation` if it actually changed,
+ // but we want it always to exist. Format `actualLocation` if it
+ // exists, otherwise use `location`.
+ const actualLocation = res.actualLocation ? {
+ sourceId: res.actualLocation.source.actor,
+ line: res.actualLocation.line,
+ column: res.actualLocation.column
+ } : location;
+
+ return BreakpointResult({
+ id: bpClient.actor,
+ actualLocation: Location(actualLocation)
+ });
});
+ },
+
+ removeBreakpoint(breakpointId) {
+ const bpClient = this._bpClients[breakpointId];
+ this._bpClients[breakpointId] = null;
+ return bpClient.remove();
}
};
@@ -53,7 +53,7 @@ const Breakpoints = React.createClass({
},
renderBreakpoint(breakpoint) {
- const snippet = breakpoint.getIn(["location", "snippet"]);
+ const snippet = breakpoint.get("text");
const locationId = breakpoint.get("locationId");
const line = breakpoint.getIn(["location", "line"]);
const isCurrentlyPaused = breakpoint.get("isCurrentlyPaused");
@@ -28,13 +28,15 @@ function isSourceForFrame(source, frame) {
const Editor = React.createClass({
propTypes: {
breakpoints: ImPropTypes.map.isRequired,
- selectedSource: ImPropTypes.map.isRequired,
+ selectedSource: ImPropTypes.map,
sourceText: PropTypes.object,
addBreakpoint: PropTypes.func,
removeBreakpoint: PropTypes.func,
selectedFrame: PropTypes.object
},
+ displayName: "Editor",
+
componentDidMount() {
this.editor = CodeMirror.fromTextArea(this.refs.editor, {
mode: "javascript",
@@ -58,12 +60,21 @@ const Editor = React.createClass({
return b.getIn(["location", "line"]) === line + 1;
});
- const applyBp = bp ? this.props.removeBreakpoint : this.props.addBreakpoint;
- applyBp({
- sourceId: this.props.selectedSource.get("id"),
- line: line + 1,
- snippet: cm.lineInfo(line).text.trim()
- });
+ if (bp) {
+ this.props.removeBreakpoint({
+ sourceId: this.props.selectedSource.get("id"),
+ line: line + 1
+ });
+ } else {
+ this.props.addBreakpoint(
+ { sourceId: this.props.selectedSource.get("id"),
+ line: line + 1 },
+ // Pass in a function to get line text because the breakpoint
+ // may slide and it needs to compute the value at the new
+ // line.
+ { getTextForLine: l => cm.getLine(l - 1).trim() }
+ );
+ }
},
clearDebugLine(line) {
@@ -22,6 +22,12 @@ function firstString(...args) {
return null;
}
+function locationMoved(location, newLocation) {
+ return location.line !== newLocation.line ||
+ (location.column != null &&
+ location.column !== newLocation.column);
+}
+
function update(state = initialState, action) {
switch (action.type) {
case constants.ADD_BREAKPOINT: {
@@ -34,6 +40,7 @@ function update(state = initialState, action) {
state = state.setIn(["breakpoints", id], bp.merge({
disabled: false,
loading: true,
+ text: "",
// We want to do an OR here, but we can't because we need
// empty strings to be truthy, i.e. an empty string is a valid
// condition.
@@ -42,38 +49,29 @@ function update(state = initialState, action) {
return state;
} else if (action.status === "done") {
- const { id: clientId, text } = action.value;
+ const { id: breakpointId, text } = action.value;
+ let location = action.breakpoint.location;
let { actualLocation } = action.value;
// If the breakpoint moved, update the map
- if (actualLocation) {
- // XXX Bug 1227417: The `setBreakpoint` RDP request rdp
- // request returns an `actualLocation` field that doesn't
- // conform to the regular { actor, line } location shape, but
- // it has a `source` field. We should fix that.
- actualLocation = { actor: actualLocation.source.actor,
- line: actualLocation.line };
-
+ if (locationMoved(location, actualLocation)) {

This comment has been minimized.

@jasonLaster

jasonLaster Jun 1, 2016

Contributor

nice

@jasonLaster

jasonLaster Jun 1, 2016

Contributor

nice

state = state.deleteIn(["breakpoints", id]);
const movedId = makeLocationId(actualLocation);
const currentBp = (state.getIn(["breakpoints", movedId]) ||
fromJS(action.breakpoint));
- const newBp = currentBp.merge({ location: actualLocation });
+ const newBp = currentBp.merge({ location: fromJS(actualLocation) });
state = state.setIn(["breakpoints", movedId], newBp);
+ location = actualLocation;
}
- const finalLocation = (
- actualLocation ? actualLocation : action.breakpoint.location
- );
- const finalLocationId = makeLocationId(finalLocation);
- state = state.mergeIn(["breakpoints", finalLocationId], {
+ const locationId = makeLocationId(location);
+ return state.mergeIn(["breakpoints", locationId], {
+ id: breakpointId,
disabled: false,
loading: false,
- clientId: clientId,
text: text
});
- return state;
} else if (action.status === "error") {
// Remove the optimistic update
return state.deleteIn(["breakpoints", id]);
@@ -107,10 +105,7 @@ function update(state = initialState, action) {
});
} else if (action.status === "done") {
return state.mergeIn(["breakpoints", id], {
- loading: false,
- // Setting a condition creates a new breakpoint client as of
- // now, so we need to update the actor
- actor: action.value.actor
+ loading: false
});
} else if (action.status === "error") {
return state.deleteIn(["breakpoints", id]);
Oops, something went wrong.