See https://groups.google.com/forum/#!topic/mozilla.dev.platform/hHR_PmAIomU.

Per the thread, this expects to add ~300 new entries (to the opt-in data set). Talking it over here with Roberto, this should in general be fine if we can accept the storage cost and potential increases in aggregation times. Longer-term we really need to have proper scalar measurements to store this kind of data in. I wonder if it makes sense to submit the use counters in a separate, optimized format here. Benjamin, heads-up in case you have input or concerns here.

Thanks, per Cameron's email in the thread from Comment 0, I think I'll just start with -moz-prefixed CSS props and we can decide where to go from there (both in terms of perf and storage overhead). So, it will be some subset of 300 (TDB).

As an exploratory measure, I think this is fine. This doesn't sound like the kind of thing we should collect permanently though, and I'm afraid we're suffering from the fact that use counters are still half-baked and inefficient. They don't support the same alerting and expiration mechanisms as the rest of telemetry, AFAIK. And perhaps most importantly, you can't turn them on for the opt-out population, which means you're going to suffer from pretty significant geo bias from beta. So we should plan on doing this only for a temporary period (3-6 months) and then focus on the ones we actively care about. If we're going to use lots of use counters, you (or Nathan) need to fix them to use the more efficient recording mechanism described at https://groups.google.com/forum/#!searchin/mozilla.dev.platform/use$20counters$20smedberg/mozilla.dev.platform/ktoEer_cdJA/Ni58spbvKgAJ

Sounds good, thanks Benjamin.

This patch adds two "sections" to UseCounters.conf 1) -moz-prefixed CSS properties (which is everything in nsCSSPropList.h that uses CSS_PROP_DOMPROP_PREFIXED in its `method` name -- except those in a \#ifndef CSS_PROP_LIST_EXCLUDE_INTERNAL section). 2) prefixed CSS property aliases (both -moz- and -webkit- prefixed aliases).

Comment on attachment 8755987 [details] [diff] [review] Add UserCounter telemetry for prefixed CSS properties and property aliases. r=? Daniel, would you mind reviewing?

Comment on attachment 8755987 [details] [diff] [review] Add UserCounter telemetry for prefixed CSS properties and property aliases. r=? Review of attachment 8755987 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but it's not complete. r=me with missing properties/aliases (noted below) added (or explained if there's a reason they're missing) ::: dom/base/UseCounters.conf @@ +83,5 @@ > +property MozColumnRuleStyle > +property MozColumnRuleWidth > +property MozColumnWidth > +property MozColumns > +property MozFloatEdge Missing MozControlCharacterVisibility (should be after MozColumns) @@ +86,5 @@ > +property MozColumns > +property MozFloatEdge > +property MozForceBrokenImageIcon > +property MozImageRegion > +property MozOrient Missing MozMinFontSizeRatio (should be after MozImageRegion) @@ +93,5 @@ > +property MozOutlineRadiusBottomleft > +property MozOutlineRadiusBottomright > +property MozOutlineRadiusTopleft > +property MozStackSizing > +property MozTabSize Missing MozOutlineRadiusTopright (should be after MozOutlineRadiusTopleft) ...and missing MozSystemFont (should be after MozStackSizing) @@ +96,5 @@ > +property MozStackSizing > +property MozTabSize > +property MozTextAlignLast > +property MozTextSizeAdjust > +property MozUserFocus Missing MozTopLayer (should be after MozTextSizeAdjust) @@ +100,5 @@ > +property MozUserFocus > +property MozUserInput > +property MozUserModify > +property MozUserSelect > +property MozWindowDragging Missing MozWindowShadow (should be after MozWindowDragging) @@ +140,5 @@ > +property MozBorderStartStyle > +property MozBorderStartWidth > +property MozHyphens > +property WebkitAnimation > +property WebkitAnimationDirection Missing WebkitAnimationDelay (should be after WebkitAnimation)

For reference, here's the list of prefixed properties & aliases that I generated locally for review (using grep and sed on layout/style/nsCSSPropList.h and nsCSSPropAliasList.h ). (I noticed the missing lines by comparing this against your added list using "meld".)

(Also: do you know that UseCounters.conf will actually work for property aliases? The documentation at the top of the file doesn't mention the word "aliases" at all. I can imagine it working just fine, but I can also imagine aliases being an edge case which weren't tested/considered until now.)

Oh, I bet some/all of the missing properties I noticed are from CSS_PROP_LIST_EXCLUDE_INTERNAL sections... checking to see if any of the ones I caught are not in that category.

Yeah, OK -- I think the only things from comment 7 that *actually* need to be added are: MozOutlineRadiusTopright WebkitAnimationDelay

Ah, great. Thank you for review and lazer-eyes abilities! Will add the two missing props before pushing. (In reply to Daniel Holbert [:dholbert] from comment #9) > (Also: do you know that UseCounters.conf will actually work for property > aliases? The documentation at the top of the file doesn't mention the word > "aliases" at all. > > I can imagine it working just fine, but I can also imagine aliases being an > edge case which weren't tested/considered until now.) As for that... not sure. Let's ask froydnj if he knows. Nathan, re: Comment #9 -- should property aliases be picked up as a UseCounter-able property? That is, would WebKitAnimation and MozAnimation be recorded as 2 distinct measurements (or would they get lumped in with animation because they call that method)?

(In reply to Mike Taylor [:miketaylr] from comment #12) > Nathan, re: Comment #9 -- should property aliases be picked up as a > UseCounter-able property? That is, would WebKitAnimation and MozAnimation be > recorded as 2 distinct measurements (or would they get lumped in with > animation because they call that method)? I think they ought to be recorded as distinct measurements. But light testing indicates that the animation properties don't seem to be use countable at all, so either I'm writing my test wrong or there's a case or two missing in the use counter support. I'll throw my test modifications up in a second.

These tests fail with messages like: 62 INFO TEST-UNEXPECTED-FAIL | dom/base/test/browser_use_counters.js | page counts for PROPERTY_MOZANIMATION after are correct - Got 0, expected 1 64 INFO TEST-UNEXPECTED-FAIL | dom/base/test/browser_use_counters.js | document counts for PROPERTY_MOZANIMATION after are correct - Got 0, expected 1 The passing of PROPERTY_ANIMATION might just be due to it not getting recorded correctly...

Looks like the use-counter machinery in the style-system only allocates a use counter for the non-alias non-shorthand properties. > 600 static const mozilla::UseCounter gPropertyUseCounter[eCSSProperty_COUNT_no_shorthands]; http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.h?rev=ecd39abf2cdb&mark=597-597,600-600#597 (eCSSProperty_COUNT_no_shorthands is a numeric "sentinel"" enum value in the list of property values, which is numerically *after* the normal properties, and *before* shorthands and aliases: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProperty.h?rev=7f41c81e14d2&mark=27-27,31-31,46-46#20 ) So, I don't think we have machinery to count property-alias usage right now.

(Yeah, bug 968923 comment 95 mentions aliases/shorthands needing some special not-yet-written handling. So I think this is just not expected to work yet.)

Thx for taking a look Nathan (I realize now I could have tested myself in browser_use_counter.js...), and thx for filing a bug Daniel.

I spun out bug 1276313 so we can land use counters for the -moz-prefixed longhand props here and take care of the shorthands and aliases once that's do-able.

Gonna remove the following CSS_PROP_SHORTHANDs (in addition to all the aliases in my previous patch): MozOutlineRadius MozColumnRule MozColumns And add the following, which aren't implemented as aliases (forgot these... oops) WebkitTextFillColor WebkitTextStrokeColor WebkitTextStrokeWidth

OK, I wrote some simple tests working off Comment #14 to make sure this was doing what I was expected... and it wasn't. As an example, my patch added "property MozAppearance" to UseCounter.conf but my test for that found 0: 62 INFO TEST-UNEXPECTED-FAIL | dom/base/test/browser_use_counters.js | page counts for PROPERTY_MOZAPPEARANCE after are correct - Got 0, expected 1 Looking again at nsCSSPropList.h we have: CSS_PROP_DISPLAY( -moz-appearance, appearance, CSS_PROP_DOMPROP_PREFIXED(Appearance), CSS_PROPERTY_PARSE_VALUE, "", VARIANT_HK, kAppearanceKTable, CSS_PROP_NO_OFFSET, eStyleAnimType_None) I had assumed it would work manually adding the prefix name, hence MozAppearance, but looking more closely at the CSS_PROP_DOMPROP_PREFIXED macro it refers to the CSS_PROP_PUBLIC_OR_PRIVATE macro. #define CSS_PROP_DOMPROP_PREFIXED(name_) \ CSS_PROP_PUBLIC_OR_PRIVATE(Moz ## name_, name_) The comment about the `method` argument to CSS_PROP and friends: "Callers using this parameter must also define the CSS_PROP_PUBLIC_OR_PRIVATE(publicname_, privatename_) macro to yield either publicname_ or privatename_." Digging in, in https://dxr.mozilla.org/mozilla-central/source/dom/base/gen-usecounters.py#58 I see: #define CSS_PROP_PUBLIC_OR_PRIVATE(publicname_, privatename_) privatename_ Nathan, is my understanding around the way CSS prop UseCounters work correct that the UseCounter.conf should use CSS_PROP(_*) macro method names without Moz prefixes, i.e., the privatename_ name? If so, this has implications for measuring aliases, since they share a private name. But that can be dealt with in Bug 1276007. (Note: I also added a test for WebKitTextStrokeColor, which passed, because it doesn't use CSS_PROP_DOMPROP_PREFIXED, I think...)

(In reply to Mike Taylor [:miketaylr] from comment #20) > Nathan, is my understanding around the way CSS prop UseCounters work correct > that the UseCounter.conf should use CSS_PROP(_*) macro method names without > Moz prefixes, i.e., the privatename_ name? Using the non-prefixed names would be contrary to what the generation of $objdir/dom/base/UseCounterList.h expects, so I think that the code in gen-usecounters.py is actually incorrect.

Somehow 5 months passed and I haven't come back to finish this. Tom said he'd be happy to investigate and carry this over the finish line though. (setting ni? as a sticky notification)

Mike, is someone planning to pick this work up? Or should we close this one?

Hey Georg, Tom was going to be looking at this. Tom?

I haven't looked at this much yet, but I can prioritize it if you'd like, Mike?

Yes, please.