Bugzilla

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?

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?