Refactoring and tests for the highly undertested file_util::CreateOrUpdateShortcutLink() method.

base/file_util_win.cc

Index: base/file_util_win.cc
diff --git a/base/file_util_win.cc b/base/file_util_win.cc
index b88e13169e8993db2ba2ebb48bd1090cbf355c3f..4383f4dfcc233d587644e46e83424326d6d1c038 100644
--- a/base/file_util_win.cc
+++ b/base/file_util_win.cc
@@ -381,73 +381,88 @@ bool ResolveShortcut(const FilePath& shortcut_path,
   return true;
 }
 
-bool CreateOrUpdateShortcutLink(const wchar_t *source,
-                                const wchar_t *destination,
-                                const wchar_t *working_dir,
-                                const wchar_t *arguments,
-                                const wchar_t *description,
-                                const wchar_t *icon,
-                                int icon_index,
-                                const wchar_t* app_id,
-                                uint32 options) {
+bool CreateOrUpdateShortcutLink(const string16& shortcut_path,
+                                const ShortcutProperties& properties,
+                                ShortcutOperation operation) {
   base::ThreadRestrictions::AssertIOAllowed();
 
-  bool create = (options & SHORTCUT_CREATE_ALWAYS) != 0;
-
-  // |source| is required when SHORTCUT_CREATE_ALWAYS is specified.
-  DCHECK(source || !create);
+  // A target is required when |operation| is SHORTCUT_CREATE_ALWAYS.
+  if (operation == SHORTCUT_CREATE_ALWAYS &&
+      (properties.bitfield & ShortcutProperties::PROPERTIES_TARGET) == 0) {
+    NOTREACHED();
+    return false;
+  }
 
   // Length of arguments and description must be less than MAX_PATH.
-  DCHECK(lstrlen(arguments) < MAX_PATH);
-  DCHECK(lstrlen(description) < MAX_PATH);
+  DCHECK(properties.arguments.size() < MAX_PATH);
+  DCHECK(properties.description.size() < MAX_PATH);

[robertshield, 2012/09/06 02:37:34]

Could these DCHECKs be moved to the setters on ShortcutProperties?

[gab, 2012/09/06 04:20:02]

Sounds good to me, unless an argument is made that these setters are getting too
complex and have to be moved in the cc file (as the DCHECKs now imply including
base/logging.h in the header (which was already done for OS_POSIX so that's no
SO bad))... I would rather keep it a simple struct fully implemented in the
header.

Done.

 
   base::win::ScopedComPtr<IShellLink> i_shell_link;
   base::win::ScopedComPtr<IPersistFile> i_persist_file;
-
-  // Get pointer to the IShellLink interface.
   if (FAILED(i_shell_link.CreateInstance(CLSID_ShellLink, NULL,
                                          CLSCTX_INPROC_SERVER)) ||
       FAILED(i_persist_file.QueryFrom(i_shell_link))) {
     return false;
   }
 
-  if (!create && FAILED(i_persist_file->Load(destination, STGM_READWRITE)))
+  if (operation == SHORTCUT_UPDATE_EXISTING &&
+      FAILED(i_persist_file->Load(shortcut_path.c_str(), STGM_READWRITE))) {
     return false;
+  }
 
-  if ((source || create) && FAILED(i_shell_link->SetPath(source)))
+  if ((properties.bitfield & ShortcutProperties::PROPERTIES_TARGET) != 0 &&
+      FAILED(i_shell_link->SetPath(properties.target.c_str()))) {
     return false;
+  }
 
-  if (working_dir && FAILED(i_shell_link->SetWorkingDirectory(working_dir)))
+  if ((properties.bitfield & ShortcutProperties::PROPERTIES_WORKING_DIR) != 0 &&
+      FAILED(i_shell_link->SetWorkingDirectory(
+          properties.working_dir.c_str()))) {
     return false;
+  }
 
-  if (arguments && FAILED(i_shell_link->SetArguments(arguments)))
+  if ((properties.bitfield & ShortcutProperties::PROPERTIES_ARGUMENTS) != 0 &&
+      FAILED(i_shell_link->SetArguments(properties.arguments.c_str()))) {
     return false;
+  }
 
-  if (description && FAILED(i_shell_link->SetDescription(description)))
+  if ((properties.bitfield & ShortcutProperties::PROPERTIES_DESCRIPTION) &&
+      FAILED(i_shell_link->SetDescription(properties.description.c_str()))) {
     return false;
+  }
 
-  if (icon && FAILED(i_shell_link->SetIconLocation(icon, icon_index)))
+  if ((properties.bitfield & ShortcutProperties::PROPERTIES_ICON) &&
+      FAILED(i_shell_link->SetIconLocation(properties.icon.c_str(),
+                                           properties.icon_index))) {
     return false;
+  }
 
-  bool is_dual_mode = (options & SHORTCUT_DUAL_MODE) != 0;
-  if ((app_id || is_dual_mode) &&
+  const bool has_app_id =

[robertshield, 2012/09/06 02:37:34]

don't think adding const here and on has_dual_mode is needed

[gab, 2012/09/06 04:20:02]

I like const here because it's one of those scenarios where the only reason not
to inline this is that the value is needed twice, but really the value of these
should never change once it has been determined; thus const helps the reader
assume from this point on that these variables are just the result of whether
they were in properties.options point final.

I'll remove the consts if you insist.

[robertshield, 2012/09/06 19:20:01]

I'll insist a bit. The additional cognitive load incurred by requiring the
reader to parse another pair of keywords exceeds the practical benefit here imo.

[gab, 2012/09/07 04:58:05]

Done.

+      (properties.bitfield & ShortcutProperties::PROPERTIES_APP_ID) != 0;
+  const bool has_dual_mode =
+      (properties.bitfield & ShortcutProperties::PROPERTIES_DUAL_MODE) != 0;

[robertshield, 2012/09/06 02:37:34]

how about:
bool has_dual_mode = (properties.bitfield &
ShortcutProperties::PROPERTIES_DUAL_MODE);

[gab, 2012/09/06 04:20:02]

That'd be an implicit cast from int to bool. Not having != 0 directly in the ifs
is fine imo (which I just did uniformly) because in C++ any int other than 0 is
true in an if, but are we fine with an implicit cast (i.e. this is different as
we assign it to a "bool" type variable rather than using it directly in the if
as an int).

[robertshield, 2012/09/06 19:20:01]

I feel it makes the code more readable and I don't see any contradicting
directives in the style guide.

[gab, 2012/09/07 04:58:05]

Tried, but I'm getting warnings->errors:

g:\src\component\src\base\file_util_win.cc(437) : error C2220: warning treated
as error - no 'object' file generated
g:\src\component\src\base\file_util_win.cc(437) : warning C4800: 'const uint32'
: forcing value to bool 'true' or 'false

If that works for you, I'll keep the "!= 0" which I find cleaner than the "!!"
alternative.

+  if ((has_app_id || has_dual_mode) &&
       base::win::GetVersion() >= base::win::VERSION_WIN7) {
     base::win::ScopedComPtr<IPropertyStore> property_store;
     if (FAILED(property_store.QueryFrom(i_shell_link)) || !property_store.get())
       return false;
 
-    if (app_id && !base::win::SetAppIdForPropertyStore(property_store, app_id))
+    if (has_app_id &&
+        !base::win::SetAppIdForPropertyStore(property_store,
+                                             properties.app_id.c_str())) {
       return false;
-    if (is_dual_mode &&
-        !base::win::SetDualModeForPropertyStore(property_store)) {
+    }
+    if (has_dual_mode &&
+        !base::win::SetDualModeForPropertyStore(property_store,
+                                                properties.dual_mode)) {
       return false;
     }
   }
 
-  HRESULT result = i_persist_file->Save(destination, TRUE);
+  HRESULT result = i_persist_file->Save(shortcut_path.c_str(), TRUE);
 
   // If we successfully updated the icon, notify the shell that we have done so.
-  if (!create && SUCCEEDED(result)) {
+  if (operation == SHORTCUT_UPDATE_EXISTING && SUCCEEDED(result)) {
     // Release the interfaces in case the SHChangeNotify call below depends on
     // the operations above being fully completed.
     i_persist_file.Release();