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)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(1 file, 2 obsolete files)
15.42 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
How does that play with your recent work on feature detection?
Flags: needinfo?(jsantell)
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
Allows more than just booleans.
Attachment #8517467 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8518190 -
Flags: review?(jryans)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 6•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=50951c925112
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+
Assignee | ||
Comment 8•10 years ago
|
||
Addressed comment 7. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=558ba6bbc641
Attachment #8518190 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8a40ada1dc8b
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 10•10 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•