Prompt user before restarting System Preferences.

remoting/host/me2me_preference_pane.mm

 // 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.
 
 #import "remoting/host/me2me_preference_pane.h"
 
 #import <Cocoa/Cocoa.h>
 #include <CommonCrypto/CommonHMAC.h>
 #include <launch.h>
 #import <PreferencePanes/PreferencePanes.h>
@@ skipping 165 matching lines @@
                                         target:self
                                       selector:@selector(refreshServiceStatus:)
                                       userInfo:nil
                                        repeats:YES] retain];
   [self updateServiceStatus];
   [self updateAuthorizationStatus];
   [self readNewConfig];
   [self updateUI];
 }
 
+- (void)didSelect {
+  [self checkInstalledVersion];
+}
+
 - (void)willUnselect {
   NSDistributedNotificationCenter* center =
       [NSDistributedNotificationCenter defaultCenter];
   [center removeObserver:self];
 
   [service_status_timer_ invalidate];
   [service_status_timer_ release];
   service_status_timer_ = nil;
   if (have_new_config_) {
     [self notifyPlugin:kUpdateFailedNotificationName];
@@ skipping 78 matching lines @@
 - (void)updateServiceStatus {
   pid_t job_pid = base::mac::PIDForJob(kServiceName);
   is_service_running_ = (job_pid > 0);
 }
 
 - (void)updateAuthorizationStatus {
   is_pane_unlocked_ = [authorization_view_ updateStatus:authorization_view_];
 }
 
 - (void)readNewConfig {
-  if ([self restartPanelIfDifferentVersionInstalled]) {
-    // Don't read any new config if the version is mismatched.  Return control
-    // to the main run-loop instead, so the application can be terminated.
+  [self checkInstalledVersion];
+  if (is_pane_disabled_) {
+    // Don't read or delete any new config here if the version is mismatched.
+    // When the pane restarts, it should see the new config file.
     return;
   }
 
   std::string file;
   if (!GetTemporaryConfigFilePath(&file)) {
     LOG(ERROR) << "Failed to get path of configuration data.";
     [self showError];
     return;
   }
   if (access(file.c_str(), F_OK) != 0)
@@ skipping 47 matching lines @@
   [status_message_ setStringValue:message];
 
   std::string email;
   if (config_.get()) {
     bool result = config_->GetString(remoting::kXmppLoginConfigPath, &email);
 
     // The config has already been checked by |IsConfigValid|.
     DCHECK(result);
   }
 
-  [disable_view_ setEnabled:(is_pane_unlocked_ && is_service_running_)];
-  [confirm_pin_view_ setEnabled:is_pane_unlocked_];
+  [disable_view_ setEnabled:(is_pane_unlocked_ && is_service_running_ &&
+                             !is_pane_disabled_)];
+  [confirm_pin_view_ setEnabled:(is_pane_unlocked_ && !is_pane_disabled_)];
   [confirm_pin_view_ setEmail:base::SysUTF8ToNSString(email)];
   NSString* applyButtonText = is_service_running_ ? @"Confirm" : @"Enable";
   [confirm_pin_view_ setButtonText:applyButtonText];
+
+  if (is_pane_disabled_) {
+    [authorization_view_ setEnabled:NO];
+  }
 }
 
 - (void)showError {
   NSAlert* alert = [[NSAlert alloc] init];
   [alert setMessageText:@"An unexpected error occurred."];
   [alert setInformativeText:@"Check the system log for more information."];
   [alert setAlertStyle:NSWarningAlertStyle];
   [alert beginSheetModalForWindow:[[self mainView] window]
                     modalDelegate:nil
                    didEndSelector:nil
@@ skipping 153 matching lines @@
 
 - (void)notifyPlugin:(const char*)message {
   NSDistributedNotificationCenter* center =
       [NSDistributedNotificationCenter defaultCenter];
   NSString* name = [NSString stringWithUTF8String:message];
   [center postNotificationName:name
                         object:nil
                       userInfo:nil];
 }
 
-- (BOOL)restartPanelIfDifferentVersionInstalled {
-  NSBundle* this_bundle = [NSBundle bundleForClass:[self class]];
-  NSDictionary* this_plist = [this_bundle infoDictionary];
-  NSString* this_version = [this_plist objectForKey:@"CFBundleVersion"];
+- (void)checkInstalledVersion {
+  // There's no point repeating the check if the pane has already been disabled
+  // from a previous call to this method.

[Jamie, 2012/06/01 18:53:26]

Please clarify that it is only the check that is skipped here, not the
consequences of the check failing. In other words, if |is_pane_disabled_| is
true, then we know there's a version mismatch--there's no need to recheck.

[Lambros, 2012/06/05 17:57:39]

Done.

+  if (!is_pane_disabled_) {
+    NSBundle* this_bundle = [NSBundle bundleForClass:[self class]];
+    NSDictionary* this_plist = [this_bundle infoDictionary];
+    NSString* this_version = [this_plist objectForKey:@"CFBundleVersion"];
 
-  NSString* bundle_path = [this_bundle bundlePath];
-  NSString* plist_path =
-      [bundle_path stringByAppendingString:@"/Contents/Info.plist"];
-  NSDictionary* disk_plist =
-      [NSDictionary dictionaryWithContentsOfFile:plist_path];
-  NSString* disk_version = [disk_plist objectForKey:@"CFBundleVersion"];
+    NSString* bundle_path = [this_bundle bundlePath];
+    NSString* plist_path =
+        [bundle_path stringByAppendingString:@"/Contents/Info.plist"];
+    NSDictionary* disk_plist =
+        [NSDictionary dictionaryWithContentsOfFile:plist_path];
+    NSString* disk_version = [disk_plist objectForKey:@"CFBundleVersion"];
 
-  if (disk_version == nil) {
-    LOG(ERROR) << "Failed to get installed version information";
-    [self showError];
-    return NO;
+    if (disk_version == nil) {
+      LOG(ERROR) << "Failed to get installed version information";
+      [self showError];
+      return;
+    }
+
+    if ([this_version isEqualToString:disk_version]) {
+      return;
+    }
+
+    is_pane_disabled_ = YES;

[Jamie, 2012/06/01 18:53:26]

This is a bit confusing. I guess the pane is disabled at this point, but only
because the sheet is open. My understanding of the purpose of this variable was
that it indicates that the pane is disabled because the user clicked Cancel.

[Lambros, 2012/06/05 17:57:39]

It indicates that the pane is disabled because version-mismatch was detected,
even before the user has chosen OK or Cancel.  The variable, if YES, prevents
the config file being deleted in the meantime.  If the user should choose
Cancel, then we delete the file and send the UpdateFailed notification.

So the variable has a dual purpose - it causes the controls to be greyed-out,
and it also prevents the config file from being deleted (until the user hits
Cancel).  I've updated the comment on that variable.

+    [self updateUI];
   }
 
-  if ([this_version isEqualToString:disk_version]) {
-    return NO;
+  NSWindow* window = [[self mainView] window];
+  if (window == nil) {
+    // Defer the alert until |didSelect| is called, which happens just after
+    // the window is created.

[Jamie, 2012/06/01 18:53:26]

Under what circumstances can this happen? Presumably, only when called from
readNewConfig, but can that be called before there's a window? Maybe a race
between didSelect and the distributed notification?

[Lambros, 2012/06/05 17:57:39]

Open the pref-pane, close it (leaving System Preferences on its main view), then
send it a notification.  The bundle will be loaded into memory, with the
pref-pane invisible.  Then this function gets called from the
notification-handler, and |window| will be nil.
Similarly, |window| is nil if this function gets called from willSelect (since
willSelect happens just before the window becomes visible).  That's why I needed
to add a |didSelect| handler in this CL.

+    return;
+  }
+
+  // This alert appears as a sheet over the top of the Chromoting pref-pane,
+  // underneath the title, so it's OK to refer to "this preference pane" rather
+  // than repeat the title "Chromoting" here.
+  NSAlert* alert = [[NSAlert alloc] init];
+  [alert setMessageText:@"System update detected"];
+  [alert setInformativeText:@"To use this preference pane, System Preferences "
+      "needs to be restarted"];
+  [alert addButtonWithTitle:@"OK"];
+  NSButton* cancel_button = [alert addButtonWithTitle:@"Cancel"];
+  [cancel_button setKeyEquivalent:@"\e"];

[Jamie, 2012/06/01 18:53:26]

No key equivalent for OK?

[Lambros, 2012/06/05 17:57:39]

I think the first button already becomes the default button.  Attempting to
change its key risks breakage.  The Cancel button, on the other hand, doesn't
trigger when I press Esc, so I have to set it manually.  The same thing is done
elsewhere, for example, chrome/browser/mac/install_from_dmg.mm

+  [alert setAlertStyle:NSWarningAlertStyle];
+  [alert beginSheetModalForWindow:window
+                    modalDelegate:self
+                   didEndSelector:@selector(
+                       mismatchAlertDidEnd:returnCode:contextInfo:)
+                      contextInfo:nil];
+  [alert release];
+}
+
+- (void)mismatchAlertDidEnd:(NSAlert*)alert
+                 returnCode:(NSInteger)returnCode
+                contextInfo:(void*)contextInfo {
+  if (returnCode == NSAlertFirstButtonReturn) {
+    // OK was pressed.
+
+    // Dismiss the alert window here, so that the application will respond to
+    // the NSApp terminate: message.
+    [[alert window] orderOut:nil];
+    [self restartSystemPreferences];
   } else {
-    // Terminate and relaunch System Preferences.
+    // Cancel was pressed.
 
-    // TODO(lambroslambrou): Improve this when System Preferences supports
-    // dynamic reloading of pref-panes.
-    NSTask* task = [[NSTask alloc] init];
-    NSArray* arguments = [NSArray arrayWithObjects:@"--relaunch-prefpane", nil];
-    [task setLaunchPath:[NSString stringWithUTF8String:kHelperTool]];
-    [task setArguments:arguments];
-    [task setStandardInput:[NSPipe pipe]];
-    [task launch];
-    [task release];
-    [NSApp terminate:nil];
-    return YES;
+    // If there is a new config file, delete it and notify the web-app of
+    // failure to apply the config.  Otherwise, the web-app will remain in a
+    // spinning state until System Preferences eventually gets restarted and
+    // the user visits this pane again.
+    std::string file;
+    if (!GetTemporaryConfigFilePath(&file)) {
+      // There's no point in alerting the user here.  The same error would
+      // happen when the pane is eventually restarted, so the user would be
+      // alerted at that time.
+      LOG(ERROR) << "Failed to get path of configuration data.";
+      return;
+    }
+    if (access(file.c_str(), F_OK) != 0)

[Jamie, 2012/06/01 18:53:26]

Is this check needed? Can't we just let remove fail?

[Lambros, 2012/06/05 17:57:39]

I don't think we want to send an UpdateFailed notification if there's no config
file (this is the logic in |willSelect| and I think we should keep consistency).
There might not be any harm in sending the notification anyway, but it's an
abuse of the name "UpdateFailed" if there was no "update" as such.

Off-topic for this CL, but I think there's a case for getting rid of
"UpdateFailed", and sending a simple "PaneClosed" notification every time the
pane is closed.  The web-app knows whether it has launched the pref-pane, so if
the web-app sees "PaneClosed" before anything else, it can infer the requested
operation failed (whether it be setting a new PIN or disabling the host).

+      return;
+
+    remove(file.c_str());
+    [self notifyPlugin:kUpdateFailedNotificationName];

[Jamie, 2012/06/01 18:53:26]

I think this needs to be moved above the early returns.

[Lambros, 2012/06/05 17:57:39]

Same as above.

   }
 }
 
+- (void)restartSystemPreferences {
+  // TODO(lambroslambrou): Improve this when System Preferences supports
+  // dynamic reloading of pref-panes.

[Jamie, 2012/06/01 18:53:26]

Is there any reason to expect that this will happen? If not, I think I'd remove
the comment.

[Lambros, 2012/06/05 17:57:39]

No :) Done.

+  NSTask* task = [[NSTask alloc] init];
+  NSArray* arguments = [NSArray arrayWithObjects:@"--relaunch-prefpane", nil];
+  [task setLaunchPath:[NSString stringWithUTF8String:kHelperTool]];
+  [task setArguments:arguments];
+  [task setStandardInput:[NSPipe pipe]];
+  [task launch];
+  [task release];
+  [NSApp terminate:nil];
+}
+
 @end