Improve activity log ad metrics

chrome/browser/extensions/activity_log/activity_actions.cc

Index: chrome/browser/extensions/activity_log/activity_actions.cc
diff --git a/chrome/browser/extensions/activity_log/activity_actions.cc b/chrome/browser/extensions/activity_log/activity_actions.cc
index def31528c0637518de5b58d37738ace0a38c59a8..b861c271b850ba74b1152f6e636b0c280b7ff60e 100644
--- a/chrome/browser/extensions/activity_log/activity_actions.cc
+++ b/chrome/browser/extensions/activity_log/activity_actions.cc
@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
+#include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -40,34 +41,18 @@ namespace keys = ad_injection_constants::keys;
 
 // The list of APIs for which we upload the URL to RAPPOR.
 const char* kApisForRapporMetric[] = {
-  "HTMLIFrameElement.src",
-  "HTMLEmbedElement.src",
-  "HTMLAnchorElement.href",
+  ad_injection_constants::kHtmlIframeSrcApiName,
+  ad_injection_constants::kHtmlEmbedSrcApiName,
+  ad_injection_constants::kHtmlAnchorHrefApiName
 };
 
 const char* kExtensionAdInjectionRapporMetricName =
     "Extensions.PossibleAdInjection";
 
-// The elements for which we check the 'src' attribute to look for ads.
-const char* kSrcElements[] = {
-  "HTMLIFrameElement",
-  "HTMLEmbedElement"
-};
-
-// The elements for which we check the 'href' attribute to look for ads.
-const char* kHrefElements[] = {
-  "HTMLAnchorElement",
-};
-
-bool IsSrcElement(const std::string& str) {
-  static const char** end = kSrcElements + arraysize(kSrcElements);
-  return std::find(kSrcElements, end, str) != end;
-}
-
-bool IsHrefElement(const std::string& str) {
-  static const char** end = kHrefElements + arraysize(kHrefElements);
-  return std::find(kHrefElements, end, str) != end;
-}
+// The names of different types of HTML elements we check for ad injection.
+const char* kIframeElementType = "HTMLIFrameElement";
+const char* kEmbedElementType = "HTMLEmbedElement";
+const char* kAnchorElementType = "HTMLAnchorElement";
 
 std::string Serialize(const base::Value* value) {
   std::string value_as_text;
@@ -80,52 +65,6 @@ std::string Serialize(const base::Value* value) {
   return value_as_text;
 }
 
-Action::InjectionType CheckDomObject(const base::DictionaryValue* object) {
-  std::string type;
-  object->GetString(keys::kType, &type);
-
-  std::string url_key;
-  if (IsSrcElement(type))
-    url_key = keys::kSrc;
-  else if (IsHrefElement(type))
-    url_key = keys::kHref;
-
-  if (!url_key.empty()) {
-    std::string url;
-    if (object->GetString(url_key, &url)) {
-      GURL gurl(url);
-      if (AdNetworkDatabase::Get()->IsAdNetwork(gurl))
-        return Action::INJECTION_NEW_AD;
-      // If the extension injected an URL which is not local to itself, there is
-      // a good chance it could be a new ad, and our database missed it.
-      // This could be noisier than other metrics, because there are perfectly
-      // acceptable uses for this, like "Show my mail".
-      if (gurl.is_valid() &&
-          !gurl.is_empty() &&
-          !gurl.SchemeIs(kExtensionScheme)) {
-        return Action::INJECTION_LIKELY_NEW_AD;
-      }
-    }
-  }
-
-  const base::ListValue* children = NULL;
-  if (object->GetList(keys::kChildren, &children)) {
-    const base::DictionaryValue* child = NULL;
-    for (size_t i = 0;
-         i < children->GetSize() &&
-             i < ad_injection_constants::kMaximumChildrenToCheck;
-         ++i) {
-      if (children->GetDictionary(i, &child)) {
-        Action::InjectionType type = CheckDomObject(child);
-        if (type != Action::NO_AD_INJECTION)
-          return type;
-      }
-    }
-  }
-
-  return Action::NO_AD_INJECTION;
-}
-
 }  // namespace
 
 using api::activity_log_private::ExtensionActivity;
@@ -174,17 +113,29 @@ Action::InjectionType Action::DidInjectAd(
   if (!AdNetworkDatabase::Get())
     return NO_AD_INJECTION;
 
-  if (api_name_ == ad_injection_constants::kHtmlIframeSrcApiName ||
-      api_name_ == ad_injection_constants::kHtmlEmbedSrcApiName ||
-      api_name_ == ad_injection_constants::kHtmlAnchorHrefApiName) {
-    return CheckSrcModification();
+  AdType ad_type = AD_TYPE_NONE;
+  InjectionType injection_type = NO_AD_INJECTION;
+  if (api_name_ == ad_injection_constants::kHtmlIframeSrcApiName)
+    ad_type = AD_TYPE_IFRAME;
+  else if (api_name_ == ad_injection_constants::kHtmlEmbedSrcApiName)
+    ad_type = AD_TYPE_EMBED;
+  else if (api_name_ == ad_injection_constants::kHtmlAnchorHrefApiName)
+    ad_type = AD_TYPE_ANCHOR;
+
+  if (ad_type != AD_TYPE_NONE) {  // All the above apis modify an element's src.

[felt, 2014/05/24 00:07:10]

nit: does it make sense to flip this else/if condition now?

[Devlin, 2014/05/27 16:05:16]

Switched 'em up in a way that I think makes it more clear what's being checked.

+    injection_type = CheckSrcModification();
   } else if (EndsWith(api_name_,
                       ad_injection_constants::kAppendChildApiSuffix,
                       true /* case senstive */)) {
-    return CheckAppendChild();
+    injection_type = CheckAppendChild(&ad_type);
   }
 
-  return NO_AD_INJECTION;
+  if (injection_type != NO_AD_INJECTION) {
+    UMA_HISTOGRAM_ENUMERATION(
+        "Extensions.AdInjection.Type", ad_type, Action::NUM_AD_TYPES);
+  }
+
+  return injection_type;
 }
 
 void Action::set_args(scoped_ptr<base::ListValue> args) {
@@ -403,16 +354,19 @@ std::string Action::PrintForDebug() const {
   return result;
 }
 
-void Action::MaybeUploadUrl(rappor::RapporService* rappor_service) const {
-  // If there's no given |rappor_service|, abort immediately.
-  if (!rappor_service)
-    return;
+bool Action::UrlCouldBeAd(const GURL& url) const {

[felt, 2014/05/24 00:07:10]

this is a good idea

+  // Ads can only be valid urls that don't match the page's host (linking to the
+  // current page should be considered valid use), and aren't local to the
+  // extension.
+  return url.is_valid() &&
+         !url.is_empty() &&
+         url.host() != page_url_.host() &&
+         !url.SchemeIs(kExtensionScheme);
+}
 
-  // If the action has no url, or the url is empty, then return.
-  if (!arg_url_.is_valid() || arg_url_.is_empty())
-    return;
-  std::string host = arg_url_.host();
-  if (host.empty())
+void Action::MaybeUploadUrl(rappor::RapporService* rappor_service) const {
+  // Don't bother recording if the url is innocuous (or no |rappor_service|).
+  if (!rappor_service || !UrlCouldBeAd(arg_url_))
     return;
 
   bool can_inject_ads = false;
@@ -429,13 +383,13 @@ void Action::MaybeUploadUrl(rappor::RapporService* rappor_service) const {
   // Record the URL - an ad *may* have been injected.
   rappor_service->RecordSample(kExtensionAdInjectionRapporMetricName,
                                rappor::ETLD_PLUS_ONE_RAPPOR_TYPE,
-                               host);
+                               arg_url_.host());
 }
 
 Action::InjectionType Action::CheckSrcModification() const {
   const AdNetworkDatabase* database = AdNetworkDatabase::Get();
 
-  bool arg_url_valid = arg_url_.is_valid() && !arg_url_.is_empty();
+  bool arg_url_could_be_ad = UrlCouldBeAd(arg_url_);
 
   GURL prev_url;
   std::string prev_url_string;
@@ -444,7 +398,7 @@ Action::InjectionType Action::CheckSrcModification() const {
 
   bool prev_url_valid = prev_url.is_valid() && !prev_url.is_empty();
 
-  bool injected_ad = arg_url_valid && database->IsAdNetwork(arg_url_);
+  bool injected_ad = arg_url_could_be_ad && database->IsAdNetwork(arg_url_);
   bool replaced_ad = prev_url_valid && database->IsAdNetwork(prev_url);
 
   if (injected_ad && replaced_ad)
@@ -457,7 +411,7 @@ Action::InjectionType Action::CheckSrcModification() const {
   // If the extension modified the URL with an external, valid URL then there's
   // a good chance it's ad injection. Log it as a likely one, which also helps
   // us determine the effectiveness of our IsAdNetwork() recognition.
-  if (arg_url_valid && !arg_url_.SchemeIs(kExtensionScheme)) {
+  if (arg_url_could_be_ad) {
     if (prev_url_valid)
       return INJECTION_LIKELY_REPLACED_AD;
     return INJECTION_LIKELY_NEW_AD;
@@ -466,12 +420,66 @@ Action::InjectionType Action::CheckSrcModification() const {
   return NO_AD_INJECTION;
 }
 
-Action::InjectionType Action::CheckAppendChild() const {
+Action::InjectionType Action::CheckAppendChild(AdType* ad_type_out) const {
   const base::DictionaryValue* child = NULL;
   if (!args_->GetDictionary(0u, &child))
     return NO_AD_INJECTION;
 
-  return CheckDomObject(child);
+  return CheckDomObject(child, ad_type_out);
+}
+
+Action::InjectionType Action::CheckDomObject(
+    const base::DictionaryValue* object,
+    AdType* ad_type_out) const {
+  DCHECK(ad_type_out);
+  std::string type;
+  object->GetString(keys::kType, &type);
+
+  AdType ad_type = AD_TYPE_NONE;
+  std::string url_key;
+  if (type == kIframeElementType) {
+    ad_type = AD_TYPE_IFRAME;
+    url_key = keys::kSrc;
+  } else if (type == kEmbedElementType) {
+    ad_type = AD_TYPE_EMBED;
+    url_key = keys::kSrc;
+  } else if (type == kAnchorElementType) {
+    ad_type = AD_TYPE_ANCHOR;
+    url_key = keys::kHref;
+  }
+
+  if (!url_key.empty()) {
+    std::string url;
+    if (object->GetString(url_key, &url)) {
+      GURL gurl(url);
+      if (UrlCouldBeAd(gurl)) {
+        *ad_type_out = ad_type;
+        if (AdNetworkDatabase::Get()->IsAdNetwork(gurl))
+          return INJECTION_NEW_AD;
+        // If the extension injected an URL which is not local to itself or the
+        // page, there is a good chance it could be a new ad, and our database
+        // missed it.
+        return INJECTION_LIKELY_NEW_AD;
+      }
+    }
+  }
+
+  const base::ListValue* children = NULL;
+  if (object->GetList(keys::kChildren, &children)) {
+    const base::DictionaryValue* child = NULL;
+    for (size_t i = 0;
+         i < children->GetSize() &&
+             i < ad_injection_constants::kMaximumChildrenToCheck;
+         ++i) {
+      if (children->GetDictionary(i, &child)) {
+        InjectionType type = CheckDomObject(child, ad_type_out);
+        if (type != NO_AD_INJECTION)
+          return type;
+      }
+    }
+  }
+
+  return NO_AD_INJECTION;
 }
 
 bool ActionComparator::operator()(