New experiment without registry deletion

chrome/browser/shell_integration_win.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.
 
 #include "chrome/browser/shell_integration.h"
 
 #include <windows.h>
 #include <shlwapi.h>
 #include <shobjidl.h>
 #include <propkey.h>  // Needs to come after shobjidl.h.
@@ skipping 235 matching lines @@
     case ShellUtil::NOT_DEFAULT:
       return ShellIntegration::NOT_DEFAULT;
     case ShellUtil::IS_DEFAULT:
       return ShellIntegration::IS_DEFAULT;
     default:
       DCHECK_EQ(ShellUtil::UNKNOWN_DEFAULT, default_state);
       return ShellIntegration::UNKNOWN_DEFAULT;
   }
 }
 
-// Resets the default browser choice for the current user.
-void ResetDefaultBrowser() {
-  static const wchar_t* const kUrlAssociationKeyFormats[] = {
-      L"SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\"
-      L"%ls\\UserChoice",
-      L"SOFTWARE\\Microsoft\\Windows\\Roaming\\OpenWith\\UrlAssociations\\"
-      L"%ls\\UserChoice"};
-  static const wchar_t* const kProtocols[] = {L"http", L"https"};
-
-  for (const wchar_t* format : kUrlAssociationKeyFormats) {
-    for (const wchar_t* protocol : kProtocols) {
-      base::win::RegKey registry_key(
-          HKEY_CURRENT_USER, base::StringPrintf(format, protocol).c_str(),
-          KEY_SET_VALUE);
-      registry_key.DeleteValue(L"Hash");
-    }
-  }
-}
-
 // Returns true if the AsyncSetAsDefault field trial is activated.
 bool IsAsyncSetAsDefaultEnabled() {
   using base::CommandLine;
 
   // Note: It's important to query the field trial state first, to ensure that
   // UMA reports the correct group.
   const std::string group_name =
-      base::FieldTrialList::FindFullName("AsyncSetAsDefault");
+      base::FieldTrialList::FindFullName(kAsyncSetAsDefaultExperimentName);
   if (CommandLine::ForCurrentProcess()->HasSwitch(kDisableAsyncSetAsDefault))
     return false;
   if (CommandLine::ForCurrentProcess()->HasSwitch(kEnableAsyncSetAsDefault))
     return true;
 
   return base::StartsWith(group_name, "Enabled", base::CompareCase::SENSITIVE);
 }
 
 bool RegisterBrowser() {
   base::FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe)) {
     NOTREACHED() << "Error getting app exe path";
     return false;
   }
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
 
   return ShellUtil::RegisterChromeBrowser(dist, chrome_exe, base::string16(),
                                           true);
 }
 
+// Returns true if the default browser choice should be reset for the current
+// user.
+bool ShouldResetDefaultBrowser() {
+  return base::FieldTrialList::FindFullName(kAsyncSetAsDefaultExperimentName) !=
+         "EnabledNoRegistry";

[gab, 2016/01/11 23:41:00]

Use StartsWith() instead of == for comparing group names (more flexible server
side after)

[Patrick Monette, 2016/01/12 00:07:27]

Done.

+}
+
+// Resets the default browser choice for the current user.
+void ResetDefaultBrowser() {

[gab, 2016/01/11 23:41:00]

What's the reason to move the method? It appears unchanged but makes this diff
bigger..?

[Patrick Monette, 2016/01/12 00:07:27]

Just to make order of definition = order of use.

+  static const wchar_t* const kUrlAssociationKeyFormats[] = {
+      L"SOFTWARE\\Microsoft\\Windows\\Shell\\Associations\\UrlAssociations\\"
+      L"%ls\\UserChoice",
+      L"SOFTWARE\\Microsoft\\Windows\\Roaming\\OpenWith\\UrlAssociations\\"
+      L"%ls\\UserChoice"};
+  static const wchar_t* const kProtocols[] = {L"http", L"https"};
+
+  for (const wchar_t* format : kUrlAssociationKeyFormats) {
+    for (const wchar_t* protocol : kProtocols) {
+      base::win::RegKey registry_key(
+          HKEY_CURRENT_USER, base::StringPrintf(format, protocol).c_str(),
+          KEY_SET_VALUE);
+      registry_key.DeleteValue(L"Hash");
+    }
+  }
+}
+
 }  // namespace
 
 // static
 bool ShellIntegration::IsSetAsDefaultAsynchronous() {
   return base::win::GetVersion() >= base::win::VERSION_WIN10 &&
          IsAsyncSetAsDefaultEnabled();
 }
 
 bool ShellIntegration::SetAsDefaultBrowser() {
   base::FilePath chrome_exe;
@@ skipping 392 matching lines @@
 
 void ShellIntegration::DefaultBrowserWorker::FinalizeSetAsDefault() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(set_as_default_initialized());
 
   async_timer_.reset();
   StartupBrowserCreator::ClearDefaultBrowserCallback();
 }
 
 // static
 bool ShellIntegration::DefaultBrowserWorker::SetAsDefaultBrowserAsynchronous() {

[gab, 2016/01/11 23:43:28]

It's weird to have a group ("EnablednoRegistry") trigger some things as part of
things triggered by a substring of it ("Enabled").

How about instead having an enum which indicates what to do and passing it to
this method based on specific group name rather than nesting group name meaning.

[Patrick Monette, 2016/01/12 00:07:27]

This won't change the fact that there is 2 meaning behind the group name. We're
juggling with 3 possible groups where 2 are very similar. I don't think it will
be easy to make only one call to FindFullName() without making the rest of the
plumbing convoluted.

There is 2 different significations encoded in the group name and the code makes
it explicit.

I don't fully understand your idea though. I probably missed something :S

   DCHECK(IsSetAsDefaultAsynchronous());
 
   // Registers chrome.exe as a browser on Windows to make sure it will be shown
   // in the "How would you like to open this?" prompt.
   if (!RegisterBrowser())
     return false;
 
-  ResetDefaultBrowser();
+  if (ShouldResetDefaultBrowser())
+    ResetDefaultBrowser();
 
   base::CommandLine cmdline(base::FilePath(L"openwith.exe"));
   cmdline.AppendArgNative(StartupBrowserCreator::GetDefaultBrowserUrl());
   return base::LaunchProcess(cmdline, base::LaunchOptions()).IsValid();
 }