(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.
Bug 1520972 Comment 5 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
#[markdown(off)] (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.
``` (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. ```
(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.