Closed
Bug 997119
Opened 11 years ago
Closed 5 years ago
Decouple ThreadActor from BrowsingContextTargetActor
Categories
(DevTools :: Framework, task, P3)
DevTools
Framework
Tracking
(Fission Milestone:M4, firefox70 fixed)
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: ochameau, Assigned: jarilvalenciano)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
bug 977043 followup.
Ideally, DebuggerServer should only be about implementing the server and doing the network and actor machinery.
But it contains some code specific to debugger actor, especially some code specific to the ThreadActor. So that if you want a very good comprehension of ThreadActor you have to read the ThreadActor class, but also a bunch of DebuggerServer methods.
Bug 977043 introduced new will-navigate/navigate events that should help moving thread actor code back to the ThreadActor class.
Comment 1•11 years ago
|
||
I believe when you write DebuggerServer above you are referring to the TabActor in webbrowser.js (DebuggerServer is in main.js).
Component: Developer Tools → Developer Tools: Debugger
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86_64 → All
Summary: Uncouple ThreadActor from DebuggerServer → Uncouple ThreadActor from TabActor
Version: unspecified → Trunk
Comment 2•10 years ago
|
||
Hi Alex. Could you clarify exactly how you would like to decouple ThreadActor from TabActor?
Flags: needinfo?(poirot.alex)
Reporter | ||
Comment 3•10 years ago
|
||
Move (if possible) all code related to ThreadActor out of main.js (most likely script.js?)
Code like this can be easily moved to script.js:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1293
TabActor should be a generic abstraction and shouldn't contain anything of the child actors it manages.
For historical reason, many code related to the debugger have been entangled within core code for the debugger server/protocol. Now we have some usage of the DebuggerServer where we don't care about the debugger (script.js, js code debugging, breakpoint, ...). And we should be able to load a barebone DebuggerServer instance without loading anything regarding the debugger. This is related to my work to strip down memory usage of the server.
Flags: needinfo?(poirot.alex)
Comment 4•10 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> Move (if possible) all code related to ThreadActor out of main.js (most
> likely script.js?)
>
> Code like this can be easily moved to script.js:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#1293
>
> TabActor should be a generic abstraction and shouldn't contain anything of
> the child actors it manages.
> For historical reason, many code related to the debugger have been entangled
> within core code for the debugger server/protocol. Now we have some usage of
> the DebuggerServer where we don't care about the debugger (script.js, js
> code debugging, breakpoint, ...). And we should be able to load a barebone
> DebuggerServer instance without loading anything regarding the debugger.
> This is related to my work to strip down memory usage of the server.
I agree. Ideally TabActor should be completely unaware of the internals of ThreadActor.
Are you aware that we've recently added some dependencies for ThreadActor on TabActor (so in the other direction?) I'm talking about stuff like makeDebugger in webbrowser.js. Are you ok with those dependencies? I assume you are only talking about dependencies of TabActor on ThreadActor.
Reporter | ||
Comment 5•10 years ago
|
||
Yes deps on the other direction are fine.
Updated•10 years ago
|
Summary: Uncouple ThreadActor from TabActor → Decouple ThreadActor from TabActor
Blocks: dt-polish-debt
Depends on: 1172897
Summary: Decouple ThreadActor from TabActor → Decouple ThreadActor from BrowsingContextTargetActor
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Component: Debugger → General
Updated•6 years ago
|
Type: defect → task
Component: General → Framework
Comment 6•5 years ago
|
||
failed test: devtools/client/debugger/test/mochitest/browser_dbg-browser-content-toolbox.js
failed test: devtools/client/debugger/test/mochitest/browser_dbg-chrome-debugging.js
failed test: devtools/client/debugger/test/mochitest/browser_dbg-windowless-workers-early-breakpoint.js
diff --git a/devtools/server/actors/targets/browsing-context.js b/devtools/server/actors/targets/browsing-context.js
index 5e71b1320a5e..d2a7dac2d85c 100644
--- a/devtools/server/actors/targets/browsing-context.js
+++ b/devtools/server/actors/targets/browsing-context.js
@@ -1341,16 +1341,6 @@ const browsingContextTargetPrototype = {
return;
}
- // Proceed normally only if the debuggee is not paused.
- // TODO bug 997119: move that code to ThreadActor by listening to
- // will-navigate
- const threadActor = this.threadActor;
- if (threadActor.state == "paused") {
- threadActor.unsafeSynchronize(Promise.resolve(threadActor.doResume()));
- threadActor.dbg.enabled = false;
- }
- threadActor.disableAllBreakpoints();
-
this.emit("tabNavigated", {
url: newURI,
nativeConsoleAPI: true,
diff --git a/devtools/server/actors/thread.js b/devtools/server/actors/thread.js
index 182bf6d46736..cfb93b7693d5 100644
--- a/devtools/server/actors/thread.js
+++ b/devtools/server/actors/thread.js
@@ -99,6 +99,7 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
this.objectGrip = this.objectGrip.bind(this);
this.pauseObjectGrip = this.pauseObjectGrip.bind(this);
this._onOpeningRequest = this._onOpeningRequest.bind(this);
+ this.onWillNavigate = this.onWillNavigate.bind(this)
if (Services.obs) {
// Set a wrappedJSObject property so |this| can be sent via the observer svc
@@ -279,6 +280,8 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
this.sources.setOptions(this._options);
this.sources.on("newSource", this.onNewSourceEvent);
+ this._parent.on("will-navigate", this.onWillNavigate);
+
// Initialize an event loop stack. This can't be done in the constructor,
// because this.conn is not yet initialized by the actor pool at that time.
this._nestedEventLoops = new EventLoopStack({
@@ -348,19 +351,29 @@ const ThreadActor = ActorClassWithSpec(threadSpec, {
return this._pauseOverlay;
}
+ onWillNavigate: function () {
+ if (this.state == "paused") {
+ this.unsafeSynchronize(Promise.resolve(this.doResume()));
+ // this.doResume();
+ this.dbg.enabled = false;
+ }
+ this.disableAllBreakpoints();
+ },
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f285ee7fedd7
Move ThreadActor logic from BrowsingContext. r=yulia
Comment 9•5 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox70:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Updated•5 years ago
|
Assignee: nobody → jarilvalenciano
Comment 10•5 years ago
|
||
Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
Fission Milestone: --- → M4
You need to log in
before you can comment on or make changes to this bug.
Description
•