Minor UseCounter clean-ups

Remove the mostly useless CountBits abstraction and better align the
generic feature and CSS property usage tracking.  Also a couple other
minor cleanups and comments warning of outstanding issues.

BUG=597963

Review-Url: https://codereview.chromium.org/2284503002
Cr-Commit-Position: refs/heads/master@{#414753}

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

@@ -42,10 +42,8 @@ namespace blink {
 
 static int totalPagesMeasuredCSSSampleId() { return 1; }
 
-int UseCounter::mapCSSPropertyIdToCSSSampleIdForHistogram(int id)
+int UseCounter::mapCSSPropertyIdToCSSSampleIdForHistogram(CSSPropertyID cssPropertyID)
 {
-    CSSPropertyID cssPropertyID = static_cast<CSSPropertyID>(id);
-
     switch (cssPropertyID) {
     // Begin at 2, because 1 is reserved for totalPagesMeasuredCSSSampleId.
     case CSSPropertyColor: return 2;
@@ -605,51 +603,61 @@ void UseCounter::recordMeasurement(Feature feature)
     if (m_muteCount)
         return;
 
-    if (!m_countBits.hasRecordedMeasurement(feature)) {
+    DCHECK(feature != PageDestruction); // PageDestruction is reserved as a scaling factor.
+    DCHECK(feature < NumberOfFeatures);
+
+    if (!m_featureBits.quickGet(feature)) {
         TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("blink.feature_usage"), "FeatureFirstUsed", "feature", feature);
+        m_featureBits.quickSet(feature);
     }
-    m_countBits.recordMeasurement(feature);
+}
+
+bool UseCounter::hasRecordedMeasurement(Feature feature) const
+{
+    if (m_muteCount)
+        return false;
+
+    DCHECK(feature != PageDestruction); // 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)
 {
-    m_CSSFeatureBits.ensureSize(lastUnresolvedCSSProperty + 1);
-    m_CSSFeatureBits.clearAll();
 }
 
 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::CountBits::updateMeasurements()
+void UseCounter::updateMeasurements()
 {
     EnumerationHistogram& featureHistogram = featureObserverHistogram();
-    for (unsigned i = 0; i < NumberOfFeatures; ++i) {
-        if (m_bits.quickGet(i))
+    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_bits.clearAll();
-}
-
-void UseCounter::updateMeasurements()
-{
-    featureObserverHistogram().count(PageVisits);
-    m_countBits.updateMeasurements();
+    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 (int i = firstCSSProperty; i <= lastUnresolvedCSSProperty; ++i) {
+    for (size_t i = firstCSSProperty; i <= lastUnresolvedCSSProperty; ++i) {
         if (m_CSSFeatureBits.quickGet(i)) {
-            int cssSampleId = mapCSSPropertyIdToCSSSampleIdForHistogram(i);
+            int cssSampleId = mapCSSPropertyIdToCSSSampleIdForHistogram(static_cast<CSSPropertyID>(i));
             cssPropertiesHistogram.count(cssSampleId);
             needsPagesMeasuredUpdate = true;
         }
@@ -663,6 +671,9 @@ void UseCounter::updateMeasurements()
 
 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();
 }
 
@@ -698,7 +709,6 @@ bool UseCounter::isCounted(CSSPropertyID unresolvedProperty)
     return m_CSSFeatureBits.quickGet(unresolvedProperty);
 }
 
-
 bool UseCounter::isCounted(Document& document, const String& string)
 {
     Frame* frame = document.frame();
@@ -756,21 +766,21 @@ void UseCounter::countCrossOriginIframe(const Document& document, Feature featur
 
 void UseCounter::count(CSSParserMode cssParserMode, CSSPropertyID feature)
 {
-    ASSERT(feature >= firstCSSProperty);
-    ASSERT(feature <= lastUnresolvedCSSProperty);
+    DCHECK(feature >= firstCSSProperty);
+    DCHECK(feature <= 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);
     }
-    m_CSSFeatureBits.quickSet(feature);
 }
 
 void UseCounter::count(Feature feature)
 {
-    ASSERT(Deprecation::deprecationMessage(feature).isEmpty());
+    DCHECK(Deprecation::deprecationMessage(feature).isEmpty());
     recordMeasurement(feature);
 }
 

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

@@ -1349,7 +1349,7 @@ public:
     static UseCounter* getFrom(const CSSStyleSheet*);
     static UseCounter* getFrom(const StyleSheetContents*);
 
-    static int mapCSSPropertyIdToCSSSampleIdForHistogram(int id);
+    static int mapCSSPropertyIdToCSSSampleIdForHistogram(CSSPropertyID);
 
     void muteForInspector();
     void unmuteForInspector();
@@ -1357,40 +1357,13 @@ public:
     void recordMeasurement(Feature);
     void updateMeasurements();
 
-    bool hasRecordedMeasurement(Feature feature) const { return m_countBits.hasRecordedMeasurement(feature); }
-
-    class CountBits {
-        DISALLOW_NEW();
-    public:
-        CountBits() : m_bits(NumberOfFeatures) { }
-
-        bool hasRecordedMeasurement(Feature feature) const
-        {
-            ASSERT(feature != PageDestruction); // PageDestruction is reserved as a scaling factor.
-            ASSERT(feature < NumberOfFeatures);
-
-            return m_bits.quickGet(feature);
-        }
-
-        void recordMeasurement(Feature feature)
-        {
-            ASSERT(feature != PageDestruction); // PageDestruction is reserved as a scaling factor.
-            ASSERT(feature < NumberOfFeatures);
-
-            m_bits.quickSet(feature);
-        }
-
-        void updateMeasurements();
-
-    private:
-        BitVector m_bits;
-    };
+    bool hasRecordedMeasurement(Feature) const;
 
 protected:
     friend class UseCounterTest;
     unsigned m_muteCount;
 
-    CountBits m_countBits;
+    BitVector m_featureBits;
     BitVector m_CSSFeatureBits;
 };
 

third_party/WebKit/Source/core/page/Page.cpp

@@ -433,6 +433,8 @@ void Page::updateAcceleratedCompositingSettings()
 void Page::didCommitLoad(LocalFrame* frame)
 {
     if (m_mainFrame == frame) {
+        // TODO(rbyers): Most of this doesn't appear to take into account that each
+        // SVGImage gets it's own Page instance.
         frameHost().consoleMessageStorage().clear();
         useCounter().didCommitLoad();
         deprecation().clearSuppression();

third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp

@@ -88,10 +88,13 @@ void WorkerGlobalScope::countDeprecation(UseCounter::Feature feature) const
     // (http://crbug.com/376039)
 
     DCHECK(isSharedWorkerGlobalScope() || isServiceWorkerGlobalScope() || isCompositorWorkerGlobalScope());
+    DCHECK(feature != UseCounter::PageDestruction);
+    DCHECK(feature < UseCounter::NumberOfFeatures);
+
     // For each deprecated feature, send console message at most once
     // per worker lifecycle.
-    if (!m_deprecationWarningBits.hasRecordedMeasurement(feature)) {
-        m_deprecationWarningBits.recordMeasurement(feature);
+    if (!m_deprecationWarningBits.quickGet(feature)) {
+        m_deprecationWarningBits.quickSet(feature);
         DCHECK(!Deprecation::deprecationMessage(feature).isEmpty());
         DCHECK(getExecutionContext());
         getExecutionContext()->addConsoleMessage(ConsoleMessage::create(DeprecationMessageSource, WarningMessageLevel, Deprecation::deprecationMessage(feature)));
@@ -296,6 +299,7 @@ WorkerGlobalScope::WorkerGlobalScope(const KURL& url, const String& userAgent, W
     , m_url(url)
     , m_userAgent(userAgent)
     , m_v8CacheOptions(V8CacheOptionsDefault)
+    , m_deprecationWarningBits(UseCounter::NumberOfFeatures)
     , m_scriptController(WorkerOrWorkletScriptController::create(this, thread->isolate()))
     , m_thread(thread)
     , m_closing(false)

third_party/WebKit/Source/core/workers/WorkerGlobalScope.h

@@ -163,7 +163,7 @@ private:
     mutable Member<WorkerLocation> m_location;
     mutable Member<WorkerNavigator> m_navigator;
 
-    mutable UseCounter::CountBits m_deprecationWarningBits;
+    mutable BitVector m_deprecationWarningBits;
 
     Member<WorkerOrWorkletScriptController> m_scriptController;
     WorkerThread* m_thread;