Add GetUserMedia ETLD+1 rappor metrics.

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/frame/HostsUsingFeatures.h"
 
 #include "bindings/core/v8/ScriptState.h"
 #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 {
 
 HostsUsingFeatures::~HostsUsingFeatures()
 {
     updateMeasurementsAndClear();
 }
 
 HostsUsingFeatures::Value::Value()
@@ skipping 47 matching lines @@
 }
 
 void HostsUsingFeatures::countName(Feature feature, const String& name)
 {
     auto result = m_valueByName.add(name, Value());
     result.storedValue->value.count(feature);
 }
 
 void HostsUsingFeatures::clear()
 {
-    m_hostAndValues.clear();
+    m_urlAndValues.clear();
     m_valueByName.clear();
 }
 
 void HostsUsingFeatures::documentDetached(Document& document)
 {
     HostsUsingFeatures::Value counter = document.HostsUsingFeaturesValue();
     if (counter.isEmpty())
         return;
 
     const KURL& url = document.url();
     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()
 {
     DCHECK(!m_valueByName.isEmpty());
 
     for (auto& nameAndValue : m_valueByName)
         nameAndValue.value.recordNameToRappor(nameAndValue.key);
 
     m_valueByName.clear();
 }
 
 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)
 {
     if (get(Feature::EventPath))
         Platform::current()->recordRappor("WebComponents.EventPath.Extensions", name);
 }
 
 } // namespace blink