Add new UseCounter metric

third_party/WebKit/Source/core/frame/UseCounter.cpp

Index: third_party/WebKit/Source/core/frame/UseCounter.cpp
diff --git a/third_party/WebKit/Source/core/frame/UseCounter.cpp b/third_party/WebKit/Source/core/frame/UseCounter.cpp
index 8220443cb7ea06f398e4ecb8b2cd3eb57f9b323e..f39d52756ff569f36eaa1f16143f5791f3bd78cc 100644
--- a/third_party/WebKit/Source/core/frame/UseCounter.cpp
+++ b/third_party/WebKit/Source/core/frame/UseCounter.cpp
@@ -582,12 +582,25 @@ int UseCounter::mapCSSPropertyIdToCSSSampleIdForHistogram(CSSPropertyID cssPrope
 // Make sure update_use_counter_css.py was run which updates histograms.xml.
 static int maximumCSSSampleId() { return 539; }
 
-static EnumerationHistogram& featureObserverHistogram()
+static EnumerationHistogram& useCounterHistogram()

[dtapuska, 2016/09/09 18:30:19]

shouldn't these statics really be in an anonymous namespace?

[Rick Byers, 2016/09/09 19:31:37]

I don't think our coding style says to prefer one over the other (it's
equivalent in these cases AFAIK), but it's certainly more modern.  Done.  This
required some shuffling though.

 {
-    DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("WebCore.FeatureObserver", UseCounter::NumberOfFeatures));
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("WebCore.UseCounter.Features", UseCounter::NumberOfFeatures));
     return histogram;

[dtapuska, 2016/09/09 18:30:19]

Perhaps we should use temporary names because we haven't solved the SVG Image
problem yet. And we want to verify that this data is going to be more accurate
fist. Then solve the SVG Image problem. And then flip the name to the sensible
name.

[Rick Byers, 2016/09/09 19:31:36]

Done.

 }
 
+static EnumerationHistogram& CSSUseCounterHistogram()
+{
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("WebCore.UseCounter.CSSProperties", maximumCSSSampleId()));
+    return histogram;
+}
+
+UseCounter::UseCounter()
+    : m_muteCount(0)
+    , m_featuresRecorded(NumberOfFeatures)
+    , m_CSSRecorded(lastUnresolvedCSSProperty + 1)
+{
+}
+
 void UseCounter::muteForInspector()
 {
     m_muteCount++;
@@ -603,13 +616,16 @@ void UseCounter::recordMeasurement(Feature feature)
     if (m_muteCount)
         return;
 
-    DCHECK(feature != PageDestruction); // PageDestruction is reserved as a scaling factor.
+    DCHECK(feature != OBSOLETE_PageDestruction && feature != PageVisits); // PageDestruction is reserved as a scaling factor.
     DCHECK(feature < NumberOfFeatures);
 
-    if (!m_featureBits.quickGet(feature)) {
+    if (!m_featuresRecorded.quickGet(feature)) {
+        // Note that HTTPArchive tooling looks specifically for this event - see https://github.com/HTTPArchive/httparchive/issues/59
         TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("blink.feature_usage"), "FeatureFirstUsed", "feature", feature);
-        m_featureBits.quickSet(feature);
+        useCounterHistogram().count(feature);
+        m_featuresRecorded.quickSet(feature);
     }
+    m_legacyCounter.countFeature(feature);

[dtapuska, 2016/09/09 18:30:19]

Can this not go up into the conditional?

[Rick Byers, 2016/09/09 19:31:37]

No, the legacy counter has it's own set of recorded bits.  I think it's
equivalent today but wont be when I fix the SVG issue.

 }
 
 bool UseCounter::hasRecordedMeasurement(Feature feature) const
@@ -617,56 +633,10 @@ bool UseCounter::hasRecordedMeasurement(Feature feature) const
     if (m_muteCount)
         return false;
 
-    DCHECK(feature != PageDestruction); // PageDestruction is reserved as a scaling factor.
+    DCHECK(feature != OBSOLETE_PageDestruction && feature != PageVisits); // PageDestruction is reserved as a scaling factor.
     DCHECK(feature < NumberOfFeatures);
 
-    return m_featureBits.quickGet(feature);
-}
-
-UseCounter::UseCounter()
-    : m_muteCount(0)
-    , m_featureBits(NumberOfFeatures)
-    , m_CSSFeatureBits(lastUnresolvedCSSProperty + 1)
-{
-}
-
-UseCounter::~UseCounter()
-{
-    // We always log PageDestruction so that we have a scale for the rest of the features.
-    // TODO(rbyers): This is flawed due to renderer fast shutdown - crbug.com/597963
-    featureObserverHistogram().count(PageDestruction);
-
-    updateMeasurements();
-}
-
-void UseCounter::updateMeasurements()
-{
-    EnumerationHistogram& featureHistogram = featureObserverHistogram();
-    featureHistogram.count(PageVisits);
-    for (size_t i = 0; i < NumberOfFeatures; ++i) {
-        if (m_featureBits.quickGet(i))
-            featureHistogram.count(i);
-    }
-    // Clearing count bits is timing sensitive.
-    m_featureBits.clearAll();
-
-    // FIXME: Sometimes this function is called more than once per page. The following
-    //        bool guards against incrementing the page count when there are no CSS
-    //        bits set. https://crbug.com/236262.
-    DEFINE_STATIC_LOCAL(EnumerationHistogram, cssPropertiesHistogram, ("WebCore.FeatureObserver.CSSProperties", maximumCSSSampleId()));
-    bool needsPagesMeasuredUpdate = false;
-    for (size_t i = firstCSSProperty; i <= lastUnresolvedCSSProperty; ++i) {
-        if (m_CSSFeatureBits.quickGet(i)) {
-            int cssSampleId = mapCSSPropertyIdToCSSSampleIdForHistogram(static_cast<CSSPropertyID>(i));
-            cssPropertiesHistogram.count(cssSampleId);
-            needsPagesMeasuredUpdate = true;
-        }
-    }
-
-    if (needsPagesMeasuredUpdate)
-        cssPropertiesHistogram.count(totalPagesMeasuredCSSSampleId());
-
-    m_CSSFeatureBits.clearAll();
+    return m_featuresRecorded.quickGet(feature);
 }
 
 void UseCounter::didCommitLoad()
@@ -674,7 +644,13 @@ void UseCounter::didCommitLoad()
     // TODO(rbyers): This gets invoked more than expected.  crbug.com/236262
     // Eg. every SVGImage has it's own Page instance, they should probably all be delegating
     // their UseCounter to the containing Page.
-    updateMeasurements();
+    m_legacyCounter.updateMeasurements();
+
+    // TODO: When exactly do we want to do this?
+    m_featuresRecorded.clearAll();
+    useCounterHistogram().count(PageVisits);
+    m_CSSRecorded.clearAll();
+    CSSUseCounterHistogram().count(totalPagesMeasuredCSSSampleId());
 }
 
 void UseCounter::count(const Frame* frame, Feature feature)
@@ -706,7 +682,7 @@ bool UseCounter::isCounted(Document& document, Feature feature)
 
 bool UseCounter::isCounted(CSSPropertyID unresolvedProperty)
 {
-    return m_CSSFeatureBits.quickGet(unresolvedProperty);
+    return m_CSSRecorded.quickGet(unresolvedProperty);
 }
 
 bool UseCounter::isCounted(Document& document, const String& string)
@@ -764,18 +740,21 @@ void UseCounter::countCrossOriginIframe(const Document& document, Feature featur
         count(frame, feature);
 }
 
-void UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID feature)
+void UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID property)
 {
-    DCHECK(feature >= firstCSSProperty);
-    DCHECK(feature <= lastUnresolvedCSSProperty);
+    DCHECK(property >= firstCSSProperty);
+    DCHECK(property <= lastUnresolvedCSSProperty);
 
     if (!isUseCounterEnabledForMode(cssParserMode) || m_muteCount)
         return;
 
-    if (!m_CSSFeatureBits.quickGet(feature)) {
-        TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("blink.feature_usage"), "CSSFeatureFirstUsed", "feature", feature);
-        m_CSSFeatureBits.quickSet(feature);
+    if (!m_CSSRecorded.quickGet(property)) {
+        // Note that HTTPArchive tooling looks specifically for this event - see https://github.com/HTTPArchive/httparchive/issues/59
+        TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("blink.feature_usage"), "CSSFeatureFirstUsed", "feature", property);
+        CSSUseCounterHistogram().count(mapCSSPropertyIdToCSSSampleIdForHistogram(property));
+        m_CSSRecorded.quickSet(property);
     }
+    m_legacyCounter.countCSS(property);

[dtapuska, 2016/09/09 18:30:19]

likewise why isn't this inside the else?

[Rick Byers, 2016/09/09 19:31:36]

I added some comments to the declarations to make it clear that the semantics of
the legacy and non-legacy bits are independent.   Better?

 }
 
 void UseCounter::count(Feature feature)
@@ -807,4 +786,71 @@ UseCounter* UseCounter::getFrom(const StyleSheetContents* sheetContents)
     return 0;
 }
 
+/*
+ *
+ * LEGACY metrics support - WebCore.FeatureObserver is to be superceded by WebCore.UseCounter
+ *
+ */
+
+static EnumerationHistogram& featureObserverHistogram()
+{
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("WebCore.FeatureObserver", UseCounter::NumberOfFeatures));
+    return histogram;
+}
+
+UseCounter::LegacyCounter::LegacyCounter()
+    : m_featureBits(NumberOfFeatures)
+    , m_CSSBits(lastUnresolvedCSSProperty + 1)
+{
+}
+
+UseCounter::LegacyCounter::~LegacyCounter()
+{
+    // PageDestruction was intended to be used as a scale, but it's broken (due to fast shutdown).
+    // See https://crbug.com/597963.
+    featureObserverHistogram().count(OBSOLETE_PageDestruction);
+    updateMeasurements();
+}
+
+void UseCounter::LegacyCounter::countFeature(Feature feature)
+{
+    m_featureBits.quickSet(feature);
+}
+
+void UseCounter::LegacyCounter::countCSS(CSSPropertyID property)
+{
+    m_CSSBits.quickSet(property);
+}
+
+void UseCounter::LegacyCounter::updateMeasurements()
+{
+    EnumerationHistogram& featureHistogram = featureObserverHistogram();
+    featureHistogram.count(PageVisits);
+    for (size_t i = 0; i < NumberOfFeatures; ++i) {
+        if (m_featureBits.quickGet(i))
+            featureHistogram.count(i);
+    }
+    // Clearing count bits is timing sensitive.
+    m_featureBits.clearAll();
+
+    // FIXME: Sometimes this function is called more than once per page. The following
+    //        bool guards against incrementing the page count when there are no CSS
+    //        bits set. https://crbug.com/236262.
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, cssPropertiesHistogram, ("WebCore.FeatureObserver.CSSProperties", maximumCSSSampleId()));
+    bool needsPagesMeasuredUpdate = false;
+    for (size_t i = firstCSSProperty; i <= lastUnresolvedCSSProperty; ++i) {
+        if (m_CSSBits.quickGet(i)) {
+            int cssSampleId = mapCSSPropertyIdToCSSSampleIdForHistogram(static_cast<CSSPropertyID>(i));
+            cssPropertiesHistogram.count(cssSampleId);
+            needsPagesMeasuredUpdate = true;
+        }
+    }
+
+    if (needsPagesMeasuredUpdate)
+        cssPropertiesHistogram.count(totalPagesMeasuredCSSSampleId());
+
+    m_CSSBits.clearAll();
+}
+
+
 } // namespace blink