Julian Descottes [:jdescottes] Reporter
	Description • 7 years ago

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

	Julian Descottes [:jdescottes] Reporter
	Comment 1 • 7 years ago

(moving attachment to dedicated bug)

	Julian Descottes [:jdescottes] Reporter
	Comment 2 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f0688bdf9c6a9ad01f6d3e2454a86ef2eec920d

	Julian Descottes [:jdescottes] 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

	Jason Laster [:jlast] 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.

	Jason Laster [:jlast] Assignee
	Comment 5 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbf4ffdbe0a09dab4a0427ead58440d331a1838c

	Jason Laster [:jlast] 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...

	Julian Descottes [:jdescottes] 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

	Pulsebot
	Comment 9 • 7 years ago

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fdf70b7c768
Refactor the script actor. r=jdescottes

	Noemi Erli[:noemi_erli]
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2fdf70b7c768