Suffix Chrome's appid on user-level installs

chrome/installer/util/shell_util.cc

Index: chrome/installer/util/shell_util.cc
diff --git a/chrome/installer/util/shell_util.cc b/chrome/installer/util/shell_util.cc
index 71a34de3de14f09658ddb2481d4370fbd4f19086..f02522a146d6dacc85d6f98fb3228134acc781f4 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -136,7 +136,7 @@ class RegistryEntry {
     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 app_id(ShellUtil::GetChromiumModelId(dist, chrome_exe));
     string16 delegate_guid;
     // TODO(grt): remove HasDelegateExecuteHandler when the exe is ever-present;
     // see also install_worker.cc's AddDelegateExecuteWorkItems.
@@ -663,7 +663,7 @@ void RemoveBadWindows8RegistrationIfNeeded(
     // suffix.
     const string16 installation_suffix(
         ShellUtil::GetCurrentInstallationSuffix(dist, chrome_exe));
-    const string16 app_id(dist->GetBrowserAppId());
+    const string16 app_id(ShellUtil::GetChromiumModelId(dist, chrome_exe));
 
     // <root hkey>\Software\Classes\<app_id>
     string16 key(ShellUtil::kRegClasses);
@@ -1074,6 +1074,55 @@ string16 ShellUtil::GetApplicationName(BrowserDistribution* dist,
   return app_name;
 }
 
+string16 ShellUtil::GetChromiumModelId(BrowserDistribution* dist,
+                                       const string16& chrome_exe) {
+  string16 app_id(dist->GetBaseAppId());
+  string16 suffix;
+  if (InstallUtil::IsPerUserInstall(chrome_exe.c_str()) &&
+      !GetUserSpecificRegistrySuffix(&suffix)) {
+    NOTREACHED();
+  }
+  // There is only one component (i.e. the suffixed appid) in this case, but it
+  // is still necessary to go through the appid constructor to make sure the
+  // returned appid is truncated if necessary.
+  std::vector<string16> components;

[grt (UTC plus 2), 2012/06/18 19:45:08]

you can construct and populate the vector in one fell swoop with:
std::vector<string16> components(1, app_id.append(suffix));

[gab, 2012/06/18 21:52:43]

Nice! I'd looked for (and obviously didn't find) a constructor that would take
just a string16, but that'll do perfect :)!

+  components.push_back(app_id.append(suffix));
+  return ConstructAppModelId(components);
+}
+
+string16 ShellUtil::ConstructAppModelId(
+    const std::vector<string16>& components) {
+  // Find the maximum numbers of characters allowed in each component
+  // (accounting for the dots added between each component).
+  const size_t available_chars =
+      chrome::kMaxAppModelIdLength - (components.size() - 1);
+  const size_t max_component_length = available_chars / components.size();
+
+  // |max_component_length| should be at least 2; otherwise the truncation logic
+  // below breaks.

[grt (UTC plus 2), 2012/06/18 19:45:08]

can you add something to the non-debug case to not cause too much breakage?

[gab, 2012/06/18 21:52:43]

Done.

+  DCHECK_GE(max_component_length, 2U);
+
+  string16 app_id;

[grt (UTC plus 2), 2012/06/18 19:45:08]

just after this line, add:
app_id.reserve(chrome::kMaxAppModelIdLength);
to avoid reallocations in the loop.

[gab, 2012/06/18 21:52:43]

Done.

+  for (std::vector<string16>::const_iterator it = components.begin();
+       it != components.end(); ++it) {
+    if (it != components.begin())
+      app_id.push_back(L'.');
+
+    const string16& component = *it;
+    if (component.length() > max_component_length) {
+      // Append a truncated version of this component. Truncate in the middle

[grt (UTC plus 2), 2012/06/18 19:45:08]

"to truncate" means to chop off the end.  i think you're safe using "to shorten"
instead, although maybe there's an even nicer verb.  in any case, please update
this comment and the one in the header accordingly.

[gab, 2012/06/18 21:52:43]

http://www.thefreedictionary.com/truncate says "shorten" is a synonym for
"truncate", but I really don't care so..

Done :)

+      // to try to avoid losing the unique parts of this component (which are
+      // usually at the beginning or end for things like usernames and paths).
+      app_id.append(component.substr(0, max_component_length / 2));

[grt (UTC plus 2), 2012/06/18 19:45:08]

app_id.append(component, 0, max_component_length / 2);

[gab, 2012/06/18 21:52:43]

Done.

+      app_id.append(

[grt (UTC plus 2), 2012/06/18 19:45:08]

app_id.append(component, component.length() - ((max_component_length + 1) / 2),
string16::npos);

[gab, 2012/06/18 21:52:43]

Done.

+          component.substr(component.length() - (max_component_length / 2)));
+    } else {
+      app_id.append(component);
+    }
+  }
+  return app_id;
+}
+
 bool ShellUtil::MakeChromeDefault(BrowserDistribution* dist,
                                   int shell_change,
                                   const string16& chrome_exe,
@@ -1432,24 +1481,25 @@ bool ShellUtil::UpdateChromeShortcut(BrowserDistribution* dist,
                                      const string16& icon_path,
                                      int icon_index,
                                      uint32 options) {
-  string16 chrome_path = FilePath(chrome_exe).DirName().value();
+  const FilePath chrome_path(FilePath(chrome_exe).DirName());
 
-  FilePath prefs_path(chrome_path);
-  prefs_path = prefs_path.AppendASCII(installer::kDefaultMasterPrefs);
-  installer::MasterPreferences prefs(prefs_path);
+  installer::MasterPreferences prefs(
+      chrome_path.AppendASCII(installer::kDefaultMasterPrefs));
   if (FilePath::CompareEqualIgnoreCase(icon_path, chrome_exe)) {
     prefs.GetInt(installer::master_preferences::kChromeShortcutIconIndex,
                  &icon_index);
   }
 
+  const string16 app_id(GetChromiumModelId(dist, chrome_exe));
+
   return file_util::CreateOrUpdateShortcutLink(
       chrome_exe.c_str(),
       shortcut.c_str(),
-      chrome_path.c_str(),
+      chrome_path.value().c_str(),
       arguments.c_str(),
       description.c_str(),
       icon_path.c_str(),
       icon_index,
-      dist->GetBrowserAppId().c_str(),
+      app_id.c_str(),
       ConvertShellUtilShortcutOptionsToFileUtil(options));
 }