Closed
Bug 1435187
Opened 7 years ago
Closed 7 years ago
Split DevTools script actor in smaller files
Categories
(DevTools :: General, enhancement, P3)
DevTools
General
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jlast)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
47.94 KB,
patch
|
Details | Diff | Splinter Review |
From the meta Bug 1432780
(In reply to Jason Laster [:jlast] from comment #2)
> Created attachment 8947711 [details] [diff] [review]
> refactor-script.patch
>
> Refactor the script actor
>
> - Extract paused scoped objects.
> - Extract event loop stack. r=jdescottes
> - Extract actor stores. r=jdescottes
> - Move script.js to actor.js. r=jdescottes
>
>
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8f0688bdf9c6a9ad01f6d3e2454a86ef2eec920d
Reporter | ||
Comment 1•7 years ago
|
||
(moving attachment to dedicated bug)
Attachment #8947734 -
Flags: review?(jdescottes)
Reporter | ||
Comment 2•7 years ago
|
||
Reporter | ||
Comment 3•7 years ago
|
||
Comment on attachment 8947734 [details] [diff] [review]
refactor-script.patch
Review of attachment 8947734 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for splitting this! I'd like to see another patch with the comments below addressed.
(also you'll need to update the commit message with the new bug number)
See try for some eslint failures.
(+ an unreported netmonitor intermittent? https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f0688bdf9c6a9ad01f6d3e2454a86ef2eec920d&selectedJob=160023641 ? unrelated I guess)
There are still 3 references to devtools/server/actors/script in /devtools, used in xpcshell tests.
- devtools/shared/security/tests/unit/testactors.js
- devtools/shared/transport/tests/unit/head_dbg.js
- devtools/shared/transport/tests/unit/testactors.js
For this kind of patches it's good to have a more global test coverage (eg try: -b do -p linux64 -u mochitests,xpcshell -t none)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0e0c7271db3db9b9a32542ab7c5d9d9b4a3a797
Started a talos run for the patch at https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=21f55ae639f22aac5c6f2e36ceeca7a7866cc0d8&newProject=try&newRevision=15ee57532416cbbe26e81c0930562235a6274d5a&framework=1
(as mentioned in the meta we should keep an eye on performance)
::: devtools/server/actors/child-process.js
@@ -6,5 @@
>
> const { Cc, Ci, Cu } = require("chrome");
> const Services = require("Services");
>
> -const { ChromeDebuggerActor } = require("devtools/server/actors/script");
follow-up fodder: wondering if we should move ChromeDebuggerActor's definition to child-process.js, (and rename it to ChromeThreadActor?), same for AddonThreadActor and addon.js. Their only difference with ThreadActor is the prefix.
::: devtools/server/actors/thread.js
@@ +9,5 @@
> const Services = require("Services");
> const { Cc, Ci, Cr } = require("chrome");
> +const { ActorPool, GeneratedLocation } = require("devtools/server/actors/common");
> +const { createValueGrip, longStringGrip } = require("devtools/server/actors/object");
> +const { SourceActorStore } = require("devtools/server/actors/utils/source-actor-store");
this can be a lazyRequire
@@ +10,5 @@
> const { Cc, Ci, Cr } = require("chrome");
> +const { ActorPool, GeneratedLocation } = require("devtools/server/actors/common");
> +const { createValueGrip, longStringGrip } = require("devtools/server/actors/object");
> +const { SourceActorStore } = require("devtools/server/actors/utils/source-actor-store");
> +const { BreakpointActorMap } = require("devtools/server/actors/utils/breakpoint-actor-map");
lazyRequire
@@ +11,5 @@
> +const { ActorPool, GeneratedLocation } = require("devtools/server/actors/common");
> +const { createValueGrip, longStringGrip } = require("devtools/server/actors/object");
> +const { SourceActorStore } = require("devtools/server/actors/utils/source-actor-store");
> +const { BreakpointActorMap } = require("devtools/server/actors/utils/breakpoint-actor-map");
> +const { PauseScopedObjectActor } = require("devtools/server/actors/pause-scoped");
lazyRequire
@@ +12,5 @@
> +const { createValueGrip, longStringGrip } = require("devtools/server/actors/object");
> +const { SourceActorStore } = require("devtools/server/actors/utils/source-actor-store");
> +const { BreakpointActorMap } = require("devtools/server/actors/utils/breakpoint-actor-map");
> +const { PauseScopedObjectActor } = require("devtools/server/actors/pause-scoped");
> +const { EventLoopStack } = require("devtools/server/actors/utils/event-loop");
lazyRequire
@@ +28,5 @@
> let Debugger = require("Debugger");
> hackDebugger(Debugger);
> return Debugger;
> });
> loader.lazyRequireGetter(this, "CssLogic", "devtools/server/css-logic", true);
not used here
@@ +30,5 @@
> return Debugger;
> });
> loader.lazyRequireGetter(this, "CssLogic", "devtools/server/css-logic", true);
> loader.lazyRequireGetter(this, "findCssSelector", "devtools/shared/inspector/css-logic", true);
> loader.lazyRequireGetter(this, "mapURIToAddonID", "devtools/server/actors/utils/map-uri-to-addon-id");
not used here
::: devtools/server/actors/utils/breakpoint-actor-map.js
@@ +1,1 @@
> +const { OriginalLocation } = require("devtools/server/actors/common");
License missing.
use strict missing.
@@ +33,5 @@
> + if (this.size === 0) {
> + return;
> + }
> +
> + function* findKeys(obj, key) {
Not related to your change, but there is no reason for this method to be defined in findActors, we should move it up in a follow up
Attachment #8947734 -
Flags: review?(jdescottes)
Assignee | ||
Comment 4•7 years ago
|
||
Thanks for the feedback julian!
+ findKeys - agreed. but given we're moving the file i'm hesitant to make another change
+ ChromeDebuggerActor => ChromeThreadActor, funny. I had the same thought. I want to get a bit more familiar with the conventions before I move a sub-class, but definitely thinking about it.
Attachment #8947734 -
Attachment is obsolete: true
Attachment #8947822 -
Flags: review?(jdescottes)
Assignee | ||
Comment 5•7 years ago
|
||
Assignee | ||
Comment 6•7 years ago
|
||
I'm curious what you think of these refactors, it's mostly oriented around making the ThreadActor more discoverable / understandable. I also hope that the stores (breakpoints, sources) will be easier to understand if they're isolated...
Flags: needinfo?(jimb)
Reporter | ||
Comment 7•7 years ago
|
||
Comment on attachment 8947822 [details] [diff] [review]
refactor-script-2.patch
Review of attachment 8947822 [details] [diff] [review]:
-----------------------------------------------------------------
Still needs to update devtools/shared/security/tests/unit/testactors.js
R+ after a green try
Attachment #8947822 -
Flags: review?(jdescottes) → review+
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8947822 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fdf70b7c768
Refactor the script actor. r=jdescottes
Keywords: checkin-needed
Comment 10•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•6 years ago
|
Product: Firefox → DevTools
Assignee | ||
Updated•5 years ago
|
Flags: needinfo?(jimb)
You need to log in
before you can comment on or make changes to this bug.
Description
•