Victor Porof [:vporof][:vp] Reporter
	Description • 12 years ago

It's currently handled in the frontend. The concerns are detailed at https://bugzilla.mozilla.org/show_bug.cgi?id=740825#c12

	Victor Porof [:vporof][:vp] Reporter
	Comment 1 • 12 years ago

I won't be able to look at this very soon.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 3 • 11 years ago

Note that our users are noticing this, like bug 958162.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 4 • 11 years ago

This could be a good bug for someone with a debugger bug or two under their belt or just someone who is a little adventurous.

We would need to add a new optional "condition" property on the "setBreakpoint" RDP request[0] that is evaluated on the BreakpointActor's |hit| method[1] and only pauses if the condition is truthy.

We would also need to update the |ThreadClient|'s |setBreakpoint| method[2] to support setting the condition property on the "setBreakpoint" request packet.

We'd need new xpcshell test(s) for the new protocol changes[3][4].

Finally, the frontend logic for conditional breakpoints[5] could be ripped out and replaced with normal |ThreadClient.prototype.setBreakpoint| calls with the new conditional parameter stuff you just implemented.

[0] https://wiki.mozilla.org/Remote_Debugging_Protocol#Breakpoints
[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#4120
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/dbg-client.jsm#1328
[3] https://wiki.mozilla.org/DevTools/Hacking#xpcshell_Tests
[4] https://developer.mozilla.org/en-US/docs/Writing_xpcshell-based_unit_tests
[5] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-controller.js#659

	Vignesh Shanmugam
	Comment 5 • 11 years ago

Hi Nick,

I am pretty good at HTML,CSS and OO Javascript. This is my first bug I am going to work on. Can I take this up?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 6 • 11 years ago

Sure thing, see comment 4 for more info.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 7 • 11 years ago

Vignesh, are you still working on this bug? Do you have any questions or need any help? If you aren't actively working on it anymore, it is polite to un-assign yourself from the bug so that someone else can take a crack at it.

	Jim Blandy :jimb
	Comment 8 • 11 years ago

One thing the excellent comment 4 doesn't mention: evaluation of the breakpoint condition expression is really running arbitrary client code, so the server needs to be prepared for:

- thrown exceptions
- termination (OOM, slow script)
- hitting some other breakpoint or a debugger statement

Ideally, all of these would cause execution to pause, with a note explaining what's going on, a clear mark on the stack for the debugger's eval, and a button to disable the breakpoint, or remove the condition.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 11 • 11 years ago

(In reply to Jim Blandy :jimb from comment #8)
> One thing the excellent comment 4 doesn't mention: evaluation of the
> breakpoint condition expression is really running arbitrary client code, so
> the server needs to be prepared for:
> 
> - thrown exceptions

Right now we are just copying the existing behavior, which is to continue execution and not pause. Filed bug 983469.

> - termination (OOM, slow script)
> - hitting some other breakpoint or a debugger statement

We already have bug 815537 for these cases; agree that it is sad but we shouldn't fix that whole bug here.

	James Long (:jlongster) Assignee
	Comment 12 • 11 years ago

This patch adds the conditional breakpoint functionality only to the server. The client does not take advantage of it yet, but that will come in the next patch.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 13 • 11 years ago

Comment on attachment 8391452 [details] [diff] [review]
812172-conditional-bp-server.patch

Review of attachment 8391452 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few nits, this is exactly what we needed!

::: toolkit/devtools/client/dbg-client.jsm
@@ +1464,4 @@
>        this.client.request(packet, function (aResponse) {
>          // Ignoring errors, since the user may be setting a breakpoint in a
>          // dead script that will reappear on a page reload.
> +        

Some trailing whitespace crept into the file here.

::: toolkit/devtools/server/actors/script.js
@@ +4258,5 @@
> +
> +    var res = aFrame.eval(this.condition);
> +    return res.return;
> +  },
> +  

More trailing whitespace.

::: toolkit/devtools/server/tests/unit/test_conditional_breakpoint-02.js
@@ +49,5 @@
> +        }
> +        catch(e) {
> +          dump('jwl: ' + DevToolsUtils.SafeErrorString(e));
> +        }
> +        

More trailing whitespace plus the try/catch is not really useful here.

@@ +52,5 @@
> +        }
> +        
> +        // Continue until the breakpoint is hit.
> +        gThreadClient.resume();
> +      });      

Moar trailing whitespace!

::: toolkit/devtools/server/tests/unit/test_conditional_breakpoint-03.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Check conditional breakpoint when condition evaluates to true.

The comment is not accurate, this test is about a condition that throws.

	James Long (:jlongster) Assignee
	Comment 14 • 11 years ago

Thanks. I've been meaning to add the "remove trailing whitespace" hook on file save. I'll fix.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 15 • 11 years ago

(In reply to James Long (:jlongster) from comment #14)
> Thanks. I've been meaning to add the "remove trailing whitespace" hook on
> file save. I'll fix.

I recommend (setq-default show-trailing-whitespace t) instead so that you don't add tons of unrelated changes from files that might accidentally have trailing whitespace (devtools is pretty good, but you never know what random old file you might be editing).

	James Long (:jlongster) Assignee
	Comment 16 • 11 years ago

(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> I recommend (setq-default show-trailing-whitespace t) instead so that you
> don't add tons of unrelated changes from files that might accidentally have
> trailing whitespace (devtools is pretty good, but you never know what random
> old file you might be editing).

Two things: I did not know about `setq-default`. I also did not know about `show-trailing-whitespace`. How good it is to be around Emacs guys again.

I like that better. I think I tried the on-save thing a while back but turned it off for that very reason.

	James Long (:jlongster) Assignee
	Comment 17 • 11 years ago

whitespace fixed, try/catch removed, and test-cond-bp-03 description updated

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 18 • 11 years ago

Comment on attachment 8392462 [details] [diff] [review]
812172-conditional-bp-server.patch

Review of attachment 8392462 [details] [diff] [review]:
-----------------------------------------------------------------

Not a big deal, but for your own convenience in the future note that you don't need to ask for review again when you get an r+, if you address the review comments. In this case though it was a good call, because I caught some trailing whitespace survivors!

::: toolkit/devtools/client/dbg-client.jsm
@@ +1464,4 @@
>        this.client.request(packet, function (aResponse) {
>          // Ignoring errors, since the user may be setting a breakpoint in a
>          // dead script that will reappear on a page reload.
> +        

This one seems to have survived.

::: toolkit/devtools/server/actors/script.js
@@ +4258,5 @@
> +
> +    var res = aFrame.eval(this.condition);
> +    return res.return;
> +  },
> +  

This one, too.

	James Long (:jlongster) Assignee
	Comment 19 • 11 years ago

Cool, thanks! Now that I turned trailing whitespace on visually it shouldn't happen in future patches.

	James Long (:jlongster) Assignee
	Comment 20 • 11 years ago

Ugh, having some problems building Firefox. I'll get this into the try server later tonight and get it committed tomorrow/wednesday.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 21 • 11 years ago

Just remembered this, and it hasn't been brought up yet: When we fix up the frontend, we will have to deal with backwards compatibility with old servers which don't support the conditional breakpoints :(

	James Long (:jlongster) Assignee
	Comment 22 • 11 years ago

Should we create a new bug for doing the client-side work or just do it here? I'm starting to look at it.

(In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> Just remembered this, and it hasn't been brought up yet: When we fix up the
> frontend, we will have to deal with backwards compatibility with old servers
> which don't support the conditional breakpoints :(

Any advice for detecting this on the client would be good. How are changes to the client usually handled in a backwards-compatible way? Is there any sort of feature detection in place?

	James Long (:jlongster) Assignee
	Comment 23 • 11 years ago

I figured out how to do it. I'm going to add a "supportsConditionalBreakpoints: true" to the traits of the root actor, so we will know client-side if it does or not.

I'm working on the client-side and I have a few question:

* Can I call setBreakpoint on the script actor twice with the same location? There doesn't seem to be any guard against it. However, in the BreakpointStore it increases the size without any checking if the breakpoint already exists in the same location. https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L82

* Currently it creates the breakpoint first, and then shows the popup to add a condition. I like how Chrome does it better: it shows an inline text input below the line of code and asks for the expression, then creates the breakpoint (I think a simple text input is fine). The popup doesn't feel as natural. Additionally, the breakpoint is a different color if it's conditional (yellow).

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 24 • 11 years ago

(In reply to James Long (:jlongster) from comment #23)
> I figured out how to do it. I'm going to add a
> "supportsConditionalBreakpoints: true" to the traits of the root actor, so
> we will know client-side if it does or not.

Yup. Although, (nitpick warning) simply "conditionalBreakpoints: true" would suffice since "supports" is implicit and none of the other traits use it.

> 
> I'm working on the client-side and I have a few question:
> 
> * Can I call setBreakpoint on the script actor twice with the same location?
> There doesn't seem to be any guard against it. However, in the
> BreakpointStore it increases the size without any checking if the breakpoint
> already exists in the same location.
> https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/
> actors/script.js#L82

It should just give the same breakpoint actor back. If BreakpointStore isn't handling it properly, that sounds like a bug. Please file!

> 
> * Currently it creates the breakpoint first, and then shows the popup to add
> a condition. I like how Chrome does it better: it shows an inline text input
> below the line of code and asks for the expression, then creates the
> breakpoint (I think a simple text input is fine). The popup doesn't feel as
> natural. Additionally, the breakpoint is a different color if it's
> conditional (yellow).

Sounds like two separate bugs: 1) conditional breakpoints should visually differentiate from normal breakpoints, and 2) replace the popup with an inline text input. Please file bugs for those as well :)

This bug should just remain about refactoring existing functionality.

> Should we create a new bug for doing the client-side work or just do it here? I'm starting to look at it.

Whatever works for you. If you stay in this bug, add [leave-open] to the whiteboard when you land the first patch so that the sheriffs don't close the bug prematurely. If you create a new bug, make it depend on this one.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 25 • 11 years ago

There is bug 812173 for using a different icon for conditional breakpoints. I have to say I like the conditional breakpoint popup, but I'm sure I'll like an inline field as well. Do note that we want to enlarge the field in bug 966254 though, which might be trickier with an inline field.

	James Long (:jlongster) Assignee
	Comment 26 • 11 years ago

All good points, thanks guys. I'll play around with it and file more bugs as necessary.

Look like my initial patch is ready to land, here's the try: https://tbpl.mozilla.org/?tree=Try&rev=12ae2177e3bd. So I can just rebase the work onto latest fx-team and then commit? Anything I should watch out for?

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 27 • 11 years ago

Push the rebased patch on fx-team or add it in the bug and set the checkin-needed keyword for the sheriffs to land it for you.

	James Long (:jlongster) Assignee
	Comment 28 • 11 years ago

Adds just the server functionality; the next patch will change the client to use it.

	James Long (:jlongster) Assignee
	Comment 29 • 11 years ago

I'll leave this bug open for the client-side work too, and file bugs for other stuff.

	Girish Sharma [:Optimizer]
	Comment 30 • 11 years ago

-              url: aLocation.url,
+
+url: aLocation.url,

indenting mistake ?

Also, checkin-needed goes into the keyword section :)

	James Long (:jlongster) Assignee
	Comment 31 • 11 years ago

Weird, not sure how that spacing got in there. Fixed! Also went ahead and added "conditionalBreakpoints" to the traits.

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/7b59b92580fa

In the future, please make sure your patches include commit information when requesting checkin.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 11 years ago

Something in this checkin-needed push caused Win7 talos xperf regressions. It wasn't clear to me which was at fault, so I backed out the entire push. Please verify that your patch wasn't at fault and re-request checkin when ready :)
https://hg.mozilla.org/integration/fx-team/rev/6d0a341040a9

https://tbpl.mozilla.org/php/getParsedLog.php?id=36500476&tree=Fx-Team

	James Long (:jlongster) Assignee
	Comment 34 • 11 years ago

bug 965527 specifically mentions a talos xperf try so it's got to be that. This shouldn't affect performance at all (the feature isn't even being used yet). I'll re-add the patch with a commit summary, sorry.

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 11 years ago

(In reply to James Long (:jlongster) from comment #34)
> bug 965527 specifically mentions a talos xperf try so it's got to be that.

Or because I asked him to do it. No Try run, no checkin.

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 11 years ago

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #36)
> Or because I asked him to do it.

That run was green, BTW.

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 11 years ago

FYI, process of elimination isn't looking good for this patch. One of the 3 has already re-landed and stuck and the other has a green Try run.

FTR, this is the Try syntax you'll need to get an xperf run:
try: -b o -p win32 -u none[Windows 7] -t xperf

More info on xperf:
https://wiki.mozilla.org/Buildbot/Talos/Tests#xperf

	James Long (:jlongster) Assignee
	Comment 39 • 11 years ago

That's weir. Here's the completed run (I copied the try syntax from one of the other bugs). It's all green. https://tbpl.mozilla.org/?tree=Try&rev=b73a32ef8ca5

If there's something I did wrong, let me know.

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 11 years ago
landing

We'll see!
https://hg.mozilla.org/integration/fx-team/rev/3281b969f3d6

	Rob Campbell [:rc] (:robcee)
	Comment 41 • 11 years ago

Comment on attachment 8394847 [details] [diff] [review]
812172-conditional-bp-server.patch

Review of attachment 8394847 [details] [diff] [review]:
-----------------------------------------------------------------

nice patch. A couple of nits for follow-up.

::: toolkit/devtools/client/dbg-client.jsm
@@ +27,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Timer.jsm");
> +Cu.import("resource://gre/modules/devtools/Console.jsm");

are we using this anywhere in this file?

::: toolkit/devtools/server/actors/root.js
@@ +186,5 @@
>          // Wether the storage inspector actor to inspect cookies, etc.
>          storageInspector: true,
>          // Wether storage inspector is read only
>          storageInspectorReadOnly: true,
> +        // Wether conditional breakpoints are supported

typo nit: "whether"

"wether: a castrated ram. ORIGIN Old English, of Germanic origin; related to Dutch weer and German Widder ."

::: toolkit/devtools/server/actors/script.js
@@ +4260,5 @@
>      }
>      this.scripts = [];
>    },
>  
> +  isValidCondition: function(aFrame) {

no method comment.

::: toolkit/devtools/server/main.js
@@ +64,5 @@
>  this.promised = promised;
>  this.all = all;
>  
>  Cu.import("resource://gre/modules/devtools/SourceMap.jsm");
> +Cu.import("resource://gre/modules/devtools/Console.jsm");

are we using this anywhere?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 42 • 11 years ago

(In reply to Rob Campbell [:rc] (:robcee) from comment #41)
> Comment on attachment 8394847 [details] [diff] [review]
> 812172-conditional-bp-server.patch
> 
> Review of attachment 8394847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nice patch. A couple of nits for follow-up.
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +27,5 @@
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/NetUtil.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  Cu.import("resource://gre/modules/Timer.jsm");
> > +Cu.import("resource://gre/modules/devtools/Console.jsm");
> 
> are we using this anywhere in this file?

I told him to leave it in because it sucks when the console isn't available and it sucks tracking down the import every time you need it. We've done this in other places (notably the debugger front end).

Should probably be a lazy module getter though.

	James Long (:jlongster) Assignee
	Comment 43 • 11 years ago

Thanks. I'll make the above changes in the next patch for this bug, and let the current one land, fwiw.

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/3281b969f3d6

	James Long (:jlongster) Assignee
	Comment 45 • 11 years ago

Finally making some progress after going through all the code. Still need to clean up several places and add tests.

We need a way to update the condition on the server. I tweaked the server so that it's safe to call `setBreakpoint` at the same location multiple times. That way we can call `setBreakpoint` on the server with the updated location and it just updates the breakpoint.

Additionally, I changed the breakpoint popup to only update the breakpoint when "enter" is pressed, not on every keypress. Seems better, and honestly for some reason with my new code the focus was being lost after every keypress (when it updated the bp on the server).

	James Long (:jlongster) Assignee
	Comment 46 • 11 years ago

Didn't make this clear in my above comment: this patch works for everything I've tried to far. If other agree that the implementation is sound, all I need to do is add comments/tests/etc.

	James Long (:jlongster) Assignee
	Comment 47 • 11 years ago

I don't know what to do about tests. I can update all the tests to use the new code (for example, the BreakpointClient now has a `condition` expression, but previously the `conditionalExpression` property was added ad-hoc). But that means that we aren't the code that evaluates conditions client-side.

It seems important to test both, but how can I load up the server with the "conditionalBreakpoints" trait set to null? Should I just monkey-patch that on the "client" object (set it to false at the beginning of the tests)?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 48 • 11 years ago

Monkey patching seems like a fine approach.

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 49 • 11 years ago

Agree that testing both is important.

	James Long (:jlongster) Assignee
	Comment 50 • 11 years ago

Ok, this is a much better patch. It adds tests and makes sure all the tests are passing. Things seem to be working great. Should be close to final, barring review.

	Victor Porof [:vporof][:vp] Reporter
	Comment 51 • 11 years ago

Comment on attachment 8398111 [details] [diff] [review]
812172-condition-bp-client.patch

Review of attachment 8398111 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! I just have a few comments. Please address them and this should get an r+ ;)

::: browser/devtools/debugger/debugger-controller.js
@@ +578,5 @@
>      let waitForNextPause = false;
>      let breakLocation = this._currentBreakpointLocation;
>      let watchExpressions = this._currentWatchExpressions;
>  
> +    if(!DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {

Nit: if\s( to be consistent with the rest of formatting in this file.

In other news, can you briefly explain why is this necessary? For compatibility with older servers? That sounds incredibly yucky. I wonder how Nick feels about this.

@@ +603,3 @@
>          return;
>        }
> +    

Nit: don't add redundant trailing whitespace.

@@ +1712,5 @@
>  
>      return this.removeAllBreakpoints();
>    },
>  
> +  supportsConditionalBreakpoints: function() {

Please document this function (even though it's pretty obvious what it does from its name, its good to describe *why* it's here).

@@ +1716,5 @@
> +  supportsConditionalBreakpoints: function() {
> +    let client = DebuggerController.activeThread.client;
> +    return client.mainRoot.traits.conditionalBreakpoints;
> +  },
> +  

Nit: redundant whitespace.

@@ +1867,5 @@
>        // are, in fact, removed from the server but preserved in the frontend,
>        // so that they may not be forgotten across target navigations.
>        let disabledPromise = this._disabled.get(identifier);
>        if (disabledPromise) {
> +        disabledPromise.then((bp) => {

Nit: it might be a good idea to rename bp to aPreviousBreakpointClient, to let readers of this code know that we're not dealing with the current breakpoint client, but the old one which got disabled and actually removed from the server.

@@ +1868,5 @@
>        // so that they may not be forgotten across target navigations.
>        let disabledPromise = this._disabled.get(identifier);
>        if (disabledPromise) {
> +        disabledPromise.then((bp) => {
> +          if(DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {

Nit: if\s(

Not a nit: You're using an arrow function here, so you have lexical scoping, and you're inside DebuggerController.Breakpoints. You can safely use `this`.

disabledPromise.then(aPreviousBreakpointClient => {
 if (this.supportsConditionalBreakpoints()) {
  ...
 }
 ...
}

@@ +1869,5 @@
>        let disabledPromise = this._disabled.get(identifier);
>        if (disabledPromise) {
> +        disabledPromise.then((bp) => {
> +          if(DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {
> +            if(bp.condition) {

Why aren't you reusing the previous |conditionalExpression| property? Why add a |condition| property?

@@ +1873,5 @@
> +            if(bp.condition) {
> +              aBreakpointClient.condition = bp.condition;
> +            }
> +          }
> +          else {

Nit: Let's try to maintain consistency. The prevalent style in this file is "} else {"

@@ +1874,5 @@
> +              aBreakpointClient.condition = bp.condition;
> +            }
> +          }
> +          else {
> +            if(bp.conditionalExpression) {

Nit: if\s(

@@ +2011,5 @@
> +   *
> +   * @param object aLocation
> +   *        @see DebuggerController.Breakpoints.addBreakpoint
> +   * @param string aClients
> +   *        The condition to set on the breakpoint

Document the return value of this function.

@@ +2016,5 @@
> +   */
> +  updateCondition: function(aLocation, aCondition) {
> +    let addedPromise = this._getAdded(aLocation);
> +    if(!addedPromise) {
> +      return;

This function should either do or do not return a promise. Otherwise you might end up with yucky errors about undefined not having .then()s on them :)

@@ +2020,5 @@
> +      return;
> +    }
> +
> +    let deferred = promise.defer();
> +    

Nit: there's trailing whitespace here.

@@ +2022,5 @@
> +
> +    let deferred = promise.defer();
> +    
> +    addedPromise.then(aBreakpointClient => {
> +      var info = aLocation;

Why are you caching aLocation into info? This doesn't make any sense to me.
Also, you should probably clone this object (which was passed as a param from the outside world) before modifying it.

@@ +2024,5 @@
> +    
> +    addedPromise.then(aBreakpointClient => {
> +      var info = aLocation;
> +      info.condition = aCondition;
> +      

Nit: trailing whitespace.

@@ +2029,5 @@
> +      gThreadClient.setBreakpoint(info, (aResponse, ignoredBreakpoint) => {
> +        if(aResponse.error) {
> +          deferred.reject(aResponse);
> +        }
> +        else {

} else {

@@ +2036,5 @@
> +        }
> +      });
> +    }, err => {
> +      console.error(err);
> +      dumpn(err);

Use DevToolsUtils.reportException with a meaningful message, instead of the double print.

::: browser/devtools/debugger/debugger-panes.js
@@ +544,5 @@
>     */
>    _openConditionalPopup: function() {
>      let breakpointItem = this._selectedBreakpointItem;
>      let attachment = breakpointItem.attachment;
> +    

Nit: trailing whitespace.

@@ +553,5 @@
>        breakpointPromise.then(aBreakpointClient => {
> +        let isConditionalBreakpoint;
> +        let condition;
> +
> +        if(DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {

Nit: if\s(

@@ +554,5 @@
> +        let isConditionalBreakpoint;
> +        let condition;
> +
> +        if(DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {
> +          isConditionalBreakpoint = "condition" in aBreakpointClient;

You can probably avoid all of this if you just used "conditionalExpression" instead of defining your own |condition| property.

@@ +561,5 @@
> +        else {
> +          isConditionalBreakpoint = "conditionalExpression" in aBreakpointClient;
> +          condition = aBreakpointClient.conditionalExpression;
> +        }
> +        

Nit: trailing whitespace.

@@ +842,5 @@
>        breakpointPromise.then(aBreakpointClient => {
> +        let isConditional = (
> +          DebuggerController.Breakpoints.supportsConditionalBreakpoints() ?
> +            "condition" in aBreakpointClient :
> +            "conditionalExpression" in aBreakpointClient

Again, why going through all this trouble? :)

@@ +911,5 @@
>      // save the current conditional epression.
>      let breakpointPromise = DebuggerController.Breakpoints._getAdded(attachment);
>      if (breakpointPromise) {
> +      breakpointPromise.then(breakpointClient => {
> +        if(DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {

Nit: if\s(

It seems that |updateCondition| in the controller should actually handle these two cases itself, instead of us having to sprinkle all of this logic everywhere in the view.

@@ +919,5 @@
> +          ).then(() => {
> +            window.emit(EVENTS.CONDITIONAL_BREAKPOINT_POPUP_HIDING);
> +          });
> +        }
> +        else {

Nit: } else {

@@ +925,5 @@
> +          window.emit(EVENTS.CONDITIONAL_BREAKPOINT_POPUP_HIDING);
> +        }
> +      }, err => {
> +        console.error(err);
> +        dumpn(err);

Use DevToolsUtils.reportException with a meaningful message here.

@@ +933,5 @@
>  
>    /**
> +   * The input listener for the breakpoints conditional expression textbox.
> +   */
> +  _onConditionalTextboxInput: function() {

Why is this function empty? Why is it here now that you've moved the logic into _onConditionalPopupHiding?

::: browser/devtools/debugger/test/browser_dbg_conditional-breakpoints-01.js
@@ +28,5 @@
> +    // This test forces conditional breakpoints to be evaluated on the
> +    // client-side
> +    var client = gPanel.target.client;
> +    client.mainRoot.traits.conditionalBreakpoints = false;
> +    

Nit: trailing whitespace.

::: browser/devtools/debugger/test/browser_dbg_server-conditional-bp-01.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Bug 740825: Test the debugger conditional breakpoints.

This comment isn't helpful. What is exactly tested here? How does this test differ from -02.js, -03.js and -04.js?

@@ +7,5 @@
> +
> +const TAB_URL = EXAMPLE_URL + "doc_conditional-breakpoints.html";
> +
> +function test() {
> +  // Linux debug test slaves are a bit slow at this test sometimes.

Is this true? Are there any try runs that suggest this? (just making sure; gratuitous use of requestLongerTimeout is bad)

::: toolkit/devtools/client/dbg-client.jsm
@@ +2187,5 @@
>   * @param aLocation object
>   *        The location of the breakpoint. This is an object with two properties:
>   *        url and line.
>   */
> +function BreakpointClient(aClient, aActor, aLocation, aCondition) {

Please document aCondition.

@@ +2194,5 @@
>    this.location = aLocation;
>    this.request = this._client.request;
> +
> +  // The condition property should only exist if it's a truthy value
> +  if(aCondition) {

Don't we check for this anyway in the frontend? I guess it's good to be sure though.

::: toolkit/devtools/server/actors/script.js
@@ +73,5 @@
>          this._breakpoints[url][line] = [];
>        }
> +      if(this._breakpoints[url][line][column]) {
> +        updating = true;
> +      }

Why are these changes here?

@@ +82,5 @@
>          this._wholeLineBreakpoints[url] = [];
>        }
> +      if(this._wholeLineBreakpoints[url][line]) {
> +        updating = true;
> +      }

Ditto.

@@ +4275,5 @@
> +   * evaluates to true in aFrame
> +   *
> +   * @param aFrame Debugger.Frame
> +   *        The frame to evaluate the condition in
> +   */

Shouldn't this have been in the backend patch?

	James Long (:jlongster) Assignee
	Comment 52 • 11 years ago

Thanks for the thorough review (nits and all)! I replied to the important ones below. I'll fix up the rest of the style tweaks and such.

(In reply to Victor Porof [:vporof][:vp] from comment #51)
> ::: browser/devtools/debugger/debugger-controller.js
> @@ +578,5 @@
> >      let waitForNextPause = false;
> >      let breakLocation = this._currentBreakpointLocation;
> >      let watchExpressions = this._currentWatchExpressions;
> >  
> > +    if(!DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {
> 
> Nit: if\s( to be consistent with the rest of formatting in this file.
> 
> In other news, can you briefly explain why is this necessary? For
> compatibility with older servers? That sounds incredibly yucky. I wonder how
> Nick feels about this.

Yep, that's exactly it. It's unfortunate that we have things like Firefox OS which move slower, but we simply can't remove the ability to set conditional breakpoints on older targets. I can file a new bug to remove the redundant code in a few versions or something.

> Why aren't you reusing the previous |conditionalExpression| property? Why
> add a |condition| property?

I decided not to so that I was forced to find all the points of contact that should be fixed up to work with the new functionality. This also makes sure that tests are testing the appropriate thing (we have a duplicate set of tests, one tests the old functionality and the other tests the new). Also, I like renaming it to the less-verbose `condition`.

I can reuse the name though. The main thing I like about keeping them separate is guaranteeing the tests will fail if not testing the right thing.

> 
> @@ +2022,5 @@
> > +
> > +    let deferred = promise.defer();
> > +    
> > +    addedPromise.then(aBreakpointClient => {
> > +      var info = aLocation;
> 
> Why are you caching aLocation into info? This doesn't make any sense to me.
> Also, you should probably clone this object (which was passed as a param
> from the outside world) before modifying it.

`setBreakpoint` takes a `{ url, line, col, condition }` object. It's not really a location object, but there are other places that take take a location object `{ url, line, col }` and a condition. I can try to clean this up a bit. You're right that it probably needs to be cloned.
> 
> @@ +933,5 @@
> >  
> >    /**
> > +   * The input listener for the breakpoints conditional expression textbox.
> > +   */
> > +  _onConditionalTextboxInput: function() {
> 
> Why is this function empty? Why is it here now that you've moved the logic
> into _onConditionalPopupHiding?

Didn't know if it should be kept around as a placeholder, didn't like that that though, I'll remove it!

> 
> ::: browser/devtools/debugger/test/browser_dbg_server-conditional-bp-01.js
> @@ +1,5 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +/**
> > + * Bug 740825: Test the debugger conditional breakpoints.
> 
> This comment isn't helpful. What is exactly tested here? How does this test
> differ from -02.js, -03.js and -04.js?

You're right, I'll customize them. Honestly 01-04 files were copied from the client-side conditional_breakpoint tests.

> 
> @@ +7,5 @@
> > +
> > +const TAB_URL = EXAMPLE_URL + "doc_conditional-breakpoints.html";
> > +
> > +function test() {
> > +  // Linux debug test slaves are a bit slow at this test sometimes.
> 
> Is this true? Are there any try runs that suggest this? (just making sure;
> gratuitous use of requestLongerTimeout is bad)

I have no idea, these files were just copied from the other conditional_breakpoint tests.

> 
> ::: toolkit/devtools/server/actors/script.js
> @@ +73,5 @@
> >          this._breakpoints[url][line] = [];
> >        }
> > +      if(this._breakpoints[url][line][column]) {
> > +        updating = true;
> > +      }
> 
> Why are these changes here?

Because updating a condition means simple setting the breakpoint again (with a new condition). The server needs to be changed so that setting a breakpoint at the exact same location works. I don't see any reason why it shouldn't. This is the only change needed; the `BreakpointStore` just shouldn't blindly increment its size.

> 
> @@ +4275,5 @@
> > +   * evaluates to true in aFrame
> > +   *
> > +   * @param aFrame Debugger.Frame
> > +   *        The frame to evaluate the condition in
> > +   */
> 
> Shouldn't this have been in the backend patch?

Yes, but it was missed. Just wanted to land the basic implementation on the server separately.

	Victor Porof [:vporof][:vp] Reporter
	Comment 53 • 11 years ago

(In reply to James Long (:jlongster) from comment #52)

Thanks for addressing my comments!

> (In reply to Victor Porof [:vporof][:vp] from comment #51)
> > ::: browser/devtools/debugger/debugger-controller.js
> > @@ +578,5 @@
> > >      let waitForNextPause = false;
> > >      let breakLocation = this._currentBreakpointLocation;
> > >      let watchExpressions = this._currentWatchExpressions;
> > >  
> > > +    if(!DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {
> > 
> > Nit: if\s( to be consistent with the rest of formatting in this file.
> > 
> > In other news, can you briefly explain why is this necessary? For
> > compatibility with older servers? That sounds incredibly yucky. I wonder how
> > Nick feels about this.
> 
> Yep, that's exactly it. It's unfortunate that we have things like Firefox OS
> which move slower, but we simply can't remove the ability to set conditional
> breakpoints on older targets. I can file a new bug to remove the redundant
> code in a few versions or something.

Can you please add a comment there explaining the situation, filing a followup bug for removing the deprecated code, and adding it in the comment?

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	Comment 54 • 11 years ago

(In reply to James Long (:jlongster) from comment #52)
> Thanks for the thorough review (nits and all)! I replied to the important
> ones below. I'll fix up the rest of the style tweaks and such.
> 
> (In reply to Victor Porof [:vporof][:vp] from comment #51)
> > ::: browser/devtools/debugger/debugger-controller.js
> > @@ +578,5 @@
> > >      let waitForNextPause = false;
> > >      let breakLocation = this._currentBreakpointLocation;
> > >      let watchExpressions = this._currentWatchExpressions;
> > >  
> > > +    if(!DebuggerController.Breakpoints.supportsConditionalBreakpoints()) {
> > 
> > Nit: if\s( to be consistent with the rest of formatting in this file.
> > 
> > In other news, can you briefly explain why is this necessary? For
> > compatibility with older servers? That sounds incredibly yucky. I wonder how
> > Nick feels about this.
> 
> Yep, that's exactly it. It's unfortunate that we have things like Firefox OS
> which move slower, but we simply can't remove the ability to set conditional
> breakpoints on older targets. I can file a new bug to remove the redundant
> code in a few versions or something.

Yeah please document this in a comment with a link to the new bug.

> 
> > Why aren't you reusing the previous |conditionalExpression| property? Why
> > add a |condition| property?
> 
> I decided not to so that I was forced to find all the points of contact that
> should be fixed up to work with the new functionality. This also makes sure
> that tests are testing the appropriate thing (we have a duplicate set of
> tests, one tests the old functionality and the other tests the new). Also, I
> like renaming it to the less-verbose `condition`.

Sounds good to me, but also something to document explicitly in an appropriately placed comment.

> > 
> > @@ +2022,5 @@
> > > +
> > > +    let deferred = promise.defer();
> > > +    
> > > +    addedPromise.then(aBreakpointClient => {
> > > +      var info = aLocation;
> > 
> > Why are you caching aLocation into info? This doesn't make any sense to me.
> > Also, you should probably clone this object (which was passed as a param
> > from the outside world) before modifying it.
> 
> `setBreakpoint` takes a `{ url, line, col, condition }` object. It's not
> really a location object, but there are other places that take take a
> location object `{ url, line, col }` and a condition. I can try to clean
> this up a bit. You're right that it probably needs to be cloned.

Easy and readable/explicit what your intent is when cloning via destructuring.

    let { url, line, column } = aLocation;
    // ...
    someFunctionCall({
      url: url,
      line: line,
      column: column,
      condition: condition
    });

Also, turn that var into a let! No vars! (Or const if it isn't ever re-assigned).

	James Long (:jlongster) Assignee
	Comment 55 • 11 years ago

I file a bug to remove the code here: https://bugzilla.mozilla.org/show_bug.cgi?id=990137

Thanks for the other comments, I will update the patch and attach a new one shortly.

	James Long (:jlongster) Assignee
	Comment 56 • 11 years ago

This includes a little refactoring so that we don't have to check for the client/server differences as much. I added a few methods to BreakpointClient. In the future when we remove the code we can simplify it.

Hopefully I fixed all the other stuff.

	Victor Porof [:vporof][:vp] Reporter
	Comment 57 • 11 years ago

Comment on attachment 8400063 [details] [diff] [review]
812172-condition-bp-client.patch

Review of attachment 8400063 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! I like this a lot. Few other small comments, mostly just minor style nits. No need to ask me for another review after dealing with them ;)

::: browser/devtools/debugger/debugger-controller.js
@@ +587,5 @@
> +    if (!client.mainRoot.traits.conditionalBreakpoints) {
> +      // Conditional breakpoints are { breakpoint, expression } tuples. The
> +      // boolean evaluation of the expression decides if the active thread
> +      // automatically resumes execution or not.
> +      // TODO: handle all of this server-side: Bug 812172.

I think it's safe to remove this TODO now, isn't it?

@@ +1868,5 @@
>        let disabledPromise = this._disabled.get(identifier);
>        if (disabledPromise) {
> +        disabledPromise.then((aPrevBreakpointClient) => {
> +          let condition = aPrevBreakpointClient.getCondition();
> +          if(condition) {

Nit: if[space][paren].

@@ +2010,5 @@
> +   *         A promise that will be resolved with the breakpoint client
> +   */
> +  updateCondition: function(aLocation, aCondition) {
> +    let addedPromise = this._getAdded(aLocation);
> +    if(!addedPromise) {

Ditto!

@@ +2014,5 @@
> +    if(!addedPromise) {
> +      return promise.reject(new Error('breakpoint does not exist ' +
> +                                      'in specified location'));
> +    }
> +    

Nit: remove trailing whitespace.

@@ +2022,5 @@
> +      DevToolsUtils.reportException("Breakpoints.prototype.updateCondition",
> +                                    err);
> +    });
> +  },
> +  

Ditto.

If your editor supports doing this automatically, it might be useful to enable it when digging into certain modules that don't have trailing whitespace.

::: browser/devtools/debugger/debugger-panes.js
@@ +540,5 @@
>     * Opens a conditional breakpoint's expression input popup.
>     */
>    _openConditionalPopup: function() {
>      let breakpointItem = this._selectedBreakpointItem;
> +    let attachment = breakpointItem.attachment;    

Trailing whitespace :)

@@ +890,5 @@
>      // Check if this is an enabled conditional breakpoint, and if so,
>      // save the current conditional epression.
>      let breakpointPromise = DebuggerController.Breakpoints._getAdded(attachment);
>      if (breakpointPromise) {
> +      breakpointPromise.then(breakpointClient => {

This seems like a great place to use Task.js :)

_onConditionalPopupHiding: Task.async(function*() {
  ...
  if (breakpointPromise) {
    let breakpointClient = yield breakpointPromise;
    let location = brekapointClient.location;
    let condition = this._cbTextbox.value;
    yield DebuggerController.Breakpoints.updateCondition(location, condition);
  }

  window.emit(EVENTS.CONDITIONAL_BREAKPOINT_POPUP_HIDING);
}),
...

No need for DevToolsUtils.reportException anymore.

::: browser/devtools/debugger/test/browser_dbg_conditional-breakpoints-01.js
@@ +28,5 @@
> +    // This test forces conditional breakpoints to be evaluated on the
> +    // client-side
> +    var client = gPanel.target.client;
> +    client.mainRoot.traits.conditionalBreakpoints = false;
> +    

Whitespacez0r.

::: toolkit/devtools/client/dbg-client.jsm
@@ +2196,5 @@
>    this.location = aLocation;
>    this.request = this._client.request;
> +
> +  // The condition property should only exist if it's a truthy value
> +  if(aCondition) {

Nit: space after `if` please.

@@ +2224,5 @@
> +    let root = this._client.mainRoot;
> +    if (root.traits.conditionalBreakpoints) {
> +      return "condition" in this;
> +    } else {
> +      return "conditionalExpression" in this;

Add an XXX comment about removing support for this, with the bug number.

@@ +2233,5 @@
> +   * Get the condition of this breakpoint. Currently we have to
> +   * support locally emulated conditional breakpoints until the
> +   * debugger servers are updated (see bug 990137). We used a
> +   * different property when moving it server-side to ensure that we
> +   * are testing the right code.

+1

@@ +2261,5 @@
> +      gThreadClient.setBreakpoint(info, (aResponse, ignoredBreakpoint) => {
> +        if(aResponse && aResponse.error) {
> +          deferred.reject(aResponse);
> +        }
> +        else {

Nit: } else {

@@ +2267,5 @@
> +          deferred.resolve(null);
> +        }
> +      });
> +    }
> +    else {

Ditto.

::: toolkit/devtools/server/actors/script.js
@@ +87,4 @@
>        this._wholeLineBreakpoints[url][line] = aBreakpoint;
>      }
>  
> +    if(!updating) {

Nit: space after `if`.

	James Long (:jlongster) Assignee
	Comment 58 • 11 years ago

Thanks for the review! D'oh about styling stuff.

(In reply to Victor Porof [:vporof][:vp] from comment #57)

> If your editor supports doing this automatically, it might be useful to enable it when digging into certain modules that don't have trailing whitespace.

I had that on for a bit, but it was messing up a few other things (I run terminal under Emacs). I will fix that now so that it's not a problem again.

	James Long (:jlongster) Assignee
	Comment 59 • 11 years ago

Try: https://tbpl.mozilla.org/?tree=Try&rev=5a28b26fe032

Those 2 failures should be completely unrelated to this.

	Ryan VanderMeulen [:RyanVM]
	Comment 60 • 11 years ago
landing

Comment on attachment 8401437 [details] [diff] [review]
812172-condition-bp-client.patch

Commit information kthx ;)

https://hg.mozilla.org/integration/fx-team/rev/0ae1f25fe06d

	James Long (:jlongster) Assignee
	Comment 61 • 11 years ago

Agh sorry. Been a hard week. Will do better!

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/0ae1f25fe06d

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 63 • 11 years ago

Great job James! Don't forget to send a PR to https://github.com/jimblandy/DebuggerDocs.
So, have you seen bug 836298? ;-)

	James Long (:jlongster) Assignee
	Comment 64 • 11 years ago

(In reply to Panos Astithas [:past] from comment #63)
> Great job James! Don't forget to send a PR to
> https://github.com/jimblandy/DebuggerDocs.
> So, have you seen bug 836298? ;-)

Thanks, I had not seen that bug. I suppose I could take a look!

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 65 • 11 years ago

Comment on attachment 8401437 [details] [diff] [review]
812172-condition-bp-client.patch

Review of attachment 8401437 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/client/dbg-client.jsm
@@ +2259,5 @@
> +        url: this.location.url,
> +        line: this.location.line,
> +        condition: aCondition
> +      };
> +      gThreadClient.setBreakpoint(info, (aResponse, ignoredBreakpoint) => {

I think this is the cause of bug 995252: ignoring a created breakpoint leaves it enabled on the server. Updating a breakpoint should create a new one and remove the old.

	James Long (:jlongster) Assignee
	Comment 66 • 11 years ago

(In reply to Panos Astithas [:past] from comment #65)
> Comment on attachment 8401437 [details] [diff] [review]
> 812172-condition-bp-client.patch
> 
> Review of attachment 8401437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/client/dbg-client.jsm
> @@ +2259,5 @@
> > +        url: this.location.url,
> > +        line: this.location.line,
> > +        condition: aCondition
> > +      };
> > +      gThreadClient.setBreakpoint(info, (aResponse, ignoredBreakpoint) => {
> 
> I think this is the cause of bug 995252: ignoring a created breakpoint
> leaves it enabled on the server. Updating a breakpoint should create a new
> one and remove the old.

I can reproduce it locally; we should write a test that reproduces this (all the tests passed). I will look into it more tomorrow morning. The above code shouldn't cause the problem, if you call `setBreakpoint` with the same url/line as an existing breakpoint, it will just update the existing breakpoint. That may not be working as intended though. It seems cumbersome to have to remove and add a new breakpoint every time we want to update the condition, but maybe that is the way to go.

	James Long (:jlongster) Assignee
	Comment 67 • 11 years ago

Let's keep this bug closed and work in bug 995252. Conditional breakpoints seem to work in most cases but that bug must be an edge case.