Adding UseCounter specific for extensions

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

Index: third_party/WebKit/Source/core/frame/UseCounterTest.cpp
diff --git a/third_party/WebKit/Source/core/frame/UseCounterTest.cpp b/third_party/WebKit/Source/core/frame/UseCounterTest.cpp
index f8c491e322df036683e25c11a0283017b024ee92..1dd55e610b2047706deafd7a29b6a6c6e8b35ded 100644
--- a/third_party/WebKit/Source/core/frame/UseCounterTest.cpp
+++ b/third_party/WebKit/Source/core/frame/UseCounterTest.cpp
@@ -16,6 +16,8 @@ const char* const kFeaturesHistogramName = "Blink.UseCounter.Features";
 const char* const kCSSHistogramName = "Blink.UseCounter.CSSProperties";
 const char* const kAnimatedCSSHistogramName =
     "Blink.UseCounter.AnimatedCSSProperties";
+const char* const kExtensionFeaturesHistogramName =
+    "Blink.UseCounter.Extensions.Features";
 
 const char* const kSVGFeaturesHistogramName =

[Alexei Svitkine (slow), 2017/05/10 15:17:30]

Nit: Use const char kSVGFeaturesHistogramName[] = syntax.

This is the preferred way to declare string constants (it results in more
compact/efficient code iirc). Please change the other entries in this file to be
consistent.

[Nico, 2017/05/10 15:38:20]

Oh, good catch. To elaborate a bit on this:

const char kFoo[] = ...

is a bit more efficient than

const char* kFoo = ...

since in that case the pointer isn't const, only the data it points to is. It's
the same as

const char* const kFoo = ...

(which this file uses), but it's more compact and looks less strange than the
double const.

[lunalu1, 2017/05/10 19:39:39]

Makes sense! Thanks for the insight =)

Although if in the case of multiple identical strings are defined. Compiler is
required to provide separate copies of arrays for const kFoo[], but not for
pointer variables.

But we are not dealing with the case above. So I will make it more code
efficient as suggested :)

     "Blink.UseCounter.SVGImage.Features";
@@ -27,6 +29,20 @@ const char* const kSVGAnimatedCSSHistogramName =
 const char* const kLegacyFeaturesHistogramName = "WebCore.FeatureObserver";
 const char* const kLegacyCSSHistogramName =
     "WebCore.FeatureObserver.CSSProperties";
+
+// Sorted list of all histograms to be tested.
+const std::vector<std::string>& all_histograms = {

[Nico, 2017/05/09 19:36:46]

1) This is just a vector, why is it a reference?
2) since it's const, call it kAllHistograms
3) Global constructors aren't great. Sure, this just just a test, but you only
use it in one place, why not put the definition right in front of its use?
4) You don't even need that this is a vector, you can use
std::initializer_list<>, or you can just say (`for (auto histogram : {
kAnimat...})`)

[lunalu1, 2017/05/10 19:39:39]

I see. Thanks!

+    kAnimatedCSSHistogramName,       kCSSHistogramName,
+    kExtensionFeaturesHistogramName, kFeaturesHistogramName,
+    kSVGAnimatedCSSHistogramName,    kSVGCSSHistogramName,
+    kSVGFeaturesHistogramName};
+
+int GetPageVisitsBucketforHistogram(std::string histogram_name) {

[Alexei Svitkine (slow), 2017/05/10 15:17:30]

const std::string&

+  if (histogram_name.find("CSS") == std::string::npos)
+    return blink::UseCounter::kPageVisits;
+  // For CSS histograms, the page visits bucket should be 1.
+  return 1;
+}
 }

[Alexei Svitkine (slow), 2017/05/10 15:17:30]

Nit: "  // namespace"

Also maybe add inner empty lines within the namespace since the namespace
content is now non-trivial.

[lunalu1, 2017/05/10 19:39:39]

Sorry my bad.

 
 namespace blink {
@@ -34,17 +50,17 @@ namespace blink {
 template <typename T>
 void HistogramBasicTest(const std::string& histogram,
                         const std::string& legacy_histogram,
-                        const std::vector<std::string>& unaffected_histograms,
                         T item,
                         T second_item,
                         std::function<bool(T)> counted,
                         std::function<void(T)> count,
                         std::function<int(T)> histogram_map,
                         std::function<void(KURL)> did_commit_load,
-                        const std::string& url,
-                        int page_visit_bucket) {
+                        const std::string& url) {
   HistogramTester histogram_tester;
 
+  int page_visit_bucket = GetPageVisitsBucketforHistogram(histogram);
+
   // Test recording a single (arbitrary) counter
   EXPECT_FALSE(counted(item));
   count(item);
@@ -109,18 +125,21 @@ void HistogramBasicTest(const std::string& histogram,
     histogram_tester.ExpectTotalCount(legacy_histogram, 5);
   }
 
-  for (size_t i = 0; i < unaffected_histograms.size(); ++i) {
-    histogram_tester.ExpectTotalCount(unaffected_histograms[i], 0);
+  for (const auto unaffected_histogram : all_histograms) {

[Alexei Svitkine (slow), 2017/05/10 15:17:30]

const auto& or const std::string&

Otherwise, this copies the string in each iteration to the var.

[lunalu1, 2017/05/10 19:39:39]

Done.

+    if (unaffected_histogram == histogram)
+      continue;
+
+    histogram_tester.ExpectTotalCountExcept(
+        unaffected_histogram,
+        GetPageVisitsBucketforHistogram(unaffected_histogram), 0);
   }
 }
 
 TEST(UseCounterTest, RecordingFeatures) {
   UseCounter use_counter;
   HistogramBasicTest<UseCounter::Feature>(
-      kFeaturesHistogramName, kLegacyFeaturesHistogramName,
-      {kSVGFeaturesHistogramName, kSVGCSSHistogramName,
-       kSVGAnimatedCSSHistogramName},
-      UseCounter::kFetch, UseCounter::kFetchBodyStream,
+      kFeaturesHistogramName, kLegacyFeaturesHistogramName, UseCounter::kFetch,
+      UseCounter::kFetchBodyStream,
       [&](UseCounter::Feature feature) -> bool {
         return use_counter.HasRecordedMeasurement(feature);
       },
@@ -129,16 +148,14 @@ TEST(UseCounterTest, RecordingFeatures) {
       },
       [](UseCounter::Feature feature) -> int { return feature; },
       [&](KURL kurl) { use_counter.DidCommitLoad(kurl); },
-      "https://dummysite.com/", UseCounter::kPageVisits);
+      "https://dummysite.com/");
 }
 
 TEST(UseCounterTest, RecordingCSSProperties) {
   UseCounter use_counter;
   HistogramBasicTest<CSSPropertyID>(
-      kCSSHistogramName, kLegacyCSSHistogramName,
-      {kSVGFeaturesHistogramName, kSVGCSSHistogramName,
-       kSVGAnimatedCSSHistogramName},
-      CSSPropertyFont, CSSPropertyZoom,
+      kCSSHistogramName, kLegacyCSSHistogramName, CSSPropertyFont,
+      CSSPropertyZoom,
       [&](CSSPropertyID property) -> bool {
         return use_counter.IsCounted(property);
       },
@@ -149,15 +166,13 @@ TEST(UseCounterTest, RecordingCSSProperties) {
         return UseCounter::MapCSSPropertyIdToCSSSampleIdForHistogram(property);
       },
       [&](KURL kurl) { use_counter.DidCommitLoad(kurl); },
-      "https://dummysite.com/", 1 /* page visit bucket */);
+      "https://dummysite.com/");
 }
 
 TEST(UseCounterTest, RecordingAnimatedCSSProperties) {
   UseCounter use_counter;
   HistogramBasicTest<CSSPropertyID>(
-      kAnimatedCSSHistogramName, "",
-      {kSVGCSSHistogramName, kSVGAnimatedCSSHistogramName}, CSSPropertyOpacity,
-      CSSPropertyVariable,
+      kAnimatedCSSHistogramName, "", CSSPropertyOpacity, CSSPropertyVariable,
       [&](CSSPropertyID property) -> bool {
         return use_counter.IsCountedAnimatedCSS(property);
       },
@@ -166,14 +181,29 @@ TEST(UseCounterTest, RecordingAnimatedCSSProperties) {
         return UseCounter::MapCSSPropertyIdToCSSSampleIdForHistogram(property);
       },
       [&](KURL kurl) { use_counter.DidCommitLoad(kurl); },
-      "https://dummysite.com/", 1 /* page visit bucket */);
+      "https://dummysite.com/");
+}
+
+TEST(UseCounterTest, RecordingExtensions) {
+  UseCounter use_counter(UseCounter::kExtensionContext);
+  HistogramBasicTest<UseCounter::Feature>(
+      kExtensionFeaturesHistogramName, kLegacyFeaturesHistogramName,
+      UseCounter::kFetch, UseCounter::kFetchBodyStream,
+      [&](UseCounter::Feature feature) -> bool {
+        return use_counter.HasRecordedMeasurement(feature);
+      },
+      [&](UseCounter::Feature feature) {
+        use_counter.RecordMeasurement(feature);
+      },
+      [](UseCounter::Feature feature) -> int { return feature; },
+      [&](KURL kurl) { use_counter.DidCommitLoad(kurl); },
+      "chrome-extension://dummysite");
 }
 
 TEST(UseCounterTest, SVGImageContextFeatures) {
   UseCounter use_counter(UseCounter::kSVGImageContext);
   HistogramBasicTest<UseCounter::Feature>(
       kSVGFeaturesHistogramName, kLegacyFeaturesHistogramName,
-      {kFeaturesHistogramName, kCSSHistogramName, kAnimatedCSSHistogramName},
       UseCounter::kSVGSMILAdditiveAnimation,
       UseCounter::kSVGSMILAnimationElementTiming,
       [&](UseCounter::Feature feature) -> bool {
@@ -183,17 +213,15 @@ TEST(UseCounterTest, SVGImageContextFeatures) {
         use_counter.RecordMeasurement(feature);
       },
       [](UseCounter::Feature feature) -> int { return feature; },
-      [&](KURL kurl) { use_counter.DidCommitLoad(kurl); }, "about:blank",
-      // In practice SVGs always appear to be loaded with an about:blank URL
-      UseCounter::kPageVisits);
+      [&](KURL kurl) { use_counter.DidCommitLoad(kurl); }, "about:blank");
+  // In practice SVGs always appear to be loaded with an about:blank URL

[Nico, 2017/05/09 19:36:46]

The comment looks strange here. Maybe do

// In practice, SVGs always appear to be loaded with an about:blank URL
const char kSvgUrl; = "about:blank";

at the top of the file and then pass kSvgUrl without the comment everywhere the
comment is currently used?

[lunalu1, 2017/05/10 19:39:39]

Done.
I did the same for HttpsUrl and ExtensionUrl (both referenced more than once).

 }
 
 TEST(UseCounterTest, SVGImageContextCSSProperties) {
   UseCounter use_counter(UseCounter::kSVGImageContext);
   HistogramBasicTest<CSSPropertyID>(
-      kSVGCSSHistogramName, kLegacyCSSHistogramName,
-      {kFeaturesHistogramName, kCSSHistogramName, kAnimatedCSSHistogramName},
-      CSSPropertyFont, CSSPropertyZoom,
+      kSVGCSSHistogramName, kLegacyCSSHistogramName, CSSPropertyFont,
+      CSSPropertyZoom,
       [&](CSSPropertyID property) -> bool {
         return use_counter.IsCounted(property);
       },
@@ -203,17 +231,15 @@ TEST(UseCounterTest, SVGImageContextCSSProperties) {
       [](CSSPropertyID property) -> int {
         return UseCounter::MapCSSPropertyIdToCSSSampleIdForHistogram(property);
       },
-      [&](KURL kurl) { use_counter.DidCommitLoad(kurl); }, "about:blank",
+      [&](KURL kurl) { use_counter.DidCommitLoad(kurl); }, "about:blank"
       // In practice SVGs always appear to be loaded with an about:blank URL
-      1 /* page visit bucket */);
+      );
 }
 
 TEST(UseCounterTest, SVGImageContextAnimatedCSSProperties) {
   UseCounter use_counter(UseCounter::kSVGImageContext);
   HistogramBasicTest<CSSPropertyID>(
-      kSVGAnimatedCSSHistogramName, "",
-      {kCSSHistogramName, kAnimatedCSSHistogramName}, CSSPropertyOpacity,
-      CSSPropertyVariable,
+      kSVGAnimatedCSSHistogramName, "", CSSPropertyOpacity, CSSPropertyVariable,
       [&](CSSPropertyID property) -> bool {
         return use_counter.IsCountedAnimatedCSS(property);
       },
@@ -221,9 +247,9 @@ TEST(UseCounterTest, SVGImageContextAnimatedCSSProperties) {
       [](CSSPropertyID property) -> int {
         return UseCounter::MapCSSPropertyIdToCSSSampleIdForHistogram(property);
       },
-      [&](KURL kurl) { use_counter.DidCommitLoad(kurl); }, "about:blank",
+      [&](KURL kurl) { use_counter.DidCommitLoad(kurl); }, "about:blank"
       // In practice SVGs always appear to be loaded with an about:blank URL
-      1 /* page visit bucket */);
+      );
 }
 
 TEST(UseCounterTest, InspectorDisablesMeasurement) {