1227135 - about:debugging should hide/disable DEBUG buttons if related debugging checkbox/prefs are off

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Alexandre Poirot [:ochameau]]

For now, we still allow clicking on DEBUG even if we know it is going to fail.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: bug1227135.wip.patch

ochameau: I could use some pointers/feedback :) ? My patch works, but I can't settle on a good pattern here.

For now I simply re-render the main React component when one of the observed preferences change. Each "target" element is then responsible for checking if it is enabled or not.

I did this to avoid adding more "preferences" observers. Alternatively, we could have the addons component listening to preference changes ?

I am also not sure if we should pass the preference as a property or just read it from the Service when needed.

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8713701 [details] [diff] [review]
bug1227135.wip.patch

Review of attachment 8713701 [details] [diff] [review]:
-----------------------------------------------------------------

Looks somewhat good, see my notes, it just need some React usages tweaks.
While working on this bug, keep in mind bug 1227137. At the end the checkbox will track two preferences.
(in case you are waiting for a nice "pref react component", this won't necessarely fit this, or be complex)

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +98,5 @@
> +      Services.prefs.addObserver(pref, onPreferenceChanged, false);
> +      this._prefListeners.push([pref, onPreferenceChanged]);
> +
> +      // Initialize the current checkbox element
> +      element.checked = Services.prefs.getBoolPref(pref);

Note that you wouldn't have to duplicate this
  element.checked = Services.prefs.getBoolPref(pref);
if `AboutDebugging` was a React component itself. The pref value would be part of its state and you would call `setState/update` here.
Also you wouldn't have to handle `this _component`.
I already suggested that to :janx. I think we should just go ahead and use React everywhere in about:debugging. Using bare DOM in some place just make the whole thing more complex.
But feel free to apply this patch first. It doesn't make things much worse than it is today. It could just be simplier once we embrace React.

::: devtools/client/aboutdebugging/components/target.js
@@ +59,5 @@
> +  isDebugEnabled() {
> +    let target = this.props.target;
> +    switch (target.type) {
> +      case "extension":
> +        return Services.prefs.getBoolPref("devtools.chrome.enabled");

To be perfect with React abstraction, this should be part of state/props.

::: devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js
@@ +7,5 @@
> +// Test that the buttons are updated dynamically if the preference changes.
> +
> +const ADDON_ID = "[e-mail]";
> +
> +add_task(function* () {

To be really safe, we should ensure setting an explicit preference value (the one we except when starting the test):
yield new Promise(done => {
    let options = {"set": [
                    ["devtools.chrome.enabled", true],
                  ]};
    SpecialPowers.pushPrefEnv(options, done);
  });
(SpecialPowers.pushPrefEnv ensure reseting the pref to the previous value once the test ends)

@@ +8,5 @@
> +
> +const ADDON_ID = "[e-mail]";
> +
> +add_task(function* () {
> +  let { tab, document} = yield openAboutDebugging("addons");

nit: space after `document`

@@ +77,5 @@
> +      resolve();
> +    });
> +    observer.observe(target, { subtree: true, attributes: true });
> +  });
> +}

We already have similar method in other browser_*.js test, like service worker timeout.
Please move that to head.js and us it everywhere.

::: devtools/client/aboutdebugging/test/head.js
@@ +28,5 @@
>      return {
>        tab,
>        document: browser.contentDocument,
> +      window: browser.contentWindow,
> +      browser: browser

I imagine that a leftover?
We shouldn't introduce unused code.

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: bug1227135.v1.patch

Thanks for the feedback!

I agree about switching the whole aboutdebugging UI to React. We discussed it shortly with janx on IRC and I would be happy to help with the React migration of the reminder of the UI. As you said I think we can still land this bug first, as it doesn't really make matters worse.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=80f898782419

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1227135.v1.patch (added reviewer in commit message)

Comment 5

[Jan Keromnes [:janx]]

(In reply to Julian Descottes [:jdescottes] from comment #3)
> I agree about switching the whole aboutdebugging UI to React. We discussed
> it shortly with janx on IRC and I would be happy to help with the React
> migration of the reminder of the UI.

Glad we're all on board with this! I filed bug 1245029 for it, and marked it as blocking this bug because I think it would make the passing down of Debug button state conceptually much cleaner.

Comment 6

[Jan Keromnes [:janx]]

Comment on attachment 8714454 [details] [diff] [review]
bug1227135.v1.patch (added reviewer in commit message)

Review of attachment 8714454 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the drive-by review!

I agree about Alex's general comments about the complexity of handling some things outside of React. That complexity already exists (and it's my fault), but this patch increases it slightly. I don't really mind the order in which we land patches, but it's obvious that this code will change when we go full-React:

If we address bug 1245029, we won't need to handle a `this._component` manually, and we could also remove `this._prefListeners`, the DOM-initializing in `get categories()`, the DOM-manipulations in `showTab()`, and the checkbox + button initializing in `init()`, which would all be huge wins.

Also, great test additions! Thank you for that.

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> While working on this bug, keep in mind bug 1227137. At the end the checkbox
> will track two preferences.
> (in case you are waiting for a nice "pref react component", this won't
> necessarely fit this, or be complex)

I'm not sure that tracking two preferences with one checkbox is the best idea. Why not show two pref-checkboxes? E.g. "[x] Enable chrome & addon debugging" and "[x] Enable remote debugging".

> But feel free to apply this patch first. It doesn't make things much worse
> than it is today. It could just be simplier once we embrace React.

I don't feel too strongly either way, but you'll probably have to rewrite some of this code for bug 1245029.

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +31,5 @@
>    "chrome://devtools/locale/aboutdebugging.properties");
>  
>  var AboutDebugging = {
>    _prefListeners: [],
> +  // current React component

Nit: Please add an empty line before such comments, uppercase the first letter, and add a "." at the end.

@@ +97,5 @@
> +
> +      Services.prefs.addObserver(pref, onPreferenceChanged, false);
> +      this._prefListeners.push([pref, onPreferenceChanged]);
> +
> +      // Initialize the current checkbox element

Nit: Please add a "." at the end.

@@ +121,5 @@
>    },
>  
> +  update() {
> +    if (this._component) {
> +      this._component.setState({});

Nit: This feels hacky. It would be cleaner if the relevant component (e.g. Addons or Workers) listened for its own prefs, then updated itself when needed, rather than being micro-managed by the overall AboutDebugging object.

::: devtools/client/aboutdebugging/components/workers.js
@@ +61,4 @@
>        React.createElement(TargetListComponent, {
>          id: "other-workers",
>          name: Strings.GetStringFromName("otherWorkers"),
> +        targets: workers.other, client, enabled: true })

Nit: Having `enabled: true` here also feels hacky. It looks like most of the time, TargetLists will be enabled. Maybe super-components like Addons and Workers could pass an optional `disabled` prop down to their sub-components? That way we could just omit it here.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8714454 [details] [diff] [review]
bug1227135.v1.patch (added reviewer in commit message)

Review of attachment 8714454 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is here, better land it than waiting for a refactoring.
So that you can free that from your mind and take as much time as you want on the refactor.

::: devtools/client/aboutdebugging/components/addons.js
@@ +45,5 @@
> +      React.createElement(TargetListComponent, {
> +        name,
> +        targets,
> +        client,
> +        enabled: Services.prefs.getBoolPref("devtools.chrome.enabled")

I would name that debugEnabled or something more explicit than just enabled as the target list by itself isn't disabled. It's really just about the DEBUG button.
(Today we miss enable/disable button for addons. If we imagine adding them, they will still be working even while debug is not)

::: devtools/client/aboutdebugging/test/browser_addons_toggle_debug.js
@@ +7,5 @@
> +// Test that the buttons are updated dynamically if the preference changes.
> +
> +const ADDON_ID = "[e-mail]";
> +
> +add_task(function* () {

Please address my previous comment about SpecialPowers.pushPrefEnv.

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Jan Keromnes [:janx] from comment #6)
> (In reply to Alexandre Poirot [:ochameau] from comment #2)
> > While working on this bug, keep in mind bug 1227137. At the end the checkbox
> > will track two preferences.
> > (in case you are waiting for a nice "pref react component", this won't
> > necessarely fit this, or be complex)
> 
> I'm not sure that tracking two preferences with one checkbox is the best
> idea. Why not show two pref-checkboxes? E.g. "[x] Enable chrome & addon
> debugging" and "[x] Enable remote debugging".

Really, no. Users don't care about the implementation details, it should just work!
Having this checkbox is already a failure, debug should work without having to enable it; so we shouldn't make that even more complex.
I would be better to cleanup these prefs to have just one to make addond debugging to work rather than complexifying the UX.

> @@ +121,5 @@
> >    },
> >  
> > +  update() {
> > +    if (this._component) {
> > +      this._component.setState({});
> 
> Nit: This feels hacky. It would be cleaner if the relevant component (e.g.
> Addons or Workers) listened for its own prefs, then updated itself when
> needed, rather than being micro-managed by the overall AboutDebugging object.

Thing is that this pref has to be listened from AboutDebugging for the checkbox.
I don't think we should care about that in this patch. But that's a good comment for the refactoring.

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: bug1227135.v2.patch (addressed review and feedback comments)

Thanks for the reviews! (carrying over r+)
Addressed feedback and review comments.

While I agree the code will be gone after the refactoring, the test should still be valid. Which is why I'd prefer to land it.

@ochameau : I did follow your comment about using SpecialPowers.pushPrefEnv, only it was encapsulated in a helper function at the bottom of the test. I moved it inside the test here for readability. Let me know if I missed something.

Comment 10

[Jan Keromnes [:janx]]

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> > I'm not sure that tracking two preferences with one checkbox is the best
> > idea. Why not show two pref-checkboxes? E.g. "[x] Enable chrome & addon
> > debugging" and "[x] Enable remote debugging".
> 
> Really, no. Users don't care about the implementation details, it should
> just work!
> Having this checkbox is already a failure, debug should work without having
> to enable it; so we shouldn't make that even more complex.
> I would be better to cleanup these prefs to have just one to make addond
> debugging to work rather than complexifying the UX.

Well we do need the two prefs to be enabled for debugging to work, and there aren't too many ways to handle that:

1. Show two pref-checkboxes, "[x] Enable chrome & addon debugging" and "[x] Enable remote debugging". Clearly suggests that you need both prefs (which is the case today). The user is taught about the prefs and can make a choice.

2. Show one magic checkbox to rule them all. A weird hybrid solution, which doesn't teach the user about the two required prefs, but still requires the user to check it for some reason. (Can be somewhat mitigated with a `title` attribute that mentions the two underlying prefs.)

3. Show no checkbox, but if the user clicks on a "Debug" button somewhere, verify that the right prefs are on. If they are not, either notify/prompt the user, or temporarily enable them and restore their state when the user is finished. Most magical solution if we don't prompt the user, but probably bad unexpected behavior under the hood.

Personally, I find Option 1 to be the least terrible solution available.

(In reply to Julian Descottes [:jdescottes] from comment #9)
> While I agree the code will be gone after the refactoring, the test should
> still be valid. Which is why I'd prefer to land it.

Fair enough, works for me.

>     let debugEnabled = true;

Nit: I still think an optional `disableDebugging` prop would be better here (doesn't require any change to components/workers.js).

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> @ochameau : I did follow your comment about using SpecialPowers.pushPrefEnv,
> only it was encapsulated in a helper function at the bottom of the test. I
> moved it inside the test here for readability. Let me know if I missed
> something.

Looks good. Just a nit, often pushPrefEnv is called first in the test.

Comment 12

[Alexandre Poirot [:ochameau]]

(In reply to Jan Keromnes [:janx] from comment #10)
> Well we do need the two prefs to be enabled for debugging to work, and there
> aren't too many ways to handle that:
> 
> 1. Show two pref-checkboxes, "[x] Enable chrome & addon debugging" and "[x]
> Enable remote debugging". Clearly suggests that you need both prefs (which
> is the case today). The user is taught about the prefs and can make a choice.

https://twitter.com/cowbs/status/516045565847535616
Checkbox: Please root my firefox to allow me to work. See <a>more info</a>

> 2. Show one magic checkbox to rule them all. A weird hybrid solution, which
> doesn't teach the user about the two required prefs, but still requires the
> user to check it for some reason. (Can be somewhat mitigated with a `title`
> attribute that mentions the two underlying prefs.)

We do that when we root phones on firefox os via settings app. There we care more about the ease of usage rather than technical correctness. I know it's not technically correct and introduce possible unexpected side effects. It would be better to actually fix the underlying issue with these prefs mess. It is like the browser toolbox prompting for connection. You just set the no-prompt pref as soon as you discover it and then end up unsafe for all your sessions.

Comment 13

[Jan Keromnes [:janx]]

(In reply to Alexandre Poirot [:ochameau] from comment #12)
> https://twitter.com/cowbs/status/516045565847535616
> Checkbox: Please root my firefox to allow me to work. See <a>more info</a>

OK fair enough, the "more info" link is a lot more discoverable than the `title` attribute I suggested in comment 10.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: bug1227135.v3.patch (applied feedback)

Carry over r+.

Moved SpecialPowers.pushPrefEnv to the beginning of the test.

As suggested by :janx, switched from debugEnabled to an optional debugDisabled preference.

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=901da9285f44

Comment 15

[Julian Descottes [:jdescottes]]

Try push looks good. 
Can you please checkin attachment 8714780 [details] [diff] [review] ?

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56432335e159