Closed
Bug 1418266
Opened 7 years ago
Closed 7 years ago
[dt-onboarding] Onboarding experiment pref should flip devtools.enabled to false
Categories
(DevTools :: General, defect, P3)
Tracking
(firefox58 fixed, firefox59 fixed)
RESOLVED
FIXED
Firefox 59
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
text/x-review-board-request
|
ochameau
:
review+
|
Details |
(deleted),
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Follow up to the patches that landed in Bug 1408339.
I assumed the shield study could flip two preferences at the same time, but this is not the case. It will only be able to set a single preference.
This means that when the onboarding experiment starts for a user, it should start by flipping the devtools.enabled preference to false once.
Furthermore after discussing with :digitarald we decided it would be nice to have two testing groups:
- one using the regular logic to initialize devtools (based on devtools.selfxss.count)
- one forcing users to go through the onboarding flow (even if devtools.selfxss.count > 0)
This means we will change the devtools.onboarding.experiment from being a simple boolean pref to a pref supporting several states:
- off (default)
- on
- force
Assignee | ||
Comment 1•7 years ago
|
||
Interesting configurations to test (and that I tested locally with the patch I am about to submit).
1 - User out of the experiment
Services.prefs.setBoolPref("devtools.enabled", false);
Services.prefs.setCharPref("devtools.onboarding.experiment", "off");
=> onboarding screen: NO
2 - User in the experiment, "regular group", not a DevTools user, first time starting the browser
Services.prefs.setBoolPref("devtools.enabled", true);
Services.prefs.setCharPref("devtools.onboarding.experiment", "on");
Services.prefs.setBoolPref("devtools.onboarding.experiment.flipped", false);
Services.prefs.setIntPref("devtools.selfxss.count", 0);
=> onboarding screen: YES
3 - User in the experiment, "regular group", not a DevTools user but already went through onboarding
Services.prefs.setBoolPref("devtools.enabled", true);
Services.prefs.setCharPref("devtools.onboarding.experiment", "on");
Services.prefs.setBoolPref("devtools.onboarding.experiment.flipped", true);
Services.prefs.setIntPref("devtools.selfxss.count", 0);
=> onboarding screen: NO
4 - User in the experiment, "regular group", flagged as DevTools user, first time starting the browser
Services.prefs.setBoolPref("devtools.enabled", true);
Services.prefs.setCharPref("devtools.onboarding.experiment", "on");
Services.prefs.setBoolPref("devtools.onboarding.experiment.flipped", false);
Services.prefs.setIntPref("devtools.selfxss.count", 5);
=> onboarding screen: NO
5 - User in the experiment, "force group", flagged as DevTools user, first time starting the browser
Services.prefs.setBoolPref("devtools.enabled", true);
Services.prefs.setCharPref("devtools.onboarding.experiment", "force");
Services.prefs.setBoolPref("devtools.onboarding.experiment.flipped", false);
Services.prefs.setIntPref("devtools.selfxss.count", 5);
=> onboarding screen: YES
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
While writing the PHD I felt that it is important to note that changing the experiment flag back; which happens when the experiment ends; should also switch devtools on again (aka resetting everything to the state before the experiment).
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 4•7 years ago
|
||
Yes, that's taken care of by the scenario 1 I mentioned above.
If "devtools.onboarding.experiment" is "off", devtools.enabled will be forced to true on startup.
Flags: needinfo?(jdescottes)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8929404 [details]
Bug 1418266 - update preference behavior for DevTools onboarding experiment;
https://reviewboard.mozilla.org/r/200728/#review206372
Looks good.
Do you plan to remove all that in FF60/61?
This code reminds me e10s rollout add-on:
https://reviewboard.mozilla.org/r/130966/diff/4#index_header
The benefit of putting all the pref logic in the add-on make it easier to strip it off after the experiment.
Attachment #8929404 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8929404 [details]
> Bug 1418266 - update preference behavior for DevTools onboarding experiment;
>
> https://reviewboard.mozilla.org/r/200728/#review206372
>
> Looks good.
>
> Do you plan to remove all that in FF60/61?
> This code reminds me e10s rollout add-on:
> https://reviewboard.mozilla.org/r/130966/diff/4#index_header
> The benefit of putting all the pref logic in the add-on make it easier to
> strip it off after the experiment.
Thanks for the review. Yes all this code should go away as soon as we are done with the experiment. And good point about the addon, that would have been easier to cleanup afterwards.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a3542899092
update preference behavior for DevTools onboarding experiment;r=ochameau
Comment 8•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Assignee | ||
Comment 9•7 years ago
|
||
Merged all patches from this bug and Bug 1408339 to have a single beta patch containing all preferences and probes needed for the devtools onboarding experiment.
Assignee | ||
Comment 10•7 years ago
|
||
Additional tests for the telemetry probe:
- Run the following code in the browser toolbox:
Services.prefs.setBoolPref("devtools.onboarding.telemetry.logged", false);
Services.prefs.setIntPref("devtools.selfxss.count", 0);
- Restart firefox
- Go to about:telemetry
- Search for "onboarding"
=> devtools.onboarding.is_devtools_user should be false
Same with
Services.prefs.setBoolPref("devtools.onboarding.telemetry.logged", false);
Services.prefs.setIntPref("devtools.selfxss.count", 5);
=> devtools.onboarding.is_devtools_user should be true
Same with
Services.prefs.setBoolPref("devtools.onboarding.telemetry.logged", true);
Services.prefs.setIntPref("devtools.selfxss.count", 5);
=> devtools.onboarding.is_devtools_user should not be present
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8932518 [details] [diff] [review]
bug-1418266-beta.patch
Approval Request Comment
[Feature/Bug causing the regression]: run a shield study in beta (See Bug 1408339)
[User impact if declined]: will need to wait for beta 59 to run the study
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet but has been on Nightly for 1 week
[Needs manual test from QE? If yes, steps to reproduce]: I detailed steps to test all the patches here in comment 1 and comment 10
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: most of the new code is deactivated by default and will only be enabled for a batch of users. the rest is only logging a new telemetry probe.
[String changes made/needed]: none
Attachment #8932518 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 12•7 years ago
|
||
Comment 13•7 years ago
|
||
Comment on attachment 8932518 [details] [diff] [review]
bug-1418266-beta.patch
Support devtools onboarding shield study. Beta58+.
Attachment #8932518 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
status-firefox58:
--- → affected
Comment 14•7 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•