Closed Bug 1235371 Opened 6 years ago Closed 6 years ago

Change EnvironmentActor to protocol.js

Categories

(DevTools :: Debugger, defect)

defect
Not set
normal

Tracking

(firefox46 affected, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: linclark, Assigned: ejpbruel)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch EnvironmentActor.patch (obsolete) — Splinter Review
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.
Blocks: 1037992
Depends on: 1235370
Blocks: 1235375
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 :-)
Assignee: nobody → ejpbruel
This patch moves EnvironmentActor into its own file.
Attachment #8702274 - Attachment is obsolete: true
Attachment #8722978 - Flags: review?(jryans)
This patch refactors EnvironmentActor to use protocol.js.
Attachment #8722979 - Flags: review?(jryans)
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...
Attachment #8722978 - Flags: review?(jryans) → review+
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.
Attachment #8722979 - Flags: review?(jryans) → feedback+
New patch with comments by jryans addressed.
Attachment #8722979 - Attachment is obsolete: true
Attachment #8723599 - Flags: review?(jryans)
Try push for moving EnvironmentActor into its own file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4e5b2ebef3b
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
Attachment #8723599 - Attachment is patch: true
Attachment #8723599 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #8723599 - Flags: review?(jryans) → review+
Whiteboard: leave-open
Try run for refactoring EnvironmentActor to use protocol.js:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda71edcbbb1
Whiteboard: leave-open
https://hg.mozilla.org/mozilla-central/rev/0a58b5b8a407
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.