I would like to see some telemetry on usage of userChrome.css. With the removal of heavyweight themes, I've seen a number of comments from people seeking to recreate such themes by hacking userChrome.css directly. (And a rise in comments from people who have accidentally broken things, or forgot that had made some such change.) This makes me mildly concerned that this may become a ticking timebomb for users, especially since userChrome.css is worse than a theme in many respects (doesn't show up in Firefox UI anywhere, can't be disabled, not minVersion/maxVersion, etc). I think it would be sufficient to add some telemetry to nsLayoutStylesheetCache::InitFromProfile() to check is mUserChromeSheer != null, as a signal that the user has created it. Bonus points if it's trivial to also to record the size (bytes? lines? # rules? something else?) to help differentiate trivial usage (e.g. hiding a menu item) from complex "theming" usage.

Jared: dolske suggested I get your initial feedback on this, before getting a final review from dbaron or another CSS peer? I'll also get the data collection review started. Eventually, it might be nice to get a deeper insights here. AIUI a non-trivial userChrome.css might be using @imports, so I'm not sure how useful a count of number of rules or even just byte count on that file would be. As that could get complicated quickly, it seems the simplest thing of just logging a boolean looks like a useful way to start getting some numbers here, in time for 58. I've checked this does the right thing with no userChrome.css, with a non-readable (permissions) userChrome.css and a valid userChrome.css. Again, if the userChrome.css is just a comment or something, we'll get noise in the data here, so its a tradeoff. I've started looking for how/where to test this and so far coming up empty. Still looking but I'll take any suggestions.

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. https://reviewboard.mozilla.org/r/200934/#review206160 ::: layout/style/nsLayoutStylesheetCache.cpp:440 (Diff revision 1) > contentFile->Clone(getter_AddRefs(chromeFile)); > if (!chromeFile) return; Because of this return you're not going to record a `false` if the "chrome" directory doesn't even exist. You could switch to a "flag" histogram to solve this.

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. https://reviewboard.mozilla.org/r/200934/#review206160 > Because of this return you're not going to record a `false` if the "chrome" directory doesn't even exist. You could switch to a "flag" histogram to solve this. The docs suggest that the "flag" type is deprecated. I'm not sure how true or significant that is, but this is easy enough to fix by adding an Telemetry::Accumulate call before that early return.

How long do we want to keep this probe around? I put until version 62 in the Histograms.json entry to get the ball rolling, but is this data we expect to want to monitor after that time? Indefinitely or for n number of releases?

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. Clearing need-info in favor of f?

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. Clearing need-info in favor of f?

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. tidying stray f? flag

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. Looks fine to me. You can request review from :francois for the Historgrams.json change and review from :jwatt for the nsLayoutStylesheetCache.cpp change.

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. I'm exploring a slightly different version of this patch - this time logging the filesize of userChrome.css. Obviously file size is not a true measure of complexity or how much of a risk of UI breakage users are exposed to, but its a little more information than simply if the file exists. The histogram details are a little arbitrary. Probably 10 buckets would suffice for example. Any thoughts - on either the patch or the utility of the data collected?

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. https://reviewboard.mozilla.org/r/200934/#review208970 I was thinking about this yesterday too. Having a rough idea of the filesize could be nice, but I worry that it could be a bit misleading. Some CSS changes can be very compact while others would require something very verbose. I would prefer to stick to the simple boolean of if it is in use or not. Also, if we still do want to go down this route, we should get a better measurement than filesize. We can check for the presence of userChrome.css in our list of stylesheets that are in the document and then store the number of rules in the stylesheet. ::: layout/style/nsLayoutStylesheetCache.cpp:439 (Diff revision 3) > return; > } > > contentFile->Clone(getter_AddRefs(chromeFile)); > - if (!chromeFile) return; > + if (!chromeFile) { > + Telemetry::Accumulate(Telemetry::USER_CHROME_CSS_FILESIZE_B, 0); This is going to be misleading because the file could exist and have a filesize of 0 bytes or it could not exist at all and we would count it as having 0 bytes. We could also store -1 if the file doesn't exist instead of using 0. ::: toolkit/components/telemetry/Histograms.json:13795 (Diff revision 3) > "expires_in_version": "never", > "kind": "enumerated", > "n_values": 64, > "description": "URL CLassifier-related (aka Safe Browsing) UI events. See nsIUrlClassifierUITelemetry.idl for the specific values." > }, > + "USER_CHROME_CSS_FILESIZE_B": { I assume the "_B" is short for bytes, might as well write "BYTES".

(In reply to Sam Foster [:sfoster] from comment #12) > Comment on attachment 8929651 [details] > Bug 1416044 - Add telemetry probe for the filesize of userChrome.css > > I'm exploring a slightly different version of this patch - this time logging > the filesize of userChrome.css. Obviously file size is not a true measure of > complexity or how much of a risk of UI breakage users are exposed to, but > its a little more information than simply if the file exists. > > The histogram details are a little arbitrary. Probably 10 buckets would > suffice for example. Any thoughts - on either the patch or the utility of > the data collected? From a WebExtensions point-of-view, I am very interested in this telemetry as well. The use of userChrome.css indicates there is a market need to customize the browser that cannot currently be satisfied any other way. Knowing the frequency that userChrome is used is a good start, but any information we can get on the contents is even better (although, more difficult, I suspect). Number of rules, as jaws proposed, is a nice start. In the end, I'm looking for information to help me provide developers with a solid set of maintainable API that provides what userChrome.css is providing today.

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. https://reviewboard.mozilla.org/r/200934/#review208970 Yeah, a rules count was my thought too. But it gets more complicated if we want to start drilling down into @imports. The filesize thing was a potential compromise, but i'm thinking now that the best path forward is to land a bool version initially, and file a seperate bug for deeper insights into userChrome.css usage. Rules count is one idea, perhaps there are others.

Ok, patch is back to the boolean "did userChrome.css get loaded" version. I've flagged :francois for review of the Histograms.json changes, and :jwatt for the changes to nsLayoutStyleSheetCache.cpp, please? Cancelling ni? on the "expires_in_version" value. I've given it 62, which is 3-4 releases which seems like a reasonable duration to give some data to get started with.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3) > Because of this return you're not going to record a `false` if the "chrome" > directory doesn't even exist. This Clone() call doesn't care what exists in the file system. nsIFile just contains a path.

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. https://reviewboard.mozilla.org/r/200934/#review209138 ::: layout/style/nsLayoutStylesheetCache.cpp:438 (Diff revision 4) > // if we don't have a profile yet, that's OK! > return; > } > > contentFile->Clone(getter_AddRefs(chromeFile)); > - if (!chromeFile) return; > + if (!chromeFile) { nsIFile is just an object that contains a path. This Clone call just clones one nsIFile to another without any knowledge of whether the path exists in the file system or not. (The NS_GetSpecialDirectory call further up also doesn't care whether the path exists FWIW.) In fact the check that Clone() succeeded seems redundant since the only reason this particular call could fail seems to be out-of-memory, which will cause us to crash. At any rate, you don't need the Telemetry::Accumulate call here. ::: layout/style/nsLayoutStylesheetCache.cpp:449 (Diff revision 4) > chromeFile->Append(NS_LITERAL_STRING("userChrome.css")); > > LoadSheetFile(contentFile, &mUserContentSheet, eUserSheetFeatures, eLogToConsole); > LoadSheetFile(chromeFile, &mUserChromeSheet, eUserSheetFeatures, eLogToConsole); > + > + Telemetry::Accumulate(Telemetry::USER_CHROME_CSS_LOADED, mUserChromeSheet != nullptr); From my almost non-existant knowledge about telemetry I'd have thought you'd want to collect a scalar, not a histogram. I'll leave it up to whichever Telemetry peer reviews to comment on that though. This change looks fine as long as you and they are okay with that. Note also that nsLayoutStylesheetCache::InitFromProfile will be called multiple times when the browser runs (once per chrome and content process).

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. https://reviewboard.mozilla.org/r/200934/#review209516 Looks good to me. We now have a new process to allow us to better audit the data that's collected by Firefox: https://wiki.mozilla.org/Firefox/Data_Collection#Step_1:_Submit_Request Essentially all you need to do is download the request.md form from Github, answer the questions and attach the resulting textfile to the bug. You can r? me on that file and I will take care of Step 2 in the process. ::: toolkit/components/telemetry/Histograms.json:13802 (Diff revision 4) > + "bug_numbers": [1416044], > + "expires_in_version": "62", > + "alert_emails": ["sfoster@mozilla.com"], > + "kind": "boolean", > + "releaseChannelCollection": "opt-out", > + "description": "Was a userChrome.css stylesheet loaded with this profile?" Just to make it clear that the probe is sent at initialization time, I'd suggest rephrasing it like: "Was a userChrome.css stylesheet loaded when the profile was initialized?"

(In reply to François Marier [:francois] from comment #20) > Comment on attachment 8929651 [details] > Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. > > https://reviewboard.mozilla.org/r/200934/#review209516 > > Looks good to me. > > We now have a new process to allow us to better audit the data that's > collected by Firefox: > https://wiki.mozilla.org/Firefox/Data_Collection#Step_1:_Submit_Request > > Essentially all you need to do is download the request.md form from Github, > answer the questions and attach the resulting textfile to the bug. > > You can r? me on that file and I will take care of Step 2 in the process. Is the r- here because I need to get the request approved before this patch can land (which I will do.) Or, because there is a problem with the patch which I need to fix?

(In reply to Sam Foster [:sfoster] from comment #21) > Is the r- here because I need to get the request approved before this patch > can land (which I will do.) Or, because there is a problem with the patch > which I need to fix? Just to block the landing until the new form is filled out and r+. I don't see any problems with your patch. Sorry I should have been more explicit.

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #19) > > contentFile->Clone(getter_AddRefs(chromeFile)); > > - if (!chromeFile) return; > > + if (!chromeFile) { > > nsIFile is just an object that contains a path. This Clone call just clones > one nsIFile to another without any knowledge of whether the path exists in > the file system or not. (The NS_GetSpecialDirectory call further up also > doesn't care whether the path exists FWIW.) Oops, of course. > Note also that nsLayoutStylesheetCache::InitFromProfile will be called > multiple times when the browser runs (once per chrome and content process). This is a reason why a flag histogram or scalar would be better.

Completed answers inline for the data request review form.

:jwatt, I updated attachment 8929651 [details] to address your comments and also this observation: > Note also that nsLayoutStylesheetCache::InitFromProfile will be called > multiple times when the browser runs (once per chrome and content process). .. by wrapping in a check we're in the parent process. I think those are trivial changes which can carry the r+, but I've not landed c++ in moz-central before so I don't want to assume. Can you confirm?

Comment on attachment 8933761 [details] data-review-bug-1416044.md Sorry, that's the wrong form. The one you filled out is for Step 2 and is the one I will be filling out in response to yours. The form for Step 1 that you should fill out is https://github.com/mozilla/data-review/blob/master/request.md

Sorry for the confusion. This should be the right stuff.

Comment on attachment 8933856 [details] data-review-bug-1416044.md 1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate? (see [here](https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_dictionary.md), [here](https://github.com/mozilla-mobile/focus/wiki/Install-and-event-tracking-with-the-Adjust-SDK), and [here](https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/index.html) for examples). Yes, Histograms.json. 2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available. Yes, telemetry setting. 3) If the request is for permanent data collection, is there someone who will monitor the data over time?** Not permanent. 4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under? ** Category 2. 5) Is the data collection request for default-on or default-off? Default-on. 6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? No. 7) Is the data collection covered by the existing Firefox privacy notice? Yes. 8) Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)** No, telemetry alerts are fine.

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. https://reviewboard.mozilla.org/r/200934/#review211234 datareview+

(In reply to Sam Foster [:sfoster] from comment #26) > :jwatt, I updated attachment 8929651 [details] to address your comments and > also this observation: > > > Note also that nsLayoutStylesheetCache::InitFromProfile will be called > > multiple times when the browser runs (once per chrome and content process). > > .. by wrapping in a check we're in the parent process. I think those are > trivial changes which can carry the r+, but I've not landed c++ in > moz-central before so I don't want to assume. Can you confirm? Yes, that should work well. (And carrying the r+ is fine.)

Pushed by sfoster@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/454b93c818d3 Add telemetry probe for if userChrome.css is loaded. r=francois,jwatt

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. Approval Request Comment [Feature/Bug causing the regression]: New telemetry for measuring pervasiveness (or not) of userChrome.css usage [User impact if declined]: No direct user impact. We would like to start have these data points on the heels of 57 though to be able to see any trends in changing usage [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Not yet, waiting for the new histogram to show up in telemetry dashboards to verify [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Low risk [Why is the change risky/not risky?]: 2-line change to add a telemetry measure at startup. [String changes made/needed]: None

Comment on attachment 8929651 [details] Bug 1416044 - Add telemetry probe for if userChrome.css is loaded. new telemetry probe, for 58.0b11

Comment on attachment 8933856 [details] data-review-bug-1416044.md > 7) What populations will you measure? > A: All users who have not disabled telemetry Per bug 1352981 comment 4 users on Tier3 platforms are also excluded. However, the ratio would probably be the same as on Linux.

(In reply to dfgsdfgsdfg from comment #39) > That is funny though. Forcing users to use the userChrome.css, as many > add-ons are no longer available in "Quantum". Then Saying it might be a time > bomb implying the suggestion to no longer allow userChrome.css I'm not sure where you are getting this idea? The act of measuring usage doesn't imply any intention to remove a feature. Regardless, this is not the place for this kind of discussion.