Don't register or use the DelegateExecute verb handler if it isn't present.

chrome/installer/util/shell_util.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // This file defines functions that integrate Chrome in Windows shell. These
 // functions can be used by Chrome as well as Chrome installer. All of the
 // work is done by the local functions defined in anonymous namespace in
 // this class.
 
 #include "chrome/installer/util/shell_util.h"
@@ skipping 71 matching lines @@
                                const string16& chrome_exe,
                                const string16& suffix,
                                std::list<RegistryEntry*>* entries) {
     string16 icon_path(ShellUtil::GetChromeIcon(dist, chrome_exe));
     string16 open_cmd(ShellUtil::GetChromeShellOpenCmd(chrome_exe));
     string16 delegate_command(ShellUtil::GetChromeDelegateCommand(chrome_exe));
     // For user-level installs: entries for the app id and DelegateExecute verb
     // handler will be in HKCU; thus we do not need a suffix on those entries.
     string16 app_id(dist->GetBrowserAppId());
     string16 delegate_guid;
     bool set_delegate_execute =

[gab, 2012/04/25 05:18:29]

Do we want to also check && dist->CanSetAsDefault()? Given there is no point
setting up a delegate execute if we can't use Metro (but it probably doesn't
hurt doing so either and it would help having the registrations ready if the
Win8 rules magically change to allow non-default browsers in Metro).

[grt (UTC plus 2), 2012/04/25 13:05:11]

This function is never called when !dist->CanSetAsDefault().  We could put in a
DCHECK if you think it's necessary, but I think it's part of the contract: this
is only for setting up the default browser.

[gab, 2012/04/25 13:44:02]

Ah ok right. A DCHECK wouldn't hurt, but to be consistent it would need to be in
every similar calls... I don't think it's necessary to add this right now.

[grt (UTC plus 2), 2012/04/25 14:33:49]

Groovy.

-        base::win::GetVersion() >= base::win::VERSION_WIN8 &&
-        dist->GetDelegateExecuteHandlerData(&delegate_guid, NULL, NULL, NULL);
+        dist->GetDelegateExecuteHandlerData(&delegate_guid, NULL, NULL, NULL) &&
+        InstallUtil::HasDelegateExecuteHandler(dist, chrome_exe);

[gab, 2012/04/25 05:18:29]

This is somewhat cleaner than the registry lookup however what I don't like is
that it depends on two independent parts of the code checking the same thing
(i.e. someone can change one and not the other).

Where as if its sequential (i.e. by checking registry entry here), we have:
delegate_execute present --> register its class
delegate_execute class registered --> register it as a handler for chrome

So that we can never do the latter if the former has not been done (which, if it
happened, would result in class not registered errors again (as we were getting
before doing the automatic registration if forgetting to call
delegate_execute.exe /regserver).

I suggest keeping the InstallUtil::HasDelegateExecuteHandler function as the
check is cleaner in this function than the inline thing I hacked together before
leaving, but changing the function to lookup in the registry. The way I had it
earlier it would look at use ExistsInRegistry which also confirms the value. I
feel we should add RegistryEntry.NameExistsInRegistry() as such:

  bool NameExistsInRegistry() {
    return (StatusInRegistryUnderRoot(HKEY_CURRENT_USER) != DOES_NOT_EXISTS ||
               StatusInRegistryUnderRoot(HKEY_LOCAL_MACHINE) !=
DOES_NOT_EXISTS));
  }

since we don't care about the value for this lookup imo.

Actually since ::RegistryEntry is not exposed outside of shell_util.cc we
probably need to move the HasDelegateExecuteHandler function here as well.

[grt (UTC plus 2), 2012/04/25 13:05:11]

I think it's safer for all logic to key off the same conditions.  With "if A
then B" and "if B then C", you leave the possibility that you end up doing C
because B happens to be satisfied when it shouldn't.  We've learned this lesson
the hard way in other parts of the installer where we have two pieces of state
that have to stay in sync.  Even though you think you've covered all your bases,
life happens and, for example, the power goes out at just the wrong time, so one
of the bits of state is left behind.

To make things more concrete, I think checking the registry is especially
un-safe in this case because all installs out there right now have the COM
registration in place.  If we somehow miss cleaning it up (entirely possible),
then this code would go ahead and put the DelegateExecute=GUID code in place.

Are you comfortable with having comments in the two checks referring to each
other so that they're less likely to drift?  Also, this check should be removed
eventually just like the other, so this is really a short-term change, is it
not?

[gab, 2012/04/25 13:44:02]

But in this case your code checking for A deletes entries B if they are not
needed, so that I don't see how we can get a B present when undesired.
The only case I see where we can't remove the COM registration in A is if it is
in HKLM and this is a user-level install, but the last patch also made sure that
all progids are registered in HKCU only for user-level installs.

I agree your method is safer code as is, but it's more programmer error-prone in
the future.
We can only remove this check in the future if we are confident that everyone
will keep dist->GetDelegateExecuteHandlerData() returning false for
distributions without the .exe ( I think this is safe to assume, but just
pointing it out ).

[grt (UTC plus 2), 2012/04/25 14:33:49]

Except when it doesn't.  Rather than mathematically prove that the
install_worker.cc codepath will be hit and do the right thing 100% of the time,
I think it's safer to make this code tolerate the cases when it isn't.
I think this is a six-of-one, half-dozen-of-the-other case.  I hope this code
doesn't live long enough into the future to be an issue.
That's the intent for BrowserDistribution::GetDelegateExecuteHandlerData.  I've
added a clarifying comment to browser_distribution.h.

 
     // DelegateExecute ProgId. Needed for Chrome Metro in Windows 8.
     if (set_delegate_execute) {

[gab, 2012/04/25 13:44:02]

FYI, this code still will not remove
\\Software\Classes\Chrome
and
\\Software\Classes\ChromeHTML\open\DelegateExecute
if !set_delegate_execute

This will be needed if we want to fix current bad installs on update.

[grt (UTC plus 2), 2012/04/25 14:33:49]

Indeed.  I've added this to a more recent patchset.

       string16 model_id_shell(ShellUtil::kRegClasses);
       model_id_shell.push_back(FilePath::kSeparators[0]);
       model_id_shell.append(app_id);
       model_id_shell.append(ShellUtil::kRegExePath);
       model_id_shell.append(ShellUtil::kRegShellPath);
 
       // <root hkey>\Software\Classes\<app_id>\.exe\shell @=open
       entries->push_front(new RegistryEntry(model_id_shell,
                                             ShellUtil::kRegVerbOpen));
 
@@ skipping 1107 matching lines @@
       chrome_exe.c_str(),
       shortcut.c_str(),
       chrome_path.c_str(),
       arguments.c_str(),
       description.c_str(),
       icon_path.c_str(),
       icon_index,
       dist->GetBrowserAppId().c_str(),
       ConvertShellUtilShortcutOptionsToFileUtil(options));
 }