this is a following up bug of bug983469. We should add some UI changes to notify the user of conditional breakpoints throwing errors.

Comment on attachment 8573692 [details] [diff] [review] add UI for breakpoint condition throwing Here's a WIP patch. As there's no response from bug762979, I couldn't add this UI change to the new breakpoint mockup. In this patch, I just display the message thrown to the breakpoint item view. Anyway, it's a start. When we are done with the UI changes and behaviours, I will write some tests.

Comment on attachment 8573692 [details] [diff] [review] add UI for breakpoint condition throwing Review of attachment 8573692 [details] [diff] [review]: ----------------------------------------------------------------- Good start! Needs test(s), of course. ::: browser/themes/shared/devtools/debugger.inc.css @@ +46,5 @@ > } > > +.dbg-breakpoint-condition-thrown-message { > + display: none; > + color: #CC1B1B; I think we should be using css variables from our theme. ni? bgrins for more info and references. ::: toolkit/devtools/server/actors/script.js @@ +4722,5 @@ > * > * @param aFrame Debugger.Frame > * The frame to evaluate the condition in > + * @returns Object > + * In form of { result: boolean|undefined, message} Would be good to document the difference between `result == false` and `result == undefined`, because that isn't clear to me from reading these docs. @@ +4792,5 @@ > + reason.message = message; > + } > + reason.actors = [ this.actorID ]; > + } > + else return undefined; Style nit: devtools js always uses braces and new lines for if/else clauses.

:bgrins, can you give us some info about the devtools theme css variables and what we should be using here?

(In reply to Nick Fitzgerald [:fitzgen] from comment #3) > > +.dbg-breakpoint-condition-thrown-message { > > + display: none; > > + color: #CC1B1B; > > I think we should be using css variables from our theme. ni? bgrins for more > info and references. Yep, you can convert this to a CSS variable. Here are the relevant text color variables for light-theme [0] and dark-theme [1]. Here's an example similar to what you are doing [2]. You can do something like this (note that I haven't checked to make sure that this color works in your case, please check in both light and dark themes to make sure it looks good and try the other options if not): color: var(--theme-body-color-alt) [0]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/light-theme.css#22 [1]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/dark-theme.css#22. [2]: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/toolbars.inc.css#61

Sorry for the delay on uploading this patch.... too busy during the last several weeks...

Comment on attachment 8579157 [details] [diff] [review] bug1135435 Review of attachment 8579157 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! A few comments below. Can you also send a PR to update the remote debugging protocol documentation to reflect this change? https://github.com/jimblandy/DebuggerDocs Thanks! ::: browser/devtools/debugger/debugger-panes.js @@ +455,5 @@ > + */ > + showBreakpointConditionThrownMessage: function(aLocation, aMessage = "") { > + let breakpointItem = this.getBreakpoint(aLocation); > + if (!breakpointItem) { > + return promise.reject(new Error("No breakpoint found.")); Since we don't otherwise return any value, I don't think that returning a rejected promise is the right thing to do here. If this code should always be able to access the breakpoint, then we should do this: DevToolsUtils.dbg_assert(!!breakpointItem, "sowBreakpointConditionThrownMessage: Should always get a breakpoint"); However, that is not quite our situation: it is possible that the user deleted the BP on the frontend at the same time as the server hit the pause, and therefore before the "paused" message reached the frontend and before the "remove breakpoint" message reached the server and prevented the BP from getting hit. I think the best thing we can do here (unfortunately) is this: if (!breakpointItem) { return; } @@ +706,5 @@ > * An object containing the breakpoint container, checkbox, > * line number and line text nodes. > */ > _createBreakpointView: function(aOptions) { > + let { location, disabled, text, message } = aOptions; Please document the "message" option in the doc comment above, like the other options are. ::: browser/devtools/debugger/test/browser.ini @@ +139,5 @@ > [browser_dbg_breakpoints-break-on-last-line-of-script-on-reload.js] > skip-if = e10s # Bug 1093535 > [browser_dbg_breakpoints-button-01.js] > [browser_dbg_breakpoints-button-02.js] > +skip-if = e10s && debug Is this supposed to be for the new test? If so, it should be below the new test, not above it. Additionally, I think you forgot to include the new test in this patch. ::: toolkit/devtools/server/actors/script.js @@ +4724,5 @@ > * @param aFrame Debugger.Frame > * The frame to evaluate the condition in > + * @returns Object > + * - result: boolean|undefined > + * Indicating whether condition evaluates to true or not. Nit: "True when the conditional breakpoint should trigger a pause, false otherwise." Callers of this function just want to know whether to pause or not -- we can paper over the cause of the pause a bit and simultaneously simplify the code a little. @@ +4797,5 @@ > + reason.message = message; > + } > + reason.actors = [ this.actorID ]; > + } > + else { Style nit: Brace and if on same line.

Comment on attachment 8579742 [details] [diff] [review] add UI for breakpoint condition throwing Updated. I will send a PR to the docs once the patch is good enough XD

Comment on attachment 8579742 [details] [diff] [review] add UI for breakpoint condition throwing Review of attachment 8579742 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks! Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33cc8d5b6485

Also, I suggest you fill out the forms needed to become a level 1 commiter, that way you can push to try yourself in the future: https://www.mozilla.org/en-US/about/governance/policies/commit/

Comment on attachment 8580526 [details] [diff] [review] add UI for breakpoint condition throws modified a unit test on breakpoint condition. try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71b0e3e03168

Comment on attachment 8580526 [details] [diff] [review] add UI for breakpoint condition throws Review of attachment 8580526 [details] [diff] [review]: ----------------------------------------------------------------- Great!