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

remoting/host/plugin/daemon_controller_mac.cc

Index: remoting/host/plugin/daemon_controller_mac.cc
diff --git a/remoting/host/plugin/daemon_controller_mac.cc b/remoting/host/plugin/daemon_controller_mac.cc
index f05bcdc9a74216f154ab552d2b4080f668a09ad9..acb43613a35428e698d19b815bd7442ba120c00c 100644
--- a/remoting/host/plugin/daemon_controller_mac.cc
+++ b/remoting/host/plugin/daemon_controller_mac.cc
@@ -37,6 +37,8 @@ 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.
@@ -48,9 +50,6 @@ const char kHostHelperScript[] = kConfigDir kServiceName ".me2me.sh";
 // 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();
@@ -75,13 +74,22 @@ class DaemonControllerMac : public remoting::DaemonController {
   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);
 };
@@ -93,6 +101,11 @@ DaemonControllerMac::DaemonControllerMac()
 
 DaemonControllerMac::~DaemonControllerMac() {
   auth_thread_.Stop();
+  CFNotificationCenterRemoveObserver(
+      CFNotificationCenterGetDistributedCenter(),
+      this,
+      CFSTR(kUpdateCompleteNotificationName),
+      NULL);
 }
 
 DaemonController::State DaemonControllerMac::GetState() {
@@ -196,18 +209,7 @@ void DaemonControllerMac::DoSetConfigAndStart(
     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(
@@ -231,12 +233,19 @@ void DaemonControllerMac::DoUpdateConfig(
   }
 
   std::string config_data = config_file.GetSerializedData();
-  bool success = ShowPreferencePane(config_data);
+  ShowPreferencePane(config_data, done_callback);
+}
 
-  done_callback.Run(success ? RESULT_OK : RESULT_FAILED);
+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::ShowPreferencePane(const std::string& config_data) {
+bool DaemonControllerMac::DoShowPreferencePane(const std::string& config_data) {
   if (!config_data.empty()) {
     FilePath config_path;
     if (!file_util::GetTempDir(&config_path)) {
@@ -284,37 +293,67 @@ bool DaemonControllerMac::ShowPreferencePane(const std::string& config_data) {
 }
 
 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;
   }
 }