Sharing shell_util_unittest code to verify shortcuts

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 1706 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;
 }
+
+bool 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;
+
+  // Get pointer to the IShellLink interface
+  // Failed to get IShellLink

[gab, 2012/08/08 16:24:27]

Remove these "Failed *" comments (here and below), the code is  explicit enough.

[Halli, 2012/08/08 18:59:13]

Done.

+  if (FAILED(i_shell_link.CreateInstance(CLSID_ShellLink, NULL,
+                                               CLSCTX_INPROC_SERVER)))

[gab, 2012/08/08 16:24:27]

nit: indentation

[Halli, 2012/08/08 18:59:13]

Done.

+    return false;
+
+  // Query IShellLink for the IPersistFile interface
+  // Failed to get IPersistFile
+  if (FAILED(i_persist_file.QueryFrom(i_shell_link)))
+    return false;
+
+  // Failed to load shortcut
+  if (FAILED(i_persist_file->Load(shortcut.c_str(), 0)))
+    return false;
+
+  wchar_t long_path[MAX_PATH] = {0};
+  wchar_t short_path[MAX_PATH] = {0};
+
+  // Failed to get long and short path names
+  if (((::GetLongPathName(exe_path.c_str(), long_path, MAX_PATH) == 0) ||

[gab, 2012/08/08 16:24:27]

In the conditionals all the way from here to the bottom of this function there a
bunch of unnecessary extra parentheses.
Please remove unnecessary parentheses.

[Halli, 2012/08/08 18:59:13]

Done.

+            (::GetShortPathName(exe_path.c_str(), short_path, MAX_PATH) == 0)))

[gab, 2012/08/08 16:24:27]

nit: indentation

[Halli, 2012/08/08 18:59:13]

Done.

+    return false;
+
+  wchar_t file_path[MAX_PATH] = {0};
+
+  // File path did not match exe path
+  if (((FAILED(i_shell_link->GetPath(file_path, MAX_PATH, NULL,
+                                          SLGP_UNCPRIORITY))) ||

[gab, 2012/08/08 16:24:27]

nit: indentation

[Halli, 2012/08/08 18:59:13]

Done.

+            ((FilePath(file_path) != FilePath(long_path)) &&

[gab, 2012/08/08 16:24:27]

nit: indentation

[Halli, 2012/08/08 18:59:13]

Done.

+             (FilePath(file_path) != FilePath(short_path)))))
+    return false;
+
+  wchar_t desc[MAX_PATH] = {0};
+
+  // Description did not match shortcut's description
+  if (((FAILED(i_shell_link->GetDescription(desc, MAX_PATH))) ||
+            (string16(desc) != string16(description))))
+    return false;
+
+  wchar_t icon_path[MAX_PATH] = {0};
+  int index = 0;
+
+  if (((FAILED(i_shell_link->GetIconLocation(icon_path, MAX_PATH,

[gab, 2012/08/08 16:24:27]

Thinking more about it (and chatting with robert) since this is used in test
code we likely want to know what went wrong, but as mentioned before we can't
add a bunch of logs to shell_util.cc.

I think your best bet is to have an enum return type that states the status of
things and then EXPECT_EQ(VERIFY_SHORTCUT_SUCCESS, ...) in tests.

You probably don't want an enum for each individual interface failure. I would
put all the interface getters in a single if at the top and then do individual
checks for the actual properties being validated.

[Halli, 2012/08/08 18:59:13]

How is this different from returning true or false since you don't want a
different enum for each failure?
On 2012/08/08 16:24:27, gab wrote:

[gab, 2012/08/08 19:17:08]

I don't want a different one for the getters (i.e. i_shell_link->Get*), etc.,
but then I do want a different one for comparison failures.

That's why I suggested grouping the getters at the top in a single if (i.e. ifs
with the same outcome should be grouped) and then just do the comparisons in
independent ifs returning various failure possibilities.

On 2012/08/08 18:59:13, Halli wrote:

[Halli, 2012/08/08 20:28:09]

Hopefully this is what you meant (:
On 2012/08/08 19:17:08, gab wrote:

+                                                  &index))) ||
+            ((FilePath(file_path) != FilePath(long_path)) &&
+             (FilePath(file_path) != FilePath(short_path))) ||
+            (index != icon_index)))
+    return false;
+
+  return true;
+}