[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..da4d6d4b6ddb112230604136075e75def68d4d15 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,14 @@ 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,
+// Gets expected app id for given Chrome (based on |command_line| and
+// |is_per_user_install|.

[grt (UTC plus 2), 2013/01/03 02:30:44]

nit: close-paren at the end of the sentence.

[gab, 2013/01/03 21:14:57]

Done.

+void GetExpectedAppId(const CommandLine& command_line,

[grt (UTC plus 2), 2013/01/03 02:30:44]

string16 is a value type, so return the expected id rather than using an out
param.

[gab, 2013/01/03 21:14:57]

Done.

+                      bool is_per_user_install,
                       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()));
-
   FilePath profile_path;
   if (command_line.HasSwitch(switches::kUserDataDir)) {
     profile_path =
@@ -149,57 +114,11 @@ 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);
-    }
-  }
 }
 
 void MigrateChromiumShortcutsCallback() {
@@ -241,7 +160,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);
+    shell_integration::internals::MigrateShortcutsInPath(chrome_exe, path,
+                                                         check_dual_mode);
   }
 }
 
@@ -261,6 +182,123 @@ ShellIntegration::DefaultWebClientState
 
 }  // namespace
 
+namespace shell_integration {
+
+namespace internals {
+
+int MigrateShortcutsInPath(
+    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 expected_app_id;
+  string16 existing_app_id;
+  BOOL existing_dual_mode;

[grt (UTC plus 2), 2013/01/03 02:30:44]

move down to line 272/3

[gab, 2013/01/03 21:14:57]

Done.

+  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())));
+
+    // Get the expected AppId for this Chrome shortcut.
+    GetExpectedAppId(command_line, is_per_user_install, &expected_app_id);
+    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))) {
+      NOTREACHED();

[grt (UTC plus 2), 2013/01/03 02:30:44]

it seems wrong for chrome to crash if there's an invalid .lnk file in |path|. i
suggest DLOG(WARNING) << "Failed loading shortcut at " << shortcut.value(); or
something like that.

[gab, 2013/01/03 21:14:57]

Done.

+      continue;
+    }
+
+    // Any properties that needs to be updated on the shortcut will be stored in

[grt (UTC plus 2), 2013/01/03 02:30:44]

needs -> need

[gab, 2013/01/03 21:14:57]

Done.

+    // |updated_properties|.
+    base::win::ShortcutProperties updated_properties;
+
+    // Get existing app id from shortcut if any.

[grt (UTC plus 2), 2013/01/03 02:30:44]

"Get the existing app id from the shortcut."

[gab, 2013/01/03 21:14:57]

Done.

+    // Note, as mentioned on MSDN at
+    // http://msdn.microsoft.com/library/windows/desktop/bb776559.aspx, if
+    // PKEY_AppUserModel_ID is not set on the shortcut: GetValue() will still
+    // return S_OK and |pv_app_id| will be set to VT_EMPTY which will in turn be

[grt (UTC plus 2), 2013/01/03 02:30:44]

please explicitly test for VT_EMPTY rather than use PropVariantToString. the
latter is overkill for your needs here.

[gab, 2013/01/03 21:14:57]

Done.

[grt (UTC plus 2), 2013/01/07 15:23:22]

Why did you mark this "Done" but not actually do it? I think that PropVariantTo*
is overkill, and adding more delayloads for unnecessary convenience code (it
only adds overhead) isn't good practice. The code you have here is quite a bit
more complex and involves extra allocations, string copies, and error handling
compare to what was here before. In what way is this an improvement?

[gab, 2013/01/07 15:37:09]

Hmm?! I did do this... i.e. check for VT_EMPTY.

If what you meant is to not use PropVariantTo* altogether that's not what I
gathered from your comment. However after having to add DelayLoad I agree that
this seems overkill, I was going to talk with you guys today to see if you feel
that directly looking in the PropVariant (as was done before) is safe since the
PropVariantTo* methods unfortunately imply this undesired overhead (seems to me
it would be safe, but I don't know if PropVariantTo* does some clever things I'm
not aware of...).

+    // converted to the empty string by PropVariantToString() as desired.
+    base::win::ScopedComPtr<IPropertyStore> property_store;
+    PROPVARIANT pv_app_id;

[grt (UTC plus 2), 2013/01/03 02:30:44]

please continue to use PropVariantInit and PropVariantClear (see the previous
code).

[gab, 2013/01/03 21:14:57]

Done, but I don't see why those are necessary; I have not been using them in
similar code in test_shortcut_win.cc, nor do I see any recommendations to use
them on MSDN. If not calling PropvariantClear results in a potential memory
leak, then test_shortcut_win.cc should also be fixed.

+    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.

[grt (UTC plus 2), 2013/01/03 02:30:44]

"not updating" -> "to not update"

[gab, 2013/01/03 21:14:57]

Done.

+      NOTREACHED();
+      continue;
+    } else {
+      size_t expected_size = expected_app_id.size() + 1;
+      HRESULT result = PropVariantToString(
+          pv_app_id, WriteInto(&existing_app_id, expected_size), expected_size);
+      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) {
+      // Note, similar to the behavior described above, if

[grt (UTC plus 2), 2013/01/03 02:30:44]

same comment about checking for VT_EMPTY instead of using PropVariantTo*

[gab, 2013/01/03 21:14:57]

Done.

+      // PKEY_AppUserModel_IsDualMode is not set on this shortcut the operations
+      // below will result in |existing_dual_mode| being set to FALSE as
+      // desired.
+      PROPVARIANT pv_dual_mode;
+      if (property_store->GetValue(PKEY_AppUserModel_IsDualMode,
+                                   &pv_dual_mode) != S_OK ||
+          PropVariantToBoolean(pv_dual_mode, &existing_dual_mode) != S_OK) {
+        // When in doubt, prefer not updating the shortcut.

[grt (UTC plus 2), 2013/01/03 02:30:44]

"not updating" -> "to not update"

[gab, 2013/01/03 21:14:57]

Done.

+        NOTREACHED();
+        continue;
+      } else if (!existing_dual_mode) {
+        updated_properties.set_dual_mode(true);
+      }
+    }
+
+    persist_file.Release();
+    shell_link.Release();
+
+    // Update the shortcut iff some of its properties need to be updated.

[grt (UTC plus 2), 2013/01/03 02:30:44]

iff -> if
(this is becoming a pet peeve of mine. :-) )

[gab, 2013/01/03 21:14:57]

Done.

+    if (updated_properties.options &&
+        base::win::CreateOrUpdateShortcutLink(
+            shortcut, updated_properties,
+            base::win::SHORTCUT_UPDATE_EXISTING)) {
+      ++shortcuts_migrated;
+    }
+  }
+  return shortcuts_migrated;
+}
+
+}  // namespace internals
+
+}  // namespace shell_integration
+
 ShellIntegration::DefaultWebClientSetPermission
     ShellIntegration::CanSetAsDefaultBrowser() {
   if (!BrowserDistribution::GetDistribution()->CanSetAsDefault())