Closed Bug 1094203 Opened 10 years ago Closed 10 years ago

Enable tools only if related actors or traits are specified

Categories

(DevTools :: Framework, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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?
Flags: needinfo?(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`.
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.
Flags: needinfo?(jsantell)
Blocks: 1088247
Attached patch patch v2 (obsolete) — Splinter Review
Allows more than just booleans.
Attachment #8517467 - Attachment is obsolete: true
Attachment #8518190 - Flags: review?(jryans)
Assignee: nobody → poirot.alex
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)
Attachment #8518190 - Flags: review?(jryans) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8a40ada1dc8b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.