Installer: adding ResolveShortcutProperties(); updating shortcut icons during shortcut migration.

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 1245 matching lines @@
   }
   return true;
 }
 
 bool ShortcutOpDelete(const base::FilePath& shortcut_path) {
   bool ret = base::DeleteFile(shortcut_path, false);
   LOG_IF(ERROR, !ret) << "Failed to remove " << shortcut_path.value();
   return ret;
 }
 
-bool ShortcutOpUpdate(const base::win::ShortcutProperties& shortcut_properties,
-                      const base::FilePath& shortcut_path) {
-  bool ret = base::win::CreateOrUpdateShortcutLink(
-      shortcut_path, shortcut_properties, base::win::SHORTCUT_UPDATE_EXISTING);
-  LOG_IF(ERROR, !ret) << "Failed to update " << shortcut_path.value();
-  return ret;
+bool ShortcutOpRetarget(const base::FilePath& old_target,
+                        const base::FilePath& new_target,
+                        const base::FilePath& shortcut_path) {
+  base::win::ShortcutProperties new_prop;
+  new_prop.set_target(new_target);
+
+  // If the old icon matches old target, then update icon while keeping the old
+  // icon index. Non-fatal if we fail to get the old icon.
+  base::win::ShortcutProperties old_prop;
+  if (base::win::ResolveShortcutProperties(
+          shortcut_path,
+          base::win::ShortcutProperties::PROPERTIES_ICON,
+          &old_prop)) {
+    if (InstallUtil::ProgramCompare(old_target).EvaluatePath(old_prop.icon))
+      new_prop.set_icon(new_target, old_prop.icon_index);
+  } else {
+    LOG(ERROR) << "Warning: failed to resolve " << shortcut_path.value();

[gab, 2014/01/02 15:51:46]

Is this a warning or an error? The severity is ERROR, but you put "Warning:" in
the error string :).

I'd say it's an error, so remove "Warning: " from the error string.

[huangs, 2014/01/02 19:58:22]

Ah, I didn't know LOG(WARNING) exists. Just removed "Warnings: ".

+  }
+
+  if (base::win::CreateOrUpdateShortcutLink(
+        shortcut_path, new_prop, base::win::SHORTCUT_UPDATE_EXISTING)) {
+    return true;
+  }
+  LOG(ERROR) << "Failed to retarget " << shortcut_path.value();
+  return false;

[gab, 2014/01/02 15:51:46]

Avoid multiple return statements here by doing:

bool result = base::win::CreateOrUpdateShortcutLink(
    shortcut_path, new_prop, base::win::SHORTCUT_UPDATE_EXISTING);
LOG_IF(ERROR, !result) << "Failed to retarget " << shortcut_path.value();
return result;

[huangs, 2014/01/02 19:58:22]

Done.

 }
 
+// TODO(huangs): Restructure this to separate reading/filtering from operating.

[gab, 2014/01/02 15:51:46]

Isn't this already what this is doing? This won't help avoiding the resolves
needed in both the filtering and operating steps...

I don't think a TODO is required here, we should only think about changing this
if a real performance impact is demonstrated.

[huangs, 2014/01/02 19:58:22]

Here {reading/filtering, operating} are integrated.  I think it's better to have
2 routines:
(1) One to read & filter, save resolved data (& shortcut path) into vector<>.
(2) A simple loop over vector<>, apply operation.

Anyway, removing TODO. :)

 // {|location|, |dist|, |level|} determine |shortcut_folder|.
 // For each shortcut in |shortcut_folder| that match |shortcut_filter|, apply
 // |shortcut_operation|. Returns true if all operations are successful.
 // All intended operations are attempted, even if failures occur.
 bool BatchShortcutAction(const ShortcutFilterCallback& shortcut_filter,
                          const ShortcutOperationCallback& shortcut_operation,
                          ShellUtil::ShortcutLocation location,
                          BrowserDistribution* dist,
                          ShellUtil::ShellChange level) {
   DCHECK(!shortcut_operation.is_null());
@@ skipping 800 matching lines @@
   if (success &&
       (location == SHORTCUT_LOCATION_START_MENU_CHROME_DIR ||
        location == SHORTCUT_LOCATION_START_MENU_CHROME_APPS_DIR ||
        location == SHORTCUT_LOCATION_APP_SHORTCUTS)) {
     success = RemoveShortcutFolderIfEmpty(location, dist, level);
   }
   return success;
 }
 
 // static
-bool ShellUtil::UpdateShortcutsWithArgs(
+bool ShellUtil::RetargetShortcutsWithArgs(
     ShellUtil::ShortcutLocation location,
     BrowserDistribution* dist,
     ShellChange level,
-    const base::FilePath& target_exe,
-    const ShellUtil::ShortcutProperties& properties) {
+    const base::FilePath& old_target_exe,
+    const base::FilePath& new_target_exe) {
   if (!ShellUtil::ShortcutLocationIsSupported(location))
     return true;  // Vacuous success.
 
-  FilterTargetEq shortcut_filter(target_exe, true);
+  FilterTargetEq shortcut_filter(old_target_exe, true);
   ShortcutOperationCallback shortcut_operation(
-      base::Bind(&ShortcutOpUpdate, TranslateShortcutProperties(properties)));
+      base::Bind(&ShortcutOpRetarget, old_target_exe, new_target_exe));
   return BatchShortcutAction(shortcut_filter.AsShortcutFilterCallback(),
                              shortcut_operation, location, dist, level);
 }
 
 bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {
   // Use a thread-safe cache for the user's suffix.
   static base::LazyInstance<UserSpecificRegistrySuffix>::Leaky suffix_instance =
       LAZY_INSTANCE_INITIALIZER;
   return suffix_instance.Get().GetSuffix(suffix);
 }
@@ skipping 51 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;
 }