Closed
Bug 1235375
Opened 9 years ago
Closed 9 years ago
Change FrameActor to protocol.js
Categories
(DevTools :: Debugger, defect)
DevTools
Debugger
Tracking
(firefox46 fixed)
RESOLVED
FIXED
Firefox 46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: linclark, Assigned: linclark)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
Splitting this out from Bug #1037992
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lclark
Assignee | ||
Comment 3•9 years ago
|
||
Turns out this does have a dependency on two other patches. It uses ProtocolError, so it depends on bug 1235370 and bug 1235371.
Assignee | ||
Comment 4•9 years ago
|
||
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).
Attachment #8702284 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Correction, the bug about the pop TODO is Bug 1235887.
Assignee | ||
Comment 6•9 years ago
|
||
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.
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•9 years ago
|
||
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.
Attachment #8702993 -
Attachment is obsolete: true
Attachment #8703612 -
Flags: review?(ejpbruel)
Comment 8•9 years ago
|
||
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.
Attachment #8703612 -
Flags: review?(ejpbruel) → review+
Assignee | ||
Comment 9•9 years ago
|
||
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
Attachment #8703612 -
Attachment is obsolete: true
Attachment #8703680 -
Flags: review?(ejpbruel)
Comment 10•9 years ago
|
||
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.
Attachment #8703680 -
Flags: review?(ejpbruel) → review+
Assignee | ||
Comment 11•9 years ago
|
||
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?
Flags: needinfo?(ejpbruel)
Comment 12•9 years ago
|
||
(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.
Flags: needinfo?(ejpbruel)
Assignee | ||
Comment 13•9 years ago
|
||
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.
Keywords: checkin-needed
Comment 14•9 years ago
|
||
Keywords: checkin-needed
Comment 15•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 16•9 years ago
|
||
[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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•