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

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 <shobjidl.h>
 #include <propkey.h>
 #include <propvarutil.h>
+#include <tchar.h>
+#include <strsafe.h>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_comptr.h"
 #include "base/win/shortcut.h"
 #include "base/win/windows_version.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/installer/setup/setup_util.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/create_reg_key_work_item.h"
 #include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/set_reg_value_work_item.h"
 #include "chrome/installer/util/shell_util.h"
 #include "chrome/installer/util/util_constants.h"
 #include "chrome/installer/util/work_item.h"
 #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 {
 
 #if defined(GOOGLE_CHROME_BUILD)
 const wchar_t kAppListAppName[] = L"ChromeAppList";
 #else
 const wchar_t kAppListAppName[] = L"ChromiumAppList";
 #endif
 
@@ skipping 26 matching lines @@
   for (size_t i = 0; i < basenames.length(); ++i) {
     if (IsAsciiAlpha(basenames[i]) ||
         IsAsciiDigit(basenames[i]) ||
         basenames[i] == L'.')
       profile_id += basenames[i];
   }
 
   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 =
         command_line.GetSwitchValuePath(switches::kUserDataDir).AppendASCII(
             chrome::kInitialProfile);
   }
 
   string16 app_name;
   if (command_line.HasSwitch(switches::kApp)) {
     app_name = UTF8ToUTF16(web_app::GenerateApplicationNameFromURL(
         GURL(command_line.GetSwitchValueASCII(switches::kApp))));
   } else if (command_line.HasSwitch(switches::kAppId)) {
     app_name = UTF8ToUTF16(web_app::GenerateApplicationNameFromExtensionId(
         command_line.GetSwitchValueASCII(switches::kAppId)));
   } else if (command_line.HasSwitch(switches::kShowAppList)) {
     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() {
   // This should run on the file thread.
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   // Get full path of chrome.
   FilePath chrome_exe;
   if (!PathService::Get(base::FILE_EXE, &chrome_exe))
     return;
@@ skipping 21 matching lines @@
   for (int i = 0; i < arraysize(kLocations); ++i) {
     FilePath path;
     if (!PathService::Get(kLocations[i].location_id, &path)) {
       NOTREACHED();
       continue;
     }
 
     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);
   }
 }
 
 ShellIntegration::DefaultWebClientState
     GetDefaultWebClientStateFromShellUtilDefaultState(
         ShellUtil::DefaultState default_state) {
   switch (default_state) {
     case ShellUtil::NOT_DEFAULT:
       return ShellIntegration::NOT_DEFAULT;
     case ShellUtil::IS_DEFAULT:
@@ skipping 191 matching lines @@
   // This needs to happen eventually (e.g. so that the appid is fixed and the
   // run-time Chrome icon is merged with the taskbar shortcut), but this is not
   // urgent and shouldn't delay Chrome startup.
   static const int64 kMigrateChromiumShortcutsDelaySeconds = 15;
   BrowserThread::PostDelayedTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&MigrateChromiumShortcutsCallback),
       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,
     base::DIR_START_MENU,
   };
   BrowserDistribution* dist = BrowserDistribution::GetDistribution();
   string16 shortcut_name(dist->GetAppShortCutName());
   FilePath shortcut;
 
   // Check both the common and the per-user Start Menu folders for system-level
   // installs.
   size_t folder =
       InstallUtil::IsPerUserInstall(chrome_exe.value().c_str()) ? 1 : 0;
   for (; folder < arraysize(kFolderIds); ++folder) {
     if (!PathService::Get(kFolderIds[folder], &shortcut)) {
       NOTREACHED();
       continue;
     }
 
     shortcut = shortcut.Append(shortcut_name).Append(shortcut_name +
                                                      installer::kLnkExt);
     if (file_util::PathExists(shortcut))
       return shortcut;
   }
 
   return FilePath();
 }