Enabling AdInjectionBrowserTest after Activity Log refactoring.

Cleaning code that became redundant.

Review URL: https://codereview.chromium.org/384983003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284067 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/activity_log/activity_actions.cc

@@ -25,7 +25,6 @@
 #include "chrome/common/chrome_switches.h"
 #include "components/rappor/rappor_service.h"
 #include "content/public/browser/web_contents.h"
-#include "extensions/common/ad_injection_constants.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/dom_action_types.h"
 #include "sql/statement.h"
@@ -37,24 +36,19 @@ namespace extensions {
 
 namespace {
 
-namespace keys = ad_injection_constants::keys;
-
-// The list of APIs for which we upload the URL to RAPPOR.
-const char* kApisForRapporMetric[] = {
-  ad_injection_constants::kHtmlIframeSrcApiName,
-  ad_injection_constants::kHtmlEmbedSrcApiName,
-  ad_injection_constants::kHtmlAnchorHrefApiName
-};
-
 // The "Extensions.PossibleAdInjection2" metric uses different Rappor
 // parameters than the original metric.
 const char* kExtensionAdInjectionRapporMetricName =
     "Extensions.PossibleAdInjection2";
 
-// 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";
+const char kBlinkSetAttributeEvent[] = "blinkSetAttribute";
+const char kBlinkAddElementEvent[] = "blinkAddElement";
+
+const char kIframe[] = "iframe";
+const char kAnchor[] = "a";
+
+const char kSrc[] = "src";
+const char kHref[] = "href";
 
 std::string Serialize(const base::Value* value) {
   std::string value_as_text;
@@ -120,21 +114,31 @@ Action::InjectionType Action::DidInjectAd(
   AdType ad_type = AD_TYPE_NONE;
   InjectionType injection_type = NO_AD_INJECTION;
 
-  if (EndsWith(api_name_,
-               ad_injection_constants::kAppendChildApiSuffix,
-               true /* case senstive */)) {
-    injection_type = CheckAppendChild(&ad_type);
-  } else {
-    // Check if the action modified an element's src/href.
-    if (api_name_ == ad_injection_constants::kHtmlIframeSrcApiName)
+  if (api_name_ == kBlinkSetAttributeEvent) {
+    std::string element_name;
+    std::string attr_name;
+    if (args_.get()) {
+      args_->GetString(0u, &element_name);
+      args_->GetString(1u, &attr_name);
+    }
+    if (element_name == kIframe && attr_name == kSrc)
+      ad_type = AD_TYPE_IFRAME;
+    else if (element_name == kAnchor && attr_name == kHref)
+      ad_type = AD_TYPE_ANCHOR;
+
+    if (ad_type != AD_TYPE_NONE)
+      injection_type = CheckAttrModification();
+  } else if (api_name_ == kBlinkAddElementEvent) {
+    std::string element_name;
+    if (args_.get())
+      args_->GetString(0u, &element_name);
+    if (element_name == kIframe)
       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)
+    else if (element_name == kAnchor)
       ad_type = AD_TYPE_ANCHOR;
 
     if (ad_type != AD_TYPE_NONE)
-      injection_type = CheckSrcModification();
+      injection_type = CheckElementAddition();
   }
 
   if (injection_type != NO_AD_INJECTION) {
@@ -373,39 +377,69 @@ bool Action::UrlCouldBeAd(const GURL& url) const {
 
 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_))
+  if (!rappor_service)
     return;
 
-  bool can_inject_ads = false;
-  for (size_t i = 0; i < arraysize(kApisForRapporMetric); ++i) {
-    if (api_name_ == kApisForRapporMetric[i]) {
-      can_inject_ads = true;
-      break;
+  GURL url;
+
+  if (api_name_ == kBlinkSetAttributeEvent) {
+    std::string element_name;
+    std::string attr_name;
+    std::string url_string;
+    if (args_.get()) {
+      args_->GetString(0u, &element_name);
+      args_->GetString(1u, &attr_name);
+    }
+    if (element_name == kIframe && attr_name == kSrc) {
+      args_->GetString(3u, &url_string);
+      url = GURL(url_string);
+    } else if (element_name == kAnchor && attr_name == kHref) {
+      args_->GetString(3u, &url_string);
+      url = GURL(url_string);
+    }
+  } else if (api_name_ == kBlinkAddElementEvent) {
+    std::string element_name;
+    std::string url_string;
+    if (args_.get())
+      args_->GetString(0u, &element_name);
+    if (element_name == kIframe) {
+      args_->GetString(1u, &url_string);
+      url = GURL(url_string);
+    } else if (element_name == kAnchor) {
+      args_->GetString(1u, &url_string);
+      url = GURL(url_string);
     }
   }
 
-  if (!can_inject_ads)
+  if (!UrlCouldBeAd(url))
     return;
 
   // Record the URL - an ad *may* have been injected.
   rappor_service->RecordSample(kExtensionAdInjectionRapporMetricName,
                                rappor::ETLD_PLUS_ONE_RAPPOR_TYPE,
-                               arg_url_.host());
+                               url.host());
 }
 
-Action::InjectionType Action::CheckSrcModification() const {
-  const AdNetworkDatabase* database = AdNetworkDatabase::Get();
+Action::InjectionType Action::CheckAttrModification() const {
+  if (api_name_ != kBlinkSetAttributeEvent)
+    return NO_AD_INJECTION;
 
-  bool arg_url_could_be_ad = UrlCouldBeAd(arg_url_);
+  const AdNetworkDatabase* database = AdNetworkDatabase::Get();
 
   GURL prev_url;
   std::string prev_url_string;
-  if (args_.get() && args_->GetString(1u, &prev_url_string))
+  if (args_.get() && args_->GetString(2u, &prev_url_string))
     prev_url = GURL(prev_url_string);
 
+  GURL new_url;
+  std::string new_url_string;
+  if (args_.get() && args_->GetString(3u, &new_url_string))
+    new_url = GURL(new_url_string);
+
+  bool new_url_could_be_ad = UrlCouldBeAd(new_url);
   bool prev_url_valid = prev_url.is_valid() && !prev_url.is_empty();
 
-  bool injected_ad = arg_url_could_be_ad && database->IsAdNetwork(arg_url_);
+  bool injected_ad = new_url_could_be_ad && database->IsAdNetwork(new_url);
   bool replaced_ad = prev_url_valid && database->IsAdNetwork(prev_url);
 
   if (injected_ad && replaced_ad)
@@ -418,7 +452,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_could_be_ad) {
+  if (new_url_could_be_ad) {
     if (prev_url_valid)
       return INJECTION_LIKELY_REPLACED_AD;
     return INJECTION_LIKELY_NEW_AD;
@@ -427,65 +461,23 @@ Action::InjectionType Action::CheckSrcModification() const {
   return NO_AD_INJECTION;
 }
 
-Action::InjectionType Action::CheckAppendChild(AdType* ad_type_out) const {
-  const base::DictionaryValue* child = NULL;
-  if (!args_->GetDictionary(0u, &child))
+Action::InjectionType Action::CheckElementAddition() const {
+  if (api_name_ != kBlinkAddElementEvent)
     return NO_AD_INJECTION;
 
-  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;
-      }
-    }
+  GURL url;
+  std::string url_string;
+  if (args_.get() && args_->GetString(1u, &url_string))
+    url = GURL(url_string);
+
+  if (UrlCouldBeAd(url)) {
+    if (AdNetworkDatabase::Get()->IsAdNetwork(url))
+      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;
   }
-
   return NO_AD_INJECTION;
 }
 

chrome/browser/extensions/activity_log/activity_actions.h

@@ -185,17 +185,11 @@ class Action : public base::RefCountedThreadSafe<Action> {
   // ad injection.
   void MaybeUploadUrl(rappor::RapporService* rappor_service) const;
 
-  // Checks an action that modified the src of an element for ad injection.
-  InjectionType CheckSrcModification() const;
-  // Checks an action with the appendChild API for ad injection.
-  // |ad_type_out| is populated with the type of ad which was injected, if there
-  // was an injection.
-  InjectionType CheckAppendChild(AdType* ad_type_out) const;
-  // Checks a DOM object (e.g. an appended child) for ad injection.
-  // |ad_type_out| is populated with the type of ad which was injected, if there
-  // was an injection.
-  InjectionType CheckDomObject(const base::DictionaryValue* object,
-                               AdType* ad_type_out) const;
+  // Checks an action that modified the src or href of an element for ad
+  // injection.
+  InjectionType CheckAttrModification() const;
+  // Checks an action that adds an element for ad injection.
+  InjectionType CheckElementAddition() const;
 
   std::string extension_id_;
   base::Time time_;

chrome/browser/extensions/activity_log/ad_injection_browsertest.cc

@@ -352,9 +352,7 @@ testing::AssertionResult AdInjectionBrowserTest::HandleJSError(
 // content script cleans up after itself. For significantly more detailed
 // comments, see
 // chrome/test/data/extensions/activity_log/ad_injection/content_script.js.
-// TODO(pmarch): Enable this test once modifications to Blink are done,
-// crbug.com/388287.
-IN_PROC_BROWSER_TEST_F(AdInjectionBrowserTest, DISABLED_DetectAdInjections) {
+IN_PROC_BROWSER_TEST_F(AdInjectionBrowserTest, DetectAdInjections) {
   const Extension* extension = LoadExtension(test_data_dir_);
   ASSERT_TRUE(extension);
 

chrome/browser/extensions/activity_log/ad_injection_unittest.cc

@@ -7,18 +7,26 @@
 #include "components/rappor/byte_vector_utils.h"
 #include "components/rappor/proto/rappor_metric.pb.h"
 #include "components/rappor/rappor_service.h"
+#include "extensions/common/value_builder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace extensions {
 
 namespace {
 
-scoped_refptr<Action> CreateAction(const std::string& api_name) {
+scoped_refptr<Action> CreateAction(const std::string& api_name,
+                                   const std::string& element,
+                                   const std::string& attr) {
   scoped_refptr<Action> action = new Action("id",
                                             base::Time::Now(),
                                             Action::ACTION_DOM_ACCESS,
                                             api_name);
-  action->set_arg_url(GURL("http://www.notarealhost.notarealtld"));
+  action->set_args(ListBuilder()
+                   .Append(element)
+                   .Append(attr)
+                   .Append("http://www.google.co.uk")
+                   .Append("http://www.google.co.uk")
+                   .Build());
   return action;
 }
 
@@ -58,24 +66,19 @@ TEST(AdInjectionUnittest, CheckActionForAdInjectionTest) {
   EXPECT_EQ(0, reports.report_size());
 
   scoped_refptr<Action> modify_iframe_src =
-      CreateAction("HTMLIFrameElement.src");
+      CreateAction("blinkSetAttribute", "iframe", "src");
   modify_iframe_src->DidInjectAd(&rappor_service);
   reports = rappor_service.GetReports();
   EXPECT_EQ(1, reports.report_size());
 
-  scoped_refptr<Action> modify_embed_src =
-      CreateAction("HTMLEmbedElement.src");
-  modify_embed_src->DidInjectAd(&rappor_service);
-  reports = rappor_service.GetReports();
-  EXPECT_EQ(1, reports.report_size());
-
   scoped_refptr<Action> modify_anchor_href =
-      CreateAction("HTMLAnchorElement.href");
+      CreateAction("blinkSetAttribute", "a", "href");
   modify_anchor_href->DidInjectAd(&rappor_service);
   reports = rappor_service.GetReports();
   EXPECT_EQ(1, reports.report_size());
 
-  scoped_refptr<Action> harmless_action = CreateAction("Location.replace");
+  scoped_refptr<Action> harmless_action =
+      CreateAction("Location.replace", "", "");
   harmless_action->DidInjectAd(&rappor_service);
   reports = rappor_service.GetReports();
   EXPECT_EQ(0, reports.report_size());

chrome/test/data/extensions/activity_log/ad_injection/content_script.js

@@ -31,8 +31,6 @@
 var kBodyHtml =
     '<iframe id="ad-iframe" src="http://www.known-ads.adnetwork"></iframe>' +
     '<iframe id="non-ad-iframe" src="http://www.not-ads.adnetwork"></iframe>' +
-    '<embed id="ad-embed" src="http://www.known-ads.adnetwork"><embed>' +
-    '<embed id="non-ad-embed" src="http://www.not-ads.adnetwork"><embed>' +
     '<a id="ad-anchor" href="http://www.known-ads.adnetwork"></a>' +
     '<a id="non-ad-anchor" href="http://www.not-ads.adnetwork"></a>' +
     '<div id="empty-div"></div>';
@@ -43,8 +41,8 @@ var kBodyHtml =
  * - Signal the C++ test that we're going to do page setup, and we shouldn't
  *   record any ad-injector events (as some of the page setup could qualify).
  * - Do the page setup, which involves creating a 'playground' for ad injectors
- *   of iframes, embeds, etc. See also ad_injectors.html. We do this set up for
- *   each test, so that none of the tests conflicts with each other.
+ *   of iframes, etc. See also ad_injectors.html. We do this set up for each
+ *   test, so that none of the tests conflicts with each other.
  * - Signal that we are done with page setup, and should start recording events.
  * - Run the next test.
  * - Signal that the test is done, and we should check for the result.
@@ -180,18 +178,6 @@ var getIframeAd = function() {
   return kIframeAdTemplate.cloneNode(true);
 };
 
-// Creates an embed ad, like so:
-// <embed src="http://www.known-ads.adnetwork"></embed>
-var kEmbedAdTemplate = document.createElement('embed');
-kEmbedAdTemplate.src = kAdNetwork;
-
-/**
- * @return An embed element which will count as ad injection in the tests.
- */
-var getEmbedAd = function() {
-  return kEmbedAdTemplate.cloneNode(true);
-};
-
 // Creates an anchor ad, like so:
 // <a href="http://www.known-ads.adnetwork"></a>
 var kAnchorAdTemplate = document.createElement('a');
@@ -241,12 +227,10 @@ var functions = [];
 functions.push(function NoAdInjection() {
   var div = document.createElement('div');
   var iframe = document.createElement('iframe');
-  var embed = document.createElement('embed');
   var anchor = document.createElement('anchor');
   var span = document.createElement('span');
   span.textContent = 'Hello, world';
   div.appendChild(iframe);
-  div.appendChild(embed);
   div.appendChild(anchor);
   div.appendChild(span);
   document.body.appendChild(div);
@@ -269,31 +253,15 @@ functions.push(function NewIframeLikelyAdNetwork() {
 
 // Modify an iframe which is currently in the DOM, switching the src to an
 // ad network.
-functions.push(function ModifyExistingIframeToAdNetwork() {
+functions.push(function ModifyExistingIframeToAdNetwork1() {
   var frame = $('non-ad-iframe');
   frame.src = kAdNetwork;
   return INJECTION_NEW_AD;
 });
 
-// Add a new embed element which serves ads.
-functions.push(function NewEmbedAdNetwork() {
-  document.body.appendChild(getEmbedAd());
-  return INJECTION_NEW_AD;
-});
-
-// Add a new embed element which does not serve ads. We should not record
-// anything.
-functions.push(function NewEmbedLikelyAdNetwork() {
-  var embed = document.createElement('embed');
-  document.body.appendChild(embed).src = kMaybeAdNetwork;
-  return INJECTION_LIKELY_NEW_AD;
-});
-
-// Modify an embed which is currently in the DOM, switching the src to an
-// ad network.
-functions.push(function ModifyExistingEmbedToAdNetwork() {
-  var embed = $('non-ad-embed');
-  embed.src = kAdNetwork;
+functions.push(function ModifyExistingIframeToAdNetwork2() {
+  var frame = $('non-ad-iframe');
+  frame.setAttribute('src', kAdNetwork);
   return INJECTION_NEW_AD;
 });
 
@@ -326,13 +294,18 @@ functions.push(function SamePageUrlNotConsideredAd() {
   return NO_AD_INJECTION;
 });
 
-
-functions.push(function ModifyExistingAnchorToAdNetwork() {
+functions.push(function ModifyExistingAnchorToAdNetwork1() {
   var anchor = $('non-ad-anchor');
   anchor.href = kAdNetwork;
   return INJECTION_NEW_AD;
 });
 
+functions.push(function ModifyExistingAnchorToAdNetwork2() {
+  var anchor = $('non-ad-anchor');
+  anchor.setAttribute('href', kAdNetwork);
+  return INJECTION_NEW_AD;
+});
+
 // Add a new element which has a nested ad, to ensure we do a deep check of
 // elements appended to the dom.
 functions.push(function NewNestedAd() {
@@ -340,27 +313,31 @@ functions.push(function NewNestedAd() {
   return INJECTION_NEW_AD;
 });
 
-// Switch an existing embed ad to a new ad network.
-functions.push(function ReplaceEmbedAd() {
-  $('ad-embed').src = kAdNetwork2;
+// Switch an existing iframe ad to a new ad network.
+functions.push(function ReplaceIframeAd1() {
+  $('ad-iframe').src = kAdNetwork2;
   return INJECTION_REPLACED_AD;
 });
 
-// Switch an existing iframe ad to a new ad network.
-functions.push(function ReplaceIframeAd() {
-  $('ad-iframe').src = kAdNetwork2;
+functions.push(function ReplaceIframeAd2() {
+  $('ad-iframe').setAttribute('src', kAdNetwork2);
   return INJECTION_REPLACED_AD;
 });
 
 // Switch an existing anchor ad to a new ad network.
-functions.push(function ReplaceAnchorAd() {
+functions.push(function ReplaceAnchorAd1() {
   $('ad-anchor').href = kAdNetwork2;
   return INJECTION_REPLACED_AD;
 });
 
-// Remove an existing embed ad by setting it's src to a non-ad network.
+functions.push(function ReplaceAnchorAd2() {
+  $('ad-anchor').setAttribute('href', kAdNetwork2);
+  return INJECTION_REPLACED_AD;
+});
+
+// Remove an existing iframe ad by setting it's src to a non-ad network.
 functions.push(function RemoveAdBySettingSrc() {
-  $('ad-embed').src = kMaybeAdNetwork;
+  $('ad-iframe').src = kMaybeAdNetwork;
   return INJECTION_REMOVED_AD;
 });
 
@@ -370,7 +347,7 @@ functions.push(function LikelyReplacedAd() {
   // Switching from one valid url src to another valid url src is very
   // suspicious behavior, and should be relatively rare. This helps us determine
   // the effectiveness of our ad network recognition.
-  $('non-ad-embed').src = 'http://www.thismightbeanadnetwork.ads';
+  $('non-ad-iframe').src = 'http://www.thismightbeanadnetwork.ads';
   return INJECTION_LIKELY_REPLACED_AD;
 });
 

extensions/common/ad_injection_constants.cc

-// Copyright 2014 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 "extensions/common/ad_injection_constants.h"
-
-#include "base/strings/string_util.h"
-
-namespace extensions {
-namespace ad_injection_constants {
-
-namespace keys {
-
-const char kType[] = "type";
-const char kChildren[] = "children";
-const char kSrc[] = "src";
-const char kHref[] = "href";
-
-}  // namespace keys
-
-const char kHtmlIframeSrcApiName[] = "HTMLIFrameElement.src";
-const char kHtmlEmbedSrcApiName[] = "HTMLEmbedElement.src";
-const char kHtmlAnchorHrefApiName[] = "HTMLAnchorElement.href";
-const char kAppendChildApiSuffix[] = "appendChild";
-
-// The maximum number of children to check when we examine a newly-added
-// element.
-extern const size_t kMaximumChildrenToCheck = 10u;
-
-// The maximum depth to check when we examine a newly-added element.
-extern const size_t kMaximumDepthToCheck = 5u;
-
-bool ApiCanInjectAds(const std::string& api) {
-  return api == kHtmlIframeSrcApiName ||
-         api == kHtmlEmbedSrcApiName ||
-         api == kHtmlAnchorHrefApiName ||
-         EndsWith(api, kAppendChildApiSuffix, true /* case sensitive */);
-}
-
-}  // namespace ad_injection_constants
-}  // namespace extensions

extensions/common/ad_injection_constants.h

-// Copyright 2014 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.
-
-#ifndef EXTENSIONS_COMMON_AD_INJECTION_CONSTANTS_H_
-#define EXTENSIONS_COMMON_AD_INJECTION_CONSTANTS_H_
-
-#include <string>
-
-#include "base/basictypes.h"
-
-namespace extensions {
-namespace ad_injection_constants {
-
-// The keys used when serializing arguments to activity action APIs.
-namespace keys {
-
-extern const char kType[];
-extern const char kChildren[];
-extern const char kSrc[];
-extern const char kHref[];
-
-}  // namespace keys
-
-extern const char kHtmlIframeSrcApiName[];
-extern const char kHtmlEmbedSrcApiName[];
-extern const char kHtmlAnchorHrefApiName[];
-extern const char kAppendChildApiSuffix[];
-
-extern const size_t kMaximumChildrenToCheck;
-extern const size_t kMaximumDepthToCheck;
-
-// Returns true if the given |api| can potentially inject ads, and should
-// therefore be examined.
-bool ApiCanInjectAds(const std::string& api);
-
-}  // namespace ad_injection_constants
-}  // namespace extensions
-
-#endif  // EXTENSIONS_COMMON_AD_INJECTION_CONSTANTS_H_

extensions/extensions.gyp

@@ -33,8 +33,6 @@
         '<(INTERMEDIATE_DIR)',
       ],
       'sources': [
-        'common/ad_injection_constants.cc',
-        'common/ad_injection_constants.h',
         'common/api/messaging/message.h',
         'common/api/sockets/sockets_manifest_data.cc',
         'common/api/sockets/sockets_manifest_data.h',

extensions/renderer/activity_log_converter_strategy.cc

@@ -6,115 +6,12 @@
 
 #include "base/logging.h"
 #include "base/values.h"
-#include "extensions/common/ad_injection_constants.h"
 #include "v8/include/v8.h"
 
 namespace extensions {
 
 namespace {
 
-typedef ActivityLogConverterStrategy::FromV8ValueCallback FromV8ValueCallback;
-
-namespace constants = ad_injection_constants;
-namespace keys = constants::keys;
-
-const char kFirstChildProperty[] = "firstElementChild";
-const char kNextElementSiblingProperty[] = "nextElementSibling";
-
-scoped_ptr<base::DictionaryValue> ParseV8Object(
-    v8::Isolate* isolate,
-    v8::Object* object,
-    const FromV8ValueCallback& callback);
-
-// Get a property from a V8 object without entering javascript. We use this
-// in order to examine the objects, while ensuring that we don't cause any
-// change in the running program.
-v8::Local<v8::Value> SafeGetProperty(v8::Isolate* isolate,
-                                     v8::Object* object,
-                                     const char* key) {
-  v8::TryCatch try_catch;
-  v8::Isolate::DisallowJavascriptExecutionScope scope(
-      isolate, v8::Isolate::DisallowJavascriptExecutionScope::THROW_ON_FAILURE);
-  v8::Local<v8::String> key_string = v8::String::NewFromUtf8(isolate, key);
-  v8::Local<v8::Value> value = object->Get(key_string);
-  if (try_catch.HasCaught() || value.IsEmpty() || value->IsUndefined() ||
-      value->IsNull()) {
-    return v8::Local<v8::Value>();
-  }
-  return value;
-}
-
-// Append a property to the given |dict| from the given |object| if the
-// property exists on |object| and can be accessed safely (i.e., without
-// triggering any javascript execution).
-void MaybeAppendV8Property(v8::Isolate* isolate,
-                           v8::Object* object,
-                           const char* property_name,
-                           base::DictionaryValue* dict,
-                           const FromV8ValueCallback& callback) {
-  v8::Handle<v8::Value> value = SafeGetProperty(isolate, object, property_name);
-  if (!value.IsEmpty()) {
-    scoped_ptr<base::Value> parsed_value(callback.Run(value, isolate));
-    if (parsed_value.get())
-      dict->Set(property_name, parsed_value.release());
-  }
-}
-
-// Parse the children of a V8 |object| and return them as a list. This will
-// return an empty scoped_ptr if no children are present, or if the children
-// cannot be read safely (without triggering javascript).
-scoped_ptr<base::ListValue> MaybeParseV8Children(
-    v8::Isolate* isolate,
-    v8::Object* object,
-    const FromV8ValueCallback& callback) {
-  scoped_ptr<base::ListValue> parsed_children(new base::ListValue());
-  v8::Local<v8::Value> child_value =
-      SafeGetProperty(isolate, object, kFirstChildProperty);
-  size_t checked_children = 0u;
-  while (!child_value.IsEmpty() &&
-         child_value->IsObject() &&
-         checked_children < constants::kMaximumChildrenToCheck) {
-    ++checked_children;
-    v8::Handle<v8::Object> child_object = child_value->ToObject();
-    scoped_ptr<base::Value> parsed_child(
-        callback.Run(child_object, isolate));
-    if (parsed_child.get())
-      parsed_children->Append(parsed_child.release());
-    child_value =
-        SafeGetProperty(isolate, *child_object, kNextElementSiblingProperty);
-  }
-
-  return parsed_children->GetSize() > 0 ? parsed_children.Pass()
-                                        : scoped_ptr<base::ListValue>();
-}
-
-// Parse a V8 |object| into a DictionaryValue. This will examine the object
-// for a few important properties, including:
-// - href
-// - src
-// - children
-// These properties are necessary to analyze whether or not the object contains
-// ads, which may have been injected.
-scoped_ptr<base::DictionaryValue> ParseV8Object(
-    v8::Isolate* isolate,
-    v8::Object* object,
-    const FromV8ValueCallback& callback) {
-  scoped_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
-
-  dict->SetString(keys::kType,
-                  *v8::String::Utf8Value(object->GetConstructorName()));
-
-  MaybeAppendV8Property(isolate, object, keys::kHref, dict.get(), callback);
-  MaybeAppendV8Property(isolate, object, keys::kSrc, dict.get(), callback);
-
-  scoped_ptr<base::ListValue> maybe_children =
-      MaybeParseV8Children(isolate, object, callback);
-  if (maybe_children.get())
-    dict->Set(keys::kChildren, maybe_children.release());
-
-  return dict.Pass();
-}
-
 // Summarize a V8 value. This performs a shallow conversion in all cases, and
 // returns only a string with a description of the value (e.g.,
 // "[HTMLElement]").
@@ -150,8 +47,7 @@ scoped_ptr<base::Value> SummarizeV8Value(v8::Isolate* isolate,
 
 }  // namespace
 
-ActivityLogConverterStrategy::ActivityLogConverterStrategy()
-    : enable_detailed_parsing_(false) {}
+ActivityLogConverterStrategy::ActivityLogConverterStrategy() {}
 
 ActivityLogConverterStrategy::~ActivityLogConverterStrategy() {}
 
@@ -177,10 +73,7 @@ bool ActivityLogConverterStrategy::FromV8Internal(
     v8::Isolate* isolate,
     const FromV8ValueCallback& callback) const {
   scoped_ptr<base::Value> parsed_value;
-  if (enable_detailed_parsing_)
-    parsed_value = ParseV8Object(isolate, *value, callback);
-  if (!parsed_value.get())
-    parsed_value = SummarizeV8Value(isolate, value);
+  parsed_value = SummarizeV8Value(isolate, value);
   *out = parsed_value.release();
 
   return true;

extensions/renderer/activity_log_converter_strategy.h

@@ -36,21 +36,12 @@ class ActivityLogConverterStrategy
                            v8::Isolate* isolate,
                            const FromV8ValueCallback& callback) const OVERRIDE;
 
-  void set_enable_detailed_parsing(bool enable_detailed_parsing) {
-    enable_detailed_parsing_ = enable_detailed_parsing;
-  }
-
  private:
   bool FromV8Internal(v8::Handle<v8::Object> value,
                       base::Value** out,
                       v8::Isolate* isolate,
                       const FromV8ValueCallback& callback) const;
 
-  // Whether or not to extract a detailed value from the passed in objects. We
-  // do this when we need more information about the arguments in order to
-  // determine, e.g., if an ad was injected.
-  bool enable_detailed_parsing_;
-
   DISALLOW_COPY_AND_ASSIGN(ActivityLogConverterStrategy);
 };
 

extensions/renderer/dom_activity_logger.cc

@@ -6,7 +6,6 @@
 
 #include "content/public/renderer/render_thread.h"
 #include "content/public/renderer/v8_value_converter.h"
-#include "extensions/common/ad_injection_constants.h"
 #include "extensions/common/dom_action_types.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/renderer/activity_log_converter_strategy.h"
@@ -29,8 +28,6 @@ void AppendV8Value(const std::string& api_name,
   DCHECK(list);
   scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
   ActivityLogConverterStrategy strategy;
-  strategy.set_enable_detailed_parsing(
-      ad_injection_constants::ApiCanInjectAds(api_name));
   converter->SetFunctionAllowed(true);
   converter->SetStrategy(&strategy);
   scoped_ptr<base::Value> value(converter->FromV8Value(