Closed Bug 1408337 Opened 7 years ago Closed 7 years ago

[dt-onboarding] Decide rules to initialize devtools.enabled

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1408337 - set devtools.enabled to true if toolbox was recently opened;
59 bytes, text/x-review-board-request
bgrins
: review+
Details
We are landing a first version of the devtools onboarding page in bug 1361080. The screen is only displayed if the preference devtools.enabled is set to false. For now this preference is always true. In the future we aim to default the preference to true on some channels (beta & release). However users who already used devtools should still bypass this onboarding flow. We need to define the set of preferences (or other information accessible at runtime) that will force devtools.enabled to true.
Blocks: dt-onboarding
No longer blocks: dt-addon-uishim
We should be able to use the following pref: devtools.telemetry.tools.opened.version It is set when logging telemetry probes that need to be set only once per version, during the toolbox opening [1]. If this preference has been modified, then the toolbox has been opened at least once. It is a pretty old pref so all profiles that used devtools should normally have it, even if they are pretty old. We could add a dedicated preference to make this more obvious (devtools.toolbox.opened ?), but for existing profiles this preference seems like a good candidate. [1] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/devtools/client/framework/toolbox.js#493
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8923842 [details] Bug 1408337 - set devtools.enabled to true if toolbox was recently opened; Brian: I think this patch should allow us to correctly initialize devtools.enabled for existing profiles. Let me know what you think.
Attachment #8923842 - Flags: feedback?(bgrinstead)
Comment on attachment 8923842 [details] Bug 1408337 - set devtools.enabled to true if toolbox was recently opened; https://reviewboard.mozilla.org/r/195004/#review200046 ::: devtools/shim/devtools-startup.js:39 (Diff revision 1) > const DEVTOOLS_ENABLED_PREF = "devtools.enabled"; > > +// This is a flag set to true when the toolbox opens. Added in Firefox 58. > +const TOOLBOX_ALREADY_OPENED_PREF = "devtools.toolbox.opened.flag"; > + > +// For profiles migrating from a Firefox older than 58, we check if the toolbox was opened I think I'd prefer if we moved this into a UI migration by bumping UI_VERSION and adding a new check at the bottom of migrateUI that sets "devtools.toolbox.opened.flag" to true if "devtools.telemetry.tools.opened.version" has a user value: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1743. Then we can assume that the new pref is always accurate and don't need the extra code + comment here.
Attachment #8923842 - Flags: feedback?(bgrinstead) → feedback+
Thanks for the feedback Brian! Adding this to the beta study blockers. For the study we want to reuse existing preferences to decide if a user is an existing DevTools user. Compared to what I did in my patch, we will probably consider that the user is a devtools user only if devtools.telemetry.tools.opened.version indicates that devtools were opened in the last version of firefox. People that haven't used devtools for more than a release would be flagged as non devtools users. Later we would like to have a better preference measuring the user engagement. This will be tracked in a separate bug.
Blocks: 1408339
See Also: → 1414005
Before re-requesting review, let's check we are okay with the approach. Now the logic is: - reads the version of the last toolbox open from our preferences [1] - compare it to the current major version - if delta is greater than 1, the user is not considered as a devtools user - if delta is 1 or lower, the user is considered a devtools user For the beta 58 experiment, this means that a user who opened devtools in 57 or 58 will be flagged as a devtools user. A user who opened it in 56 or earlier (or never) will not. [1] we add a new preference devtools.opened.version to keep track of this. It gets initialized from the existing telemetry preference if needed.
Flags: needinfo?(hkirschner)
Comment on attachment 8923842 [details] Bug 1408337 - set devtools.enabled to true if toolbox was recently opened; https://reviewboard.mozilla.org/r/195004/#review202426 ::: devtools/shim/devtools-startup.js:455 (Diff revision 3) > + */ > + getOpenedVersionFallback() { > + let pref = Services.prefs.getCharPref("devtools.telemetry.tools.opened.version"); > + // The preference holds a stringified JSON object. Keys are telemetry probe names, > + // values are the last FF version where the telemetry probe was saved. > + let obj = JSON.parse(pref); I'd be defensive here and try/catch. We don't want isDevToolsUser to throw and we don't know what will be in that pref ::: devtools/shim/devtools-startup.js:507 (Diff revision 3) > } > > let hasToolbarPref = Services.prefs.getBoolPref(TOOLBAR_VISIBLE_PREF, false); > - if (hasDevToolsFlag || hasToolbarPref) { > + let isDevToolsUser = this.isDevToolsUser(); > + > + if (hasDevToolsFlag || hasToolbarPref || isDevToolsUser) { Do we want to run this always or can it be guarded only when needed i.e. `hasDevToolsFlag || hasToolbarPref || this.isDevToolsUser()` so that we can avoid this extra work and pref migration? It seems like that would be OK since even though we would skip the pref writing due to `!lastOpenVersion` the version would be set (correctly) once the toolbox opened.
Attachment #8923842 - Flags: review?(bgrinstead)
Comment on attachment 8923842 [details] Bug 1408337 - set devtools.enabled to true if toolbox was recently opened; https://reviewboard.mozilla.org/r/195004/#review202426 Thanks for the review. Changeset updated. > I'd be defensive here and try/catch. We don't want isDevToolsUser to throw and we don't know what will be in that pref Good point! Fixed. > Do we want to run this always or can it be guarded only when needed i.e. `hasDevToolsFlag || hasToolbarPref || this.isDevToolsUser()` so that we can avoid this extra work and pref migration? > > It seems like that would be OK since even though we would skip the pref writing due to `!lastOpenVersion` the version would be set (correctly) once the toolbox opened. Good point, let's guard it. I fear that most of the time hasDevToolsFlag and hasToolbarPref will be false though and we will go into the method. But that still doesn't hurt. For now this is fine since devtools.enabled is true everywhere, but I will log a follow up to assess the impact of this code on startup for when we switch devtools.enabled to false. I think it would be nice to make sure our initialization code is fast for most of the FF users.
Comment on attachment 8923842 [details] Bug 1408337 - set devtools.enabled to true if toolbox was recently opened; https://reviewboard.mozilla.org/r/195004/#review202516 ::: devtools/shim/devtools-startup.js:493 (Diff revision 4) > + let currentMajorVersion = currentVersion.match(/\d+/)[0]; > + > + lastMajorVersion = parseInt(lastMajorVersion, 10); > + currentMajorVersion = parseInt(currentMajorVersion, 10); > + > + return currentMajorVersion - lastMajorVersion <= PREVIOUS_OPEN_VERSION_GAP; I'm not sure how accurate this needs to be, but what if someone skipped a Firefox update cycle but was still using devtools in the meantime? We'd end up returning false here. Anyway, the code changes seem fine assuming that this measures what we need it to.
Attachment #8923842 - Flags: review?(bgrinstead) → review+
WFM on the preference logic. Maybe as an alternative, what do you think about devtools.selfxss.count? It has the benefit of including only users who pasted into the Console before; which excludes the accidental usage that might lead to the .opened.version pref being set. It might miss some users who just have not used the Console, which I expect to be small though (can be tested as hypothesis in the experiment). On the data side, we are flying blind right now if we experiment as we don’t know how many users we disable/enable devtools for. We also need to land another telemetry for how many profiles automatically have devtools enabled vs disabled based on the heuristic.
Flags: needinfo?(hkirschner)
I'm fine with using the selfxss pref. The implementation will be simpler and I think we will force devtools.enabled to true for less users. I'll add a probe to measure how many users get flagged as devtools-users vs non devtools users.
(In reply to :Harald Kirschner :digitarald from comment #13) > WFM on the preference logic. Maybe as an alternative, what do you think > about devtools.selfxss.count? It has the benefit of including only users who > pasted into the Console before; It's actually a bit wider than that. The preference is a counter that increases every time you use the console. If it reaches 5 then the self-xss protection goes away. So this will catch all the users who: - either pasted in the console, and wrote "allow pasting" - or used the console 5 times
(In reply to :Harald Kirschner :digitarald from comment #13) > > On the data side, we are flying blind right now if we experiment as we don’t > know how many users we disable/enable devtools for. We also need to land > another telemetry for how many profiles automatically have devtools enabled > vs disabled based on the heuristic. Harald: Quick question about the probe. Do we want to get the data for any user, or only for users in the onboarding experiment?
Flags: needinfo?(hkirschner)
In the last patch I am no longer creating a new preference to hold represent "is this a devtools user". I feel like this is really just a temporary measure for the experiment, that should only live until we define a really good pref to measure a user's engagement (Bug 1414005)
Comment on attachment 8923842 [details] Bug 1408337 - set devtools.enabled to true if toolbox was recently opened; Sorry I meant to flag for review again!
Attachment #8923842 - Flags: review+ → review?(bgrinstead)
Comment on attachment 8923842 [details] Bug 1408337 - set devtools.enabled to true if toolbox was recently opened; https://reviewboard.mozilla.org/r/195004/#review203428 Works for me - much simpler
Attachment #8923842 - Flags: review?(bgrinstead) → review+
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4222ef3c65ac set devtools.enabled to true if toolbox was recently opened;r=bgrins
https://hg.mozilla.org/mozilla-central/rev/4222ef3c65ac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
> Harald: Quick question about the probe. Do we want to get the data for any user, or only for users in the onboarding experiment? Sorry, thought I replied here. All users!
Flags: needinfo?(hkirschner)
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: