Today we still have various condition where we whitelist/blacklist by target type "target.isSomething". We end up spreading our tool behavior in various places or our codebase because of that. Ideally tools would be enabled if the related actor is available and if the feature isn't related to a whole actor being available or not, we should just check for some traits.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=14060d68c43f Ryan, So I tried to do what you suggested on irc. Looks for some traits on the target's form and then fallback to root actor traits. I think it's a good idea, I just had issues with the addon target's form. Today we hack its form in connect.js and toolbox-window-process.js. Instead of just passing the server side form, we convert it. It would be better to just pass it as-is and tweak the code we wanted to workaround by doing this form convertion. Tell me if you want me to split the patch. I think we can pull off the form hack removal for addon target.

How does that play with your recent work on feature detection?

Comment on attachment 8517467 [details] [diff] [review] patch Review of attachment 8517467 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/target.js @@ +282,5 @@ > } > + > + // If the targeted actor exposes traits and has a defined value for this traits, > + // overload the root actor traits > + if (this.form.traits && typeof(this.form.traits[traitName]) == "boolean") { Traits can be other objects too, like a string or an array, right? Maybe check if it is defined with `in`, or that it `this.form.traits[traitName] != null` or just strictly check for `undefined`.

Looks good -- I wasn't aware of the hacky stuff done for debugging addons compared to other forms, but this looks like it's using those feature detection methods well. If addon target can have those actors on it, then we no longer need the !isAddon check for tool compatibility.

Allows more than just booleans.

Comment on attachment 8518190 [details] [diff] [review] patch v2 Review of attachment 8518190 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/target.js @@ +281,5 @@ > throw new Error("TabTarget#getTrait() can only be called on remote tabs."); > } > + > + // If the targeted actor exposes traits and has a defined value for this traits, > + // overload the root actor traits Nit: override ::: browser/devtools/framework/toolbox.js @@ +53,5 @@ > // (By default, supported target is only local tab) > const ToolboxButtons = [ > { id: "command-button-pick", > + isTargetSupported: target => ( > + target.getTrait("highlightable") This is a odd style to me... Multi-line arrow function without explicit return keyword. Seems easy to confuse "(" with "{" visually, which would need a return AIUI. ::: browser/devtools/main.js @@ +361,5 @@ > inMenu: true, > > isTargetSupported: function(target) { > return target.isLocalTab || > + target.hasActor("storage") && Let's keep the part after || in parentheses for easier reading (I assume that's the intended meaning)

Addressed comment 7. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=558ba6bbc641