Refactor of BrowserDistribution.

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 200 matching lines @@
   }
 
   // This method returns a list of all the registry entries that
   // are needed to register this installation's ProgId and AppId.
   // These entries need to be registered in HKLM prior to Win8.
   static void GetProgIdEntries(BrowserDistribution* dist,
                                const string16& chrome_exe,
                                const string16& suffix,
                                ScopedVector<RegistryEntry>* entries) {
     string16 icon_path(
-        ShellUtil::FormatIconLocation(chrome_exe, dist->GetIconIndex()));
+        ShellUtil::FormatIconLocation(chrome_exe, dist->
+            GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index));

[gab, 2013/05/18 23:57:22]

In general for this sort of wrapping use something like:

string16 icon_path(ShellUtil::FormatIconLocation(
    chrome_exe,
    dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index));

(i.e. wrapping at the parenthesis for params is only ok if it fits, but can't be
mixed with 4 space indent if it doesn't).

[calamity, 2013/05/24 02:03:11]

Done.

     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(
         ShellUtil::GetBrowserModelId(
             dist, InstallUtil::IsPerUserInstall(chrome_exe.c_str())));
     string16 delegate_guid;
     bool set_delegate_execute =
         IsChromeMetroSupported() &&
@@ skipping 77 matching lines @@
       string16 chrome_application(chrome_html_prog_id +
                                   ShellUtil::kRegApplication);
       entries->push_back(new RegistryEntry(
           chrome_application, ShellUtil::kRegAppUserModelId, app_id));
       entries->push_back(new RegistryEntry(
           chrome_application, ShellUtil::kRegApplicationIcon, icon_path));
       // TODO(grt): http://crbug.com/75152 Write a reference to a localized
       // resource for name, description, and company.
       entries->push_back(new RegistryEntry(
           chrome_application, ShellUtil::kRegApplicationName,
-          dist->GetAppShortCutName()));
+          dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name));

[huangs, 2013/05/21 21:35:41]

Might need another "name" routine; see comment in browser_distribution.h.

       entries->push_back(new RegistryEntry(
           chrome_application, ShellUtil::kRegApplicationDescription,
           dist->GetAppDescription()));
       entries->push_back(new RegistryEntry(
           chrome_application, ShellUtil::kRegApplicationCompany,
           dist->GetPublisherName()));
     }
   }
 
   // This method returns a list of the registry entries needed to declare a
@@ skipping 12 matching lines @@
   // this installation in "RegisteredApplications" on Windows (to appear in
   // Default Programs, StartMenuInternet, etc.).
   // These entries need to be registered in HKLM prior to Win8.
   // If |suffix| is not empty, these entries are guaranteed to be unique on this
   // machine.
   static void GetShellIntegrationEntries(BrowserDistribution* dist,
                                          const string16& chrome_exe,
                                          const string16& suffix,
                                          ScopedVector<RegistryEntry>* entries) {
     const string16 icon_path(
-        ShellUtil::FormatIconLocation(chrome_exe, dist->GetIconIndex()));
+        ShellUtil::FormatIconLocation(chrome_exe, dist->
+            GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index));
     const string16 quoted_exe_path(L"\"" + chrome_exe + L"\"");
 
     // Register for the Start Menu "Internet" link (pre-Win7).
     const string16 start_menu_entry(GetBrowserClientKey(dist, suffix));
     // Register Chrome's display name.
     // TODO(grt): http://crbug.com/75152 Also set LocalizedString; see
     // http://msdn.microsoft.com/en-us/library/windows/desktop/cc144109(v=VS.85).aspx#registering_the_display_name
     entries->push_back(new RegistryEntry(
-        start_menu_entry, dist->GetAppShortCutName()));
+        start_menu_entry,
+        dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name));

[huangs, 2013/05/21 21:35:41]

Might need another "name" routine; see comment in browser_distribution.h.

     // Register the "open" verb for launching Chrome via the "Internet" link.
     entries->push_back(new RegistryEntry(
         start_menu_entry + ShellUtil::kRegShellOpen, quoted_exe_path));
     // Register Chrome's icon for the Start Menu "Internet" link.
     entries->push_back(new RegistryEntry(
         start_menu_entry + ShellUtil::kRegDefaultIcon, icon_path));
 
     // Register installation information.
     string16 install_info(start_menu_entry + L"\\InstallInfo");
     // Note: not using CommandLine since it has ambiguous rules for quoting
@@ skipping 15 matching lines @@
     // Write out Chrome's Default Programs info.
     // TODO(grt): http://crbug.com/75152 Write a reference to a localized
     // resource rather than this.
     entries->push_back(new RegistryEntry(
         capabilities, ShellUtil::kRegApplicationDescription,
         dist->GetLongAppDescription()));
     entries->push_back(new RegistryEntry(
         capabilities, ShellUtil::kRegApplicationIcon, icon_path));
     entries->push_back(new RegistryEntry(
         capabilities, ShellUtil::kRegApplicationName,
-        dist->GetAppShortCutName()));
+        dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name));
 
     entries->push_back(new RegistryEntry(capabilities + L"\\Startmenu",
         L"StartMenuInternet", reg_app_name));
 
     const string16 html_prog_id(GetBrowserProgId(suffix));
     for (int i = 0; ShellUtil::kPotentialFileAssociations[i] != NULL; i++) {
       entries->push_back(new RegistryEntry(
           capabilities + L"\\FileAssociations",
           ShellUtil::kPotentialFileAssociations[i], html_prog_id));
     }
@@ skipping 85 matching lines @@
     for (int i = 0; ShellUtil::kDefaultFileAssociations[i] != NULL; i++) {
       string16 ext_key(ShellUtil::kRegClasses);
       ext_key.push_back(base::FilePath::kSeparators[0]);
       ext_key.append(ShellUtil::kDefaultFileAssociations[i]);
       entries->push_back(new RegistryEntry(ext_key, html_prog_id));
     }
 
     // Protocols associations.
     string16 chrome_open = ShellUtil::GetChromeShellOpenCmd(chrome_exe);
     string16 chrome_icon =
-        ShellUtil::FormatIconLocation(chrome_exe, dist->GetIconIndex());
+        ShellUtil::FormatIconLocation(chrome_exe, dist->
+            GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index);
     for (int i = 0; ShellUtil::kBrowserProtocolAssociations[i] != NULL; i++) {
       GetXPStyleUserProtocolEntries(ShellUtil::kBrowserProtocolAssociations[i],
                                     chrome_icon, chrome_open, entries);
     }
 
     // start->Internet shortcut.
     string16 start_menu(ShellUtil::kRegStartMenuInternet);
     string16 app_name = dist->GetBaseAppName() + suffix;
     entries->push_back(new RegistryEntry(start_menu, app_name));
   }
@@ skipping 384 matching lines @@
 // Associates Chrome with |protocol| in the registry. This should not be
 // required on Vista+ but since some applications still read these registry
 // keys directly, we have to do this on Vista+ as well.
 // See http://msdn.microsoft.com/library/aa767914.aspx for more details.
 bool RegisterChromeAsDefaultProtocolClientXPStyle(BrowserDistribution* dist,
                                                   const string16& chrome_exe,
                                                   const string16& protocol) {
   ScopedVector<RegistryEntry> entries;
   const string16 chrome_open(ShellUtil::GetChromeShellOpenCmd(chrome_exe));
   const string16 chrome_icon(
-      ShellUtil::FormatIconLocation(chrome_exe, dist->GetIconIndex()));
+      ShellUtil::FormatIconLocation(chrome_exe,
+          dist->
+          GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).icon_index));
   RegistryEntry::GetXPStyleUserProtocolEntries(protocol, chrome_icon,
                                                chrome_open, &entries);
   // Change the default protocol handler for current user.
   if (!AddRegistryEntries(HKEY_CURRENT_USER, entries)) {
     LOG(ERROR) << "Could not make Chrome default protocol client (XP).";
     return false;
   }
 
   return true;
 }
 
 // Returns |properties.shortcut_name| if the property is set, otherwise it
-// returns dist->GetAppShortcutName(). In any case, it makes sure the
-// return value is suffixed with ".lnk".
+// returns dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name. In
+// any case, it makes sure the return value is suffixed with ".lnk".
 string16 ExtractShortcutNameFromProperties(
     BrowserDistribution* dist,
     const ShellUtil::ShortcutProperties& properties) {
   DCHECK(dist);
   string16 shortcut_name;

[huangs, 2013/05/21 21:35:41]

string shortcut_name(
    properties.has_shortcut_name() ? properties.shortcut_name :
    dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name);

[calamity, 2013/05/24 02:03:11]

Done.

[calamity, 2013/05/24 02:03:11]

Done.

-  if (properties.has_shortcut_name())
+  if (properties.has_shortcut_name()) {
     shortcut_name = properties.shortcut_name;
-  else
-    shortcut_name = dist->GetAppShortCutName();
+  } else {
+    shortcut_name =
+        dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name;
+  }
 
   if (!EndsWith(shortcut_name, installer::kLnkExt, false))
     shortcut_name.append(installer::kLnkExt);
 
   return shortcut_name;
 }
 
 // Converts ShellUtil::ShortcutOperation to the best-matching value in
 // base::win::ShortcutOperation.
 base::win::ShortcutOperation TranslateShortcutOperation(
@@ skipping 447 matching lines @@
     default:
       NOTREACHED();
       return false;
   }
 
   if (!PathService::Get(dir_key, path) || path->empty()) {
     NOTREACHED() << dir_key;
     return false;
   }
 
-  if (add_folder_for_dist)
-    *path = path->Append(dist->GetAppShortCutName());
+  if (add_folder_for_dist) {
+    *path = path->Append(
+        dist->GetShortcutInfo(BrowserDistribution::SHORTCUT_CHROME).name);

[huangs, 2013/05/21 21:35:41]

Add TODO.

[calamity, 2013/05/24 02:03:11]

Done.

+  }
 
   return true;
 }
 
 bool ShellUtil::CreateOrUpdateShortcut(
     ShellUtil::ShortcutLocation location,
     BrowserDistribution* dist,
     const ShellUtil::ShortcutProperties& properties,
     ShellUtil::ShortcutOperation operation) {
   // Explicitly whitelist locations to which this is applicable.
@@ skipping 72 matching lines @@
       base::win::GetVersion() >= base::win::VERSION_WIN7) {
     ret = base::win::TaskbarPinShortcutLink(chosen_path->value().c_str());
     if (!ret) {
       LOG(ERROR) << "Failed to pin " << chosen_path->value();
     }
   }
 
   return ret;
 }
 
 string16 ShellUtil::FormatIconLocation(const string16& icon_path,

[huangs, 2013/05/21 21:35:41]

This is a convenience routine. Consider replacing |icon_index| with:
  BrowserDistribution* dist,
  BrowserDistribution::ShortcutEnum* shortcut_enum
to simplify caller work.

(Need to change browser/shell_integration_win.cc).

[calamity, 2013/05/24 02:03:11]

Any reason we pass in an icon_path? Shouldn't ShortcutInfo have the "correct"
icon path so that we can just that in? In fact, why do most distributions have
IconFilename set to an empty string16?

[gab, 2013/05/24 15:01:35]

Distributions with an empty string16 as the icon filename are supposed to be
distributions with no shortcuts (I think).
I see that this is not the case for browser_distribution.cc (Chromium) and this
is probably a bug.

The reason the icon_path is passed by the caller here is because it's the full
path to chrome.exe (which the BrowserDistribution constants can't hardcode as it
is install-path-dependent).

Since the icons will soon always be stored in chrome.exe (such is the plan for
the app launcher icon I think if that's not already the case) it probably makes
sense to remove GetIconFilename() eventually (not in this CL) -- in fact
GetIconFilename() was added exactly to support the double executables holding
icons in http://crrev.com/168722 and we should soon be able to get rid of it.

Only the icon index in chrome.exe changes based on the distribution.

[gab, 2013/05/24 15:01:35]

I agree with Sam though that for convenience for the caller we can probably just
pass in the ShortcutEnum here instead of the icon_index directly (haven't looked
at all callers to see if that's how it's used everywhere though).

I don't think we can work around passing |icon_path| though.

                                        int icon_index) {
   string16 icon_string(icon_path);
   icon_string.append(L",");
   icon_string.append(base::IntToString16(icon_index));
   return icon_string;
 }
 
 string16 ShellUtil::GetChromeShellOpenCmd(const string16& chrome_exe) {
   return L"\"" + chrome_exe + L"\" -- \"%1\"";
 }
@@ skipping 576 matching lines @@
     // are any left...).
     if (free_bits >= 8 && next_byte_index < size) {
       free_bits -= 8;
       bit_stream += bytes[next_byte_index++] << free_bits;
     }
   }
 
   DCHECK_EQ(ret.length(), encoded_length);
   return ret;
 }