1522573 - Better align target front class and their base target class

Status: RESOLVED DUPLICATE of bug 1740292

(DevTools :: Framework, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

This bug is to followup on many comments from Julian from bug 1503628 review, most of them are about how to better align these classes (target fronts) with their base class (Target):
https://phabricator.services.mozilla.com/D15831

• Subclasses seem to consistently implement:
  • attach
  • detach (missing in content-process for now)
  • attachThread
  • reconfigure
    It would be nice to either have the information in the description of the mixin or (better in my opinion) add abstract implementations in the mixin eg

attach() {
  throw new Error("attach should be overridden by the TargetMixin subclass");
}

We also expect this.targetForm to be set by the subclass. So similarly a get targetForm() { throw ... } could be nice here.

• follow up: we should consistently use this._tab in the mixin

• follow up: Related to my other comment (about adding abstract methods) why not implement an empty detach in content-process.js, explaining why it is not supported there?

• We discussed about cleaning this _attach already, but I didn't realize it was cleaned up here. It is slightly weird to clear it here since it's only created in the subclasses. I would prefer cleaning it in the destroy() of each target front, but none of them have a destroy at the moment. So maybe simply a comment pointing out that all subclasses are creating this._attach in their own attach() method?

• the relationship between destroy and detach is still confusing for me.
  Some examples:

  • target-mixin destroy calls detach in some situations
  • worker-target-front detach calls destroy (which calls target-mixin destroy, which sometimes call detach, but not in this case)
    Overall, if it doesn't make sense to keep detached target fronts around, should we try to merge detach inside of destroy?

Comment 1

[Chris Peterson [:cpeterson]]

Tracking Fission DevTools bugs for Fission Nightly (M6)

Comment 2

[Chris Peterson [:cpeterson]]

dt-fission-reserve bugs do not need to block Fission Nightly (M6).

Comment 3

[Chris Peterson [:cpeterson]]

Tracking dt-fission-reserve bugs for Fission MVP until we know whether they really need to block Fission or not.

Comment 4

[Chris Peterson [:cpeterson]]

Moving old "dt-fission-reserve" bugs to "dt-fission-future" because they don't block Fission MVP.

Comment 5

[Alexandre Poirot [:ochameau]]

I believe bug 1740292 drastically simplified this issues.