1094203 - Enable tools only if related actors or traits are specified

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch

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.

Comment 2

[Paul Rouget [:paul]]

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

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

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`.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Allows more than just booleans.

Comment 6

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=50951c925112

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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)

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Addressed comment 7.

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/8a40ada1dc8b

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a40ada1dc8b