1235375 - Change FrameActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Lin Clark [:linclark]]

Splitting this out from Bug #1037992

Comment 1

[Lin Clark [:linclark]]

Attachment: FrameActor.patch

Comment 2

[Lin Clark [:linclark]]

This was r+ in the last issue and it has no dependencies, so I think we can land it if it passes tests.

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

Comment 3

[Lin Clark [:linclark]]

Turns out this does have a dependency on two other patches. It uses ProtocolError, so it depends on bug 1235370 and bug 1235371.

Comment 4

[Lin Clark [:linclark]]

Attachment: Bug1235375.patch

This patch fixes a couple of comments and adds a bug number for a TODO.

I'm a little concerned about this patch, though. It is converting onPop to pop. I'm not sure where/whether this code is currently being used. Removing it completely does not break any tests. This means one of two things:

Either:
1. There are no callers for this function. If this is the case, I suggest we remove it and leave any discussion of implementing it to the issue queue.
2. There are callers and I could be breaking something here. We wouldn't know it because we don't have test coverage.

I've created an issue to figure out if we want to implement this or not (Bug 235887).

Comment 5

[Lin Clark [:linclark]]

Correction, the bug about the pop TODO is Bug 1235887.

Comment 6

[Lin Clark [:linclark]]

I have a patch in to remove the pop method (Bug 1235901). It has already been r+-ed, so once it is committed, I'll update this patch to remove the pop parts.

Comment 7

[Lin Clark [:linclark]]

Attachment: Bug1235375.patch

This patch simply switches FrameActor to be a subclass of ActorClass. This will make it easy to add protocol.js methods in the future if any are needed.

Should we break these actors out into their own files? I noticed that a lot of other actors that use ActorClass are in their own file.

Comment 8

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8703612 [details] [diff] [review]
Bug1235375.patch

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

Patch looks good to me.

To answer your question: I definitely think we should move these actors to their own separate files. In fact, this has been on my to do list for some time, but it was never high priority enough for me to actually get to it. If you feel like doing it, I'd be much obliged if you did :-) (You can do so in a followup patch instead of this one, if you want).

::: devtools/server/actors/script.js
@@ +3114,5 @@
>    "threadGrip": PauseScopedObjectActor.prototype.onThreadGrip,
>  });
>  
>  /**
> + * An actor for a specified stack frame.

I like this convention of separating out the comment describing the actor from the one describing its constructor. Let's stick to this when you convert the other actors.

Comment 9

[Lin Clark [:linclark]]

Attachment: Bug1235375.patch

Had a conversation in IRC with ochameau, ejpbruel and jlongster. The META issue was updated to include breaking the actors into separate files.

Based on this, I broke FrameActor out into its own file.

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

Comment 10

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8703680 [details] [diff] [review]
Bug1235375.patch

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

For the sake of consistency, if we are going to move each actor to its own file, I think each file should have the same name as the actor it contains. In my experience, this makes it easier to find things if you're not already familiar with the code. With that in mind, I propose we name the file 'frame-actor.js'.

r+ with the above comment addressed.

Comment 11

[Lin Clark [:linclark]]

I originally thought the same, but when I looked at the other files (like chrome.js, object.js, etc), they all have the word actor in the function name but not in the file name.

I'm thinking that we should keep it consistent with these other files, which means keeping it as frame.js. Eddy, do you agree?

Comment 12

[Eddy Bruel [:ejpbruel]]

(In reply to Lin Clark from comment #11)
> I originally thought the same, but when I looked at the other files (like
> chrome.js, object.js, etc), they all have the word actor in the function
> name but not in the file name.
> 
> I'm thinking that we should keep it consistent with these other files, which
> means keeping it as frame.js. Eddy, do you agree?

Well, I think the current way we do it is wrong, so the argument that we shouldn't change it because this is the current way we do it is not a particularly strong one for me.

That said, this really is not important enough to have a discussion over, so I'll leave the final decision up to you.

Comment 13

[Lin Clark [:linclark]]

I'm going to leave it as it is in the patch, then. 

If we decide to have actor files include the word "actor", we can do it in a different issue as a mass rename to make sure that it's consistent.

Comment 14

[Pulsebot]

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

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/70691265926c

Comment 16

[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