Closed Bug 1405166 Opened 2 years ago Closed 2 years ago

Remove system-disabled pref from Screenshots

Categories

(Firefox :: Screenshots, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: _6a68, Assigned: _6a68)

Details

Attachments

(2 files)

No version update for this, we're going to import several small bugs (in parallel) and then bump the version.

This is a fix for this bug: https://github.com/mozilla-services/screenshots/issues/3549

This is an export of this PR: https://github.com/mozilla-services/screenshots/pull/3554
Comment on attachment 8914814 [details]
Bug 1405166 - Remove system-disabled pref from Screenshots;

https://reviewboard.mozilla.org/r/186094/#review191194

There are still a bunch of in-tree references to this preference:

http://searchfox.org/mozilla-central/search?q=screenshots.system-disabled&case=true&path=

Please remove them as well.

Thanks
Attachment #8914814 - Flags: review?(kmaglione+bmo)
Comment on attachment 8914814 [details]
Bug 1405166 - Remove system-disabled pref from Screenshots;

Updated. Mind taking another look?

Unit tests are passing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df879bfb25229628fd8370dd30ccf969dcae5d9
Attachment #8914814 - Flags: review?(kmaglione+bmo)
Comment on attachment 8914966 [details]
Bug 1405166 - Update occurrences of system-disabled pref outside screenshots system addon;

https://reviewboard.mozilla.org/r/186226/#review191938
Attachment #8914966 - Flags: review+
Comment on attachment 8914814 [details]
Bug 1405166 - Remove system-disabled pref from Screenshots;

https://reviewboard.mozilla.org/r/186094/#review191940
Attachment #8914814 - Flags: review?(kmaglione+bmo) → review+
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 2 changesets with 6 changes to 6 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 82078bee1e89 needs "Bug N" or "No bug" in the commit message.
remote: Jared Hirsch <ohai@6a68.net>
remote: Update occurrences of system-disabled pref outside screenshots system addon r=kmag
remote: 
remote: MozReview-Commit-ID: 24PV53VM6te
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
Pushed by jhirsch@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77d4d9eb87cd
Remove system-disabled pref from Screenshots; r=kmag
https://hg.mozilla.org/integration/autoland/rev/d15f945998db
Update occurrences of system-disabled pref outside screenshots system addon; r=kmag
This patch removes the `extensions.screenshots.system-disabled` pref that was being monitored by Telemetry. We're aiming to uplift to 57. Anything that needs to happen on the Telemetry side?
Flags: needinfo?(gfritzsche)
Comment on attachment 8914814 [details]
Bug 1405166 - Remove system-disabled pref from Screenshots;

Approval Request Comment
[Feature/Bug causing the regression]:

Screenshots rollout was controlled by a shield study that flipped a pref. For users to exit the shield study without losing screenshots, we need this patch, which makes Screenshots ignore the pref.

[User impact if declined]:

Without uplift, users will remain in the shield study until this patch rides the trains to 58. Users would only lose screenshots if an unknown error were to occur with the shield recipe.

[Is this code covered by automated tests?]:

No

[Has the fix been verified in Nightly?]:

No

[Needs manual test from QE? If yes, steps to reproduce]: 

No

[List of other uplifts needed for the feature/fix]:

Both commits attached to bug 1405166

[Is the change risky?]:

Not risky

[Why is the change risky/not risky?]:

This patch removes a pref that was used to gradually roll out screenshots. Its removal ensures screenshots stays enabled for the release audience.

[String changes made/needed]:

None
Attachment #8914814 - Flags: approval-mozilla-beta?
Assignee: nobody → jhirsch
Status: NEW → ASSIGNED
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #10)
> This patch removes the `extensions.screenshots.system-disabled` pref that
> was being monitored by Telemetry. We're aiming to uplift to 57. Anything
> that needs to happen on the Telemetry side?

No, that's all that is required.
Mark, is there anything needed on the pipeline side to remove user prefs from datasets like longitudinal?
Flags: needinfo?(gfritzsche) → needinfo?(mreid)
I don't think this will impact any of the existing datasets - in Longitudinal, prefs are stored as a string:string map, and in main_summary we don't have this pref at all.
Flags: needinfo?(mreid)
https://hg.mozilla.org/mozilla-central/rev/77d4d9eb87cd
https://hg.mozilla.org/mozilla-central/rev/d15f945998db
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8914814 [details]
Bug 1405166 - Remove system-disabled pref from Screenshots;

must fix, Beta57+
Attachment #8914814 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This reenabled the Screenshots thingie. It would have been nicer if this new extensions.screenshots.disabled pref was initialized to the value of the old extensions.screenshots.system-disabled
Reproduced the initial behavior using  57.0a1 (2017-08-03). 
57.0b7 build1 (20171009192146) and 58.0a1 (2017-10-10) builds are verified fixed, using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.