1235374 - Change BreakpointActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Lin Clark [:linclark]]

Attachment: BreakpointActor.patch

Splitting this out from Bug #1037992

This is the latest review from Bug 1037992, comment 14, by jryans:

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

::: devtools/server/actors/script.js
@@ +3255,2 @@
>  
> +  initialize: function (aThreadActor, aOriginalLocation) {

You could convert away from `aArg` style to `arg` if you wanted...

@@ +3392,5 @@
> +  delete: method(function () {
> +    return this._delete();
> +  }),
> +
> +  _delete: function () {

You don't need 2 separate functions.  It's safe for an actor to call a function defined via p.js `method(...)` directly, because `method(...)` returns the function itself, which will be available on the actor object like normal.

Comment 1

[Lin Clark [:linclark]]

Attachment: Bug1235374.patch

This patch addresses the issues raised in the last review.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c99c6737571f

Comment 2

[Lin Clark [:linclark]]

Attachment: Bug1235374.patch

This patch addresses the previous feedback and moves BreakpointActor to its own file (as described in the META).

Comment 3

[Lin Clark [:linclark]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=de5cd72475c6

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8706943 [details] [diff] [review]
Bug1235374.patch

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

Overall seems good!  Just a few more cleanup nits to fix.

::: devtools/server/actors/breakpoint.js
@@ +46,5 @@
> +  /**
> +   * Called when this same breakpoint is added to another Debugger.Script
> +   * instance.
> +   *
> +   * @param aScript Debugger.Script

Nit: Comment needs updating

@@ +80,5 @@
> +   *            `result` will be `undefined`.
> +   *          - message: string
> +   *            If the condition throws, this is the thrown message.
> +   */
> +  checkCondition: function(aFrame) {

Nit: Rename to frame and update comment

@@ +112,5 @@
> +
> +  /**
> +   * A function that the engine calls when a breakpoint has been hit.
> +   *
> +   * @param aFrame Debugger.Frame

Nit: Update arg name

@@ +115,5 @@
> +   *
> +   * @param aFrame Debugger.Frame
> +   *        The stack frame that contained the breakpoint.
> +   */
> +  hit: function (frame) {

Nit: No space after function

@@ +157,5 @@
> +
> +  /**
> +   * Handle a protocol request to remove this breakpoint.
> +   *
> +   * @param aRequest object

Nit: Remove unused arg from comment

Comment 5

[Lin Clark [:linclark]]

Attachment: Bug1235374.patch

Gah, how did I leave so many nits for you to find? Sorry about that, and thanks for finding them.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bebe069cb0e&selectedJob=15764166

Comment 6

[Lin Clark [:linclark]]

Attachment: Bug1235374.patch

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/76a10b1ae6ab

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/76a10b1ae6ab

Comment 9

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected