Turn off tools by default if actor doesn't exist

RESOLVED FIXED in Firefox 35



Developer Tools
3 years ago
3 years ago


(Reporter: jlongster, Assigned: jlongster)


Firefox 36
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)



(2 attachments, 1 obsolete attachment)



3 years ago
In Fever Dream we want to only show tools that are supported. We indicate support by having the actor on our TabActor. It's pretty easy to do this, we just need to add the `target.hasActor('inspector')` check in `isTargetSupported` for each tool.

I just tested this and it appears to work in various scenarios. The tools act the same way as before, but now in Fever Dream only the ones we support are displayed.

I don't know how to disable scratchpad, actually, if we are to do that. There isn't an actor on the backend for it.

Comment 1

3 years ago
Created attachment 8510552 [details] [diff] [review]

I didn't add a check in all the tools because some of them are off by default, or wasn't sure how to do it. For example jsdebugger doesn't have the check because there isn't a backend actor to check, I'd need to check something else. A thread actor is always going to exist.

Also still not sure how to turn of scratchpad.

Also not 100% sure this doesn't break something else, but I tested and it seems fine.
Attachment #8510552 - Flags: review?(jryans)
Comment on attachment 8510552 [details] [diff] [review]

Review of attachment 8510552 [details] [diff] [review]:

To test for debugger, I think you can use a trait like so:

1. Add a new trait with "true" in fx-team's server
2. Set the trait "false" in FD
3. In isTargetSupported, check:

target.getTrait("debugger") !== false

This would allow the debugger for all existing targets, where it would be undefined.

Or you can leave it as-is, and ensure the debugger works well in FD. ;)

Scratchpad appears to work okay here with iOS, but you could also play the above trait game if you wish.

::: browser/devtools/main.js
@@ +198,5 @@
>    inMenu: true,
>    commands: "devtools/styleeditor/styleeditor-commands",
>    isTargetSupported: function(target) {
> +    return target.hasActor('styleEditor') && !target.isAddon;

Looks like[1] "styleEditor" is the old one, so check for this actor OR the "styleSheets" actor.

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleeditor/styleeditor-panel.js#60-66

@@ +339,5 @@
>    inMenu: true,
>    isTargetSupported: function(target) {
>      let root = target.client.mainRoot;
> +    return target.hasActor('monitor') &&

"monitor" is this generic data thing WebIDE / b2g uses, so it's not the Net Monitor.

Net Monitor works via the Web Console actor, so it is harder to detect independently.

However, the trait in another part of this condition was added server-side in March.  We should be able to change this one to:

!target.isAddon && target.getTrait("networkMonitor")

(You can now remove let root line too.)

This trait is already false in FD, so it should just work.
Attachment #8510552 - Flags: review?(jryans) → review+

Comment 3

3 years ago
> "monitor" is this generic data thing WebIDE / b2g uses, so it's not the Net Monitor.

What's really weird then is that the net monitor disappeared when I added that check... Must be a side effect that wasn't caused by what I thought.

Thanks for explaining the other stuff, I'll update my patch!


3 years ago
Assignee: nobody → jlong

Comment 4

3 years ago
Created attachment 8512241 [details] [diff] [review]

Updated with changes from jryan's review (I ignored the debugger, we assume it will always be on for now)
Attachment #8510552 - Attachment is obsolete: true
I believe this should also be uplifted to Aurora.
Blocks: 1054353


3 years ago
Keywords: checkin-needed

Comment 7

3 years ago
(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> I believe this should also be uplifted to Aurora.

Yep, I will request that once it lands to m-c.
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36

Comment 10

3 years ago
Comment on attachment 8512241 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: The toolbox in Dev Edition will show certain broken developer tools when using them on iOS or Chrome targets
[Describe test coverage new/current, TBPL]: It's landed on m-c and all tests are passing. This is a very tiny change and didn't require any new tests (we don't have a way to test out-of-process targets yet, but this is a tiny change anyway)
[Risks and why]: There is a risk that this new logic for hiding/showing specific devtools is wrong for certain targets. If so the user will not see a devtool when it should be there. However this risk is low because this logic is extremely simple and might only go wrong in rare cases, and it will be easy to fix in the future.
[String/UUID change made/needed]:
Attachment #8512241 - Flags: approval-mozilla-aurora?
Attachment #8512241 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
status-firefox35: --- → affected
status-firefox36: --- → fixed
Flags: needinfo?(jlong)
Keywords: branch-patch-needed

Comment 12

3 years ago
Created attachment 8515129 [details] [diff] [review]

Rebased for aurora. Conflict was easy to resolve.
Flags: needinfo?(jlong)

Comment 13

3 years ago
I don't know if there's anything else I need to do besides attach a new patch. Doesn't this mean that the trees will conflict now when they are merged (if 2 different rebased patches were applied to each)?
status-firefox35: affected → fixed
Keywords: branch-patch-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/7228bed65ad0

Sorry, that should have been:

This also blew up mochitest-dt, FWIW.

Comment 17

3 years ago
That's really strange that it busted Aurora so badly. I can't think of a difference between Aurora and m-c that would make this happen. I'll check it out on Monday.

Comment 18

3 years ago
Turns out this depend on bug 1069673 which hasn't been uplifted to Aurora so we need to do that first. I will work on that.
Flags: needinfo?(jlong)
Whoops, didn't mean to queue this one up yet. Backed out again.
status-firefox35: fixed → affected
Depends on: 1094203
You need to log in before you can comment on or make changes to this bug.