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

base/win/shortcut.cc

Index: base/win/shortcut.cc
diff --git a/base/win/shortcut.cc b/base/win/shortcut.cc
index 57a93dc6524e1540cad465eb639482ed6f6e90c2..f69592d6c2bac403631544bef11f96c2ce9fde26 100644
--- a/base/win/shortcut.cc
+++ b/base/win/shortcut.cc
@@ -172,52 +172,82 @@ bool CreateOrUpdateShortcutLink(const FilePath& shortcut_path,
   return succeeded;
 }
 
-bool ResolveShortcut(const FilePath& shortcut_path,
-                     FilePath* target_path,
-                     string16* args) {
+bool ResolveShortcutProperties(const FilePath& shortcut_path,
+                               uint32 options,
+                               ShortcutProperties* properties) {
+  DCHECK(properties);
+  DCHECK(options);

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

Switch order to match argument order (and/or group with &&).

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

Going with && for implicit cast to bool.

   base::ThreadRestrictions::AssertIOAllowed();
 
-  HRESULT result;
   ScopedComPtr<IShellLink> i_shell_link;
 
   // Get pointer to the IShellLink interface.
-  result = i_shell_link.CreateInstance(CLSID_ShellLink, NULL,
-                                       CLSCTX_INPROC_SERVER);
-  if (FAILED(result))
+  if (FAILED(i_shell_link.CreateInstance(CLSID_ShellLink, NULL,
+                                         CLSCTX_INPROC_SERVER))) {
     return false;
+  }
 
   ScopedComPtr<IPersistFile> persist;
   // Query IShellLink for the IPersistFile interface.
-  result = persist.QueryFrom(i_shell_link);
-  if (FAILED(result))
+  if (FAILED(persist.QueryFrom(i_shell_link)))
     return false;
 
   // Load the shell link.
-  result = persist->Load(shortcut_path.value().c_str(), STGM_READ);
-  if (FAILED(result))
+  if (FAILED(persist->Load(shortcut_path.value().c_str(), STGM_READ)))
     return false;
 
+  properties->options = 0;

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

Empty line below (otherwise it feels like this is only related to the block
below).

Also add a comment like:

// Reset |properties|.

above.

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

Done.

   WCHAR temp[MAX_PATH];
-  if (target_path) {
+  if (options & ShortcutProperties::PROPERTIES_TARGET) {
     // Try to find the target of a shortcut.
-    result = i_shell_link->Resolve(0, SLR_NO_UI | SLR_NOSEARCH);
-    if (FAILED(result))
+    if (FAILED(i_shell_link->Resolve(0, SLR_NO_UI | SLR_NOSEARCH)))
       return false;
 
-    result = i_shell_link->GetPath(temp, MAX_PATH, NULL, SLGP_UNCPRIORITY);
-    if (FAILED(result))
+    if (FAILED(i_shell_link->GetPath(temp, MAX_PATH, NULL, SLGP_UNCPRIORITY)))
       return false;
 
-    *target_path = FilePath(temp);
+    properties->set_target(FilePath(temp));
   }
 
-  if (args) {
-    result = i_shell_link->GetArguments(temp, MAX_PATH);
-    if (FAILED(result))
+  if (options & ShortcutProperties::PROPERTIES_ARGUMENTS) {
+    if (FAILED(i_shell_link->GetArguments(temp, MAX_PATH)))
       return false;
 
-    *args = string16(temp);
+    properties->set_arguments(string16(temp));
   }
+
+  if (options & ShortcutProperties::PROPERTIES_ICON) {
+    int temp_index;
+    if (FAILED(i_shell_link->GetIconLocation(temp, MAX_PATH, &temp_index)))
+      return false;
+
+    properties->set_icon(FilePath(temp), temp_index);
+  }
+
+  // Implement other options if the need arise.

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

Add to the comment in the header to mention which options are currently
supported.

And add:

// No unsupported options.
DCHECK_EQ(0, options & (~ShortcutProperties::PROPERTIES_TARGET &
                        ~ShortcutProperties::PROPERTIES_ARGUMENTS &
                        ~ShortcutProperties::PROPERTIES_ICON));

at the top of this method.

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

I realized that base\test\test_shortcut_win.cc: ValidateShortcut() 

https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sho...

reads everything; so may as well do the work, and add sanity check unit test!

Since everything is in, maybe I won't have to check for unsupported options?
CreateOrUpdateShortcutLink() doesn't check either.

+
+  return true;
+}
+
+bool ResolveShortcut(const FilePath& shortcut_path,
+                     FilePath* target_path,
+                     string16* args) {
+  uint32 options = 0;
+  if (target_path)
+    options |= ShortcutProperties::PROPERTIES_TARGET;
+  if (args)
+    options |= ShortcutProperties::PROPERTIES_ARGUMENTS;
+  if (!options)
+    return true;  // Vacuous success if we want nothing.

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

I don't think it makes sense to call ResolveShortcut and want nothing.

Update the header comment to disallow this and let the DCHECK hit in
ResolveShortcutProperties.

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

Done, but adding DCHECK in this routine.

+
+  ShortcutProperties p;

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

s/p/properties

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

Done.

+  if (!ResolveShortcutProperties(shortcut_path, options, &p))
+    return false;
+
+  if (target_path)
+    *target_path = p.target;
+  if (args)
+    *args = p.arguments;
   return true;
 }