Allow chrome to become the os default handler for arbitrary protocols on mac/win.

chrome/browser/shell_integration_mac.mm

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.

[Mark Mentovai, 2011/05/23 00:23:52]

You should familiarize yourself with the Objective-C style guide at
http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/shell_integration.h"
 
 #include "base/mac/mac_util.h"
 #include "chrome/browser/platform_util.h"
 #import "third_party/mozilla/NSWorkspace+Utils.h"
 
+// FIXME: These functions should be rolled into

[Mark Mentovai, 2011/05/23 00:23:52]

Unless you actually plan to upstream this code and Mozilla is willing to accept
it, I don’t see any reason for you to have written it in an Objective-C
category. You can simply write the quick one-line LaunchServices calls into the
code below.

Unless you actually plan to upstream this code really soon, I don’t see the
reason to have it abstracted out in an Objective-C category in this way. Make
the LS calls in the code below as I suggested, and if and when you succeed in
getting these proposed additions upstreamed to Mozilla, we can pull a new
version of NSWorkspace+Utils.h into our third_party and revise the code below to
call into that instead of calling into LS directly.

Note that there’s a much stronger argument for having
-[NSWorkspace(CaminoDefaultBrowserAdditions) setDefaultBrowserWithIdentifier:]
factored out into its own method than there is for having
-[NSWorkspace(ChromiumDefaultProtocolClientAdditions
setDefaultClientWithIdentifier:forProtocol:] because the former needs to call
into LaunchServices multiple times to account for different URL protocols and
content types. Your proposed interface only puts a single call into LS.

Speaking of that:

Your change only accounts for URL protocols. Have you given any thought to
integrating support for registering a content type handler with the system, so
that Chrome could (for example) respond to a user double-clicking a .doc file in
the Finder by launching and loading Google Docs?

[benwells, 2011/05/23 06:08:37]

OK, will make the change to remove the objective c category.

This change is for url protocols, specifically to support the implementation of
navigator.registerProtocolHandler(). 

Registering also for content types would be great, and is planned for when we
support registerContentHandler().

On 2011/05/23 00:23:52, Mark Mentovai wrote:

[benwells, 2011/05/24 06:10:38]

Done.

+// third_party/mozilla/NSWorkspace+Utils.h
+
+@interface NSWorkspace(ChromiumDefaultProtocolClientAdditions)

[Mark Mentovai, 2011/05/23 00:23:52]

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Categ...
Same on line 22.

+
+- (NSString*)defaultClientIdentifierForProtocol:(NSString*)protocol;

[Mark Mentovai, 2011/05/23 00:23:52]

You should define what this does if nothing’s registered for |protocol|. Does it
return an empty string? Return nil? Crash?

(I prefer “return nil.”)

+- (void)setDefaultClientWithIdentifier:(NSString*)bundleID

[Mark Mentovai, 2011/05/23 00:23:52]

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho...
Same on line 30.

+      forProtocol:(NSString*)protocol;
+
+@end
+
+@implementation NSWorkspace(ChromiumDefaultProtocolClientAdditions)
+
+- (NSString*)defaultClientIdentifierForProtocol:(NSString*)protocol

[Mark Mentovai, 2011/05/23 00:23:52]

Objective-C follows C++ style unless otherwise specified. In this case:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

+{
+  return [(NSString*)LSCopyDefaultHandlerForURLScheme((CFStringRef)protocol)

[Mark Mentovai, 2011/05/23 00:23:52]

Familiarize yourself with NSToCFCast and CFToNSCast from
<base/mac/foundation_util.h> and use them as appropriate.

Same on line 33.

See also CFTypeRefToNSObjectAutorelease from <base/mac/foundation_util.h>.

[benwells, 2011/05/24 06:10:38]

Done. Please check that I've used these correctly - in particular the usage of
CFTypeRefToNSObjectAutorelease (this code is now in
IsIdentififerDefaultProtocolClient).

+      autorelease];
+}
+
+- (void)setDefaultClientWithIdentifier:(NSString*)bundleID
+    forProtocol:(NSString*)protocol
+{
+  LSSetDefaultHandlerForURLScheme((CFStringRef)protocol, (CFStringRef)bundleID);

[Mark Mentovai, 2011/05/23 00:23:52]

Make this method return a BOOL, and return |YourCode() == noErr|. You can use
the result in ShellIntegration::SetAsDefaultProtocolClient.

[benwells, 2011/05/24 06:10:38]

Done.

+}
+
+@end
+
 // Sets Chromium as default browser (only for current user). Returns false if
 // this operation fails.
 bool ShellIntegration::SetAsDefaultBrowser() {
   if (!platform_util::CanSetAsDefaultBrowser())
     return false;
 
   // We really do want the main bundle here, not base::mac::MainAppBundle(),
   // which is the bundle for the framework.
   NSString* identifier = [[NSBundle mainBundle] bundleIdentifier];
   [[NSWorkspace sharedWorkspace] setDefaultBrowserWithIdentifier:identifier];
   return true;
 }
 
+// Sets Chromium as client application for the provided protocol (only for

[Mark Mentovai, 2011/05/23 00:23:52]

Revise this comment. Just because the comment you’re copying needs help doesn’t
mean that this one should too.

[benwells, 2011/05/24 06:10:38]

Done. Also revised comment for SetAsDefaultBrowser, let me know if I missed
something in particular that needs more help.

+// current user). Returns false if this operation fails.
+bool ShellIntegration::SetAsDefaultProtocolClient(const std::string& protocol) {

[Mark Mentovai, 2011/05/23 00:23:52]

I think you should return false if |protocol| is empty, and perhaps do this in
the Windows file as well. There’s no telling what might happen if you give an
empty string for |protocol| to the system, but it’s almost certainly nothing
you’d ever want to do.

[benwells, 2011/05/24 06:10:38]

Done.

+  if (!platform_util::CanSetAsDefaultProtocolClient(protocol))
+    return false;
+
+  // We really do want the main bundle here, not base::mac::MainAppBundle(),
+  // which is the bundle for the framework.
+  NSString* identifier = [[NSBundle mainBundle] bundleIdentifier];

[Mark Mentovai, 2011/05/23 00:23:52]

IsDefaultProtocolClient specifically handles the case where there is no bundle
ID. This doesn’t. Any reason for the discrepancy?

[benwells, 2011/05/24 06:10:38]

Done. It was like this because I had copied the template from IsDefaultBrowser,
i.e. no good reason. I updated IsDefaultBrowser to check if the bundle ID is OK.

+  NSString* nsProtocol = [NSString stringWithCString: protocol.c_str()

[Mark Mentovai, 2011/05/23 00:23:52]

In Chrome (at least at this layer), std::string is not encoded with +[NSString
defaultCStringEncoding], it’s defined to be encoded as UTF-8 without exception.

Use base::SysUTF8ToNSString from <base/sys_string_conversions.h>, or use
+[NSString stringWithUTF8String:].

[benwells, 2011/05/24 06:10:38]

Done.

+                                   encoding:[NSString defaultCStringEncoding]];
+  [[NSWorkspace sharedWorkspace] setDefaultClientWithIdentifier:identifier

[Mark Mentovai, 2011/05/23 00:23:52]

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho...

[benwells, 2011/05/24 06:10:38]

Done (or more accurately, no longer needed).

+                                 forProtocol: nsProtocol];

[Mark Mentovai, 2011/05/23 00:23:52]

There’s never a space between a colon and an argument. See the examples in the
style guide.

http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho...

[benwells, 2011/05/24 06:10:38]

Done.

+  return true;
+}
+
 namespace {
 
 // Returns true if |identifier| is the bundle id of the default browser.
 bool IsIdentifierDefaultBrowser(NSString* identifier) {
   NSString* defaultBrowser =
       [[NSWorkspace sharedWorkspace] defaultBrowserIdentifier];
   if (!defaultBrowser)
     return false;
   // We need to ensure we do the comparison case-insensitive as LS doesn't
   // persist the case of our bundle id.
   NSComparisonResult result =
     [defaultBrowser caseInsensitiveCompare:identifier];
   return result == NSOrderedSame;
 }
 
+// Returns true if |identifier| is the bundle id of the default client
+// application for the given protocol.
+bool IsIdentifierDefaultProtocolClient(NSString* identifier,
+                                       NSString* protocol) {
+  NSString* defaultProtocolClient =
+  [[NSWorkspace sharedWorkspace] defaultClientIdentifierForProtocol:protocol];

[Mark Mentovai, 2011/05/23 00:23:52]

The continuation line needs to be indented somehow from the line that it’s
continuing. We generally indent it by four spaces, but often there are better
ways of handling the indentation, sometimes even by putting something on the
preceding line. See line 72, where you copied this from, for one example of how
we handle this. (I can appreciate that you removed the extra indentation here to
avoid the awkward wrapping that results from the 80-column limit, but this is no
good either.)

[benwells, 2011/05/24 06:10:38]

Done.

+  if (!defaultProtocolClient)
+    return false;
+  // We need to ensure we do the comparison case-insensitive as LS doesn't

[Mark Mentovai, 2011/05/23 00:23:52]

I like a blank line before a new comment other than one beginning a new deeper
nesting level, and I like a blank line after an early return. In this case,
you’ve got both. It makes the code easier to read by breaking up the flow and
better highlighting the comment.

[benwells, 2011/05/24 06:10:38]

Done.

+  // persist the case of our bundle id.
+  NSComparisonResult result =
+  [defaultProtocolClient caseInsensitiveCompare:identifier];

[Mark Mentovai, 2011/05/23 00:23:52]

As above with respect to indenting continuation lines.

[benwells, 2011/05/24 06:10:38]

Done.

+  return result == NSOrderedSame;
+}
+
 }  // namespace
 
 // Attempt to determine if this instance of Chrome is the default browser and
 // return the appropriate state. (Defined as being the handler for HTTP/HTTPS
 // protocols; we don't want to report "no" here if the user has simply chosen
 // to open HTML files in a text editor and FTP links with an FTP client.)
-ShellIntegration::DefaultBrowserState ShellIntegration::IsDefaultBrowser() {
+ShellIntegration::DefaultClientAppState ShellIntegration::IsDefaultBrowser() {
   // As above, we want to use the real main bundle.
   NSString* myIdentifier = [[NSBundle mainBundle] bundleIdentifier];
   if (!myIdentifier)
-    return UNKNOWN_DEFAULT_BROWSER;
-  return IsIdentifierDefaultBrowser(myIdentifier) ? IS_DEFAULT_BROWSER
-                                                  : NOT_DEFAULT_BROWSER;
+    return UNKNOWN_DEFAULT_CLIENT_APP;
+  return IsIdentifierDefaultBrowser(myIdentifier) ? IS_DEFAULT_CLIENT_APP
+                                                  : NOT_DEFAULT_CLIENT_APP;
 }
 
 // Returns true if Firefox is the default browser for the current user.
 bool ShellIntegration::IsFirefoxDefaultBrowser() {
   return IsIdentifierDefaultBrowser(@"org.mozilla.firefox");
 }
+
+  // Attempt to determine if this instance of Chrome is the default client

[Mark Mentovai, 2011/05/23 00:23:52]

Odd indentation on the comment.

+  // application for the given protocol and return the appropriate state.
+ShellIntegration::DefaultClientAppState
+    ShellIntegration::IsDefaultProtocolClient(const std::string& protocol) {
+  // As above, we want to use the real main bundle.

[Mark Mentovai, 2011/05/23 00:23:52]

“Above” and “below” are relative, subject to change, and may be invalidated by
future maintenance. It’s also possible that so much code will be inserted
between “here” and “above” that the description will no longer be useful. It’s
better to say the name of the function you’re referring to.

(Again, I get that you just copied this from elsewhere. Now’s as good a time as
any to make the world better.)

[benwells, 2011/05/24 06:10:38]

Done.

+  NSString* myIdentifier = [[NSBundle mainBundle] bundleIdentifier];

[Mark Mentovai, 2011/05/23 00:23:52]

Since this is a C++ method, use c++ style_naming and not Objective-C
styleNaming.

[benwells, 2011/05/24 06:10:38]

Done. As these are all C++ methods (please correct me if that is incorrect) I
updated the naming throughout the file.

+  if (!myIdentifier)
+    return UNKNOWN_DEFAULT_CLIENT_APP;
+  NSString* nsProtocol = [NSString stringWithCString: protocol.c_str()

[Mark Mentovai, 2011/05/23 00:23:52]

Encodings and conversions as on line 60.

[Mark Mentovai, 2011/05/23 00:23:52]

As on line 122, this should be ns_protocol, or I’d usually write protocol_ns.
Either is fine, but nsProtocol is not in a C++ function.

[Mark Mentovai, 2011/05/23 00:23:52]

Blank line before an early return as on line 90.

[benwells, 2011/05/24 06:10:38]

Done.

[benwells, 2011/05/24 06:10:38]

Done.

+                                   encoding:[NSString defaultCStringEncoding]];
+  return IsIdentifierDefaultProtocolClient(myIdentifier,

[Mark Mentovai, 2011/05/23 00:23:52]

This whole statement is eating up a lot of vertical space. I think that this is
just as readable:

  return IsIdentifierDefaultProtocolClient(myIdentifier, nsProtocol) ?
      IS_DEFAULT_CLIENT_APP : NOT_DEFAULT_CLIENT_APP;

[benwells, 2011/05/24 06:10:38]

Done.

+                                           nsProtocol) ? IS_DEFAULT_CLIENT_APP
+                                                       : NOT_DEFAULT_CLIENT_APP;
+}