General comment cleaning / refactoring for Mac Installer

Review-Url: https://codereview.chromium.org/2293923005
Cr-Commit-Position: refs/heads/master@{#415640}

chrome/installer/mac/app/AppDelegate.mm

@@ -39,7 +39,9 @@
 
 // Sets up the main window and begins the downloading process.
 - (void)applicationDidFinishLaunching:(NSNotification*)aNotification {
-  // TODO: fix UI not loading until after asking for authorization.
+  // TODO: Despite what the code implies -- when the installer is run, the main
+  // window of the application is not visible until the user has taken action on
+  // the Authorization modal.
   window_.delegate = self;
   installerWindowController_ =
       [[InstallerWindowController alloc] initWithWindow:window_];
@@ -63,7 +65,9 @@
 // applicationShouldTerminateAfterLastWindowClosed: to make sure that the
 // application does correctly terminate after closing the installer, but does
 // not terminate when we call orderOut: to hide the installer during its
-// tear-down steps.
+// tear-down steps. If the user force-quits the application, the below delegate
+// method gets called. However, when orderOut is called, the below delegate
+// method does not get called.
 - (BOOL)windowShouldClose:(id)sender {
   [self exit];
   return YES;
@@ -84,7 +88,7 @@
 
 - (void)onLoadInstallationToolFailure {
   NSError* loadToolError = [NSError
-       errorForAlerts:@"Could not load installion tool"
+       errorForAlerts:@"Internal Error"
       withDescription:
           @"Your Chrome Installer may be corrupted. Download and try again."
         isRecoverable:NO];
@@ -135,14 +139,15 @@
 - (void)unpacker:(Unpacker*)unpacker onMountSuccess:(NSString*)tempAppPath {
   SecStaticCodeRef diskStaticCode;
   SecRequirementRef diskRequirement;
-  // TODO: flush out error handling more
+  // TODO: Include some better error handling below than NSLog
   OSStatus oserror;
   oserror = SecStaticCodeCreateWithPath(
       (__bridge CFURLRef)[NSURL fileURLWithPath:tempAppPath isDirectory:NO],
       kSecCSDefaultFlags, &diskStaticCode);
   if (oserror != errSecSuccess)
     NSLog(@"code %d", oserror);
-  // TODO: add in a more specific code sign requirement
+  // TODO: The below requirement is too general as most signed entities have the
+  // below requirement; replace it with something adequately specific.
   oserror =
       SecRequirementCreateWithString((CFStringRef) @"anchor apple generic",
                                      kSecCSDefaultFlags, &diskRequirement);
@@ -171,6 +176,11 @@
   if ([installerWindowController_ isDefaultBrowserChecked])
     [installerSettings
         addObject:[NSString
+                      // NOTE: the |kMakeDefaultBrowser| constant used as a
+                      // command-line switch here only will apply at a user
+                      // level, since the application itself is not running with
+                      // privileges. grt@ suggested this constant should be
+                      // renamed |kMakeDefaultBrowserforUser|.
                       stringWithUTF8String:switches::kMakeDefaultBrowser]];
 
   NSError* error = nil;
@@ -186,7 +196,7 @@
     NSLog(@"Chrome failed to launch: %@", error);
   }
 
-  // Begin teardown stuff!
+  // Begin teardown step!
   dispatch_async(dispatch_get_main_queue(), ^{
     [window_ orderOut:nil];
   });
@@ -196,7 +206,7 @@
 
 - (void)unpacker:(Unpacker*)unpacker onMountFailure:(NSError*)error {
   NSError* extractError =
-      [NSError errorForAlerts:@"Install Failure"
+      [NSError errorForAlerts:@"Install Error"
               withDescription:@"Unable to add Google Chrome to Applications."
                 isRecoverable:NO];
   [self displayError:extractError];
@@ -209,9 +219,9 @@
 
 - (void)unpacker:(Unpacker*)unpacker onUnmountFailure:(NSError*)error {
   NSLog(@"error unmounting");
-  // NOTE: since we are not deleting the temporary folder if the unmount fails,
+  // NOTE: Since we are not deleting the temporary folder if the unmount fails,
   // we'll just leave it up to the computer to delete the temporary folder on
-  // its own time, and to unmount the disk during a restart at some point. There
+  // its own time and to unmount the disk during a restart at some point. There
   // is no other work to be done in the mean time.
   [self exit];
 }

chrome/installer/mac/app/AuthorizedInstall.h

@@ -10,7 +10,8 @@
 
 @interface AuthorizedInstall : NSObject
 
-// Attempts to gain elevated permissions and starts a subprocess.
+// Attempts to gain elevated permissions, then starts the subprocess with the
+// appropriate level of privilege.
 - (BOOL)loadInstallationTool;
 
 // Signals the tool to begin the installation. Returns the path to the

chrome/installer/mac/app/AuthorizedInstall.m

@@ -11,7 +11,7 @@
 @end
 
 @implementation AuthorizedInstall
-// Does the setup needed to authorize a tool to run as admin.
+// Does the setup needed to authorize a subprocess to run as root.
 - (OSStatus)setUpAuthorization:(AuthorizationRef*)authRef {
   OSStatus status = AuthorizationCreate(NULL, kAuthorizationEmptyEnvironment,
                                         kAuthorizationFlagDefaults, authRef);
@@ -82,10 +82,6 @@
   }
 }
 
-// Attempts to gain authorization to run installation tool with elevated
-// permissions.
-// Then starts the tool with the appropiate paths for the tools elevation
-// status.
 - (BOOL)loadInstallationTool {
   AuthorizationRef authRef = NULL;
   OSStatus status = [self setUpAuthorization:&authRef];
@@ -117,11 +113,6 @@
   return true;
 }
 
-- (NSString*)startInstall:(NSString*)appBundlePath {
-  [self sendMessageToTool:appBundlePath];
-  return destinationAppBundlePath_;
-}
-
 // Sends a message to the tool's stdin. The tool is using 'read' to wait for
 // input. 'read' adds to its buffer until it receives a newline to continue so
 // append '\n' to the message to end the read.
@@ -130,4 +121,9 @@
                dataUsingEncoding:NSUTF8StringEncoding]];
 }
 
+- (NSString*)startInstall:(NSString*)appBundlePath {
+  [self sendMessageToTool:appBundlePath];
+  return destinationAppBundlePath_;
+}
+
 @end

chrome/installer/mac/app/Downloader.m

@@ -33,7 +33,6 @@
   [delegate_ downloader:self percentProgress:downloadProgressPercentage];
 }
 
-// Delegate method to move downloaded disk image to user's Download directory.
 - (void)URLSession:(NSURLSession*)session
                  downloadTask:(NSURLSessionDownloadTask*)downloadTask
     didFinishDownloadingToURL:(NSURL*)location {

chrome/installer/mac/app/InstallerWindowController.m

@@ -19,24 +19,13 @@
 
 @implementation InstallerWindowController
 
-// Most buttons have the same style and differ only by their title, this method
-// simplifies styling the buttons and provides an argument for the title.
+// Simplify styling and naming buttons.
 - (void)stylizeButton:(NSButton*)button withTitle:(NSString*)title {
   button.buttonType = NSSwitchButton;
   button.bezelStyle = NSRoundedBezelStyle;
   button.title = title;
 }
 
-// Similar to stylizeButton except works with NSTextField objects instead.
-- (void)stylizeTextField:(NSTextField*)textField
-         withDescription:(NSString*)description {
-  textField.backgroundColor = NSColor.clearColor;
-  textField.textColor = NSColor.blackColor;
-  textField.stringValue = description;
-  textField.bezeled = NO;
-  textField.editable = NO;
-}
-
 // Positions and stylizes buttons.
 - (void)setUpButtons {
   importButton_ = [[NSButton alloc] initWithFrame:NSMakeRect(30, 20, 300, 25)];
@@ -60,6 +49,16 @@
   [launchButton_ setAction:@selector(launchButtonClicked)];
 }
 
+// Simplfy styling NSTextField objects.
+- (void)stylizeTextField:(NSTextField*)textField
+         withDescription:(NSString*)description {
+  textField.backgroundColor = NSColor.clearColor;
+  textField.textColor = NSColor.blackColor;
+  textField.stringValue = description;
+  textField.bezeled = NO;
+  textField.editable = NO;
+}
+
 // Positions and stylizes textfields.
 - (void)setUpTextfields {
   statusDescription_ =
@@ -84,9 +83,9 @@
   progressBar_.doubleValue = 0.0;
 }
 
-// Positions and adds the rest of the UI elements to main window. Prevents
-// resizing the window so that the absolute position will look the same on all
-// computers. Window is hidden until all positioning is finished.
+// Positions the main window and adds the rest of the UI elements to it.
+// Prevents resizing the window so that the absolute position will look the same
+// on all computers. Window is hidden until all positioning is finished.
 - (id)initWithWindow:(NSWindow*)window {
   if (self = [super initWithWindow:window]) {
     [window setFrame:NSMakeRect(0, 0, 430, 220) display:YES];
@@ -111,11 +110,10 @@
 }
 
 - (void)updateStatusDescription:(NSString*)text {
-  // First setStringValue statement is required to clear the original string.
-  // Omitting the first line will cause occasional ghosting of the previous
-  // string.
-  // TODO: Find a real solution to the ghosting problem.
-  //  downloadProgressDescription_.stringValue = @"";
+  // TODO: This method somehow causes ghosting of the previous string's contents
+  // after a redraw. The below line of code is a temporary hack to clear the
+  // ghosting behavior, but it should be replaced with a legitimate bug fix.
+  downloadProgressDescription_.stringValue = @"";
   downloadProgressDescription_.stringValue = text;
 }
 
@@ -123,6 +121,9 @@
   if (progressPercent > 0.0) {
     progressBar_.doubleValue = progressPercent;
   } else {
+    // After the progress bar is made indeterminate, it will not need to track
+    // determinate progress any more. Therefore, there is nothing implemented to
+    // set indeterminate to NO.
     progressBar_.doubleValue = 0.0;
     progressBar_.indeterminate = YES;
     [progressBar_ startAnimation:nil];

chrome/installer/mac/app/NSError+ChromeInstallerAdditions.h

@@ -8,8 +8,8 @@
 #import <Foundation/Foundation.h>
 
 @interface NSError (ChromeInstallerAdditions)
-// Creates a custom error object to be used as the popup alert that the user
-// will be shown.
+// Creates a custom error object to be used to create an alert to be shown to
+// the user.
 + (NSError*)errorForAlerts:(NSString*)message
            withDescription:(NSString*)description
              isRecoverable:(BOOL)recoverable;

chrome/installer/mac/app/OmahaCommunication.h

@@ -23,8 +23,7 @@
 - (id)init;
 - (id)initWithBody:(NSXMLDocument*)xmlBody;
 
-// Asks the Omaha servers for the most updated version of Chrome by sending a
-// request using this function.
+// Asks the Omaha servers for the most updated version of Chrome.
 - (void)fetchDownloadURLs;
 
 @end

chrome/installer/mac/app/OmahaCommunication.m

@@ -7,7 +7,7 @@
 #import "OmahaXMLRequest.h"
 #import "OmahaXMLParser.h"
 
-// TODO: turn this string to a command-line flag
+// TODO: Turn the below string into a command line flag for testing.
 static NSString* const omahaURLPath =
     @"https://tools.google.com/service/update2";
 

chrome/installer/mac/app/OmahaXMLParser.h

@@ -9,9 +9,9 @@
 
 @interface OmahaXMLParser : NSObject
 
-// Parses an XML document and extracts the URLs and name of the Chrome DMG from
-// Omaha, then returns an array with all the URLs concatenated with the
-// filename.
+// Parses the XML body from Omaha's HTTP response and extracts the URLs and name
+// of the Chrome disk image. Then, returns an array with all the URLs
+// concatenated with the filename.
 + (NSArray*)parseXML:(NSData*)omahaResponseXML error:(NSError**)error;
 
 @end

chrome/installer/mac/app/OmahaXMLParser.m

@@ -32,9 +32,10 @@
   }
 
   if ([completeDownloadURLs count] < 1) {
-    // TODO: currently whatever error is passed in doesn't matter... we should
-    // make it so that the type of error informs what the installer will do
-    // about the error
+    // TODO: The below error exists only so the caller of this method would
+    // catch the error created here. A better way to handle this is to make the
+    // error's contents inform what the installer will try next when it attempts
+    // to recover from an issue.
     *error = [NSError errorWithDomain:@"ChromeErrorDomain" code:1 userInfo:nil];
     return nil;
   }
@@ -42,10 +43,9 @@
   return completeDownloadURLs;
 }
 
-// Method implementation for XMLParserDelegate.
 // Searches the XML data for the tag "URL" and the subsequent "codebase"
 // attribute that indicates a URL follows. Copies each URL into an array.
-// Note that the URLs in the XML file are incomplete. They need the filename
+// NOTE: The URLs in the XML file are incomplete. They need the filename
 // appended to end. The second if statement checks for the tag "package" which
 // contains the filename needed to complete the URLs.
 - (void)parser:(NSXMLParser*)parser

chrome/installer/mac/app/OmahaXMLRequest.m

@@ -23,7 +23,8 @@
 // user attributes that Omaha actually looks at. The other parameters are useful
 // for logging purposes but otherwise not directly used.
 + (NSXMLDocument*)createXMLRequestBody {
-  // TODO: not hard-code protocol version #?
+  // TODO: This protocol version number probably shouldn't be hard-coded. Check
+  // with borisv@ regarding changing protocol verions.
   NSString* protocol = @"3.0";
 
   NSString* platform = @"mac";

chrome/installer/mac/app/README.md

@@ -21,7 +21,9 @@ The installer breaks down its task into the following steps:
 Each of the above modules are designed to carry one action before returning via
 delegate method. All of these main steps occur on a primary working thread
 (non-UI), with the exception of `AuthorizedInstall`, which makes use of an
-authorized-if-able subprocess.
+authorized-if-able subprocess. If the user does not provide permission to
+escalate privileges, `AuthorizedInstall` still does its job, but opts for the
+User's Applications folder instead of the system Applications folder.
 
 The OmahaXML* classes and SystemInfo class are simply classes to help
 OmahaCommunication do its work.

chrome/installer/mac/app/SystemInfo.h

@@ -13,7 +13,7 @@
 
 @interface SystemInfo : NSObject
 // Gets the CPU architecture type of the client's system, which will be used
-// when crafting the query to Omaha. This will return either "x84_64h" for
+// when crafting the query to Omaha. This should return either "x84_64h" for
 // systems running on Intel Haswell chips, "i486" for other Intel machines, or
 // strings representing other CPU types ("amd", "pentium", and "i686", for
 // example, are all possible; however, due to the above macro, the possible

chrome/installer/mac/app/SystemInfo.m

@@ -9,6 +9,8 @@
 @implementation SystemInfo
 
 + (NSString*)getArch {
+  // NOTE: It seems the below function `NSGetLocalArchInfo` returns an
+  // arch->name that is either "x84_64h" or "i486".
   const NXArchInfo* arch = NXGetLocalArchInfo();
   NSString* archName = [NSString stringWithUTF8String:arch->name];
   return archName;

chrome/installer/mac/app/Unpacker.m

@@ -49,15 +49,16 @@ static void unmount_callback(DADiskRef disk,
 - (void)cleanUp {
   [mountTask_ terminate];
   // It's not the end of the world if this temporary directory is not removed
-  // here. It will be deleted when the operating system itself decides to
-  // anyway.
+  // here. The directory will be deleted when the operating system itself
+  // decides to anyway.
   [[NSFileManager defaultManager] removeItemAtURL:temporaryDirectoryURL_
                                             error:nil];
   [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
-// TODO: the failure delegate methods need to be revised to be more meaningfully
-// deal with the errors (pipe in stderr / stdout)
+// TODO: The failure delegate methods need to be revised to meaningfully deal
+// with the errors (pipe in stderr / stdout to handle the error according to
+// what the error was).
 - (void)mountDMGFromURL:(NSURL*)fileURL {
   NSError* error = nil;
   temporaryDirectoryURL_ = [[NSFileManager defaultManager]

chrome/installer/mac/app/copy_to_disk.sh

@@ -12,9 +12,9 @@
 # used on the first line to prevent bash from setting the effective user ID to
 # the real user ID (dropping root privileges).
 
-# The 'e' flag causes the script to terminate if it comes across an error while
-# running. The 'u' flag will raise an error if a variable isn't set.
-# 'u pipefail' will set the return exit code to the last non-zero  error code.
+# 'e': terminate if error arises
+# 'u': raise an error if a variable isn't set
+# 'o pipefail': set the return exit code to the last non-zero error code
 set -euo pipefail
 
 # Waits for the main app to pass the path to the app bundle inside the mounted
@@ -26,9 +26,11 @@ APPBUNDLENAME=$(basename "${SRC}")
 FULL_DEST="${DEST}"/"${APPBUNDLENAME}"
 
 # Starts the copy
-# The 'l' flag tells rsync to copy symlinks as symlinks. 'r' is for recursive,
-# so copy all files, 'p' is to preserve permisisons. 't' is to preserve times.
-# 'q' is for quiet mode so rynsc will only log to console if an error occurs.
+# 'l': copy symlinks as symlinks
+# 'r': recursive copy
+# 'p': preserve permissions
+# 't': preserve times
+# 'q': quiet mode, so rynsc will only log to console if an error occurs
 rsync -lrptq "${SRC}" "${DEST}"
 
 # If this script is run as root, change ownership to root and set elevated

chrome/installer/mac/app/testing/Unpacker_test.mm

@@ -74,12 +74,12 @@
 namespace {
 
 TEST(UnpackerTest, IntegrationTest) {
-  // create objects and semaphore
+  // Create objects and semaphore
   Unpacker* unpack = [[Unpacker alloc] init];
   TestDelegate* test_delegate = [[TestDelegate alloc] init];
   unpack.delegate = test_delegate;
 
-  // get a disk image to use to test
+  // Get a disk image to use to test
   base::FilePath originalPath;
   PathService::Get(base::DIR_SOURCE_ROOT, &originalPath);
   originalPath = originalPath.AppendASCII("chrome/test/data/mac_installer/");
@@ -88,22 +88,25 @@ TEST(UnpackerTest, IntegrationTest) {
       (originalPath.AppendASCII("test-dmg.dmg")).value());
   NSString* diskImageCopiedPath = base::SysUTF8ToNSString(
       (originalPath.AppendASCII("test-dmg2.dmg")).value());
+  // The unpacker moves (not copies) a downloaded disk image directly into its
+  // own temporary directory, so if the below copy didn't happen, `test-dmg.dmg`
+  // would disappear every time this test was run
   [[NSFileManager defaultManager] copyItemAtPath:diskImageOriginalPath
                                           toPath:diskImageCopiedPath
                                            error:nil];
   NSURL* dmgURL = [NSURL fileURLWithPath:diskImageCopiedPath isDirectory:NO];
-  // start mount step
+  // Start mount step
   [unpack mountDMGFromURL:dmgURL];
   [test_delegate wait];
 
-  // is the disk image mounted?
+  // Is the disk image mounted?
   ASSERT_TRUE([test_delegate pass]);
 
-  // start unmount step
+  // Start unmount step
   [unpack unmountDMG];
   [test_delegate wait];
 
-  // is the disk image gone?
+  // Is the disk image gone?
   EXPECT_TRUE([test_delegate pass]);
 }