Cleaning up PolicyWatcher Create methods

This change is related to the TaskScheduler cleanup work.
PolicyWatcher::Create was tagged to have BrowserThread::FILE removed,
however it was only mentioned in the comments.  This comment could
have been removed, but I wanted to update the method names / params
to make the PolicyWatcher initialization mechanism more obvious.

This change splits the Create() method into two methods and updates
the callsites.  I wanted to keep a single, private Create method to
keep the per-OS creation logic intact.


Bug: 
Change-Id: Ie4b4788a4d313414819b31ab5e1fca77781d1df3
Reviewed-on: https://chromium-review.googlesource.com/572442Reviewed-by: Sergey Ulanov <sergeyu@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487361}

chrome/browser/extensions/api/messaging/native_message_host_chromeos.cc

@@ -109,8 +109,8 @@ std::unique_ptr<NativeMessageHost> CreateIt2MeHost() {
           base::CreateSingleThreadTaskRunnerWithTraits(
               {base::MayBlock(), base::TaskPriority::BACKGROUND}));
   std::unique_ptr<remoting::PolicyWatcher> policy_watcher =
-      remoting::PolicyWatcher::Create(g_browser_process->policy_service(),
-                                      context->file_task_runner());
+      remoting::PolicyWatcher::CreateWithPolicyService(
+          g_browser_process->policy_service());
   std::unique_ptr<NativeMessageHost> host(
       new remoting::It2MeNativeMessagingHost(
           /*needs_elevation=*/false, std::move(policy_watcher),

remoting/host/it2me/it2me_native_messaging_host_main.cc

@@ -207,7 +207,7 @@ int It2MeNativeMessagingHostMain(int argc, char** argv) {
       ChromotingHostContext::Create(new remoting::AutoThreadTaskRunner(
           message_loop.task_runner(), run_loop.QuitClosure()));
   std::unique_ptr<PolicyWatcher> policy_watcher =
-      PolicyWatcher::Create(nullptr, context->file_task_runner());
+      PolicyWatcher::CreateWithTaskRunner(context->file_task_runner());
   std::unique_ptr<extensions::NativeMessageHost> host(
       new It2MeNativeMessagingHost(needs_elevation, std::move(policy_watcher),
                                    std::move(context), std::move(factory)));

remoting/host/policy_watcher.cc

@@ -384,17 +384,15 @@ std::unique_ptr<PolicyWatcher> PolicyWatcher::CreateFromPolicyLoader(
       std::move(policy_provider), std::move(schema_registry)));
 }
 
-std::unique_ptr<PolicyWatcher> PolicyWatcher::Create(
-    policy::PolicyService* policy_service,
-    const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner) {
-#if defined(OS_CHROMEOS)
-  // On Chrome OS the PolicyService is owned by the browser.
+std::unique_ptr<PolicyWatcher> PolicyWatcher::CreateWithPolicyService(
+    policy::PolicyService* policy_service) {
   DCHECK(policy_service);
   return base::WrapUnique(new PolicyWatcher(policy_service, nullptr, nullptr,
                                             CreateSchemaRegistry()));
-#else  // !defined(OS_CHROMEOS)
-  DCHECK(!policy_service);
+}
 
+std::unique_ptr<PolicyWatcher> PolicyWatcher::CreateWithTaskRunner(
+    const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner) {
   // Create platform-specific PolicyLoader. Always read the Chrome policies
   // (even on Chromium) so that policy enforcement can't be bypassed by running
   // Chromium.
@@ -422,12 +420,14 @@ std::unique_ptr<PolicyWatcher> PolicyWatcher::Create(
   return base::WrapUnique(new PolicyWatcher(
       owned_policy_service.get(), std::move(owned_policy_service), nullptr,
       CreateSchemaRegistry()));
+#elif defined(OS_CHROMEOS)
+  NOTREACHED() << "CreateWithPolicyService() should be used on ChromeOS.";
+  return nullptr;
 #else
 #error OS that is not yet supported by PolicyWatcher code.
 #endif
 
   return PolicyWatcher::CreateFromPolicyLoader(std::move(policy_loader));
-#endif  // !(OS_CHROMEOS)
 }
 
 std::unique_ptr<PolicyWatcher> PolicyWatcher::CreateFromPolicyLoaderForTesting(

remoting/host/policy_watcher.h

@@ -66,22 +66,21 @@ class PolicyWatcher : public policy::PolicyService::Observer {
   static std::unique_ptr<base::DictionaryValue> GetDefaultPolicies();
 
   // Specify a |policy_service| to borrow (on Chrome OS, from the browser
-  // process) or specify nullptr to internally construct and use a new
-  // PolicyService (on other OS-es). PolicyWatcher must be used on the thread on
-  // which it is created. |policy_service| is called on the same thread.
+  // process). PolicyWatcher must be used on the thread on which it is created.
+  // |policy_service| is called on the same thread.
   //
-  // When |policy_service| is null, then |file_task_runner| is used for reading
-  // the policy from files / registry / preferences (which are blocking
-  // operations). |file_task_runner| should be of TYPE_IO type.
+  // When |policy_service| is specified then BrowserThread::UI is used for
+  // PolicyUpdatedCallback and PolicyErrorCallback.
+  static std::unique_ptr<PolicyWatcher> CreateWithPolicyService(
+      policy::PolicyService* policy_service);
+
+  // Construct and a new PolicyService for non-ChromeOS platforms.
+  // PolicyWatcher must be used on the thread on which it is created.
   //
-  // When |policy_service| is specified then |file_task_runner| argument is
-  // ignored and 1) BrowserThread::UI is used for PolicyUpdatedCallback and
-  // PolicyErrorCallback and 2) BrowserThread::FILE is used for reading the
-  // policy from files / registry / preferences (although (2) is just an
-  // implementation detail and should likely be ignored outside of
-  // PolicyWatcher).
-  static std::unique_ptr<PolicyWatcher> Create(
-      policy::PolicyService* policy_service,
+  // |file_task_runner| is used for reading the policy from files / registry /
+  // preferences (which are blocking operations). |file_task_runner| should be
+  // of TYPE_IO type.
+  static std::unique_ptr<PolicyWatcher> CreateWithTaskRunner(
       const scoped_refptr<base::SingleThreadTaskRunner>& file_task_runner);
 
   // Creates a PolicyWatcher from the given loader instead of loading the policy

remoting/host/remoting_me2me_host.cc

@@ -811,7 +811,7 @@ void HostProcess::StartOnUiThread() {
   }
 
   policy_watcher_ =
-      PolicyWatcher::Create(nullptr, context_->file_task_runner());
+      PolicyWatcher::CreateWithTaskRunner(context_->file_task_runner());
   policy_watcher_->StartWatching(
       base::Bind(&HostProcess::OnPolicyUpdate, base::Unretained(this)),
       base::Bind(&HostProcess::OnPolicyError, base::Unretained(this)));