1235371 - Change EnvironmentActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Lin Clark [:linclark]]

Attachment: EnvironmentActor.patch

Splitting this out from Bug #1037992

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


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

::: devtools/server/actors/script.js
@@ +3416,5 @@
>   *        The lexical environment that will be used to create the actor.
>   * @param ThreadActor aThreadActor
>   *        The parent thread actor that contains this environment.
>   */
> +let EnvironmentActor = ActorClass({

I guess these actor will manage their lifetimes outside of p.js's usual flow?  I see this one is added a to a pool, for example.

Comment 1

[Eddy Bruel [:ejpbruel]]

Hi Lin. There has been no activity on this bug for two months. I'm going to go out on a limb here and assume that you're no longer working on it. If that's not the case, I apologise for stealing your bug :-)

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Move EnvironmentActor into its own file.

This patch moves EnvironmentActor into its own file.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor EnvironmentActor to use protocol.js.

This patch refactors EnvironmentActor to use protocol.js.

Comment 4

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

Comment on attachment 8722978 [details] [diff] [review]
Move EnvironmentActor into its own file.

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

::: devtools/server/actors/environment.js
@@ +170,5 @@
> +   */
> +  onAssign: function (aRequest) {
> +    // TODO: enable the commented-out part when getVariableDescriptor lands
> +    // (bug 725815).
> +    /*let desc = this.obj.getVariableDescriptor(aRequest.name);

Seems like someone should just delete this thing...

Comment 5

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

Comment on attachment 8722979 [details] [diff] [review]
Refactor EnvironmentActor to use protocol.js.

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

::: devtools/server/actors/environment.js
@@ +168,1 @@
>     * @param aRequest object

Update comments

@@ +168,4 @@
>     * @param aRequest object
>     *        The protocol request object.
>     */
> +  assign: method(function (name, value) {

aRequest is gone now, update usages.

@@ +193,5 @@
> +    request: {
> +      name: Arg(1),
> +      value: Arg(2)
> +    },
> +    response: {}

Can omit when empty.

@@ +204,5 @@
>     * @param aRequest object
>     *        The protocol request object.
>     */
> +  bindings: method(function () {
> +    return this._bindings();

This indirection should be unnecessary.  Rename _bindings to bindings and add the method stuff there.

@@ +206,5 @@
>     */
> +  bindings: method(function () {
> +    return this._bindings();
> +  }, {
> +    request: {},

Can omit when empty.

Comment 6

[Eddy Bruel [:ejpbruel]]

Attachment: Refactor EnvironmentActor to use protocol.js.

New patch with comments by jryans addressed.

Comment 7

[Eddy Bruel [:ejpbruel]]

Try push for moving EnvironmentActor into its own file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e5b2ebef3b

Comment 8

[Eddy Bruel [:ejpbruel]]

Looks like a single test fails because I did not update the path to the environment actor in webconsole.js.

Here is a new try run for a patch that addresses that issue:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1de29504511

Comment 9

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

Comment on attachment 8723599 [details] [diff] [review]
Refactor EnvironmentActor to use protocol.js.

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

::: devtools/server/actors/environment.js
@@ +196,5 @@
>      }
>  
>      return bindings;
> +  }, {
> +    request: {},

Omit when empty.

Comment 10

[Pulsebot]

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

Comment 11

[Eddy Bruel [:ejpbruel]]

Try run for refactoring EnvironmentActor to use protocol.js:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda71edcbbb1

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Pulsebot]

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

Comment 14

[Carsten Book [:Tomcat]]

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