Closed Bug 1168563 Opened 5 years ago Closed 4 years ago

Make "Reset and enable full DevTools" into a toggle

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+, b2g-master verified)

VERIFIED FIXED
2.2 S14 (12june)
blocking-b2g 2.5+
Tracking Status
b2g-master --- verified

People

(Reporter: drs, Assigned: yzen)

References

Details

(Whiteboard: [spark])

Attachments

(3 files, 1 obsolete file)

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."
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.
blocking-b2g: --- → spark+
Flags: needinfo?(yzenevich)
Flags: needinfo?(fabrice)
Flags: needinfo?(arthur.chen)
Blocks: spark-dev-mode
No longer blocks: spark
Whiteboard: [spark]
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Flags: needinfo?(yzenevich)
Flags: needinfo?(fabrice)
Flags: needinfo?(arthur.chen)
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.
Flags: needinfo?(ptheriault)
Adding ni? for Fabrice, since he mentioned he would comment on the bug.
Flags: needinfo?(fabrice)
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.
Flags: needinfo?(fabrice)
Attached patch 1168563 patch for Gecko (obsolete) — Splinter Review
Enabling resetting of prefs and settings if factory reset is wither wipe or normal.
Attachment #8613019 - Flags: review?(fabrice)
Comment on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

Depends on the gecko patch landing first.
Attachment #8613568 - Flags: review?(arthur.chen)
(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 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.
Attachment #8613019 - Flags: review?(fabrice) → feedback+
Depends on: 1170263
Comment on attachment 8613019 [details] [diff] [review]
1168563 patch for Gecko

Moved this patch with comments addressed into its own bug 1170263.
Attachment #8613019 - Attachment is obsolete: true
(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 on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

Please check my comments in github, thanks!
Attachment #8613568 - Flags: review?(arthur.chen)
Comment on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

Took care of the comments, Thanks!
Attachment #8613568 - Flags: review?(arthur.chen)
Comment on attachment 8613568 [details] [review]
[gaia] yzen:bug-1168563 > mozilla-b2g:master

r=me with a few nits addressed, thanks!
Attachment #8613568 - Flags: review?(arthur.chen) → review+
https://github.com/mozilla-b2g/gaia/commit/ff80db87926a5c2769e158801090465b4ed117fa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Yura, can you confirm if the old strings are still used (reset-devtools*)? If they're not, why are we keeping them around?
Flags: needinfo?(yzenevich)
Status: RESOLVED → REOPENED
Flags: needinfo?(yzenevich)
Resolution: FIXED → ---
Attachment #8615303 - Flags: review?(arthur.chen)
Attachment #8615303 - Flags: review?(arthur.chen) → review+
https://github.com/mozilla-b2g/gaia/commit/5f72017b3c3ece3797c115f1f11f5df94826de12
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Flags: needinfo?(ptheriault)
Duplicate of this bug: 1124345
blocking-b2g: spark+ → 2.5+
Target Milestone: --- → 2.2 S14 (12june)
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
QA Whiteboard: [QAnalyst-Triage?][failed-verification]
Flags: needinfo?(yzenevich)
Flags: needinfo?(ktucker)
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.
Flags: needinfo?(yzenevich)
QA Whiteboard: [QAnalyst-Triage?][failed-verification] → [QAnalyst-Triage+][failed-verification]
Flags: needinfo?(ktucker) → needinfo?(pcheng)
Just landed fix for bug 1184806, would you be able to see if the issue in comment 20 is resolved?
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?
Flags: needinfo?(pcheng) → needinfo?(yzenevich)
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?
Flags: needinfo?(yzenevich) → needinfo?(pcheng)
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+][failed-verification] → [QAnalyst-Triage?]
Flags: needinfo?(pcheng) → needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
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?
Flags: needinfo?(yzenevich)
(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
Flags: needinfo?(yzenevich)
You need to log in before you can comment on or make changes to this bug.