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)

Product:
DevTools
Component:
General
Version:
58 Branch
Type:
defect

Tracking

(firefox58 fixed, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1418266 - update preference behavior for DevTools onboarding experiment;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
bug-1418266-beta.patch
7.24 KB, 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
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
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)
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 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+
(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
https://hg.mozilla.org/mozilla-central/rev/4a3542899092
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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.
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
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?
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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: