Add GetUserMedia ETLD+1 rappor metrics.

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

Index: third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp
diff --git a/third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp b/third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp
index 8277ba8423c8d111fd9994ea6c5ab40342b99bdf..ec70a55037507db20b4fcf0725b9b30758b22876 100644
--- a/third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp
+++ b/third_party/WebKit/Source/core/frame/HostsUsingFeatures.cpp
@@ -8,6 +8,7 @@
 #include "core/dom/Document.h"
 #include "core/frame/LocalDOMWindow.h"
 #include "core/page/Page.h"
+#include "platform/weborigin/KURL.h"
 #include "public/platform/Platform.h"
 
 namespace blink {
@@ -75,7 +76,7 @@ void HostsUsingFeatures::countName(Feature feature, const String& name)
 
 void HostsUsingFeatures::clear()
 {
-    m_hostAndValues.clear();
+    m_urlAndValues.clear();
     m_valueByName.clear();
 }
 
@@ -89,37 +90,37 @@ void HostsUsingFeatures::documentDetached(Document& document)
     if (!url.protocolIsInHTTPFamily())
         return;
 
-    m_hostAndValues.append(std::make_pair(url.host(), counter));
+    m_urlAndValues.append(std::make_pair(url.getString(), counter));

[jww, 2016/06/11 18:43:20]

nit: Per my comment in HostsUsingFeatures.h, I'd prefer that you store the KURL
in the pair rather than the serialized URL since the value is just turned back
into a KURL later.

[Guido Urdaneta, 2016/06/12 11:06:22]

Done.

     document.HostsUsingFeaturesValue().clear();
     DCHECK(document.HostsUsingFeaturesValue().isEmpty());
 }
 
 void HostsUsingFeatures::updateMeasurementsAndClear()
 {
-    if (!m_hostAndValues.isEmpty())
-        recordHostToRappor();
+    if (!m_urlAndValues.isEmpty())
+        recordURLToRappor();
     if (!m_valueByName.isEmpty())
         recordNamesToRappor();
 }
 
-void HostsUsingFeatures::recordHostToRappor()
+void HostsUsingFeatures::recordURLToRappor()
 {
-    DCHECK(!m_hostAndValues.isEmpty());
+    DCHECK(!m_urlAndValues.isEmpty());
 
-    // Aggregate values by hosts.
+    // Aggregate values by URLs.
     HashMap<String, HostsUsingFeatures::Value> aggregatedByHost;

[jww, 2016/06/11 18:43:20]

Things are a bit confused here, as represented by the variable name
|aggregatedByHost|. What you've written up here is, in fact, aggregating by URL,
not Host, but you left the variable name the same.

However, I think that you actually need both. That is, you need one aggregation
by Host for all of the old metrics, and then you need to aggregate by URL for
your one new metric. So maybe you still need recordHostToRappor() as a method in
addition to a new recordUrlToRappor()?

You'll also need to be careful in creating the new version of recordHostToRappor
since you'll have multiple URLs with potentially the same host, so you can't
just blindly add aggregatedByHost.add(urlAndValue.first.host()); you'll need to
check to see if the host is already in the HashMap and potentially add to it.

[Guido Urdaneta, 2016/06/12 11:06:22]

Fixed. Now there are separate functions to aggregate by URL and Host.

-    for (const auto& hostAndValue : m_hostAndValues) {
-        DCHECK(!hostAndValue.first.isEmpty());
-        auto result = aggregatedByHost.add(hostAndValue.first, hostAndValue.second);
+    for (const auto& urlAndValue : m_urlAndValues) {
+        DCHECK(!urlAndValue.first.isEmpty());
+        auto result = aggregatedByHost.add(urlAndValue.first, urlAndValue.second);
         if (!result.isNewEntry)
-            result.storedValue->value.aggregate(hostAndValue.second);
+            result.storedValue->value.aggregate(urlAndValue.second);
     }
 
     // Report to RAPPOR.
-    for (auto& hostAndValue : aggregatedByHost)
-        hostAndValue.value.recordHostToRappor(hostAndValue.key);
+    for (auto& urlAndValue : aggregatedByHost)
+        urlAndValue.value.recordURLToRappor(KURL(ParsedURLString, urlAndValue.key));
 
-    m_hostAndValues.clear();
+    m_urlAndValues.clear();
 }
 
 void HostsUsingFeatures::recordNamesToRappor()
@@ -137,32 +138,38 @@ void HostsUsingFeatures::Value::aggregate(HostsUsingFeatures::Value other)
     m_countBits |= other.m_countBits;
 }
 
-void HostsUsingFeatures::Value::recordHostToRappor(const String& host)
+void HostsUsingFeatures::Value::recordURLToRappor(const KURL& url)

[jww, 2016/06/11 18:43:21]

See my comments above in recordURLToRappor(), but I think you want this to
remain recordHostToRappor() and to add a new method recordURLRappor that only
counts up your URLs.

[Guido Urdaneta, 2016/06/12 11:06:22]

Done.

 {
     if (get(Feature::ElementCreateShadowRoot))
-        Platform::current()->recordRappor("WebComponents.ElementCreateShadowRoot", host);
+        Platform::current()->recordRappor("WebComponents.ElementCreateShadowRoot", url.host());
     if (get(Feature::ElementAttachShadow))
-        Platform::current()->recordRappor("WebComponents.ElementAttachShadow", host);
+        Platform::current()->recordRappor("WebComponents.ElementAttachShadow", url.host());
     if (get(Feature::DocumentRegisterElement))
-        Platform::current()->recordRappor("WebComponents.DocumentRegisterElement", host);
+        Platform::current()->recordRappor("WebComponents.DocumentRegisterElement", url.host());
     if (get(Feature::EventPath))
-        Platform::current()->recordRappor("WebComponents.EventPath", host);
+        Platform::current()->recordRappor("WebComponents.EventPath", url.host());
     if (get(Feature::DeviceMotionInsecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.DeviceMotion.Insecure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.DeviceMotion.Insecure", url.host());
     if (get(Feature::DeviceOrientationInsecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.DeviceOrientation.Insecure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.DeviceOrientation.Insecure", url.host());
     if (get(Feature::FullscreenInsecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.Fullscreen.Insecure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.Fullscreen.Insecure", url.host());
     if (get(Feature::GeolocationInsecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.Geolocation.Insecure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.Geolocation.Insecure", url.host());
     if (get(Feature::GetUserMediaInsecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.GetUserMedia.Insecure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.GetUserMedia.Insecure", url.host());
     if (get(Feature::GetUserMediaSecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.GetUserMedia.Secure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.GetUserMedia.Secure", url.host());
     if (get(Feature::ApplicationCacheManifestSelectInsecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.ApplicationCacheManifestSelect.Insecure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.ApplicationCacheManifestSelect.Insecure", url.host());
     if (get(Feature::ApplicationCacheAPIInsecureHost))
-        Platform::current()->recordRappor("PowerfulFeatureUse.Host.ApplicationCacheAPI.Insecure", host);
+        Platform::current()->recordRappor("PowerfulFeatureUse.Host.ApplicationCacheAPI.Insecure", url.host());
+
+    // Platform::recordRapporURL uses the ETLD+1 of the given URL

[jww, 2016/06/11 18:43:21]

nit: Would you mind updating the comment above Platform::recordRapporURL in
third_party/WebKit/public/platform/Platform.h to mention this as well? That way
when we forget all of this for the next metric we write up, it's a little more
discoverable ;-)

[Guido Urdaneta, 2016/06/12 11:06:22]

Done.

+    if (get(Feature::GetUserMediaInsecureETLDPlus1))
+        Platform::current()->recordRapporURL("PowerfulFeatureUse.ETLDPlus1.GetUserMedia.Insecure", WebURL(url));
+    if (get(Feature::GetUserMediaSecureETLDPlus1))
+        Platform::current()->recordRapporURL("PowerfulFeatureUse.ETLDPlus1.GetUserMedia.Secure", WebURL(url));
 }
 
 void HostsUsingFeatures::Value::recordNameToRappor(const String& name)