Currently, log points are implemented as a special kind of breakpoint condition --- the logged value is wrapped in a console.log() call and added to the breakpoint's condition. It would be nicer if log points were handled in the server instead. This provides the following advantages:

• The server can more carefully manage how the log point is implemented. One bug in the current implementation is that logged messages have a 'debugger eval' filename associated with them, rather than the location of the log point. While the attached patch does not fix this, I don't think this can be fixed without handling the log in the server. Log points have a separate implementation when replaying, and this can be tapped into much more easily if the server knows it is dealing with a log point.

• The client's state can be more easily managed when the logging string is separate state from the condition. The current implementation can expose the 'console.log()' call to users, if for example a log point is added and its condition is then edited.

The attached patch makes this change: the condition and logged string are both stored in an options object which is attached to the breakpoint in both the server and client. A few other aspects of breakpoint handling are tidied up, such as the need to delete breakpoints when changing their condition.

Patch with both server and client changes. I'm going to try to split this up into two pieces --- a client side piece which can land to debugger.html by itself, and another operating on the server and the server/client interface.

This patch applies on top of the debugger.html changes in https://github.com/devtools-html/debugger.html/pull/7749 and takes care of changes to the server/client interface and the server itself.

(In reply to Jason Laster [:jlast] from comment #4)

Comment on attachment 9037446 [details] [diff] [review]
patch

Review of attachment 9037446 [details] [diff] [review]:

a couple comments. But looking good.

::: devtools/server/actors/breakpoint.js
@@ +101,5 @@

},

• // Update any state affected by changing options on a script this breakpoint
• // is associated with.
• _updateOptionsForScript(script, offsets, oldOptions, newOptions) {

maybe we should leave this out of this patch and add it in 1513118

Well, the logic in this function has already been reviewed in part 3 of bug 1513118. It's a little awkward given the complicated history of these two bugs for it to have moved over here, but I don't think it needs to be reviewed again and can be ignored in this patch.

@@ +218,5 @@

•  }
  

•  // In the non-replaying case, log values are handled by treating them as
  

•  // conditions. console.log() never returns true so we will not pause.
  

•  const condition = this.options.logValue ? `console.log(${this.options.logValue})`
  

i forgot why we went with logValue and not log

I picked logValue because it has a clearer meaning when reading and is easier to grep for. I don't feel strongly one way or the other though.

@@ +219,5 @@

•  // In the non-replaying case, log values are handled by treating them as
  

•  // conditions. console.log() never returns true so we will not pause.
  

•  const condition = this.options.logValue ? `console.log(${this.options.logValue})`
  

•                                          : this.options.condition;
  

It would be nice to have conditional log points,

1. check if the condition is true
2. if true, evaluate console.log(logValue)

we can do that here or in a follow up

Sure, I can fix that now. I thought about doing this but didn't for some reason that I don't remember now.

Updated patch with server-side support for conditional log points.

What do you think about having a server unit test for a breakpoint with a log + condition.

e.g. https://searchfox.org/mozilla-central/source/devtools/server/tests/unit/test_conditional_breakpoint-01.js

(In reply to Jason Laster [:jlast] from comment #7)

What do you think about having a server unit test for a breakpoint with a log + condition.

e.g. https://searchfox.org/mozilla-central/source/devtools/server/tests/unit/test_conditional_breakpoint-01.js

Sure, I can add a unit test before landing.

https://hg.mozilla.org/mozilla-central/rev/89bc47c1926c

https://hg.mozilla.org/mozilla-central/rev/d618df7a018e