Clean up ContentSettingsObserver.

Change-Id: Ie24494686e8a054aed8280320f27090e2703b974
Reviewed-on: https://chromium-review.googlesource.com/746150
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513006}

chrome/renderer/content_settings_observer.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/renderer/content_settings_observer.h"
 
+#include <vector>
+
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/common/client_hints.mojom.h"
@@ -75,7 +77,6 @@ ContentSetting GetContentSettingFromRules(
     const ContentSettingsForOneType& rules,
     const WebFrame* frame,
     const URL& secondary_url) {
-  ContentSettingsForOneType::const_iterator it;
   // If there is only one rule, it's the default rule and we don't need to match
   // the patterns.
   if (rules.size() == 1) {
@@ -85,10 +86,10 @@ ContentSetting GetContentSettingFromRules(
   }
   const GURL& primary_url = GetOriginOrURL(frame);
   const GURL& secondary_gurl = secondary_url;
-  for (it = rules.begin(); it != rules.end(); ++it) {
-    if (it->primary_pattern.Matches(primary_url) &&
-        it->secondary_pattern.Matches(secondary_gurl)) {
-      return it->GetContentSetting();
+  for (const auto& rule : rules) {
+    if (rule.primary_pattern.Matches(primary_url) &&
+        rule.secondary_pattern.Matches(secondary_gurl)) {
+      return rule.GetContentSetting();
     }
   }
   NOTREACHED();
@@ -99,6 +100,11 @@ bool IsScriptDisabledForPreview(const content::RenderFrame* render_frame) {
   return render_frame->GetPreviewsState() & content::NOSCRIPT_ON;
 }
 
+bool IsUniqueFrame(WebFrame* frame) {
+  return frame->GetSecurityOrigin().IsUnique() ||
+         frame->Top()->GetSecurityOrigin().IsUnique();
+}
+
 }  // namespace
 
 ContentSettingsObserver::ContentSettingsObserver(
@@ -113,7 +119,7 @@ ContentSettingsObserver::ContentSettingsObserver(
       extension_dispatcher_(extension_dispatcher),
 #endif
       allow_running_insecure_content_(false),
-      content_setting_rules_(NULL),
+      content_setting_rules_(nullptr),
       is_interstitial_page_(false),
       current_request_id_(0),
       should_whitelist_(should_whitelist) {
@@ -153,10 +159,8 @@ bool ContentSettingsObserver::IsPluginTemporarilyAllowed(
     const std::string& identifier) {
   // If the empty string is in here, it means all plugins are allowed.
   // TODO(bauerb): Remove this once we only pass in explicit identifiers.
-  return (temporarily_allowed_plugins_.find(identifier) !=
-          temporarily_allowed_plugins_.end()) ||
-         (temporarily_allowed_plugins_.find(std::string()) !=
-          temporarily_allowed_plugins_.end());
+  return base::ContainsKey(temporarily_allowed_plugins_, identifier) ||
+         base::ContainsKey(temporarily_allowed_plugins_, std::string());
 }
 
 void ContentSettingsObserver::DidBlockContentType(
@@ -242,8 +246,7 @@ bool ContentSettingsObserver::AllowDatabase(const WebString& name,
                                             const WebString& display_name,
                                             unsigned estimated_size) {
   WebFrame* frame = render_frame()->GetWebFrame();
-  if (frame->GetSecurityOrigin().IsUnique() ||
-      frame->Top()->GetSecurityOrigin().IsUnique())
+  if (IsUniqueFrame(frame))
     return false;
 
   bool result = false;
@@ -257,19 +260,18 @@ bool ContentSettingsObserver::AllowDatabase(const WebString& name,
 void ContentSettingsObserver::RequestFileSystemAccessAsync(
     const WebContentSettingCallbacks& callbacks) {
   WebFrame* frame = render_frame()->GetWebFrame();
-  if (frame->GetSecurityOrigin().IsUnique() ||
-      frame->Top()->GetSecurityOrigin().IsUnique()) {
+  if (IsUniqueFrame(frame)) {
     WebContentSettingCallbacks permissionCallbacks(callbacks);
     permissionCallbacks.DoDeny();
     return;
   }
   ++current_request_id_;
-  std::pair<PermissionRequestMap::iterator, bool> insert_result =
-      permission_requests_.insert(
-          std::make_pair(current_request_id_, callbacks));
+  bool inserted = permission_requests_
+                      .insert(std::make_pair(current_request_id_, callbacks))
+                      .second;
 
   // Verify there are no duplicate insertions.
-  DCHECK(insert_result.second);
+  DCHECK(inserted);
 
   Send(new ChromeViewHostMsg_RequestFileSystemAccessAsync(
       routing_id(), current_request_id_,
@@ -301,8 +303,7 @@ bool ContentSettingsObserver::AllowImage(bool enabled_per_settings,
 bool ContentSettingsObserver::AllowIndexedDB(const WebString& name,
                                              const WebSecurityOrigin& origin) {
   WebFrame* frame = render_frame()->GetWebFrame();
-  if (frame->GetSecurityOrigin().IsUnique() ||
-      frame->Top()->GetSecurityOrigin().IsUnique())
+  if (IsUniqueFrame(frame))
     return false;
 
   bool result = false;
@@ -327,7 +328,7 @@ bool ContentSettingsObserver::AllowScript(bool enabled_per_settings) {
     return it->second;
 
   // Evaluate the content setting rules before
-  // |IsWhitelistedForContentSettings|; if there is only the default rule
+  // IsWhitelistedForContentSettings(); if there is only the default rule
   // allowing all scripts, it's quicker this way.
   bool allow = true;
   if (content_setting_rules_) {
@@ -364,8 +365,7 @@ bool ContentSettingsObserver::AllowScriptFromSource(
 
 bool ContentSettingsObserver::AllowStorage(bool local) {
   blink::WebLocalFrame* frame = render_frame()->GetWebFrame();
-  if (frame->GetSecurityOrigin().IsUnique() ||
-      frame->Top()->GetSecurityOrigin().IsUnique())
+  if (IsUniqueFrame(frame))
     return false;
 
   StoragePermissionsKey key(
@@ -471,16 +471,17 @@ void ContentSettingsObserver::PersistClientHints(
   // browser side or the renderer. If the value needs to be overridden,
   // this method should not return early if |update_count| is 0.
   std::vector<::blink::mojom::WebClientHintsType> client_hints;
+  static constexpr size_t kWebClientHintsCount =
+      static_cast<size_t>(blink::mojom::WebClientHintsType::kLast) + 1;
+  client_hints.reserve(kWebClientHintsCount);
 
-  size_t update_count = 0;
-  for (size_t i = 0;
-       i < static_cast<int>(blink::mojom::WebClientHintsType::kLast) + 1; ++i) {
+  for (size_t i = 0; i < kWebClientHintsCount; ++i) {
     if (enabled_client_hints.IsEnabled(
             static_cast<blink::mojom::WebClientHintsType>(i))) {
       client_hints.push_back(static_cast<blink::mojom::WebClientHintsType>(i));
-      update_count++;
     }
   }
+  size_t update_count = client_hints.size();
   if (update_count == 0)
     return;
 
@@ -565,16 +566,17 @@ bool ContentSettingsObserver::IsPlatformApp() {
 const extensions::Extension* ContentSettingsObserver::GetExtension(
     const WebSecurityOrigin& origin) const {
   if (origin.Protocol().Ascii() != extensions::kExtensionScheme)
-    return NULL;
+    return nullptr;
 
   const std::string extension_id = origin.Host().Utf8().data();
   if (!extension_dispatcher_->IsExtensionActive(extension_id))
-    return NULL;
+    return nullptr;
 
   return extensions::RendererExtensionRegistry::Get()->GetByID(extension_id);
 }
 #endif
 
+// static
 bool ContentSettingsObserver::IsWhitelistedForContentSettings() const {
   if (should_whitelist_)
     return true;

chrome/renderer/content_settings_observer.h

@@ -67,7 +67,7 @@ class ContentSettingsObserver
   void DidBlockContentType(ContentSettingsType settings_type,
                            const base::string16& details);
 
-  // blink::WebContentSettingsClient implementation.
+  // blink::WebContentSettingsClient:
   bool AllowDatabase(const blink::WebString& name,
                      const blink::WebString& display_name,
                      unsigned estimated_size) override;
@@ -143,6 +143,8 @@ class ContentSettingsObserver
   // True if |render_frame()| contains content that is white-listed for content
   // settings.
   bool IsWhitelistedForContentSettings() const;
+
+  // Exposed for unit tests.
   static bool IsWhitelistedForContentSettings(
       const blink::WebSecurityOrigin& origin,
       const blink::WebURL& document_url);

chrome/renderer/content_settings_observer_browsertest.cc

@@ -16,6 +16,7 @@
 #include "components/content_settings/core/common/content_settings_utils.h"
 #include "content/public/renderer/render_view.h"
 #include "ipc/ipc_message_macros.h"
+#include "ipc/ipc_test_sink.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/web/WebFrameContentDumper.h"
@@ -26,20 +27,37 @@ using testing::DeleteArg;
 
 namespace {
 
+constexpr char kScriptHtml[] =
+    "<html>"
+    "<head>"
+    "<script src='data:foo'></script>"
+    "</head>"
+    "<body>"
+    "</body>"
+    "</html>";
+
 class MockContentSettingsObserver : public ContentSettingsObserver {
  public:
   MockContentSettingsObserver(content::RenderFrame* render_frame,
                               service_manager::BinderRegistry* registry);
+  ~MockContentSettingsObserver() override {}
 
-  virtual bool Send(IPC::Message* message);
+  bool Send(IPC::Message* message) override;
 
   MOCK_METHOD2(OnContentBlocked,
                void(ContentSettingsType, const base::string16&));
 
   MOCK_METHOD5(OnAllowDOMStorage,
                void(int, const GURL&, const GURL&, bool, IPC::Message*));
-  GURL image_url_;
-  std::string image_origin_;
+
+  const GURL& image_url() const { return image_url_; }
+  const std::string& image_origin() const { return image_origin_; }
+
+ private:
+  const GURL image_url_;
+  const std::string image_origin_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockContentSettingsObserver);
 };
 
 MockContentSettingsObserver::MockContentSettingsObserver(
@@ -84,8 +102,8 @@ class CommitTimeConditionChecker : public content::RenderFrameObserver {
   }
 
  private:
-  Predicate predicate_;
-  bool expectation_;
+  const Predicate predicate_;
+  const bool expectation_;
 
   DISALLOW_COPY_AND_ASSIGN(CommitTimeConditionChecker);
 };
@@ -229,7 +247,7 @@ TEST_F(ChromeRenderViewTest, ImagesBlockedByDefault) {
   observer->SetContentSettingRules(&content_setting_rules);
   EXPECT_CALL(mock_observer,
               OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES, base::string16()));
-  EXPECT_FALSE(observer->AllowImage(true, mock_observer.image_url_));
+  EXPECT_FALSE(observer->AllowImage(true, mock_observer.image_url()));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   // Create an exception which allows the image.
@@ -237,13 +255,13 @@ TEST_F(ChromeRenderViewTest, ImagesBlockedByDefault) {
       image_setting_rules.begin(),
       ContentSettingPatternSource(
           ContentSettingsPattern::Wildcard(),
-          ContentSettingsPattern::FromString(mock_observer.image_origin_),
+          ContentSettingsPattern::FromString(mock_observer.image_origin()),
           content_settings::ContentSettingToValue(CONTENT_SETTING_ALLOW),
           std::string(), false));
 
   EXPECT_CALL(mock_observer, OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES,
                                               base::string16())).Times(0);
-  EXPECT_TRUE(observer->AllowImage(true, mock_observer.image_url_));
+  EXPECT_TRUE(observer->AllowImage(true, mock_observer.image_url()));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 }
 
@@ -268,7 +286,7 @@ TEST_F(ChromeRenderViewTest, ImagesAllowedByDefault) {
   observer->SetContentSettingRules(&content_setting_rules);
   EXPECT_CALL(mock_observer, OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES,
                                               base::string16())).Times(0);
-  EXPECT_TRUE(observer->AllowImage(true, mock_observer.image_url_));
+  EXPECT_TRUE(observer->AllowImage(true, mock_observer.image_url()));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 
   // Create an exception which blocks the image.
@@ -276,12 +294,12 @@ TEST_F(ChromeRenderViewTest, ImagesAllowedByDefault) {
       image_setting_rules.begin(),
       ContentSettingPatternSource(
           ContentSettingsPattern::Wildcard(),
-          ContentSettingsPattern::FromString(mock_observer.image_origin_),
+          ContentSettingsPattern::FromString(mock_observer.image_origin()),
           content_settings::ContentSettingToValue(CONTENT_SETTING_BLOCK),
           std::string(), false));
   EXPECT_CALL(mock_observer,
               OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES, base::string16()));
-  EXPECT_FALSE(observer->AllowImage(true, mock_observer.image_url_));
+  EXPECT_FALSE(observer->AllowImage(true, mock_observer.image_url()));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 }
 
@@ -300,24 +318,11 @@ TEST_F(ChromeRenderViewTest, ContentSettingsBlockScripts) {
   observer->SetContentSettingRules(&content_setting_rules);
 
   // Load a page which contains a script.
-  const char kHtml[] =
-      "<html>"
-      "<head>"
-      "<script src='data:foo'></script>"
-      "</head>"
-      "<body>"
-      "</body>"
-      "</html>";
-  LoadHTML(kHtml);
+  LoadHTML(kScriptHtml);
 
   // Verify that the script was blocked.
-  bool was_blocked = false;
-  for (size_t i = 0; i < render_thread_->sink().message_count(); ++i) {
-    const IPC::Message* msg = render_thread_->sink().GetMessageAt(i);
-    if (msg->type() == ChromeViewHostMsg_ContentBlocked::ID)
-      was_blocked = true;
-  }
-  EXPECT_TRUE(was_blocked);
+  EXPECT_TRUE(render_thread_->sink().GetFirstMessageMatching(
+      ChromeViewHostMsg_ContentBlocked::ID));
 }
 
 TEST_F(ChromeRenderViewTest, ContentSettingsAllowScripts) {
@@ -335,24 +340,11 @@ TEST_F(ChromeRenderViewTest, ContentSettingsAllowScripts) {
   observer->SetContentSettingRules(&content_setting_rules);
 
   // Load a page which contains a script.
-  const char kHtml[] =
-      "<html>"
-      "<head>"
-      "<script src='data:foo'></script>"
-      "</head>"
-      "<body>"
-      "</body>"
-      "</html>";
-  LoadHTML(kHtml);
+  LoadHTML(kScriptHtml);
 
   // Verify that the script was not blocked.
-  bool was_blocked = false;
-  for (size_t i = 0; i < render_thread_->sink().message_count(); ++i) {
-    const IPC::Message* msg = render_thread_->sink().GetMessageAt(i);
-    if (msg->type() == ChromeViewHostMsg_ContentBlocked::ID)
-      was_blocked = true;
-  }
-  EXPECT_FALSE(was_blocked);
+  EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching(
+      ChromeViewHostMsg_ContentBlocked::ID));
 }
 
 // Regression test for crbug.com/232410: Load a page with JS blocked. Then,
@@ -426,24 +418,11 @@ TEST_F(ChromeRenderViewTest, ContentSettingsSameDocumentNavigation) {
   MockContentSettingsObserver mock_observer(view_->GetMainRenderFrame(),
                                             registry_.get());
   // Load a page which contains a script.
-  const char kHtml[] =
-      "<html>"
-      "<head>"
-      "<script src='data:foo'></script>"
-      "</head>"
-      "<body>"
-      "</body>"
-      "</html>";
-  LoadHTML(kHtml);
+  LoadHTML(kScriptHtml);
 
   // Verify that the script was not blocked.
-  bool was_blocked = false;
-  for (size_t i = 0; i < render_thread_->sink().message_count(); ++i) {
-    const IPC::Message* msg = render_thread_->sink().GetMessageAt(i);
-    if (msg->type() == ChromeViewHostMsg_ContentBlocked::ID)
-      was_blocked = true;
-  }
-  EXPECT_FALSE(was_blocked);
+  EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching(
+      ChromeViewHostMsg_ContentBlocked::ID));
 
   // Block JavaScript.
   RendererContentSettingRules content_setting_rules;
@@ -489,29 +468,16 @@ TEST_F(ChromeRenderViewTest, ContentSettingsInterstitialPages) {
   observer->OnSetAsInterstitial();
 
   // Load a page which contains a script.
-  const char kHtml[] =
-      "<html>"
-      "<head>"
-      "<script src='data:foo'></script>"
-      "</head>"
-      "<body>"
-      "</body>"
-      "</html>";
-  LoadHTML(kHtml);
+  LoadHTML(kScriptHtml);
 
   // Verify that the script was allowed.
-  bool was_blocked = false;
-  for (size_t i = 0; i < render_thread_->sink().message_count(); ++i) {
-    const IPC::Message* msg = render_thread_->sink().GetMessageAt(i);
-    if (msg->type() == ChromeViewHostMsg_ContentBlocked::ID)
-      was_blocked = true;
-  }
-  EXPECT_FALSE(was_blocked);
+  EXPECT_FALSE(render_thread_->sink().GetFirstMessageMatching(
+      ChromeViewHostMsg_ContentBlocked::ID));
 
   // Verify that images are allowed.
   EXPECT_CALL(mock_observer, OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES,
                                               base::string16())).Times(0);
-  EXPECT_TRUE(observer->AllowImage(true, mock_observer.image_url_));
+  EXPECT_TRUE(observer->AllowImage(true, mock_observer.image_url()));
   ::testing::Mock::VerifyAndClearExpectations(&observer);
 }