Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Commit

Permalink
make breakpoints more robust and improve protocol interface
Browse files Browse the repository at this point in the history
  • Loading branch information
@jlongster
jlongster committed May 31, 2016
1 parent 2d7e425 commit ef8d9f0
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 98 deletions.
89 changes: 30 additions & 59 deletions public/js/actions/breakpoints.js
View file
Expand Up @@ -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.
Expand All @@ -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();
Expand All @@ -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) : ""
};
})
});
Expand All @@ -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(),
Expand All @@ -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" }));
Expand All @@ -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 = {
Expand Down
28 changes: 16 additions & 12 deletions public/js/clients/chrome.js
View file
Expand Up @@ -54,19 +54,23 @@ let APIClient = {
scriptId: location.sourceId,
lineNumber: location.line - 1,
columnNumber: location.column
}, (_, breakpointId, actualLocation) => ([
{},
{
}, (_, breakpointId, actualLocation) => {
return {
id: breakpointId,
remove: () => {
// TODO: resolve promise when request is completed.
return new Promise((resolve, reject) => {
resolve(debuggerAgent.removeBreakpoint(breakpointId));
});
},
setCondition: () => {},
}
]));
actualLocation: {
sourceId: actualLocation.scriptId,
line: actualLocation.lineNumber + 1,
column: actualLocation.columnNumber
}
};
});
},

removeBreakpoint(breakpointId) {
// TODO: resolve promise when request is completed.
return new Promise((resolve, reject) => {
resolve(debuggerAgent.removeBreakpoint(breakpointId));
});
}
};

Expand Down
24 changes: 24 additions & 0 deletions public/js/clients/firefox.js
View file
Expand Up @@ -13,6 +13,8 @@ let currentTabTarget = null;
// API implementation

let APIClient = {
_bpClients: {},

resume() {
return new Promise(resolve => {
currentThreadClient.resume(resolve);
Expand Down Expand Up @@ -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 {
id: bpClient.actor,
actualLocation: actualLocation
};
});
},

removeBreakpoint(breakpointId) {
const bpClient = this._bpClients[breakpointId];
this._bpClients[breakpointId] = null;
return bpClient.remove();
}
};

Expand Down
2 changes: 1 addition & 1 deletion public/js/components/Breakpoints.js
View file
Expand Up @@ -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");
Expand Down
18 changes: 12 additions & 6 deletions public/js/components/Editor.js
View file
Expand Up @@ -58,12 +58,18 @@ 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 },
{ getTextForLine: l => cm.getLine(l - 1).trim() }
);
}
},

clearDebugLine(line) {
Expand Down
35 changes: 15 additions & 20 deletions public/js/reducers/breakpoints.js
View file
Expand Up @@ -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: {
Expand All @@ -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.
Expand All @@ -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)) {
state = state.deleteIn(["breakpoints", id]);

const movedId = makeLocationId(actualLocation);
const currentBp = (state.getIn(["breakpoints", movedId]) ||
fromJS(action.breakpoint));
const newBp = currentBp.merge({ location: 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]);
Expand Down Expand Up @@ -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]);
Expand Down

0 comments on commit ef8d9f0

Please sign in to comment.