Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6961013:
Allow chrome to become the os default handler for arbitrary protocols on mac/win. (Closed)
|
Created:
9 years, 7 months ago by benwells Modified:
9 years, 7 months ago Reviewers:
robert, Lei Zhang, xiyuan, cpu_(ooo_6.6-7.5), Evan Stade, Mark Mentovai, robertshield, Noel Gordon, grt (UTC plus 2) CC:
chromium-reviews, pam+watch_chromium.org, Mike Lawther (Google), koz (OOO until 15th September) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAllow chrome to become the os default handler for arbitrary protocols on mac/win.
Note this is unused currently, but will be hooked up to
navigator.registerProtocolHandler in a subsequent change.
BUG=83556
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86793
Patch Set 1 #
Total comments: 64
Patch Set 2 : Responded to comments #
Total comments: 74
Patch Set 3 : More changes in response to comments. #
Total comments: 12
Patch Set 4 : More changes for comments; fix for a bug introduced with DefaultWebClientWorker refactoring #
Total comments: 1
Messages
Total messages: 26 (0 generated)
This change is so that koz's registerProtocolHandler feature can integrate into the OS. So if you register gmail or hotmail as your mailto: handler in chrome, chrome will also register itself with mac or windows as the default mailto handler. The change on mac was pretty simple but on windows got a bit hairy, requiring changes to be made to the setup.exe for elevation, changes to how chrome registers itself with windows and also changes to how the default browser is set. Changes for linux are not implemented in this patch, gnome2 code is ready but this needs some changes to the third party xdg-utils upstream. I am not sure who is the best person to review this, it would be good to get someone who is familiar with the installer / windows registration stuff to have a look. Thanks, Ben.
Not the right person to review this. -Ben On Thu, May 19, 2011 at 10:47 PM, <benwells@chromium.org> wrote: > Reviewers: Ben Goodger, Evan Stade, > > Message: > This change is so that koz's registerProtocolHandler feature can integrate > into > the OS. So if you register gmail or hotmail as your mailto: handler in > chrome, > chrome will also register itself with mac or windows as the default mailto > handler. > > The change on mac was pretty simple but on windows got a bit hairy, > requiring > changes to be made to the setup.exe for elevation, changes to how chrome > registers itself with windows and also changes to how the default browser > is > set. > > Changes for linux are not implemented in this patch, gnome2 code is ready > but > this needs some changes to the third party xdg-utils upstream. > > I am not sure who is the best person to review this, it would be good to > get > someone who is familiar with the installer / windows registration stuff to > have > a look. > > Thanks, > Ben. > > Description: > Allow chrome to become the os default handler for arbitrary protocols on > mac/win. > > Note this is unused currently, but will be hooked up to > navigator.registerProtocolHandler in a subsequent change. > > > Please review this at http://codereview.chromium.org/6961013/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files: > M chrome/browser/platform_util.h > M chrome/browser/platform_util_common_linux.cc > M chrome/browser/platform_util_mac.mm > M chrome/browser/platform_util_win.cc > M chrome/browser/shell_integration.h > M chrome/browser/shell_integration.cc > M chrome/browser/shell_integration_linux.cc > M chrome/browser/shell_integration_mac.mm > M chrome/browser/shell_integration_win.cc > M chrome/browser/ui/webui/options/browser_options_handler.h > M chrome/browser/ui/webui/options/browser_options_handler.cc > M chrome/installer/setup/setup_main.cc > M chrome/installer/setup/uninstall.cc > M chrome/installer/util/shell_util.h > M chrome/installer/util/shell_util.cc > M chrome/installer/util/util_constants.h > M chrome/installer/util/util_constants.cc > > >
can you send this by the trybots? +thestig for xdg utils consideration. We do have edits to third_party/xdg-utils so we don't need to rely on upstream (although it is nice to upstream our changes if possible). I'm not sure who to add for Mac or Win. Added thakis and cpu, hopefully they will be able to help or point you in the right direction. http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... File chrome/browser/platform_util_common_linux.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... chrome/browser/platform_util_common_linux.cc:159: bool CanSetAsDefaultMailClient() { ?
On 2011/05/20 23:16:51, Evan Stade wrote: > can you send this by the trybots? > > +thestig for xdg utils consideration. We do have edits to third_party/xdg-utils > so we don't need to rely on upstream (although it is nice to upstream our > changes if possible). > > I'm not sure who to add for Mac or Win. Added thakis and cpu, hopefully they > will be able to help or point you in the right direction. > > http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... > File chrome/browser/platform_util_common_linux.cc (right): > > http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... > chrome/browser/platform_util_common_linux.cc:159: bool > CanSetAsDefaultMailClient() { > ? What does arbitrary protocol mean on Linux? Do you really intend to be able to set Chromium as the default handler for any protocol, or just a certain set of protocols?
I think Mark is an excellent reviewer for the mac side of this. Please add a BUG= line to this bug (and file a feature bug for this feature if you haven't already).
On 2011/05/20 23:25:33, Lei Zhang wrote: > What does arbitrary protocol mean on Linux? Do you really intend to be able to > set Chromium as the default handler for any protocol, or just a certain set of > protocols? On Linux under gnome2 it uses gconftool-2 to update /dekstop/gnome/url-handlers settings, (or it will when that patch is ready). These functions will be used by the registerProtocolHandler which will use a white list of protocols that are allowed to be registered.
On 2011/05/21 00:47:51, benwells wrote: > On 2011/05/20 23:25:33, Lei Zhang wrote: > > What does arbitrary protocol mean on Linux? Do you really intend to be able to > > set Chromium as the default handler for any protocol, or just a certain set of > > protocols? > > On Linux under gnome2 it uses gconftool-2 to update /dekstop/gnome/url-handlers > settings, (or it will when that patch is ready). > > These functions will be used by the registerProtocolHandler which will use a > white list of protocols that are allowed to be registered. (chatted with mdm about this) Supporting Linux can be tricky - you'll want to get your code into xdg-utils, rather than directly talking to gconf. However, we prefer the system xdg-utils over our own, since many Linux distros have patched theirs to work specifically with their distro. This means your xdg-utils changes will have to go upstream and then get pulled into a Linux distro before you get to use it. Also, you'd probably want to at support KDE as well. Anyway, all this Linux talk is outside the scope of this CL since it's Win/Mac only.
On 2011/05/21 01:29:31, Lei Zhang wrote: <snip> > (chatted with mdm about this) > Supporting Linux can be tricky - you'll want to get your code into xdg-utils, > rather than directly talking to gconf. However, we prefer the system xdg-utils > over our own, since many Linux distros have patched theirs to work specifically > with their distro. This means your xdg-utils changes will have to go upstream > and then get pulled into a Linux distro before you get to use it. Also, you'd > probably want to at support KDE as well. > The gconf stuff I mentioned is in xdg-settings, part of xdg-util. This isn't ready for two reasons - the upstreaming / versioning issues and also support for KDE and maybe gnome3. This seemed complicated so was pulled out of this patch. > Anyway, all this Linux talk is outside the scope of this CL since it's Win/Mac > only. Sure, agreed.
I’m also going to amplify a design question I asked in shell_util_mac.mm, because I believe it’s important and is distinct from the code comments I had: > 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? http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... File chrome/browser/platform_util_common_linux.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... chrome/browser/platform_util_common_linux.cc:159: bool CanSetAsDefaultMailClient() { Yeah, this should be CanSetAsDefaultProtocolClient(const std::string&). http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... File chrome/browser/shell_integration.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. I am not yet examining this file heavily as it will likely change substantially in light of the comments I made in its header. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integration.h File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:41: enum DefaultClientAppState { In isolation, DefaultClientAppState doesn’t tell me anything about this being used as “default client for a URL protocol.” Maybe the name should have the word “protocol” in it as you’ve done with SetAsDefaultProtocolClient, IsDefaultProtocolClient, and CanSetAsDefaultProtocolClient. As it stands now, for all anyone knows, DefaultClientAppState could just as well mean something like “do I start maximized?” http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:44: UNKNOWN_DEFAULT_CLIENT_APP My own preference would be for the UNKNOWN element to have value -1. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:152: // file thread since registry access is involved and this can be slow. Any comment in this file that mentions the “registry” should be revised to use more generic language than “registry” or should limit what it says about the registry to Windows. This is not the only occurrence in this file. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:153: class DefaultClientAppWorker I believe that this interface is now too cumbersome to be part of ShellIntegration’s public API. A DefaultClientAppWorker is now an object that can behave in four different ways depending on which of the four Start methods is called. There doesn’t appear to be any protection against calling more than one of them on a single object, although doing so would be hazardous. At this point, I believe that this needs to be refactored to either make the bulk of this class an internal (and hidden) implementation detail of ShellIntegration, or to provide distinct classes for the DefaultBrowser and DefaultProtocolClient operations (perhaps descended from a common base), or both. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... File chrome/browser/shell_integration_linux.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_linux.cc:258: return false; Is there a bug filed to track adding this feature to Linux? If not, file one. Regardless, reference the bug here. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... File chrome/browser/shell_integration_mac.mm (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. You should familiarize yourself with the Objective-C style guide at http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:11: // FIXME: These functions should be rolled into 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? http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:14: @interface NSWorkspace(ChromiumDefaultProtocolClientAdditions) http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Categ... > There should be a single space between the class name and the opening > parenthesis of the category. Same on line 22. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:16: - (NSString*)defaultClientIdentifierForProtocol:(NSString*)protocol; 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.”) http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:17: - (void)setDefaultClientWithIdentifier:(NSString*)bundleID http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho... > If you have too many parameters to fit on one line, giving each its > own line is preferred. If multiple lines are used, align each using > the colon before the parameter. Same on line 30. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:24: - (NSString*)defaultClientIdentifierForProtocol:(NSString*)protocol Objective-C follows C++ style unless otherwise specified. In this case: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > The open curly brace is always at the end of the same line as the last > parameter. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:26: return [(NSString*)LSCopyDefaultHandlerForURLScheme((CFStringRef)protocol) 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>. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:33: LSSetDefaultHandlerForURLScheme((CFStringRef)protocol, (CFStringRef)bundleID); Make this method return a BOOL, and return |YourCode() == noErr|. You can use the result in ShellIntegration::SetAsDefaultProtocolClient. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:51: // Sets Chromium as client application for the provided protocol (only for Revise this comment. Just because the comment you’re copying needs help doesn’t mean that this one should too. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:53: bool ShellIntegration::SetAsDefaultProtocolClient(const std::string& protocol) { 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. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:59: NSString* identifier = [[NSBundle mainBundle] bundleIdentifier]; IsDefaultProtocolClient specifically handles the case where there is no bundle ID. This doesn’t. Any reason for the discrepancy? http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:60: NSString* nsProtocol = [NSString stringWithCString: protocol.c_str() 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:]. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:62: [[NSWorkspace sharedWorkspace] setDefaultClientWithIdentifier:identifier http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho... > Invocations should have all arguments on one line […] or have one > argument per line, with colons aligned: http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:63: forProtocol: nsProtocol]; 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... http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:87: [[NSWorkspace sharedWorkspace] defaultClientIdentifierForProtocol:protocol]; 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.) http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:90: // We need to ensure we do the comparison case-insensitive as LS doesn't 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. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:93: [defaultProtocolClient caseInsensitiveCompare:identifier]; As above with respect to indenting continuation lines. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:117: // Attempt to determine if this instance of Chrome is the default client Odd indentation on the comment. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:121: // As above, we want to use the real main bundle. “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.) http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:122: NSString* myIdentifier = [[NSBundle mainBundle] bundleIdentifier]; Since this is a C++ method, use c++ style_naming and not Objective-C styleNaming. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:125: NSString* nsProtocol = [NSString stringWithCString: protocol.c_str() Encodings and conversions as on line 60. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:125: NSString* nsProtocol = [NSString stringWithCString: protocol.c_str() 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. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:125: NSString* nsProtocol = [NSString stringWithCString: protocol.c_str() Blank line before an early return as on line 90. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:127: return IsIdentifierDefaultProtocolClient(myIdentifier, 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; http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_win.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. I haven’t reviewed this file. http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/setup_ma... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/setup_ma... chrome/installer/setup/setup_main.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. I haven’t reviewed this file. http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/uninstal... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/uninstal... chrome/installer/setup/uninstall.cc:563: &ShellUtil::kPotentialProtocolAssociations[0]; I find this difficult to read because you’ve got three lines, which usually correspond to the three expressions inside a “for (…)” construct. Your first expression is spread across two lines, and then the remaining two expressions share a line. Also, the indentation on the second and third lines is incorrect. Here’s one option to clean this up: for (const wchar_t* const* proto = &ShellUtil::kPotentialProtocolAssociations[0]; *proto != NULL; ++proto) { Here’s another, with a slight semantic change in |proto|’s scope: const wchar_t* const* proto; for (proto = &ShellUtil::kPotentialProtocolAssociations[0]; *proto; ++proto) { There are other valid ways to address this. http://codereview.chromium.org/6961013/diff/1/chrome/installer/util/shell_util.h File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/6961013/diff/1/chrome/installer/util/shell_uti... chrome/installer/util/shell_util.h:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. I didn’t review this file or anything else in chrome/installer/util.
Extracting from one of the replies: 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(). http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integration.h File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:41: enum DefaultClientAppState { Agreed, it doesn't say enough. I avoided the word 'protocol' as the default browser is not exactly a default protocol handler, it is the default handler for two protocols plus some extra stuff depending on the OS. I will update to DefaultWebClientState. On 2011/05/23 00:23:52, Mark Mentovai wrote: > In isolation, DefaultClientAppState doesn’t tell me anything about this being > used as “default client for a URL protocol.” Maybe the name should have the word > “protocol” in it as you’ve done with SetAsDefaultProtocolClient, > IsDefaultProtocolClient, and CanSetAsDefaultProtocolClient. As it stands now, > for all anyone knows, DefaultClientAppState could just as well mean something > like “do I start maximized?” http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:153: class DefaultClientAppWorker Yes, there would be some hazard if you tried to re-use the same object. Refactoring so that there are two classes, with the protocol client class being constructed with the protocol, might be the best approach. On 2011/05/23 00:23:52, Mark Mentovai wrote: > I believe that this interface is now too cumbersome to be part of > ShellIntegration’s public API. A DefaultClientAppWorker is now an object that > can behave in four different ways depending on which of the four Start methods > is called. There doesn’t appear to be any protection against calling more than > one of them on a single object, although doing so would be hazardous. > > At this point, I believe that this needs to be refactored to either make the > bulk of this class an internal (and hidden) implementation detail of > ShellIntegration, or to provide distinct classes for the DefaultBrowser and > DefaultProtocolClient operations (perhaps descended from a common base), or > both. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... File chrome/browser/shell_integration_mac.mm (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:11: // FIXME: These functions should be rolled into 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: > 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? http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/uninstal... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/uninstal... chrome/installer/setup/uninstall.cc:563: &ShellUtil::kPotentialProtocolAssociations[0]; Ah great, thanks for the suggestion. That looks much better. On 2011/05/23 00:23:52, Mark Mentovai wrote: > I find this difficult to read because you’ve got three lines, which usually > correspond to the three expressions inside a “for (…)” construct. Your first > expression is spread across two lines, and then the remaining two expressions > share a line. > > Also, the indentation on the second and third lines is incorrect. > > Here’s one option to clean this up: > > for (const wchar_t* const* proto = > &ShellUtil::kPotentialProtocolAssociations[0]; > *proto != NULL; > ++proto) { > > Here’s another, with a slight semantic change in |proto|’s scope: > > const wchar_t* const* proto; > for (proto = &ShellUtil::kPotentialProtocolAssociations[0]; *proto; ++proto) { > > There are other valid ways to address this.
http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... File chrome/browser/platform_util_common_linux.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/platform_util_co... chrome/browser/platform_util_common_linux.cc:159: bool CanSetAsDefaultMailClient() { On 2011/05/20 23:16:51, Evan Stade wrote: > ? Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integration.h File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:41: enum DefaultClientAppState { On 2011/05/23 00:23:52, Mark Mentovai wrote: > In isolation, DefaultClientAppState doesn’t tell me anything about this being > used as “default client for a URL protocol.” Maybe the name should have the word > “protocol” in it as you’ve done with SetAsDefaultProtocolClient, > IsDefaultProtocolClient, and CanSetAsDefaultProtocolClient. As it stands now, > for all anyone knows, DefaultClientAppState could just as well mean something > like “do I start maximized?” Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:44: UNKNOWN_DEFAULT_CLIENT_APP On 2011/05/23 00:23:52, Mark Mentovai wrote: > My own preference would be for the UNKNOWN element to have value -1. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:152: // file thread since registry access is involved and this can be slow. On 2011/05/23 00:23:52, Mark Mentovai wrote: > Any comment in this file that mentions the “registry” should be revised to use > more generic language than “registry” or should limit what it says about the > registry to Windows. > > This is not the only occurrence in this file. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration.h:153: class DefaultClientAppWorker On 2011/05/23 00:23:52, Mark Mentovai wrote: > I believe that this interface is now too cumbersome to be part of > ShellIntegration’s public API. A DefaultClientAppWorker is now an object that > can behave in four different ways depending on which of the four Start methods > is called. There doesn’t appear to be any protection against calling more than > one of them on a single object, although doing so would be hazardous. > > At this point, I believe that this needs to be refactored to either make the > bulk of this class an internal (and hidden) implementation detail of > ShellIntegration, or to provide distinct classes for the DefaultBrowser and > DefaultProtocolClient operations (perhaps descended from a common base), or > both. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... File chrome/browser/shell_integration_linux.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_linux.cc:258: return false; On 2011/05/23 00:23:52, Mark Mentovai wrote: > Is there a bug filed to track adding this feature to Linux? > > If not, file one. > > Regardless, reference the bug here. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... File chrome/browser/shell_integration_mac.mm (right): http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:11: // FIXME: These functions should be rolled into On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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? Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:26: return [(NSString*)LSCopyDefaultHandlerForURLScheme((CFStringRef)protocol) On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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>. Done. Please check that I've used these correctly - in particular the usage of CFTypeRefToNSObjectAutorelease (this code is now in IsIdentififerDefaultProtocolClient). http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:33: LSSetDefaultHandlerForURLScheme((CFStringRef)protocol, (CFStringRef)bundleID); On 2011/05/23 00:23:52, Mark Mentovai wrote: > Make this method return a BOOL, and return |YourCode() == noErr|. You can use > the result in ShellIntegration::SetAsDefaultProtocolClient. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:51: // Sets Chromium as client application for the provided protocol (only for On 2011/05/23 00:23:52, Mark Mentovai wrote: > Revise this comment. Just because the comment you’re copying needs help doesn’t > mean that this one should too. Done. Also revised comment for SetAsDefaultBrowser, let me know if I missed something in particular that needs more help. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:53: bool ShellIntegration::SetAsDefaultProtocolClient(const std::string& protocol) { On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:59: NSString* identifier = [[NSBundle mainBundle] bundleIdentifier]; On 2011/05/23 00:23:52, Mark Mentovai wrote: > IsDefaultProtocolClient specifically handles the case where there is no bundle > ID. This doesn’t. Any reason for the discrepancy? 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. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:60: NSString* nsProtocol = [NSString stringWithCString: protocol.c_str() On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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:]. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:62: [[NSWorkspace sharedWorkspace] setDefaultClientWithIdentifier:identifier On 2011/05/23 00:23:52, Mark Mentovai wrote: > http://google-styleguide.googlecode.com/svn/trunk/objcguide.xml?showone=Metho... > > > Invocations should have all arguments on one line […] or have one > > argument per line, with colons aligned: Done (or more accurately, no longer needed). http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:63: forProtocol: nsProtocol]; On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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... Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:87: [[NSWorkspace sharedWorkspace] defaultClientIdentifierForProtocol:protocol]; On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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.) Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:90: // We need to ensure we do the comparison case-insensitive as LS doesn't On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:93: [defaultProtocolClient caseInsensitiveCompare:identifier]; On 2011/05/23 00:23:52, Mark Mentovai wrote: > As above with respect to indenting continuation lines. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:121: // As above, we want to use the real main bundle. On 2011/05/23 00:23:52, Mark Mentovai wrote: > “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.) Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:122: NSString* myIdentifier = [[NSBundle mainBundle] bundleIdentifier]; On 2011/05/23 00:23:52, Mark Mentovai wrote: > Since this is a C++ method, use c++ style_naming and not Objective-C > styleNaming. Done. As these are all C++ methods (please correct me if that is incorrect) I updated the naming throughout the file. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:125: NSString* nsProtocol = [NSString stringWithCString: protocol.c_str() On 2011/05/23 00:23:52, Mark Mentovai wrote: > Encodings and conversions as on line 60. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:125: NSString* nsProtocol = [NSString stringWithCString: protocol.c_str() On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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. Done. http://codereview.chromium.org/6961013/diff/1/chrome/browser/shell_integratio... chrome/browser/shell_integration_mac.mm:127: return IsIdentifierDefaultProtocolClient(myIdentifier, On 2011/05/23 00:23:52, Mark Mentovai wrote: > 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; Done. http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/uninstal... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/6961013/diff/1/chrome/installer/setup/uninstal... chrome/installer/setup/uninstall.cc:563: &ShellUtil::kPotentialProtocolAssociations[0]; On 2011/05/23 00:23:52, Mark Mentovai wrote: > I find this difficult to read because you’ve got three lines, which usually > correspond to the three expressions inside a “for (…)” construct. Your first > expression is spread across two lines, and then the remaining two expressions > share a line. > > Also, the indentation on the second and third lines is incorrect. > > Here’s one option to clean this up: > > for (const wchar_t* const* proto = > &ShellUtil::kPotentialProtocolAssociations[0]; > *proto != NULL; > ++proto) { > > Here’s another, with a slight semantic change in |proto|’s scope: > > const wchar_t* const* proto; > for (proto = &ShellUtil::kPotentialProtocolAssociations[0]; *proto; ++proto) { > > There are other valid ways to address this. Done.
Added xiyuan as a reviewer as he has touched shell_integration_win.cc a lot. I am very keen to get feedback for Windows as the changes there are non-trivial given we're nearing the end of the M13 feature window.
LGTM on all of the Mac bits and the generic cross-platform stuff. Nice work refactoring that class. I haven’t looked at the Windows things. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:109: state)); This would fit on the previous line. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:130: if (observer_) { It doesn’t seem like you need this |observer_| check. Seems dangerous in case something like CompleteCheckIsDefault ever winds up doing something more involved than just calling through to the observer. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:33: // Sets Chrome as client application for the given protocol (only for as the client application http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:34: // current user). Returns false if this operation fails. for the current user http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:147: virtual ~DefaultWebClientObserver() {} Nit (to fix the existing code): within public, private, and protected, the destructor should precede any methods. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:160: // Checks to see if Chrome is the default web client application. For this and the next “Start” call, the header comment should say how the observer gets called. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:183: // the check and sends the result back to the UI thread. Instead of “sends the result back to the UI thread,” this should say what function gets posted to the UI thread. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:188: // Once it is finished it posts a task back to the UI thread to complete Same: say what will be called on the UI thread. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:193: // back to the UI thread from the file thread. And again, for this and the next one, it’s more useful to say where the call comes from, not just the thread. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_linux.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_linux.cc:258: // FIXME: Implement this for Linux - crbug.com/83557 Here and on line 288, we use TODO, not FIXME. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C... http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_mac.mm (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_mac.mm:13: // applies only for current user. Returns false if this cannot be done, or if for the current user http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_mac.mm:93: No blank line here.
http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:101: nit: nuke one empty line. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:135: // The current default browser / client application UI state nit: did you mean "web client application"? http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:404: IApplicationAssociationRegistration* pAAR; suggest to use base::win::ScopedComPtr<...> so that it will automatically released when out of scope. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:169: std::list<RegistryEntry*>* entries) { nit: please fix args alignment http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:220: chrome_icon, chrome_open, entries); nit: should use either 4 spaces indentation or align with first-line args http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:714: IApplicationAssociationRegistration* pAAR; nit: please fix indentation http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:714: IApplicationAssociationRegistration* pAAR; suggest to use ScopedComPtr http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:725: if (SUCCEEDED(hr)) { Think you should break out of loop when !SUCCEEDED(hr). Otherwise, the failure is ignored. You can put SUCCEEDED(hr) as part of the loop end condition. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:733: if (SUCCEEDED(hr)) { similarly here, need to get out of loop when failed. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:787: IApplicationAssociationRegistration* pAAR; similar to above, please fix indentation and suggest to use ScopedComPtr http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:912: nit: nuke the empty line
Added Robert for the changes in the installer. It is not clear to me what does the web page can actually insert into the registry. The fundamental problem here is that the command line of chrome is considered trusted, so I we must prevent arbitrary strings to be pushed into registry values. Secondly, what is the use case for multiple profiles? as far as I can see the default profile is what it would be used for the handlers while it might be more appropiate to use the current profile.
http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:430: GetShortPathName(app_path.value().c_str(), This has a variety of failure modes. Check the return value and act appropriately: if it returns zero, PLOG something useful; if it returns > MAX_PATH, short_app_path is too small. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:447: GetShortPathName(command_line.GetProgram().value().c_str(), Same comment as above. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:449: if (!FilePath::CompareEqualIgnoreCase(short_path, short_app_path)) Ah, I see now that this code is using string comparisons to determine if two paths indicate the same file. chrome/installer/setup_util.cc has code that does a simple string compare (without bothering to shorten the names) followed by, in essence, checking the inode number. Consider switching to that, as my hunch is that it's less error prone and less overhead than getting the short names of the two paths. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:842: // make any user specific changes in this option. Please update this comment so that it accurately describes the new --register-url-protocol switch. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:472: const wchar_t* ShellUtil::kRegURLProtocol = L"URL Protocol"; Remove this and use kRegUrlProtocol (defined below). http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:742: LOG(ERROR) << "Could not make Chrome default browser."; How about logging hr so that we get some idea of why registration failed? http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:914: base::win::GetVersion() >= base::win::VERSION_VISTA) { align with the 'e' in "(elevate" above http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:180: // elevate_if_not_admin: On Vista if user is not admin, try to elevate for Fix comment http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:215: // as the default handler for the protocol is Vista and later versions of is -> in
http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:841: // be used when setup.exe is launched with admin rights. We do not The behaviour of RegisterChromeBrowser below if setup.exe is NOT launched with admin rights is to silently avoid attempting HKLM registration. If the comment is correct here and really means that setup.exe MUST be running with admin rights when this flag is used then could we DCHECK() / LOG(DFATAL) or some such if the user is not currently elevated? http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:857: false); The return value here is a bool and is being shoved into an int. While that may work, it looks unintentional. Can we check the return value instead and use one of the installer::InstallStatus constants instead? http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:213: // given protocol. If necessary it will also call RegisterChromeBrowser above. please define "If necessary".
On 2011/05/24 19:47:21, cpu wrote: > Added Robert for the changes in the installer. > > It is not clear to me what does the web page can actually insert into the > registry. The fundamental problem here is that the command line of chrome is > considered trusted, so I we must prevent arbitrary strings to be pushed into > registry values. > > Secondly, what is the use case for multiple profiles? as far as I can see the > default profile is what it would be used for the handlers while it might be more > appropiate to use the current profile. For the first question, I think the worry is that a website could use registerProtocolHandler to inject arbitrary data into the registry as an open command? If that's the case, they should not be able to do that. An example: a website registers url u?%s for protocol p. Chrome then tells the OS that it is the default handler for p, but doesn't write anything into the registry about u?%s. Taking the example further, if a user opens url p:blah via the operating system, Chrome will be opened with p:blah as an argument. Chrome will then open u?P:blah. About the profiles, at this stage we are only planning on starting the default profile from the OS level.
Hi reviewers, we're trying to get registerProtocolHandler stuff in this week. If possible, if there are only comments, indenting and non-critical suggestions remaining please LGTM. I will address the suggestions ASAP as a separate issue, but the LGTM will let us move forward testing registerProtocolHandler. (I am not meaning to suggest commenting, indenting, style and all the other improvements suggested so far aren't important. The feedback on this CL has been invaluable for my calibration to chromium!) http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:101: On 2011/05/24 17:55:20, xiyuan wrote: > nit: nuke one empty line. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:109: state)); On 2011/05/24 16:28:53, Mark Mentovai wrote: > This would fit on the previous line. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.cc:130: if (observer_) { On 2011/05/24 16:28:53, Mark Mentovai wrote: > It doesn’t seem like you need this |observer_| check. Seems dangerous in case > something like CompleteCheckIsDefault ever winds up doing something more > involved than just calling through to the observer. Interesting and valid point. Instead of checking here, I've put this check in StartCheckIsDefault. Currently the meaning of the checking process is to check and report back to the observer; if no observer is present this process is meaningless and can be skipped, there are no side effects. If this ever changes - that is, if what CheckIsDefault is meant to do changes - StartCheckIsDefault will need to be updated, which is not unexpected and shouldn't catch anyone out. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration.h (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:33: // Sets Chrome as client application for the given protocol (only for On 2011/05/24 16:28:53, Mark Mentovai wrote: > as the client application Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:34: // current user). Returns false if this operation fails. On 2011/05/24 16:28:53, Mark Mentovai wrote: > for the current user Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:135: // The current default browser / client application UI state On 2011/05/24 17:55:20, xiyuan wrote: > nit: did you mean "web client application"? Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:147: virtual ~DefaultWebClientObserver() {} On 2011/05/24 16:28:53, Mark Mentovai wrote: > Nit (to fix the existing code): within public, private, and protected, the > destructor should precede any methods. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar... Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:160: // Checks to see if Chrome is the default web client application. On 2011/05/24 16:28:53, Mark Mentovai wrote: > For this and the next “Start” call, the header comment should say how the > observer gets called. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:183: // the check and sends the result back to the UI thread. On 2011/05/24 16:28:53, Mark Mentovai wrote: > Instead of “sends the result back to the UI thread,” this should say what > function gets posted to the UI thread. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:188: // Once it is finished it posts a task back to the UI thread to complete On 2011/05/24 16:28:53, Mark Mentovai wrote: > Same: say what will be called on the UI thread. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration.h:193: // back to the UI thread from the file thread. On 2011/05/24 16:28:53, Mark Mentovai wrote: > And again, for this and the next one, it’s more useful to say where the call > comes from, not just the thread. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_linux.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_linux.cc:258: // FIXME: Implement this for Linux - crbug.com/83557 On 2011/05/24 16:28:53, Mark Mentovai wrote: > Here and on line 288, we use TODO, not FIXME. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C... Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_mac.mm (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_mac.mm:13: // applies only for current user. Returns false if this cannot be done, or if On 2011/05/24 16:28:53, Mark Mentovai wrote: > for the current user Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_mac.mm:93: On 2011/05/24 16:28:53, Mark Mentovai wrote: > No blank line here. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:404: IApplicationAssociationRegistration* pAAR; On 2011/05/24 17:55:20, xiyuan wrote: > suggest to use base::win::ScopedComPtr<...> so that it will automatically > released when out of scope. Done (also for IsDefaultBrowser earlier). http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:430: GetShortPathName(app_path.value().c_str(), On 2011/05/24 20:27:53, grt wrote: > This has a variety of failure modes. Check the return value and act > appropriately: if it returns zero, PLOG something useful; if it returns > > MAX_PATH, short_app_path is too small. See later... http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:447: GetShortPathName(command_line.GetProgram().value().c_str(), On 2011/05/24 20:27:53, grt wrote: > Same comment as above. See later ... http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:449: if (!FilePath::CompareEqualIgnoreCase(short_path, short_app_path)) On 2011/05/24 20:27:53, grt wrote: > Ah, I see now that this code is using string comparisons to determine if two > paths indicate the same file. chrome/installer/setup_util.cc has code that does > a simple string compare (without bothering to shorten the names) followed by, in > essence, checking the inode number. Consider switching to that, as my hunch is > that it's less error prone and less overhead than getting the short names of the > two paths. The ProgramCompare looks very useful here but when I try to use it chrome_browser won't link. I have added http://crbug.com/83852 to address this issue (which also exists for checking IsDefaultBrowser) after registerProtocolHandler is in. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:841: // be used when setup.exe is launched with admin rights. We do not On 2011/05/24 21:33:44, robertshield wrote: > The behaviour of RegisterChromeBrowser below if setup.exe is NOT launched with > admin rights is to silently avoid attempting HKLM registration. If the comment > is correct here and really means that setup.exe MUST be running with admin > rights when this flag is used then could we DCHECK() / LOG(DFATAL) or some such > if the user is not currently elevated? I am reluctant to make this change now as part of this CL, as I feel it will warrant some specific testing on various OS versions (i.e. XP and 7) before landing it. I've added http://crbug.com/83845 to make this change in a way that won't hold up landing the registerProtocolHandler feature. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:842: // make any user specific changes in this option. On 2011/05/24 20:27:53, grt wrote: > Please update this comment so that it accurately describes the new > --register-url-protocol switch. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:857: false); On 2011/05/24 21:33:44, robertshield wrote: > The return value here is a bool and is being shoved into an int. While that may > work, it looks unintentional. Can we check the return value instead and use one > of the installer::InstallStatus constants instead? Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:169: std::list<RegistryEntry*>* entries) { On 2011/05/24 17:55:20, xiyuan wrote: > nit: please fix args alignment Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:220: chrome_icon, chrome_open, entries); On 2011/05/24 17:55:20, xiyuan wrote: > nit: should use either 4 spaces indentation or align with first-line args Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:472: const wchar_t* ShellUtil::kRegURLProtocol = L"URL Protocol"; On 2011/05/24 20:27:53, grt wrote: > Remove this and use kRegUrlProtocol (defined below). Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:714: IApplicationAssociationRegistration* pAAR; On 2011/05/24 17:55:20, xiyuan wrote: > nit: please fix indentation Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:714: IApplicationAssociationRegistration* pAAR; On 2011/05/24 17:55:20, xiyuan wrote: > suggest to use ScopedComPtr Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:725: if (SUCCEEDED(hr)) { On 2011/05/24 17:55:20, xiyuan wrote: > Think you should break out of loop when !SUCCEEDED(hr). Otherwise, the failure > is ignored. You can put SUCCEEDED(hr) as part of the loop end condition. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:733: if (SUCCEEDED(hr)) { On 2011/05/24 17:55:20, xiyuan wrote: > similarly here, need to get out of loop when failed. Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:742: LOG(ERROR) << "Could not make Chrome default browser."; On 2011/05/24 20:27:53, grt wrote: > How about logging hr so that we get some idea of why registration failed? Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:787: IApplicationAssociationRegistration* pAAR; On 2011/05/24 17:55:20, xiyuan wrote: > similar to above, please fix indentation and suggest to use ScopedComPtr Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:912: On 2011/05/24 17:55:20, xiyuan wrote: > nit: nuke the empty line Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:914: base::win::GetVersion() >= base::win::VERSION_VISTA) { On 2011/05/24 20:27:53, grt wrote: > align with the 'e' in "(elevate" above Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.h (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:180: // elevate_if_not_admin: On Vista if user is not admin, try to elevate for On 2011/05/24 20:27:53, grt wrote: > Fix comment Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:213: // given protocol. If necessary it will also call RegisterChromeBrowser above. On 2011/05/24 21:33:44, robertshield wrote: > please define "If necessary". Done. http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.h:215: // as the default handler for the protocol is Vista and later versions of On 2011/05/24 20:27:53, grt wrote: > is -> in Done.
Looks nice overall. Getting really close. http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:449: if (!FilePath::CompareEqualIgnoreCase(short_path, short_app_path)) On 2011/05/25 08:07:19, benwells wrote: > On 2011/05/24 20:27:53, grt wrote: > > Ah, I see now that this code is using string comparisons to determine if two > > paths indicate the same file. chrome/installer/setup_util.cc has code that > does > > a simple string compare (without bothering to shorten the names) followed by, > in > > essence, checking the inode number. Consider switching to that, as my hunch > is > > that it's less error prone and less overhead than getting the short names of > the > > two paths. > > The ProgramCompare looks very useful here but when I try to use it > chrome_browser won't link. I have added http://crbug.com/83852 to address this > issue (which also exists for checking IsDefaultBrowser) after > registerProtocolHandler is in. If you choose to defer using the same mechanism as ProgramCompare, you must check the return value of the calls to GetShortPathName above. Otherwise, short_path and short_app_path could contain random memory (possibly unterminated strings). http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:725: if (SUCCEEDED(hr)) { On 2011/05/25 08:07:19, benwells wrote: > On 2011/05/24 17:55:20, xiyuan wrote: > > Think you should break out of loop when !SUCCEEDED(hr). Otherwise, the failure > > is ignored. You can put SUCCEEDED(hr) as part of the loop end condition. > > Done. I wonder about this: why not make registration a best-effort deal? If the third call to SetAppAsDefault fails, you don't go back and un-set the first two, so why not continue in the face of error and do the best you can? In this case, you should consider whether it's better to return true if any changes where made ("i did something!") or only if all desired changes were made ("do ALL the registrations!"). And, of course, document the function accordingly. http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:34: #include "chrome/installer/setup/setup_util.h" this goes above the installer/util includes http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:335: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, CLSCTX_INPROC); http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:404: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, CLSCTX_INPROC); http://codereview.chromium.org/6961013/diff/22001/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/6961013/diff/22001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:844: // The --register-url-protocol performs the same actions as Please use good grammar in comments. It's hard to tell what this is really saying. According to the code, --register-url-protocol requires --register-chrome-browser, yet that isn't clear from the comment. Also, the code makes it look like this command: --register-chrome-browser C:\blah\chrome.exe --register-url-protocol spam is not a pure superset of this command: --register-chrome-browser C:\blah\chrome.exe (the latter calls RegisterChromeBrowser, the former calls RegisterChromeForProtocol.) Does registering a protocol also do all of the normal associations? http://codereview.chromium.org/6961013/diff/22001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/6961013/diff/22001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:715: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, CLSCTX_INPROC) http://codereview.chromium.org/6961013/diff/22001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:790: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, CLSCTX_INPROC);
lgtm from me
LGTM
http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:449: if (!FilePath::CompareEqualIgnoreCase(short_path, short_app_path)) On 2011/05/25 14:35:45, grt wrote: > On 2011/05/25 08:07:19, benwells wrote: > > On 2011/05/24 20:27:53, grt wrote: > > > Ah, I see now that this code is using string comparisons to determine if two > > > paths indicate the same file. chrome/installer/setup_util.cc has code that > > does > > > a simple string compare (without bothering to shorten the names) followed > by, > > in > > > essence, checking the inode number. Consider switching to that, as my hunch > > is > > > that it's less error prone and less overhead than getting the short names of > > the > > > two paths. > > > > The ProgramCompare looks very useful here but when I try to use it > > chrome_browser won't link. I have added http://crbug.com/83852 to address this > > issue (which also exists for checking IsDefaultBrowser) after > > registerProtocolHandler is in. > > If you choose to defer using the same mechanism as ProgramCompare, you must > check the return value of the calls to GetShortPathName above. Otherwise, > short_path and short_app_path could contain random memory (possibly unterminated > strings). Done (and same change was made in IsDefaultBrowser). http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/6961013/diff/14001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:725: if (SUCCEEDED(hr)) { On 2011/05/25 14:35:45, grt wrote: > On 2011/05/25 08:07:19, benwells wrote: > > On 2011/05/24 17:55:20, xiyuan wrote: > > > Think you should break out of loop when !SUCCEEDED(hr). Otherwise, the > failure > > > is ignored. You can put SUCCEEDED(hr) as part of the loop end condition. > > > > Done. > > I wonder about this: why not make registration a best-effort deal? If the third > call to SetAppAsDefault fails, you don't go back and un-set the first two, so > why not continue in the face of error and do the best you can? In this case, > you should consider whether it's better to return true if any changes where made > ("i did something!") or only if all desired changes were made ("do ALL the > registrations!"). And, of course, document the function accordingly. I have left the return value as is as it is consistent with ShellIntegration::IsDefaultBrowser, which returns 'no' if any of these registrations are missing. Ideally if there is a failure we would attempt to undo anything we've done, I've added a TODO and http://crbug.com/83970 for this. This is documented in shell_util.h. Alternatively it might be better to keep going and do the best we can, this might set as default successfully as far as the user is concerned (e.g. protocols handled but not files) but would leave the UI inconsistent. Given that this shouldn't happen (i.e. if you've registered your capability successfully you should be able to become default) it seems better to keep the UI consistent with the state. If there are further changes to make here / further discussion perhaps we can take it to http://crbug.com/83970 if it isn't critical to address now. http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... File chrome/browser/shell_integration_win.cc (right): http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:34: #include "chrome/installer/setup/setup_util.h" On 2011/05/25 14:35:45, grt wrote: > this goes above the installer/util includes Done. http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:335: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, On 2011/05/25 14:35:45, grt wrote: > pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, > CLSCTX_INPROC); Done. http://codereview.chromium.org/6961013/diff/22001/chrome/browser/shell_integr... chrome/browser/shell_integration_win.cc:404: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, On 2011/05/25 14:35:45, grt wrote: > pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, > CLSCTX_INPROC); Done. http://codereview.chromium.org/6961013/diff/22001/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/6961013/diff/22001/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:844: // The --register-url-protocol performs the same actions as On 2011/05/25 14:35:45, grt wrote: > Please use good grammar in comments. It's hard to tell what this is really > saying. According to the code, --register-url-protocol requires > --register-chrome-browser, yet that isn't clear from the comment. Also, the > code makes it look like this command: > --register-chrome-browser C:\blah\chrome.exe --register-url-protocol spam > is not a pure superset of this command: > --register-chrome-browser C:\blah\chrome.exe > (the latter calls RegisterChromeBrowser, the former calls > RegisterChromeForProtocol.) Does registering a protocol also do all of the > normal associations? Done. http://codereview.chromium.org/6961013/diff/22001/chrome/installer/util/shell... File chrome/installer/util/shell_util.cc (right): http://codereview.chromium.org/6961013/diff/22001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:715: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, On 2011/05/25 14:35:45, grt wrote: > pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, > CLSCTX_INPROC) Done. http://codereview.chromium.org/6961013/diff/22001/chrome/installer/util/shell... chrome/installer/util/shell_util.cc:790: HRESULT hr = CoCreateInstance(CLSID_ApplicationAssociationRegistration, On 2011/05/25 14:35:45, grt wrote: > pAAR.CreateInstance(CLSID_ApplicationAssociationRegistration, NULL, > CLSCTX_INPROC); Done.
LGTM. http://codereview.chromium.org/6961013/diff/29005/chrome/installer/setup/setu... File chrome/installer/setup/setup_main.cc (right): http://codereview.chromium.org/6961013/diff/29005/chrome/installer/setup/setu... chrome/installer/setup/setup_main.cc:851: // ShellUtil::kPotentialProtocolAssociations. Is registration for a protocol not found in kPotentialProtocolAssociations removed on uninstall? If not, please file a bug for this, as we have some customers who expect uninstall to not leave bad registrations behind.
On 2011/05/26 02:42:53, grt wrote: > LGTM. > > http://codereview.chromium.org/6961013/diff/29005/chrome/installer/setup/setu... > File chrome/installer/setup/setup_main.cc (right): > > http://codereview.chromium.org/6961013/diff/29005/chrome/installer/setup/setu... > chrome/installer/setup/setup_main.cc:851: // > ShellUtil::kPotentialProtocolAssociations. > Is registration for a protocol not found in kPotentialProtocolAssociations > removed on uninstall? If not, please file a bug for this, as we have some > customers who expect uninstall to not leave bad registrations behind. Thanks all, grt: Agreed, uninstall should be clean. These particular keys should be OK, but I need to test this. Other keys like the XP protocol associations are more likely not to be cleaned. I will file any bugs found after testing.
LGTM. Right let's push to dev. |
