Use distributed notifications instead of polling to determine success/failure of the preference pan…

remoting/host/plugin/daemon_controller_mac.cc

 // Copyright (c) 2012 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 "remoting/host/plugin/daemon_controller.h"
 
 #include <launch.h>
 #include <stdio.h>
 #include <sys/types.h>
 
@@ skipping 19 matching lines @@
 namespace {
 
 // The NSSystemDirectories.h header has a conflicting definition of
 // NSSearchPathDirectory with the one in base/mac/foundation_util.h.
 // Foundation.h would work, but it can only be included from Objective-C files.
 // Therefore, we define the needed constants here.
 const int NSLibraryDirectory = 5;
 
 // The name of the Remoting Host service that is registered with launchd.
 #define kServiceName "org.chromium.chromoting"
+
+#define kUpdateCompleteNotificationName kServiceName ".update_complete"
 #define kConfigDir "/Library/PrivilegedHelperTools/"
 
 // This helper script is used to get the installed host version.
 const char kHostHelperScript[] = kConfigDir kServiceName ".me2me.sh";
 
 // Use a single configuration file, instead of separate "auth" and "host" files.
 // This is because the SetConfigAndStart() API only provides a single
 // dictionary, and splitting this into two dictionaries would require
 // knowledge of which keys belong in which files.
 const char kHostConfigFile[] = kConfigDir kServiceName ".json";
 
-const int kPrefPaneWaitRetryLimit = 60;
-const int kPrefPaneWaitTimeout = 1000;
-
 class DaemonControllerMac : public remoting::DaemonController {
  public:
   DaemonControllerMac();
   virtual ~DaemonControllerMac();
 
   virtual State GetState() OVERRIDE;
   virtual void GetConfig(const GetConfigCallback& callback) OVERRIDE;
   virtual void SetConfigAndStart(
       scoped_ptr<base::DictionaryValue> config,
       const CompletionCallback& done_callback) OVERRIDE;
   virtual void UpdateConfig(scoped_ptr<base::DictionaryValue> config,
                             const CompletionCallback& done_callback) OVERRIDE;
   virtual void Stop(const CompletionCallback& done_callback) OVERRIDE;
   virtual void SetWindow(void* window_handle) OVERRIDE;
   virtual void GetVersion(const GetVersionCallback& done_callback) OVERRIDE;
 
  private:
   void DoGetConfig(const GetConfigCallback& callback);
   void DoGetVersion(const GetVersionCallback& callback);
   void DoSetConfigAndStart(scoped_ptr<base::DictionaryValue> config,
                            const CompletionCallback& done_callback);
   void DoUpdateConfig(scoped_ptr<base::DictionaryValue> config,
                       const CompletionCallback& done_callback);
   void DoStop(const CompletionCallback& done_callback);
-  void NotifyOnState(DaemonController::State state,
-                     const CompletionCallback& done_callback,
-                     int tries_remaining,
-                     const base::TimeDelta& sleep);
-  bool ShowPreferencePane(const std::string& config_data);
+
+  void ShowPreferencePane(const std::string& config_data,
+                          const CompletionCallback& done_callback);
+  void RegisterForPreferencePaneNotifications(
+      const CompletionCallback &done_callback);
+  void PreferencePaneCallbackDelegate(CFStringRef name,
+                                      CFDictionaryRef user_info);
+  static bool DoShowPreferencePane(const std::string& config_data);
+  static void PreferencePaneCallback(CFNotificationCenterRef center,
+                                     void* observer,
+                                     CFStringRef name,
+                                     const void* object,
+                                     CFDictionaryRef user_info);
 
   base::Thread auth_thread_;
+  CompletionCallback current_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(DaemonControllerMac);
 };
 
 DaemonControllerMac::DaemonControllerMac()
     : auth_thread_("Auth thread") {
   auth_thread_.Start();
 }
 
 DaemonControllerMac::~DaemonControllerMac() {
   auth_thread_.Stop();
+  CFNotificationCenterRemoveObserver(
+      CFNotificationCenterGetDistributedCenter(),
+      this,
+      CFSTR(kUpdateCompleteNotificationName),
+      NULL);
 }
 
 DaemonController::State DaemonControllerMac::GetState() {
   pid_t job_pid = base::mac::PIDForJob(kServiceName);
   if (job_pid < 0) {
     return DaemonController::STATE_NOT_INSTALLED;
   } else if (job_pid == 0) {
     // Service is stopped, or a start attempt failed.
     return DaemonController::STATE_STOPPED;
   } else {
@@ skipping 83 matching lines @@
     }
   }
   callback.Run(version);
 }
 
 void DaemonControllerMac::DoSetConfigAndStart(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& done_callback) {
   std::string config_data;
   base::JSONWriter::Write(config.get(), &config_data);
-
-  bool result = ShowPreferencePane(config_data);
-
-  if (!result) {
-    done_callback.Run(RESULT_FAILED);
-  }
-
-  // TODO(jamiewalch): Replace this with something a bit more robust
-  NotifyOnState(DaemonController::STATE_STARTED,
-                done_callback,
-                kPrefPaneWaitRetryLimit,
-                base::TimeDelta::FromMilliseconds(kPrefPaneWaitTimeout));
+  ShowPreferencePane(config_data, done_callback);
 }
 
 void DaemonControllerMac::DoUpdateConfig(
     scoped_ptr<base::DictionaryValue> config,
     const CompletionCallback& done_callback) {
   FilePath config_file_path(kHostConfigFile);
   JsonHostConfig config_file(config_file_path);
   if (!config_file.Read()) {
     done_callback.Run(RESULT_FAILED);
     return;
   }
   for (DictionaryValue::key_iterator key(config->begin_keys());
        key != config->end_keys(); ++key) {
     std::string value;
     if (!config->GetString(*key, &value)) {
       LOG(ERROR) << *key << " is not a string.";
       done_callback.Run(RESULT_FAILED);
       return;
     }
     config_file.SetString(*key, value);
   }
 
   std::string config_data = config_file.GetSerializedData();
-  bool success = ShowPreferencePane(config_data);
-
-  done_callback.Run(success ? RESULT_OK : RESULT_FAILED);
+  ShowPreferencePane(config_data, done_callback);
 }
 
-bool DaemonControllerMac::ShowPreferencePane(const std::string& config_data) {
+void DaemonControllerMac::ShowPreferencePane(
+    const std::string& config_data, const CompletionCallback& done_callback) {
+  if (DoShowPreferencePane(config_data)) {
+    RegisterForPreferencePaneNotifications(done_callback);
+  } else {
+    done_callback.Run(RESULT_FAILED);
+  }
+}
+
+bool DaemonControllerMac::DoShowPreferencePane(const std::string& config_data) {
   if (!config_data.empty()) {
     FilePath config_path;
     if (!file_util::GetTempDir(&config_path)) {
       LOG(ERROR) << "Failed to get filename for saving configuration data.";
       return false;
     }
     config_path = config_path.Append(kServiceName ".json");
 
     int written = file_util::WriteFile(config_path, config_data.data(),
                                        config_data.size());
@@ skipping 27 matching lines @@
   }
 
   CFNotificationCenterRef center =
       CFNotificationCenterGetDistributedCenter();
   CFNotificationCenterPostNotification(center, CFSTR(kServiceName), NULL, NULL,
                                        TRUE);
   return true;
 }
 
 void DaemonControllerMac::DoStop(const CompletionCallback& done_callback) {
-  if (!ShowPreferencePane("")) {
-    done_callback.Run(RESULT_FAILED);
+  ShowPreferencePane("", done_callback);
+}
+
+// CFNotificationCenterAddObserver ties the thread on which distributed
+// notifications are received to the one on which it is first called.
+// This is safe because HostNPScriptObject::InvokeAsyncResultCallback
+// bounces the invocation to the correct thread, so it doesn't matter
+// which thread CompletionCallbacks are called on.
+void DaemonControllerMac::RegisterForPreferencePaneNotifications(
+    const CompletionCallback& done_callback) {
+  // We can only have one callback registered at a time. This is enforced by the
+  // UX flow of the web-app.
+  DCHECK(current_callback_.is_null());

[Lambros, 2012/05/15 18:55:35]

DCHECK seems too strong.  Maybe the user could hit F5 while the pref-pane is
shown, and go through the same flow again?  LOG(ERROR) feels safer.

[Jamie, 2012/05/15 20:53:50]

I think DCHECK is correct. If the user refreshes the web-page then the old
plugin is destroyed before a new one is created, so |current_callback_| should
always be null when we get here.

+  current_callback_ = done_callback;
+
+  CFNotificationCenterAddObserver(
+      CFNotificationCenterGetDistributedCenter(),
+      this,
+      &DaemonControllerMac::PreferencePaneCallback,
+      CFSTR(kUpdateCompleteNotificationName),
+      NULL,
+      CFNotificationSuspensionBehaviorDeliverImmediately);
+}
+
+void DaemonControllerMac::PreferencePaneCallbackDelegate(
+    CFStringRef name,
+    CFDictionaryRef result_dictionary) {
+  if (CFStringCompare(name, CFSTR(kUpdateCompleteNotificationName), 0) !=
+          kCFCompareEqualTo) {
+    LOG(WARNING) << "Ignoring unexpected notification: " << name;
     return;
   }
-
-  // TODO(jamiewalch): Replace this with something a bit more robust
-  NotifyOnState(DaemonController::STATE_STOPPED,
-                done_callback,
-                kPrefPaneWaitRetryLimit,
-                base::TimeDelta::FromMilliseconds(kPrefPaneWaitTimeout));
+  AsyncResult result = RESULT_FAILED;
+  CFStringRef result_string =
+      base::mac::GetValueFromDictionary<CFStringRef>(result_dictionary,
+                                                     CFSTR("result"));
+  if (CFStringCompare(result_string, CFSTR("OK"), 0) == kCFCompareEqualTo) {
+    result = RESULT_OK;
+  } else if (CFStringCompare(result_string,
+                             CFSTR("CANCEL"), 0) == kCFCompareEqualTo) {
+    result = RESULT_CANCELLED;
+  }
+  current_callback_.Run(result);

[Lambros, 2012/05/15 18:55:35]

DCHECK current_callback_ ?

[Jamie, 2012/05/15 20:53:50]

Done.

+  current_callback_.Reset();
+  CFNotificationCenterRemoveObserver(
+      CFNotificationCenterGetDistributedCenter(),
+      this,
+      CFSTR(kUpdateCompleteNotificationName),
+      NULL);
 }
 
-void DaemonControllerMac::NotifyOnState(
-    DaemonController::State state,
-    const CompletionCallback& done_callback,
-    int tries_remaining,
-    const base::TimeDelta& sleep) {
-  if (GetState() == state) {
-    done_callback.Run(RESULT_OK);
-  } else if (tries_remaining == 0) {
-    done_callback.Run(RESULT_FAILED);
+void DaemonControllerMac::PreferencePaneCallback(CFNotificationCenterRef center,
+                                                 void* observer,
+                                                 CFStringRef name,
+                                                 const void* object,
+                                                 CFDictionaryRef user_info) {
+  DaemonControllerMac* self = reinterpret_cast<DaemonControllerMac*>(observer);
+  if (self) {
+    self->PreferencePaneCallbackDelegate(name, user_info);
   } else {
-    auth_thread_.message_loop_proxy()->PostDelayedTask(
-        FROM_HERE,
-        base::Bind(&DaemonControllerMac::NotifyOnState,
-                   base::Unretained(this),
-                   state,
-                   done_callback,
-                   tries_remaining - 1,
-                   sleep),
-        sleep);
+    LOG(WARNING) << "Ignoring notification with NULL observer: " << name;
   }
 }
 
 }  // namespace
 
 scoped_ptr<DaemonController> remoting::DaemonController::Create() {
   return scoped_ptr<DaemonController>(new DaemonControllerMac());
 }
 
 }  // namespace remoting