[Fixit-Dec-2012] Also add dual_mode to Start Menu shortcuts in MigrateChromiumShortcuts.

chrome/browser/shell_integration_win.cc

Index: chrome/browser/shell_integration_win.cc
diff --git a/chrome/browser/shell_integration_win.cc b/chrome/browser/shell_integration_win.cc
index a22e9141b2eccfd6d508c519aaa4226876f76e05..00e57f99de93d5dc36c2915d97f6638519c3a64a 100644
--- a/chrome/browser/shell_integration_win.cc
+++ b/chrome/browser/shell_integration_win.cc
@@ -8,6 +8,8 @@
 #include <shobjidl.h>
 #include <propkey.h>
 #include <propvarutil.h>
+#include <tchar.h>
+#include <strsafe.h>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -37,6 +39,9 @@
 #include "chrome/installer/util/work_item_list.h"
 #include "content/public/browser/browser_thread.h"
 
+// propsys.lib is required for PropvariantTo*().
+#pragma comment(lib, "propsys.lib")
+
 using content::BrowserThread;
 
 namespace {
@@ -83,54 +88,10 @@ string16 GetProfileIdFromPath(const FilePath& profile_path) {
   return profile_id;
 }
 
-bool GetShortcutAppId(IShellLink* shell_link, string16* app_id) {
-  DCHECK(shell_link);
-  DCHECK(app_id);
-
-  app_id->clear();
-
-  base::win::ScopedComPtr<IPropertyStore> property_store;
-  if (FAILED(property_store.QueryFrom(shell_link)))
-    return false;
-
-  PROPVARIANT appid_value;
-  PropVariantInit(&appid_value);
-  if (FAILED(property_store->GetValue(PKEY_AppUserModel_ID, &appid_value)))
-    return false;
-
-  if (appid_value.vt == VT_LPWSTR || appid_value.vt == VT_BSTR)
-    app_id->assign(appid_value.pwszVal);
-
-  PropVariantClear(&appid_value);
-  return true;
-}
-
-// Gets expected app id for given chrome shortcut. Returns true if the shortcut
-// points to chrome and expected app id is successfully derived.
-bool GetExpectedAppId(const FilePath& chrome_exe,
-                      IShellLink* shell_link,
-                      string16* expected_app_id) {
-  DCHECK(shell_link);
-  DCHECK(expected_app_id);
-
-  expected_app_id->clear();
-
-  // Check if the shortcut points to chrome_exe.
-  string16 source;
-  if (FAILED(shell_link->GetPath(WriteInto(&source, MAX_PATH), MAX_PATH, NULL,
-                                 SLGP_RAWPATH)) ||
-      lstrcmpi(chrome_exe.value().c_str(), source.c_str()))
-    return false;
-
-  string16 arguments;
-  if (FAILED(shell_link->GetArguments(WriteInto(&arguments, MAX_PATH),
-                                      MAX_PATH)))
-    return false;
-
-  // Get expected app id from shortcut command line.
-  CommandLine command_line = CommandLine::FromString(base::StringPrintf(
-      L"\"%ls\" %ls", source.c_str(), arguments.c_str()));
-
+// Gets expected app id for given Chrome (based on |command_line| and
+// |is_per_user_install|).
+string16 GetExpectedAppId(const CommandLine& command_line,
+                          bool is_per_user_install) {
   FilePath profile_path;
   if (command_line.HasSwitch(switches::kUserDataDir)) {
     profile_path =
@@ -149,57 +110,10 @@ bool GetExpectedAppId(const FilePath& chrome_exe,
     app_name = kAppListAppName;
   } else {
     BrowserDistribution* dist = BrowserDistribution::GetDistribution();
-    app_name = ShellUtil::GetBrowserModelId(
-        dist, InstallUtil::IsPerUserInstall(chrome_exe.value().c_str()));
+    app_name = ShellUtil::GetBrowserModelId(dist, is_per_user_install);
   }
 
-  expected_app_id->assign(
-      ShellIntegration::GetAppModelIdForProfile(app_name, profile_path));
-  return true;
-}
-
-void MigrateWin7ShortcutsInPath(
-    const FilePath& chrome_exe, const FilePath& path) {
-  // Enumerate all pinned shortcuts in the given path directly.
-  file_util::FileEnumerator shortcuts_enum(
-      path, false,  // not recursive
-      file_util::FileEnumerator::FILES, FILE_PATH_LITERAL("*.lnk"));
-
-  for (FilePath shortcut = shortcuts_enum.Next(); !shortcut.empty();
-       shortcut = shortcuts_enum.Next()) {
-    // Load the shortcut.
-    base::win::ScopedComPtr<IShellLink> shell_link;
-    if (FAILED(shell_link.CreateInstance(CLSID_ShellLink, NULL,
-                                         CLSCTX_INPROC_SERVER))) {
-      NOTREACHED();
-      return;
-    }
-
-    base::win::ScopedComPtr<IPersistFile> persist_file;
-    if (FAILED(persist_file.QueryFrom(shell_link)) ||
-        FAILED(persist_file->Load(shortcut.value().c_str(), STGM_READ))) {
-      NOTREACHED();
-      return;
-    }
-
-    // Get expected app id from shortcut.
-    string16 expected_app_id;
-    if (!GetExpectedAppId(chrome_exe, shell_link, &expected_app_id) ||
-        expected_app_id.empty())
-      continue;
-
-    // Get existing app id from shortcut if any.
-    string16 existing_app_id;
-    GetShortcutAppId(shell_link, &existing_app_id);
-
-    if (expected_app_id != existing_app_id) {
-      base::win::ShortcutProperties properties_app_id_only;
-      properties_app_id_only.set_app_id(expected_app_id);
-      base::win::CreateOrUpdateShortcutLink(
-          shortcut, properties_app_id_only,
-          base::win::SHORTCUT_UPDATE_EXISTING);
-    }
-  }
+  return ShellIntegration::GetAppModelIdForProfile(app_name, profile_path);
 }
 
 void MigrateChromiumShortcutsCallback() {
@@ -241,7 +155,9 @@ void MigrateChromiumShortcutsCallback() {
     if (kLocations[i].sub_dir)
       path = path.Append(kLocations[i].sub_dir);
 
-    MigrateWin7ShortcutsInPath(chrome_exe, path);
+    bool check_dual_mode = (kLocations[i].location_id == base::DIR_START_MENU);
+    ShellIntegration::MigrateShortcutsInPathInternal(chrome_exe, path,
+                                                     check_dual_mode);
   }
 }
 
@@ -453,6 +369,124 @@ void ShellIntegration::MigrateChromiumShortcuts() {
       base::TimeDelta::FromSeconds(kMigrateChromiumShortcutsDelaySeconds));
 }
 
+int ShellIntegration::MigrateShortcutsInPathInternal(
+    const FilePath& chrome_exe, const FilePath& path, bool check_dual_mode) {
+  DCHECK(base::win::GetVersion() >= base::win::VERSION_WIN7);
+
+  // Enumerate all pinned shortcuts in the given path directly.
+  file_util::FileEnumerator shortcuts_enum(
+      path, false,  // not recursive
+      file_util::FileEnumerator::FILES, FILE_PATH_LITERAL("*.lnk"));
+
+  bool is_per_user_install =
+      InstallUtil::IsPerUserInstall(chrome_exe.value().c_str());
+
+  int shortcuts_migrated = 0;
+  FilePath target_path;
+  string16 arguments;
+  string16 existing_app_id;
+  for (FilePath shortcut = shortcuts_enum.Next(); !shortcut.empty();
+       shortcut = shortcuts_enum.Next()) {
+    // TODO(gab): Use ProgramCompare instead of comparing FilePaths below once
+    // it is fixed to work with FilePaths with spaces.
+    if (!base::win::ResolveShortcut(shortcut, &target_path, &arguments) ||
+        chrome_exe != target_path) {
+      continue;
+    }
+    CommandLine command_line(CommandLine::FromString(base::StringPrintf(
+        L"\"%ls\" %ls", target_path.value().c_str(), arguments.c_str())));

[robertshield, 2013/01/03 22:41:59]

Just to double check, is there any way that target_path.value() would already be
quoted?

[gab, 2013/01/03 23:12:43]

Good question, this code was already there, I find it ugly, but didn't bother
touching that part...

MSDN says this about IShellLink.GetPath(): (...) receives the path and file name
of the target of the Shell link object.

I doubt this would be quoted...

FWIW, I added a space to the path of the tested shortcuts and they still pass (I
even added a dump locally to be sure the path returned with a space had no
quotes).

[robertshield, 2013/01/04 01:38:14]

For peace of mind, could you edit the shortcut in explorer to manually add
quotes to the target and see what happens?

[gab, 2013/01/04 16:41:44]

I just tried and this is not even doable in Explorer, i.e. the quotes added to
the target path are removed as soon as I click "Ok" or "Apply". I think we're
safe :).

+
+    // Get the expected AppId for this Chrome shortcut.
+    string16 expected_app_id(
+        GetExpectedAppId(command_line, is_per_user_install));
+    if (expected_app_id.empty())
+      continue;
+
+    // Load the shortcut.
+    base::win::ScopedComPtr<IShellLink> shell_link;
+    base::win::ScopedComPtr<IPersistFile> persist_file;
+    if (FAILED(shell_link.CreateInstance(CLSID_ShellLink, NULL,
+                                         CLSCTX_INPROC_SERVER)) ||
+        FAILED(persist_file.QueryFrom(shell_link)) ||
+        FAILED(persist_file->Load(shortcut.value().c_str(), STGM_READ))) {
+      DLOG(WARNING) << "Failed loading shortcut at " << shortcut.value();
+      continue;
+    }
+
+    // Any properties that need to be updated on the shortcut will be stored in
+    // |updated_properties|.
+    base::win::ShortcutProperties updated_properties;
+
+    // Validate the existing app id for the shortcut.
+    base::win::ScopedComPtr<IPropertyStore> property_store;
+    PROPVARIANT pv_app_id;
+    PropVariantInit(&pv_app_id);
+    if (FAILED(property_store.QueryFrom(shell_link)) ||
+        property_store->GetValue(PKEY_AppUserModel_ID, &pv_app_id) != S_OK) {
+      // When in doubt, prefer not updating the shortcut.
+      NOTREACHED();
+      continue;
+    } else if (pv_app_id.vt == VT_EMPTY) {
+      // If there is no app_id set, set our app_id if one is expected.
+      if (!expected_app_id.empty())
+        updated_properties.set_app_id(expected_app_id);
+    } else {
+      // Validate that the existing app_id is the expected app_id; if not, set
+      // the expected app_id on the shortcut.
+      size_t expected_size = expected_app_id.size() + 1;
+      HRESULT result = PropVariantToString(
+          pv_app_id, WriteInto(&existing_app_id, expected_size), expected_size);
+      PropVariantClear(&pv_app_id);
+      if (result != S_OK && result != STRSAFE_E_INSUFFICIENT_BUFFER) {
+        // Accept the STRSAFE_E_INSUFFICIENT_BUFFER error state as it means the
+        // existing appid is longer than |expected_app_id| and thus we will
+        // simply assume inequality.
+        NOTREACHED();
+        continue;
+      } else if (result == STRSAFE_E_INSUFFICIENT_BUFFER ||
+                 expected_app_id != existing_app_id) {
+        updated_properties.set_app_id(expected_app_id);
+      }
+    }
+
+    if (check_dual_mode) {
+      BOOL existing_dual_mode;
+      PROPVARIANT pv_dual_mode;
+      PropVariantInit(&pv_dual_mode);
+      if (property_store->GetValue(PKEY_AppUserModel_IsDualMode,
+                                   &pv_dual_mode) != S_OK) {
+        // When in doubt, prefer to not update the shortcut.
+        NOTREACHED();
+        continue;
+      } else if (pv_dual_mode.vt == VT_EMPTY) {
+        // If dual_mode is not set at all, make sure it gets set to true.
+        updated_properties.set_dual_mode(true);
+      } else {
+        // If it is set to false, make sure it gets set to true as well.
+        if (PropVariantToBoolean(pv_dual_mode, &existing_dual_mode) != S_OK) {
+          NOTREACHED();
+          continue;
+        }
+        PropVariantClear(&pv_dual_mode);

[robertshield, 2013/01/03 22:41:59]

Should we not clear the PropVariant in the if branch immediately above?

Don't have to do this in this CL, but this looks to me to be crying out for a
ScopedPropVariant. There's the beginnings of one in jumplist_win.cc that could
perhaps be used or moved.

[gab, 2013/01/03 23:12:43]

Right, MSDN is not clear about when clearing is required or not... I agree that
a ScopedPropVariant would be nice... added it to the installer fixit list.

https://docs.google.com/a/chromium.org/document/d/1Js17FLROy5L83Q8Qp9G6l6hRq1...

[robertshield, 2013/01/04 01:38:14]

sgtm

+        if (!existing_dual_mode)
+          updated_properties.set_dual_mode(true);
+      }
+    }
+
+    persist_file.Release();
+    shell_link.Release();
+
+    // Update the shortcut if some of its properties need to be updated.
+    if (updated_properties.options &&
+        base::win::CreateOrUpdateShortcutLink(

[robertshield, 2013/01/03 22:41:59]

minor nit: DoFooToShortcutLink reads a little bit like DoFooToDirectoryFolder,
are the words "Shortcut" and "Link" redundant here? If you feel like changing
it, don't have to do it in this CL if it would interfere with merging.

[gab, 2013/01/03 23:12:43]

I agree that this name is redundant, added this to the fixit as well, but it
touches base so I really don't want to do this in this CL.

+            shortcut, updated_properties,
+            base::win::SHORTCUT_UPDATE_EXISTING)) {
+      ++shortcuts_migrated;
+    }
+  }
+  return shortcuts_migrated;
+}
+
 FilePath ShellIntegration::GetStartMenuShortcut(const FilePath& chrome_exe) {
   static const int kFolderIds[] = {
     base::DIR_COMMON_START_MENU,