We should track if people are actually using the dark theme. Need this prior to ship. Joe, can you squeeze this in?

Mike, the question is - how many people dislike the devtools theme? Can we measure how many people turn it off?

Jeff - suppose 10% of people turn it off. What does that mean? Suppose 20%? What then? What number means that people don't like it? What number means we just need to tweak it? My reaction to Axel/Paul not liking it is that we need to make sure people know how to turn it off. i.e. SUMO, or if it's clear that this isn't working, some short text in the devtools options by the theme selector pointing people at the button.

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #2) > Jeff - suppose 10% of people turn it off. What does that mean? Suppose 20%? > What then? > What number means that people don't like it? What number means we just need > to tweak it? Measuring this is due dilligence. I'm not going to speculate on the use of the data until I've seen the data. Having said that, I think large numbers of people going back to Australis would be a sign we need more info on why - is it the darkness or instead the compactness? > My reaction to Axel/Paul not liking it is that we need to make sure people > know how to turn it off. i.e. SUMO, or if it's clear that this isn't > working, some short text in the devtools options by the theme selector > pointing people at the button. We are doing the following to communicate the 'bailout' process: 1. adding text to the updated and firstrun pages linking to a page on MDN 2. the MDN page will describe the 2-step process for bailing out of dev edition ( first the profile pref, then the theme pref ) 3. there will be a screencast that shows the user the specific process for doing this We will not be *highly featuring* the link out on the update / firstrun pages, but they should exist. We should also link to this material from a as-yet-to-be-created MDN page for dev edition. Will and I have a rashly plotted out plan for this.

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #2) > Jeff - suppose 10% of people turn it off. What does that mean? Suppose 20%? > What then? > What number means that people don't like it? What number means we just need > to tweak it? I agree with Jeff that we need to see the numbers before we define exactly how we use the numbers, but one interpretation of the number that I'm particularly keen on watching is how it relates to retention. Let's say that at first, 30% of users turn it off. That's fine, in isolation. Let's further say that at a month, we have 40% attrition[1]. If the attrition group doesn't affect dark-theme users vs. australis users at the expected ratio (roughly 70/30) then we have reason to believe that dark theme is either helping retention (if more australis users leave) or hurting it (if more dark theme users leave). If we find ourselves in that situation, we'll look for more things to measure to confirm that suspicion. [1] Which, for what it's worth, is roughly what the metrics folks told us we can expect.

Easy to add a probe, but what is the current method of disabling the theme? Is that implemented? Just an option in the toolbox options, like dark/light theme for the toolbox?

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #5) > Easy to add a probe, but what is the current method of disabling the theme? > Is that implemented? Just an option in the toolbox options, like dark/light > theme for the toolbox? There is a pref (devedition.theme.enabled), and a button in the customize mode that controls it. Note that it's still up in the air how what will happen when people switch to light devtools theme - we will either be switching to Australis at that point or keeping the dark theme applied. Either way the devedition.theme.enabled pref will probably not be affected by it.

In my opinion we should use the theme that the majority of users like so if 51% of users like the light theme we would default to that.

Try: https://tbpl.mozilla.org/?tree=Try&rev=ad039760f579 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ad039760f579 Not sure whether to land this on nightly, aurora or gum... I am guessing at nightly and gum.

Comment on attachment 8514937 [details] [diff] [review] telemetry-dark-theme-1091796.patch Review of attachment 8514937 [details] [diff] [review]: ----------------------------------------------------------------- LGTM but I'm not a telemetry expert. The code changes look simple enough though.

This is the devedition theme and not just the toolbox theme so I still need to add the other probe.

Asking bgrins for review as he is around for a while yet. It is a very simple patch so it won't take long.

Comment on attachment 8518270 [details] [diff] [review] telemetry-dark-theme-1091796.patch Review of attachment 8518270 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the code changes to browser/devtools/* with a note that this may not be logging the data what you want. I'm not familiar enough with telemetry to review the changes to Histograms.json - it looks pretty simple but you should probably find someone else for that anyway... ::: browser/devtools/framework/toolbox.js @@ +304,5 @@ > > this._telemetry.toolOpened("toolbox"); > > + let toolboxTheme = Services.prefs.getCharPref("devtools.theme"); > + this._telemetry.log(THEME_HISTOGRAM_TOOLBOX, This will only get logs when the toolbox is opened, and will get a new one every time it's opened. So if a user is a frequent toolbox opener, that will skew the theme results pretty heavily. Likewise, if someone used the Web IDE but didn't open the toolbox very frequently, their data wouldn't be logged. It feels like this is something that should go in gDevTools.jsm or browser-devedition.js so that it could be logged only once on browser startup (and on theme change as well if inside of browser-devedition.js). Just a thought though, I haven't caught up on the context of this bug so I'll leave it up to you and Jeff. ::: browser/devtools/shared/test/browser_telemetry_toolbox.js @@ +13,3 @@ > let {Promise: promise} = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {}); > let {Services} = Cu.import("resource://gre/modules/Services.jsm", {}); > +let {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm", {}); This should be imported in head.js, where it is referenced

(In reply to Brian Grinstead [:bgrins] from comment #12) > Comment on attachment 8518270 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8518270 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ on the code changes to browser/devtools/* with a note that this may not > be logging the data what you want. > > I'm not familiar enough with telemetry to review the changes to > Histograms.json - it looks pretty simple but you should probably find > someone else for that anyway... > I am the telemetry guy for our team so I am happy to r=me that change. > ::: browser/devtools/framework/toolbox.js > @@ +304,5 @@ > > > > this._telemetry.toolOpened("toolbox"); > > > > + let toolboxTheme = Services.prefs.getCharPref("devtools.theme"); > > + this._telemetry.log(THEME_HISTOGRAM_TOOLBOX, > > This will only get logs when the toolbox is opened, and will get a new one > every time it's opened. So if a user is a frequent toolbox opener, that > will skew the theme results pretty heavily. Likewise, if someone used the > Web IDE but didn't open the toolbox very frequently, their data wouldn't be > logged. It feels like this is something that should go in gDevTools.jsm or > browser-devedition.js so that it could be logged only once on browser > startup (and on theme change as well if inside of browser-devedition.js). > Yes, that was my original implementation before I realized that thinking is flawed. The problem with only checking at browser startup is that if somebody opens the toolbox or browser prefs and switches the dark themes off we have no indication that they have switched them off until they restart the browser... this gives dark themes a false lead of 100% of users as it is the original theme when they start the browser. Pinging telemetry each time the toolbox opens should give us more accurate figures because the ratio of heavy light users to heavy dark users should remain the same. Of course, dark themes still get a false lead, but that number would become insignificant due to the number of times users re-open the toolbox. I will needinfo Jeff about this. > Just a thought though, I haven't caught up on the context of this bug so > I'll leave it up to you and Jeff. > > ::: browser/devtools/shared/test/browser_telemetry_toolbox.js > @@ +13,3 @@ > > let {Promise: promise} = Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", {}); > > let {Services} = Cu.import("resource://gre/modules/Services.jsm", {}); > > +let {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm", {}); > > This should be imported in head.js, where it is referenced Didn't copy this across... well spotted.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13) > (In reply to Brian Grinstead [:bgrins] from comment #12) > > ::: browser/devtools/framework/toolbox.js > > @@ +304,5 @@ > > > > > > this._telemetry.toolOpened("toolbox"); > > > > > > + let toolboxTheme = Services.prefs.getCharPref("devtools.theme"); > > > + this._telemetry.log(THEME_HISTOGRAM_TOOLBOX, > > > > This will only get logs when the toolbox is opened, and will get a new one > > every time it's opened. So if a user is a frequent toolbox opener, that > > will skew the theme results pretty heavily. Likewise, if someone used the > > Web IDE but didn't open the toolbox very frequently, their data wouldn't be > > logged. It feels like this is something that should go in gDevTools.jsm or > > browser-devedition.js so that it could be logged only once on browser > > startup (and on theme change as well if inside of browser-devedition.js). > > > > Yes, that was my original implementation before I realized that thinking is > flawed. > > The problem with only checking at browser startup is that if somebody opens > the toolbox or browser prefs and switches the dark themes off we have no > indication that they have switched them off until they restart the > browser... this gives dark themes a false lead of 100% of users as it is the > original theme when they start the browser. > > Pinging telemetry each time the toolbox opens should give us more accurate > figures because the ratio of heavy light users to heavy dark users should > remain the same. Of course, dark themes still get a false lead, but that > number would become insignificant due to the number of times users re-open > the toolbox. Jeff, what do you think we should do here?

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #14) ...> > > > > Yes, that was my original implementation before I realized that thinking is > > flawed. > > > > The problem with only checking at browser startup is that if somebody opens > > the toolbox or browser prefs and switches the dark themes off we have no > > indication that they have switched them off until they restart the > > browser... this gives dark themes a false lead of 100% of users as it is the > > original theme when they start the browser. > > > > Pinging telemetry each time the toolbox opens should give us more accurate > > figures because the ratio of heavy light users to heavy dark users should > > remain the same. Of course, dark themes still get a false lead, but that > > number would become insignificant due to the number of times users re-open > > the toolbox. > > Jeff, what do you think we should do here? We're interested in tracking dark theme usage over time, it's no problem if it's spikey at the beginning because as you say it's obvious what the cause is. I want to see if people stick with it.

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13) > (In reply to Brian Grinstead [:bgrins] from comment #12) > > Comment on attachment 8518270 [details] [diff] [review] > > telemetry-dark-theme-1091796.patch > > > > Review of attachment 8518270 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > r+ on the code changes to browser/devtools/* with a note that this may not > > be logging the data what you want. > > > > I'm not familiar enough with telemetry to review the changes to > > Histograms.json - it looks pretty simple but you should probably find > > someone else for that anyway... > > > > I am the telemetry guy for our team so I am happy to r=me that change. > > > ::: browser/devtools/framework/toolbox.js > > @@ +304,5 @@ > > > > > > this._telemetry.toolOpened("toolbox"); > > > > > > + let toolboxTheme = Services.prefs.getCharPref("devtools.theme"); > > > + this._telemetry.log(THEME_HISTOGRAM_TOOLBOX, > > > > This will only get logs when the toolbox is opened, and will get a new one > > every time it's opened. So if a user is a frequent toolbox opener, that > > will skew the theme results pretty heavily. Likewise, if someone used the > > Web IDE but didn't open the toolbox very frequently, their data wouldn't be > > logged. It feels like this is something that should go in gDevTools.jsm or > > browser-devedition.js so that it could be logged only once on browser > > startup (and on theme change as well if inside of browser-devedition.js). > > > > Yes, that was my original implementation before I realized that thinking is > flawed. > > The problem with only checking at browser startup is that if somebody opens > the toolbox or browser prefs and switches the dark themes off we have no > indication that they have switched them off until they restart the > browser... this gives dark themes a false lead of 100% of users as it is the > original theme when they start the browser. > > Pinging telemetry each time the toolbox opens should give us more accurate > figures because the ratio of heavy light users to heavy dark users should > remain the same. Of course, dark themes still get a false lead, but that > number would become insignificant due to the number of times users re-open > the toolbox. > > I will needinfo Jeff about this. We could have a listener for browser.devedition.theme.enabled and devtools.theme, along with checking at startup. This should give the full picture of changes. Technically checking at startup shouldn't even be necessary since we are catching it being disabled on the pref change but it may be a good heuristic for usage. Here are the cases I think we care about: 1) Devedition theme on and uses dark devtools theme 2) Devedition theme on and uses light devtools theme 3) Devedition theme off and uses dark devtools theme 4) Devedition theme off and uses light devtools theme I'm not sure how to best measure how well people 'stick with it' if you mean on an individual level - that would require keeping tabs on when the last time the pref was changed. But if you mean on a more general level - how many people are using each variation of the theme, I think those 4 cases would handle it.

Comment on attachment 8518882 [details] [diff] [review] telemetry-dark-theme-1091796.patch Review of attachment 8518882 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/test/browser_telemetry_toolbox.js @@ +75,5 @@ > return element > 0; > }); > > ok(okay, "All " + histId + " entries have time > 0"); > + } else if (histId === THEME_HISTOGRAM_TOOLBOX) { Do we have any checks that these lines are even called, like pushing all values of `histId` to an array and checking that `histId === THEME_HISTOGRAM_TOOLBOX` and `THEME_HISTOGRAM_BROWSER` 4 times each?

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #18) > Comment on attachment 8518882 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8518882 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/shared/test/browser_telemetry_toolbox.js > @@ +75,5 @@ > > return element > 0; > > }); > > > > ok(okay, "All " + histId + " entries have time > 0"); > > + } else if (histId === THEME_HISTOGRAM_TOOLBOX) { > > Do we have any checks that these lines are even called, like pushing all > values of `histId` to an array and checking that `histId === > THEME_HISTOGRAM_TOOLBOX` and `THEME_HISTOGRAM_BROWSER` 4 times each? We do check that the values are correct but you are right that our telemetry tests should check that the values are set otherwise the checks could be missed.

Just went through it to give it another pair of eyes -- the tests do indeed get those histogram triggers, so maybe we can add that test in the future. Also running through with the patch, the histogram is also triggering and nothing terrible happens. Looks good to land to fx-team and gum, ni? joe on for checkin on gum, `checkin-needed` for fx-team.

Pushed to Gum https://tbpl.mozilla.org/?tree=Gum&rev=2e401372dcf5

(In reply to Brian Grinstead [:bgrins] from comment #17) ... > Here are the cases I think we care about: > > 1) Devedition theme on and uses dark devtools theme > 2) Devedition theme on and uses light devtools theme > 3) Devedition theme off and uses dark devtools theme > 4) Devedition theme off and uses light devtools theme > > I'm not sure how to best measure how well people 'stick with it' if you mean > on an individual level - that would require keeping tabs on when the last > time the pref was changed. But if you mean on a more general level - how > many people are using each variation of the theme, I think those 4 cases > would handle it. Sorry, I just saw this. Tracking these 4 cases does indeed handle what we need to know.

This has failures on Linux x64 ASAN: https://tbpl.mozilla.org/?tree=Gum&rev=2e401372dcf5 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/shared/test/browser_telemetry_toolbox.js | Devtools toolbox theme correctly logged

Pushing the same patch with some more logging to figure out what failed on the Linux x64 ASAN build, and also to see if it's an intermittent. This is on fx-team, but should have same information as gum for this. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2f1d7b741186

Latest try doesn't have the same issue as the one previously -- granted that was on gum, but looks like it was an intermittent, is my guess.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #25) > Latest try doesn't have the same issue as the one previously -- granted that > was on gum, but looks like it was an intermittent, is my guess. It looks like this didn't include linux ASAN build - try to include "linux64-asan" in the platforms for the try commit message.

Woops -- anyway, here's one with just linux ASAN build https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aba80912a9e2

Looks cleared..

(In reply to Jeff Griffiths (:canuckistani) from comment #22) > (In reply to Brian Grinstead [:bgrins] from comment #17) > ... > > Here are the cases I think we care about: > > > > 1) Devedition theme on and uses dark devtools theme > > 2) Devedition theme on and uses light devtools theme > > 3) Devedition theme off and uses dark devtools theme > > 4) Devedition theme off and uses light devtools theme > > > > I'm not sure how to best measure how well people 'stick with it' if you mean > > on an individual level - that would require keeping tabs on when the last > > time the pref was changed. But if you mean on a more general level - how > > many people are using each variation of the theme, I think those 4 cases > > would handle it. > > Sorry, I just saw this. Tracking these 4 cases does indeed handle what we > need to know. I still think that checking what these values are on browser shutdown makes the most sense but since you want us to send these pings when the pref is changed we can do that instead.

> I still think that checking what these values are on browser shutdown makes > the most sense but since you want us to send these pings when the pref is > changed we can do that instead. Curious, why on browser shutdown instead of pref change?

(In reply to Brian Grinstead [:bgrins] from comment #30) > > I still think that checking what these values are on browser shutdown makes > > the most sense but since you want us to send these pings when the pref is > > changed we can do that instead. > > Curious, why on browser shutdown instead of pref change? Because whatever the themes are set to on shutdows shows what the user settled with... using that figure it is very easy to say x% are using a dark theme and y% are using a light theme. I have written a patch that uses pref changes instead but still need to fix some problems with the tests. On the plus side, it seems like we don't need to monkeypatch telemetry.js like we do in our tests as Services.telemetry.histogramSnapshots contains telemetry results... for some reason pings sent from browser.js are not in this collection so I am trying to debug that at the moment.

You have already reviewed most of this but I have added a test, which also needs review.

Comment on attachment 8523028 [details] [diff] [review] telemetry-dark-theme-1091796.patch Review of attachment 8523028 [details] [diff] [review]: ----------------------------------------------------------------- r+ for the stuff in devtools/* with some nits. We should have Gijs review the browser-devedition.js changes. ::: browser/base/content/browser-devedition.js @@ +21,5 @@ > > init: function () { > + let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); > + let Telemetry = devtools.require("devtools/shared/telemetry"); > + this._telemetry = new Telemetry(); Do we also want to log the pref choices at startup or shutdown? ::: browser/devtools/shared/test/browser_telemetry_toolbox.js @@ +6,5 @@ > // Because we need to gather stats for the period of time that a tool has been > // opened we make use of setTimeout() to create tool active times. > const TOOL_DELAY = 200; > > +const DARK_TOOLBOX_THEME_PREF = "devtools.theme"; Nit: rename to TOOLBOX_THEME_PREF @@ +7,5 @@ > // opened we make use of setTimeout() to create tool active times. > const TOOL_DELAY = 200; > > +const DARK_TOOLBOX_THEME_PREF = "devtools.theme"; > +const DARK_BROWSER_THEME_PREF = "browser.devedition.theme.enabled"; Nit: rename this to DEVEDITION_BROWSER_THEME_PREF since it isn't necessarily dark anymore @@ +81,5 @@ > > function finishUp() { > gBrowser.removeCurrentTab(); > > + Services.prefs.setCharPref(DARK_TOOLBOX_THEME_PREF, oldTheme); You could also just clearUserPref here and not have to keep track of oldTheme ::: browser/devtools/shared/test/head.js @@ +155,5 @@ > +XPCOMUtils.defineLazyGetter(this, 'gDeveloperButtonInNavbar', function() { > + return getAreaWidgetIds(CustomizableUI.AREA_NAVBAR).indexOf("developer-button") != -1; > +}); > + > +function getAreaWidgetIds(areaId) { Nit: move this logic inline (or scope the function) to the getter - it doesn't have any other relevance to this file ::: toolkit/components/telemetry/Histograms.json @@ +4886,5 @@ > + }, > + "DEVTOOLS_SELECTED_BROWSER_THEME_BOOLEAN": { > + "expires_in_version": "never", > + "kind": "boolean", > + "description": "Is the devtools browser theme enabled?" "Is the developer edition theme enabled?"

Comment on attachment 8523028 [details] [diff] [review] telemetry-dark-theme-1091796.patch Review of attachment 8523028 [details] [diff] [review]: ----------------------------------------------------------------- So this was really urgent but then we shipped without it? :-\ I don't really understand why this lives in browser-devedition rather than somewhere inside devtools, which would make more sense to me. Assuming there's some reason for that, in principle, the browser-devedition.js change looks fine. Comments about the test, which would probably cause me to set r- if I was more sure about what was going on. I've not looked at the rest. ::: browser/devtools/shared/test/browser_telemetry_toolbox.js @@ +27,5 @@ > info("Toolbox opened"); > > toolbox.once("destroyed", function() { > if (pass++ === 3) { > + switchThemes(); This would be clearer, IMO, if it called switchThemes and then checkResults, and switchThemes didn't call checkResults itself, as checkResults checks things that aren't effected by switchThemes but by other code, and likewise it's not obvious here that switchThemes will go and check results from the stuff happening in here. @@ +40,5 @@ > }).then(null, console.error); > } > > +function switchThemes() { > + for (let theme of ["dark", "light", "dark", "light", "dark", "light", "dark"]) { This is making an assumption about what the default is when this function gets fired initially, isn't it? Or rather, checkResults is. That is, AFAICT the change count (checked in checkResults) is going to be different if the theme starts out as "light" than it is if it starts out as "dark", which means this test is either orange on m-c or will go orange on uplift to either m-a or m-b, depending on what it's currently based on. You can avoid this by (a) having an even number of each themes in this array (not sure why you're doing this 3/4 times rather than 2 times each, btw), and (b) using ary.push(ary.shift()) if the initial item in the list is the same as oldTheme. That would ensure that we switch N times to each color, no matter what the initial color is.

(In reply to :Gijs Kruitbosch from comment #34) > Comment on attachment 8523028 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8523028 [details] [diff] [review]: > ----------------------------------------------------------------- > > So this was really urgent but then we shipped without it? :-\ We had to back it out right before the merge because of a test failure on linux x64 ASAN builds.

(In reply to :Gijs Kruitbosch from comment #34) > Comment on attachment 8523028 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8523028 [details] [diff] [review]: > ----------------------------------------------------------------- > > So this was really urgent but then we shipped without it? :-\ > > I don't really understand why this lives in browser-devedition rather than > somewhere inside devtools, which would make more sense to me. Assuming > there's some reason for that, in principle, the browser-devedition.js change > looks fine. Comments about the test, which would probably cause me to set r- > if I was more sure about what was going on. I've not looked at the rest. > It could also live in gDevTools.jsm, but it was originally in toolbox.js which is reloaded every time a toolbox is opened so I suggested here or gDevTools.jsm - see Comment 12. Here was a bit more convenient since the pref listeners were already there (although as a downside it does have to use the devtools loader to get the telemetry object and also we have to bug a browser peer every time we want to change it).

Well if you are really interested in shy people stop using the developer dark theme it is because if you have "The addon bar (restored)" extension and the forecastfox extensinon installed it is incompatible with this theme under Linux.

So I suspect it might b the same on a MAC which is the platform you asked about. Oddly it is NOT an issue on windows.

(In reply to Brian Grinstead [:bgrins] from comment #36) > (In reply to :Gijs Kruitbosch from comment #34) > > Comment on attachment 8523028 [details] [diff] [review] > > telemetry-dark-theme-1091796.patch > > > > Review of attachment 8523028 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > So this was really urgent but then we shipped without it? :-\ > > > > I don't really understand why this lives in browser-devedition rather than > > somewhere inside devtools, which would make more sense to me. Assuming > > there's some reason for that, in principle, the browser-devedition.js change > > looks fine. Comments about the test, which would probably cause me to set r- > > if I was more sure about what was going on. I've not looked at the rest. > > > > It could also live in gDevTools.jsm, but it was originally in toolbox.js > which is reloaded every time a toolbox is opened so I suggested here or > gDevTools.jsm - see Comment 12. Here was a bit more convenient since the > pref listeners were already there (although as a downside it does have to > use the devtools loader to get the telemetry object and also we have to bug > a browser peer every time we want to change it). To be clear, I now think gDevTools.jsm is a better place to put this because of these points (despite me suggesting both optoins earlier and not having a preference between them). But I'll leave it up to Mike if he wants to change it now, since it's here in the first place on my suggestion.

Addressed review nits

(In reply to Brian Grinstead [:bgrins] from comment #33) > Comment on attachment 8523028 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8523028 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ for the stuff in devtools/* with some nits. We should have Gijs review > the browser-devedition.js changes. > > ::: browser/base/content/browser-devedition.js > @@ +21,5 @@ > > > > init: function () { > > + let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {}); > > + let Telemetry = devtools.require("devtools/shared/telemetry"); > > + this._telemetry = new Telemetry(); > > Do we also want to log the pref choices at startup or shutdown? > Added check to shutdown. See gDevTools._pingTelemetry (lines 489, 518 & 519). > ::: browser/devtools/shared/test/browser_telemetry_toolbox.js > @@ +6,5 @@ > > // Because we need to gather stats for the period of time that a tool has been > > // opened we make use of setTimeout() to create tool active times. > > const TOOL_DELAY = 200; > > > > +const DARK_TOOLBOX_THEME_PREF = "devtools.theme"; > > Nit: rename to TOOLBOX_THEME_PREF > Done > @@ +7,5 @@ > > // opened we make use of setTimeout() to create tool active times. > > const TOOL_DELAY = 200; > > > > +const DARK_TOOLBOX_THEME_PREF = "devtools.theme"; > > +const DARK_BROWSER_THEME_PREF = "browser.devedition.theme.enabled"; > > Nit: rename this to DEVEDITION_BROWSER_THEME_PREF since it isn't necessarily > dark anymore > Done > @@ +81,5 @@ > > > > function finishUp() { > > gBrowser.removeCurrentTab(); > > > > + Services.prefs.setCharPref(DARK_TOOLBOX_THEME_PREF, oldTheme); > > You could also just clearUserPref here and not have to keep track of oldTheme > Done > ::: browser/devtools/shared/test/head.js > @@ +155,5 @@ > > +XPCOMUtils.defineLazyGetter(this, 'gDeveloperButtonInNavbar', function() { > > + return getAreaWidgetIds(CustomizableUI.AREA_NAVBAR).indexOf("developer-button") != -1; > > +}); > > + > > +function getAreaWidgetIds(areaId) { > > Nit: move this logic inline (or scope the function) to the getter - it > doesn't have any other relevance to this file > We don't need this code any longer as I have changed the test to check which themes are applied and act from there. We don't care which browser edition we are using. > ::: toolkit/components/telemetry/Histograms.json > @@ +4886,5 @@ > > + }, > > + "DEVTOOLS_SELECTED_BROWSER_THEME_BOOLEAN": { > > + "expires_in_version": "never", > > + "kind": "boolean", > > + "description": "Is the devtools browser theme enabled?" > > "Is the developer edition theme enabled?" Changed (In reply to :Gijs Kruitbosch from comment #34) > Comment on attachment 8523028 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8523028 [details] [diff] [review]: > ----------------------------------------------------------------- > > So this was really urgent but then we shipped without it? :-\ > Issues with different browser versions. > I don't really understand why this lives in browser-devedition rather than > somewhere inside devtools, which would make more sense to me. Assuming > there's some reason for that, in principle, the browser-devedition.js change > looks fine. Comments about the test, which would probably cause me to set r- > if I was more sure about what was going on. I've not looked at the rest. > It made sense to put it there because the pref observer already existed. Because we need to get a browser peer review when we change things in that file I have moved this code to gDevTools. > ::: browser/devtools/shared/test/browser_telemetry_toolbox.js > @@ +27,5 @@ > > info("Toolbox opened"); > > > > toolbox.once("destroyed", function() { > > if (pass++ === 3) { > > + switchThemes(); > > This would be clearer, IMO, if it called switchThemes and then checkResults, > and switchThemes didn't call checkResults itself, as checkResults checks > things that aren't effected by switchThemes but by other code, and likewise > it's not obvious here that switchThemes will go and check results from the > stuff happening in here. > We now do this as it is clearer: if (pass++ === 3) { switchThemes(); checkResults(); finishUp(); } else { openToolboxFourTimes(); } > @@ +40,5 @@ > > }).then(null, console.error); > > } > > > > +function switchThemes() { > > + for (let theme of ["dark", "light", "dark", "light", "dark", "light", "dark"]) { > > This is making an assumption about what the default is when this function > gets fired initially, isn't it? Or rather, checkResults is. > > That is, AFAICT the change count (checked in checkResults) is going to be > different if the theme starts out as "light" than it is if it starts out as > "dark", which means this test is either orange on m-c or will go orange on > uplift to either m-a or m-b, depending on what it's currently based on. > > You can avoid this by (a) having an even number of each themes in this array > (not sure why you're doing this 3/4 times rather than 2 times each, btw), > and (b) using ary.push(ary.shift()) if the initial item in the list is the > same as oldTheme. That would ensure that we switch N times to each color, no > matter what the initial color is. It actually did work because checkResults checked a different result when using dev edition. Anyhow, I have changed the test to check which themes are applied and act from there. We don't care which browser edition we are using. (In reply to Brian Grinstead [:bgrins] from comment #39) > (In reply to Brian Grinstead [:bgrins] from comment #36) > > (In reply to :Gijs Kruitbosch from comment #34) > > > Comment on attachment 8523028 [details] [diff] [review] > > > telemetry-dark-theme-1091796.patch > > > > > > Review of attachment 8523028 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > So this was really urgent but then we shipped without it? :-\ > > > > > > I don't really understand why this lives in browser-devedition rather than > > > somewhere inside devtools, which would make more sense to me. Assuming > > > there's some reason for that, in principle, the browser-devedition.js change > > > looks fine. Comments about the test, which would probably cause me to set r- > > > if I was more sure about what was going on. I've not looked at the rest. > > > > > > > It could also live in gDevTools.jsm, but it was originally in toolbox.js > > which is reloaded every time a toolbox is opened so I suggested here or > > gDevTools.jsm - see Comment 12. Here was a bit more convenient since the > > pref listeners were already there (although as a downside it does have to > > use the devtools loader to get the telemetry object and also we have to bug > > a browser peer every time we want to change it). > > To be clear, I now think gDevTools.jsm is a better place to put this because > of these points (despite me suggesting both optoins earlier and not having a > preference between them). But I'll leave it up to Mike if he wants to > change it now, since it's here in the first place on my suggestion. Agreed, I have moved it there. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=31a41374e8eb https://tbpl.mozilla.org/?tree=Try&rev=31a41374e8eb

Rebased

Comment on attachment 8530537 [details] [diff] [review] telemetry-dark-theme-1091796.patch https://hg.mozilla.org/integration/fx-team/rev/33a786d18780

Backed out in https://hg.mozilla.org/integration/fx-team/rev/25013dd11944 along with bug 1098374

This code now needs rebasing as the devedition theme code has now been moved to browser/base/content/browser-devedition.js

Rearranged priorities so this patch could be made smaller. The new approach is about checking the values when the browser closes.

Comment on attachment 8579320 [details] [diff] [review] telemetry-dark-theme-1091796.patch Review of attachment 8579320 [details] [diff] [review]: ----------------------------------------------------------------- Nice and simple. 2 questions: - Should Gijs review the changes to browser-devedition.js? - The last patch had a test, this one does not, why no tests :( ? ::: browser/base/content/browser-devedition.js @@ +1,4 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + Should this file be re-reviewed by Gijs? Those changes look OK to me, but I noticed Gijs was the one who reviewed it last and there has been changes since. ::: browser/devtools/framework/gDevTools.jsm @@ +1334,5 @@ > gDevTools.off("toolbox-ready", gDevToolsBrowser._connectToProfiler); > Services.prefs.removeObserver("devtools.", gDevToolsBrowser); > Services.obs.removeObserver(gDevToolsBrowser.destroy, "quit-application"); > }, > +}; nit: maybe remove this change so that this file isn't even changed in this patch. Even smaller/simpler patch! And no polluting blame data.

Comment on attachment 8579320 [details] [diff] [review] telemetry-dark-theme-1091796.patch Review of attachment 8579320 [details] [diff] [review]: ----------------------------------------------------------------- Half-driveby considering the latest comment. Unfortunately, I have some comments. See below. ::: browser/base/content/browser-devedition.js @@ +133,5 @@ > + > + telemetry.log(THEME_HISTOGRAM_TOOLBOX, > + this._getToolboxThemeIndex(toolboxTheme)); > + > + let deveditionThemeEnabled = Services.prefs.getBoolPref(this._prefName); bgrins is changing how all this works, so please work out with him in what order you want to do this. He's going to make this a lightweight theme in itself (which is good because your code, for instance, will currently say that the devtools theme is enabled when a lwt or external theme is actually overriding it!). I expect the simplest is to wait on his changes before doing this part. Could followup it anyway, considering that the summary says you care about light/dark, not about the devedition theme... @@ +150,5 @@ > + return 0; > + case "dark": > + return 1; > + default: > + return 2; Why a histogram? I didn't think that producing averages etc. of this would be that useful? Seems like you could just stick this in the Browser UI Telemetry as a "regular" value and pass the actual theme name, which would also allow you to figure out which non-default toolbox themes people use.

(In reply to :Gijs Kruitbosch from comment #48) > ::: browser/base/content/browser-devedition.js > @@ +133,5 @@ > > + > > + telemetry.log(THEME_HISTOGRAM_TOOLBOX, > > + this._getToolboxThemeIndex(toolboxTheme)); > > + > > + let deveditionThemeEnabled = Services.prefs.getBoolPref(this._prefName); > > bgrins is changing how all this works, so please work out with him in what > order you want to do this. He's going to make this a lightweight theme in > itself (which is good because your code, for instance, will currently say > that the devtools theme is enabled when a lwt or external theme is actually > overriding it!). I expect the simplest is to wait on his changes before > doing this part. Could followup it anyway, considering that the summary says > you care about light/dark, not about the devedition theme... For the devedition enabled/disabled stuff, please wait for Bug 1094821 to land. I'm fine with rebasing on top of this if you remove that part. > Why a histogram? I didn't think that producing averages etc. of this would > be that useful? Seems like you could just stick this in the Browser UI > Telemetry as a "regular" value and pass the actual theme name, which would > also allow you to figure out which non-default toolbox themes people use. And it could possibly be done the same for whether devedition theme is applied. Since it's just going to be a lightweight theme we could just store the value of the selected lightweight theme, or check to see if this value is already tracked?

I am not at all sure what the purpose of tracking this is. From My point of view you either need to ignore all input form Linux and possibly mac, or at least ignore input if the "The Addon Bar (restored) add-on in installed on non-Windows systems, because it only seems to work correctly with the Developer dark theme under Windows. Otherwise, you will get rally skewed results.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #47) > Comment on attachment 8579320 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8579320 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nice and simple. > 2 questions: > - Should Gijs review the changes to browser-devedition.js? > - The last patch had a test, this one does not, why no tests :( ? > All of the current telemetry tests are hugely buggy and don't even detect when no telemetry is logged at all. Bug 1098374 is about accessing the actual telemetry data and reading the real results rather that relying on monkeypatching our telemetry module. The problem is that the patch that fixes this causes aborts in histogram.cc e.g. 10:37:35 INFO - [1945] ###!!! ABORT: file /builds/slave/fx-team-l64-d-0000000000000000/build/src/ipc/chromium/src/base/histogram.cc, line 732 Seems like a platform bug but it is on me to narrow it down and write a simple test case. Of course, it only happens on try. > ::: browser/base/content/browser-devedition.js > @@ +1,4 @@ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > + > > Should this file be re-reviewed by Gijs? Those changes look OK to me, but I > noticed Gijs was the one who reviewed it last and there has been changes > since. > Sure, he can. > ::: browser/devtools/framework/gDevTools.jsm > @@ +1334,5 @@ > > gDevTools.off("toolbox-ready", gDevToolsBrowser._connectToProfiler); > > Services.prefs.removeObserver("devtools.", gDevToolsBrowser); > > Services.obs.removeObserver(gDevToolsBrowser.destroy, "quit-application"); > > }, > > +}; > > nit: maybe remove this change so that this file isn't even changed in this > patch. Even smaller/simpler patch! And no polluting blame data. We could do that.

(In reply to Bill Gianopoulos [:WG9s] from comment #50) > I am not at all sure what the purpose of tracking this is. Originally Jeff wanted to check how many users were using the light theme vs. how many were using the dark theme. I find myself agreeing that I am not sure that there is still any point in tracking this. > From My point of > view you either need to ignore all input form Linux and possibly mac, Why? Do you have a problem with the current theme on these operating systems? > or at > least ignore input if the "The Addon Bar (restored) add-on in installed on > non-Windows systems, because it only seems to work correctly with the > Developer dark theme under Windows. Otherwise, you will get rally skewed > results. With just under 600,000 downloads you are probably right. (In reply to :Gijs Kruitbosch from comment #48) > Comment on attachment 8579320 [details] [diff] [review] > telemetry-dark-theme-1091796.patch > > Review of attachment 8579320 [details] [diff] [review]: > ----------------------------------------------------------------- > > Half-driveby considering the latest comment. > > Unfortunately, I have some comments. See below. > > ::: browser/base/content/browser-devedition.js > @@ +150,5 @@ > > + return 0; > > + case "dark": > > + return 1; > > + default: > > + return 2; > > Why a histogram? I didn't think that producing averages etc. of this would > be that useful? Seems like you could just stick this in the Browser UI > Telemetry as a "regular" value and pass the actual theme name, which would > also allow you to figure out which non-default toolbox themes people use. Awesome, keyed histograms were added in November. I would totally prefer to use them rather than using integer values linked to the discription... it is a shame that there is no way to convert data in some of our other histograms to use them. Before I go any further with this: @canuckistani: Do we wtill want to gather this information?

If we have newer, nicer ways to track this, that's great!

What's the status here? We have a use for this data and I've just realized it was never added.

It is totally something we should add... I am working on bugs as opposed to features this quarter so I will add this to my list.

Bug 1396811 appears to now record this data.