1591973 - [meta] Improve patterns used for DevTools actor traits

Status: NEW

(DevTools :: General, task)

Description

[Julian Descottes [:jdescottes]]

Several DevTools actors define traits, which act as a flags.

Most of the time they are created when a new API is added on the actor. This way the client can check if the actor has the new method or not, and fallback properly in case the actor is old (backward compatibility).

Sometimes they are also use for feature detection regardless of backward compatibility concern. For instance the root actor has the networkMonitor trait set to true, while the content process target actor has it set to false.

The goal of this meta is to improve the current usage of traits in DevTools codebase. Below are a few suggestions / issue descriptions.

1 - Add a consistent API

Generally speaking, traits are returned in the form of an actor, most often in form.traits. When the client needs to read traits there is not a single API today:

• target-mixin defines a handy getTrait helper which reads traits from the corresponding target actor .... as well as traits from the root actor (which is why contentProcessTargetActor can "override" the networkMonitor trait defined on the root actor
• some fronts assign form.traits on this.traits
• some fronts just read this._form.traits directly (because they save form on this._form)

TLDR; it's quite messy and before you add a new trait, you might have to add the traits to the actor's form as well as doing some plumbing on the front so that you can read your trait.

It would be nice if this was a built in feature of actors and fronts and if adding traits was easy to do.

2 - Provide default traits for supported methods

Say you added a new method on the actor, called "someMethod". Since the client side always sees "up to date" specs, the front can't simply check super.someMethod, because thanks to the spec, this method will always be created. That's why 99% of the time we create a trait which has only one purpose: say that the actor implements "someMethod".

For this very simple scenario, it would be nice if by default the traits could list the supported methods of the actor, without having to manually create traits.

3 - Automatically detect unused traits

We have a tendency to pile up traits, and we don't clean up old traits after removing all the consumer code. If we had consistent patterns for reading traits on the client getTrait("hasSomeFeature") and if we imposed all traits to have a unique name (maybe by declaring them in a shared constant file?), we could add an automated test checking that traits are still used (and if not, suggest to remove them!)

4 - Differenciate Backward compatibility and Feature detection

When the consumer code is still available in the client side, we might still want to remove a trait if the JSDoc mentions it was for it was added in an old Firefox version. However we need to be careful that some traits can be used for feature detection as well. I don't have a good solution for this. I think each trait should be accompanied with a good comment to explain why the trait was added in the first place. But a trait added for backward compatibility might later be used for feature detection or vice versa, so I think this is something we will always have to be careful around

Comment 1

[Julian Descottes [:jdescottes]]

2 - Provide default traits for supported methods

Say you added a new method on the actor, called "someMethod". Since the client side always sees "up to date" specs,
the front can't simply check super.someMethod, because thanks to the spec, this method will always be created.
That's why 99% of the time we create a trait which has only one purpose: say that the actor implements "someMethod".

For this very simple scenario, it would be nice if by default the traits could list the supported methods of the actor,
without having to manually create traits.

Reading the patch from Micah at https://phabricator.services.mozilla.com/D52754, I was reminded that we already have target.actorHasMethod(actorName, methodName) that does this. The current implementation is a bit weird in the sense that the actor must have been loaded in order to return the correct value, but if we could just expose a similar API on the actor directly, that would satisfy this requirement.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The meta keyword is there, the bug doesn't depend on other bugs and there is no activity for 12 months.
:Honza, maybe it's time to close this bug?

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Julian, can you please set priority and severity or close if not valid anymore, thanks.

Honza

Comment 4

[Julian Descottes [:jdescottes]]

I think this is still needed. We just fixed https://bugzilla.mozilla.org/show_bug.cgi?id=1653926 which is closely related and traits are still messy. Cf section about traits in https://docs.firefox-dev.tools/backend/backward-compatibility.html

Items 3 & 4 from the summary will be somewhat mitigated by https://bugzilla.mozilla.org/show_bug.cgi?id=1673535

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #3)

Julian, can you please set priority and severity or close if not valid anymore, thanks.

Should we set priorities for meta bugs? The ni? was triggered because we don't have blocking bugs for this meta. I will file some

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

You are right, priority not needed (I didn't notice this is a meta)

The ni? was triggered because we don't have blocking bugs for this meta. I will file some

Thanks!

Honza