Actor methods and events about sources should move from ThreadActor to TabActor-ish parents

NEW
Unassigned

Status

()

Firefox
Developer Tools: Debugger
2 years ago
2 months ago

People

(Reporter: jryans, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox47 affected)

Details

A while ago, the TabSources object was moved from the ThreadActor to TabActor-ish parents who now own it.

However, we left all the actor methods and events on the ThreadActor (not sure why).

We should finally do the work to clean this up, using traits as needed to preserve compatibility.
We had a conversation on the mailing list about this a while ago. I think the change to move the TabSources object to TabActor didn't make sense in the first place, because the SourceActors that it stores are still owned by the ThreadActor. With the introduction of workers, we now have multiple ThreadActors, and the sources for those threads will go away when the corresponding thread does.

I think the TabSources object should be moved back to ThreadActor, and we should rethink our strategy for giving other tools access to the sources for each thread. With that in mind, I'm voting to wontfix this bug.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 2

2 years ago
It does not seem to me that any conclusion was reached on the mailing list.  Eddy posted a thread titled "Sources should be scoped by thread, not by tab" and offered the view he mentions in the comment above.  Nick replied with the opposite perspective.  No conclusion or action has so far been described on the mailing list as far as I can see.
Flags: needinfo?(ejpbruel)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> It does not seem to me that any conclusion was reached on the mailing list. 
> Eddy posted a thread titled "Sources should be scoped by thread, not by tab"
> and offered the view he mentions in the comment above.  Nick replied with
> the opposite perspective.  No conclusion or action has so far been described
> on the mailing list as far as I can see.

You're right. I was overzealous here. Let's reopen this bug pending a decision on the mailing list.
Status: RESOLVED → REOPENED
Flags: needinfo?(ejpbruel)
Resolution: WONTFIX → ---
I'm trying to speedup the browser toolbox, and I see tons of newSource messages. Like double-tons of them

This bug seems to be about that.
These duplicated messages seems to be introduced in bug 1177279, which is recent.

Could we get rid of them? Is this only to support older clients? We are not supporting old client on a more recent server, only the opposite. So we could get rid of it...
I'm talking about this:
https://hg.mozilla.org/mozilla-central/annotate/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/devtools/server/actors/script.js#l1903
  onSourceEvent: function (name, source) {
    this.conn.send({
      from: this._parent.actorID,
      type: name,
      source: source.form()
    });

    // For compatibility and debugger still using `newSource` on the thread client,
    // still emit this event here. Clean up in bug 1247084
    if (name === "newSource") {
      this.conn.send({
        from: this.actorID,
        type: name,
        source: source.form()
      });
    }
(Reporter)

Updated

2 years ago
Status: REOPENED → NEW
(Reporter)

Comment 6

2 years ago
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> I'm trying to speedup the browser toolbox, and I see tons of newSource
> messages. Like double-tons of them
> 
> This bug seems to be about that.
> These duplicated messages seems to be introduced in bug 1177279, which is
> recent.
> 
> Could we get rid of them? Is this only to support older clients? We are not
> supporting old client on a more recent server, only the opposite. So we
> could get rid of it...

This bug is about completing the transition to TabActor.  The specific issue you are seeing with duplicated events seems like a side issue, so let's move it to a different bug.  I filed bug 1269919.
Blocks: 1402832
You need to log in before you can comment on or make changes to this bug.