Add threadsafe version of PermissionManager::GetPermissionStatus

chrome/browser/permissions/permission_manager_unittest.cc

Index: chrome/browser/permissions/permission_manager_unittest.cc
diff --git a/chrome/browser/permissions/permission_manager_unittest.cc b/chrome/browser/permissions/permission_manager_unittest.cc
index ae115f8eb3263488030346ec37bacd1a2cc08b31..93743fb48fd3042f82edfec7252383444eafdeec 100644
--- a/chrome/browser/permissions/permission_manager_unittest.cc
+++ b/chrome/browser/permissions/permission_manager_unittest.cc
@@ -5,11 +5,14 @@
 #include "chrome/browser/permissions/permission_manager.h"
 
 #include "base/macros.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "build/build_config.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/permissions/permission_manager_factory.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -40,30 +43,42 @@ class PermissionManagerTest : public testing::Test {
     callback_result_ = permission;
   }
 
+  void TestGetPermissionStatus();
+
  protected:
   PermissionManagerTest()
-      : url_("https://example.com"),
+      : thread_bundle_(content::TestBrowserThreadBundle::REAL_IO_THREAD),
+        url_("https://example.com"),
         other_url_("https://foo.com"),
         callback_called_(false),
-        callback_result_(PermissionStatus::ASK) {}
+        callback_result_(PermissionStatus::ASK) {
+    host_content_settings_map_ =
+        HostContentSettingsMapFactory::GetForProfile(&profile_);
+  }
 
   PermissionManager* GetPermissionManager() {
     return profile_.GetPermissionManager();
   }
 
   HostContentSettingsMap* GetHostContentSettingsMap() {
-    return HostContentSettingsMapFactory::GetForProfile(&profile_);
+    return host_content_settings_map_;
   }
 
   void CheckPermissionStatus(PermissionType type,
                              PermissionStatus expected) {
-    EXPECT_EQ(expected, GetPermissionManager()->GetPermissionStatus(
-                            type, url_.GetOrigin(), url_.GetOrigin()));
+    if (content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
+      EXPECT_EQ(expected, GetPermissionManager()->GetPermissionStatus(
+                              type, url_.GetOrigin(), url_.GetOrigin()));
+    } else {
+      EXPECT_EQ(expected, GetPermissionManager()->GetPermissionStatus(
+                              host_content_settings_map_, type,
+                              url_.GetOrigin(), url_.GetOrigin()));
+    }
   }
 
   void SetPermission(ContentSettingsType type, ContentSetting value) {
-    HostContentSettingsMapFactory::GetForProfile(&profile_)
-        ->SetContentSettingDefaultScope(url_, url_, type, std::string(), value);
+    GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+        url_, url_, type, std::string(), value);
   }
 
   const GURL& url() const {
@@ -86,15 +101,16 @@ class PermissionManagerTest : public testing::Test {
   }
 
  private:
+  content::TestBrowserThreadBundle thread_bundle_;
   const GURL url_;
   const GURL other_url_;
   bool callback_called_;
   PermissionStatus callback_result_;
-  content::TestBrowserThreadBundle thread_bundle_;
   PermissionManagerTestingProfile profile_;
+  HostContentSettingsMap* host_content_settings_map_;
 };
 
-TEST_F(PermissionManagerTest, GetPermissionStatusDefault) {
+void PermissionManagerTest::TestGetPermissionStatus() {
   CheckPermissionStatus(PermissionType::MIDI_SYSEX, PermissionStatus::ASK);
   CheckPermissionStatus(PermissionType::PUSH_MESSAGING, PermissionStatus::ASK);
   CheckPermissionStatus(PermissionType::NOTIFICATIONS, PermissionStatus::ASK);
@@ -105,6 +121,20 @@ TEST_F(PermissionManagerTest, GetPermissionStatusDefault) {
 #endif
 }
 
+TEST_F(PermissionManagerTest, GetPermissionStatusDefault) {
+  TestGetPermissionStatus();
+}
+
+TEST_F(PermissionManagerTest, GetPermissionStatusNonMainThread) {
+  base::RunLoop run_loop;
+  content::BrowserThread::PostTaskAndReply(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&PermissionManagerTest::TestGetPermissionStatus,
+                 base::Unretained(this)),
+      run_loop.QuitClosure());
+  run_loop.Run();

[mlamouri (slow - plz ping), 2016/10/24 10:40:51]

Looks fine but I would have done this a bit differently:
- one test checking that calling the regular GetPermissionStatus on DEBUG will
crash
- one test checking that calling the GetPermissionStatus will
HostContentSettingsMap returns the correct value (and doesn't crash ;))

I'm not super comfortable with the magic thread-check in the test.

However, lgtm either way :)

+}
+
 TEST_F(PermissionManagerTest, GetPermissionStatusAfterSet) {
   SetPermission(CONTENT_SETTINGS_TYPE_GEOLOCATION, CONTENT_SETTING_ALLOW);
   CheckPermissionStatus(PermissionType::GEOLOCATION, PermissionStatus::GRANTED);