1088247 - Turn off tools by default if actor doesn't exist

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[James Long (:jlongster)]

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

[James Long (:jlongster)]

Attachment: 1088247.patch

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.

Comment 2

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

Comment on attachment 8510552 [details] [diff] [review]
1088247.patch

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.

Comment 3

[James Long (:jlongster)]

> "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!

Comment 4

[James Long (:jlongster)]

Attachment: 1088247.patch

Updated with changes from jryan's review (I ignored the debugger, we assume it will always be on for now)

Comment 5

[James Long (:jlongster)]

https://tbpl.mozilla.org/?tree=Try&rev=136ef960d5d3

Comment 6

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

I believe this should also be uplifted to Aurora.

Comment 7

[James Long (:jlongster)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e36b555b3140

Comment 10

[James Long (:jlongster)]

Comment on attachment 8512241 [details] [diff] [review]
1088247.patch

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]:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 12

[James Long (:jlongster)]

Attachment: 1088247-aurora.patch

Rebased for aurora. Conflict was easy to resolve.

Comment 13

[James Long (:jlongster)]

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)?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/16b8d9e3ef7c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for permafail.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7228bed65ad0

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=361159&repo=mozilla-aurora

Comment 16

[Ryan VanderMeulen [:RyanVM]]

(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:
https://hg.mozilla.org/releases/mozilla-aurora/rev/26df140c486d

This also blew up mochitest-dt, FWIW.

Comment 17

[James Long (:jlongster)]

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

[James Long (:jlongster)]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0eab3abfb5dc

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Whoops, didn't mean to queue this one up yet. Backed out again.
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e185b59b51e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a079b5be356e

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f761614cd044