1168563 - Make "Reset and enable full DevTools" into a toggle

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect, P1)

Description

[Doug Sherk (:drs) (inactive)]

See bug 1163889 comment 7. Currently, the "Reset and enable full DevTools" button in the Settings app can only enable all of the dev tools prefs and settings. We should make it a toggle that disables the prefs and settings if they're already enabled. The text should be "Disable full DevTools."

Comment 1

[Doug Sherk (:drs) (inactive)]

This is important because without it, users can't disable developer mode, and are thus stuck with compromised security.

Fabrice, Arthur, Yura, can one of you take this? I'm asking all 3 of you since I don't know the current availability and workload within the Settings team. It looks like you guys are pretty busy.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Both factoryReset operations with 'root' and 'normal' wipe /data partitions. This means that in order to reset from root we still need to explicitly state to the user (via Dialog) what will happen. Hopefully, this is acceptable. ni'ing Paul to confirm.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

Adding ni? for Fabrice, since he mentioned he would comment on the bug.

Comment 4

[[:fabrice] Fabrice Desré]

I have a small objection on using a toggle UI element since it may look like a reversible operation to the user, but it's not since we are wiping the /data partition. I guess it's not a huge deal if we show a dialog to the user each time we change state.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1168563 patch for Gecko

Enabling resetting of prefs and settings if factory reset is wither wipe or normal.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] yzen:bug-1168563 > mozilla-b2g:master

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

Depends on the gecko patch landing first.

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Yura Zenevich [:yzen] from comment #7)
> Comment on attachment 8613568 [details] [review]
> [gaia] yzen:bug-1168563 > mozilla-b2g:master
> 
> Depends on the gecko patch landing first.

Please move the gaia patch to it's build since the rules for closing bugs are different in gecko & gaia land.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8613019 [details] [diff] [review]
1168563 patch for Gecko

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

::: b2g/components/ProcessGlobal.js
@@ +61,5 @@
>        this.logLevel = Ci.nsIConsoleMessage.debug;
>        break;
>    }
>  }
> +function toggleUnrestrictedDevtools(unrestrict) {

nit: call the parameter `unrestricted`

@@ +62,5 @@
>        break;
>    }
>  }
> +function toggleUnrestrictedDevtools(unrestrict) {
> +  Services.prefs.setBoolPref('devtools.debugger.forbid-certified-apps',

All quotes are double quotes in this file, please follow the local style.

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8613019 [details] [diff] [review]
1168563 patch for Gecko

Moved this patch with comments addressed into its own bug 1170263.

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> (In reply to Yura Zenevich [:yzen] from comment #7)
> > Comment on attachment 8613568 [details] [review]
> > [gaia] yzen:bug-1168563 > mozilla-b2g:master
> > 
> > Depends on the gecko patch landing first.
> 
> Please move the gaia patch to it's build since the rules for closing bugs
> are different in gecko & gaia land.

This bug is now Gaia specific, gecko changes are tracked in bug 1170263

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

Please check my comments in github, thanks!

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

Took care of the comments, Thanks!

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

r=me with a few nits addressed, thanks!

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

https://github.com/mozilla-b2g/gaia/commit/ff80db87926a5c2769e158801090465b4ed117fa

Comment 16

[Francesco Lodolo [:flod]]

Yura, can you confirm if the old strings are still used (reset-devtools*)? If they're not, why are we keeping them around?

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Follow up removing unused string.

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

https://github.com/mozilla-b2g/gaia/commit/5f72017b3c3ece3797c115f1f11f5df94826de12

Comment 20

[Pi Wei Cheng [:piwei] (inactive)]

This issue is NOT fixed. The "Factory Reset and Enable Full DevTools" button is now a toggle, however it cannot be toggled off once it is on. The warning message does not seem to match their action either - if I have the toggle already set to ON, tapping on it shows the warning that it will "enable" dev mode which doesn't make sense because it is already on.

STR:
1) Flash to an Aries latest build
2) Go to Settings > Developer > toggle ON 'Factory Reset and Enable Full DevTools
3) After it has been turned on, try to toggle it off

Observe the warning message is different than what was displayed at step 2, however they both warn the user that this will turn on full devtools - in this case it is true because if I proceed to confirm the action, this setting is still ON after it goes through reset.

NI assignee on failed verification.

Tested on:
Device: Aries 2.5
BuildID: 20150803200840
Gaia: dbacf8364f4505d021b7d8fb9cabea325004dbcc
Gecko: abc56d57f6e1
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 42.0a1 (2.5) 
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Comment 21

[Yura Zenevich [:yzen] (please use "needinfo")]

I believe what you are observing is going to be addressed with bug 1184806, there dialogs were mixed up some time in the past and should be working better once 1184806 lands. I'll ni? you after it happens to see if it is fixed.

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Just landed fix for bug 1184806, would you be able to see if the issue in comment 20 is resolved?

Comment 23

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: screenshot of warning when attempting to go from enabled to disabled

Tested with this build: https://tools.taskcluster.net/task-inspector/#2gcUJb8nRimPi473CgnJww/

The toggle now seems to work fine (fine as in I can toggle it on and off), but the warning going from enabled to disabled doesn't seem clear to me? I've attached a screenshot.

From a user point of view it does not tell me that I will no longer have full devtools privileges which seems odd to me. But reading from bug 1184806 comment 8 it seems we're taking out this full devtools feature anyway so I'm not sure if we care about it?

Comment 24

[Yura Zenevich [:yzen] (please use "needinfo")]

Yes soon we will get rid of the toggle. At the moment the dialog when we disable is the same as the one for factory reset in "about" settings section. We can update the wording as well, but i m wondering if it is necessary of it is going to go away before the release. Thoughts?

Comment 25

[Pi Wei Cheng [:piwei] (inactive)]

I'd say we're okay keeping it that way since it is phasing out. But I don't think I'm in a position to make that call.

As for this bug I'm changing it to verified fixed.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

2.5 has branched and the dialog to disable the full devtools privileges still uses the string to enable them (reset-devtools-warning-body vs. unreset-devtools-warning-body). Will this get fixed before release?

Comment 27

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #26)
> 2.5 has branched and the dialog to disable the full devtools privileges
> still uses the string to enable them (reset-devtools-warning-body vs.
> unreset-devtools-warning-body). Will this get fixed before release?

I would suggest opening a specific bug for the issue describing what it is that needs to be fixed and nominating it for 2.5