Sharing shell_util_unittest code to verify shortcuts

chrome/installer/util/shell_util.cc

Index: chrome/installer/util/shell_util.cc
===================================================================
--- chrome/installer/util/shell_util.cc	(revision 150342)
+++ chrome/installer/util/shell_util.cc	(working copy)
@@ -1657,6 +1657,46 @@
       ConvertShellUtilShortcutOptionsToFileUtil(options));
 }
 
+ShellUtil::VerifyShortcuts ShellUtil::VerifyChromeShortcut(
+    const string16& exe_path, const string16& shortcut,
+    const string16& description, int icon_index) {
+  base::win::ScopedComPtr<IShellLink> i_shell_link;
+  base::win::ScopedComPtr<IPersistFile> i_persist_file;
+

[gab, 2012/08/09 02:52:32]

nit: Remove this line to have all declarations grouped in one block.

[Halli, 2012/08/09 16:07:57]

Done.

+  wchar_t long_path[MAX_PATH] = {0};
+  wchar_t short_path[MAX_PATH] = {0};
+  wchar_t file_path[MAX_PATH] = {0};
+  wchar_t icon_path[MAX_PATH] = {0};
+  wchar_t desc[MAX_PATH] = {0};
+  int index = 0;
+
+  // Get pointer to the IShellLink interface

[gab, 2012/08/09 02:52:32]

Change this comment to something like "Get the shortcut's properties."

[Halli, 2012/08/09 16:07:57]

Done.

+  if (FAILED(i_shell_link.CreateInstance(CLSID_ShellLink, NULL,
+                                         CLSCTX_INPROC_SERVER)) ||
+      FAILED(i_persist_file.QueryFrom(i_shell_link)) ||
+      FAILED(i_persist_file->Load(shortcut.c_str(), 0)) ||
+      ::GetLongPathName(exe_path.c_str(), long_path, MAX_PATH) == 0 ||
+      ::GetShortPathName(exe_path.c_str(), short_path, MAX_PATH) == 0 ||
+      FAILED(i_shell_link->GetPath(file_path, MAX_PATH, NULL,
+      SLGP_UNCPRIORITY)) ||

[gab, 2012/08/09 02:52:32]

nit: align with |file_path|

[Halli, 2012/08/09 16:07:57]

Done.

+      FAILED(i_shell_link->GetIconLocation(icon_path, MAX_PATH, &index)) ||
+      FAILED(i_shell_link->GetDescription(desc, MAX_PATH)) ||
+      FAILED(i_shell_link->GetDescription(desc, MAX_PATH)))
+    return VERIFY_SHORTCUT_FAILURE_UNEXPECTED;
+
+  if (FilePath(file_path) != FilePath(long_path) &&

[gab, 2012/08/09 02:52:32]

Change && to ||

[Halli, 2012/08/09 16:07:57]

Original code uses &&
Isn't && correct? I think this 
On 2012/08/09 02:52:32, gab wrote:

+     FilePath(file_path) != FilePath(short_path))

[gab, 2012/08/09 02:52:32]

nit: indent one more space

[gab, 2012/08/09 02:52:32]

nit: as-per chromium style, since this conditional spans more than one line,
surround the return statement in curly braces.

[gab, 2012/08/09 02:52:32]

You are creating a FilePath(file_path) twice, extract that in a constant
variable right above this if.

[Halli, 2012/08/09 16:07:57]

Done.

[Halli, 2012/08/09 16:07:57]

Done.

[Halli, 2012/08/09 16:07:57]

Line is less than 80 chars with the constant from below (:
On 2012/08/09 02:52:32, gab wrote:

+    return VERIFY_SHORTCUT_FAILURE_PATH;
+
+  if (string16(desc) != string16(description))
+    return VERIFY_SHORTCUT_FAILURE_DESCRIPTION;
+
+  if (index != icon_index)
+    return VERIFY_SHORTCUT_FAILURE_ICON_INDEX;
+
+  return VERIFY_SHORTCUT_SUCCESS;
+}
+
 bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {
   // Use a thread-safe cache for the user's suffix.
   static base::LazyInstance<UserSpecificRegistrySuffix>::Leaky suffix_instance =