Tidy FileVersionInfoWin.

- Refactor GetValue() to use an existing struct.
- Make the parameter names in the .cc / .h files match for GetValue().
- Mark GetValue() and GetStringValue() const.
- Make all ::VerQueryValue() callers consistent.
- Add a comment to note |fixed_file_info_| is never nullptr.
- Fix various nits.

Change-Id: I4e73bf5791045005c2cfdeb6a039815c39e953ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1837490
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702890}

base/file_version_info_win.cc

@@ -7,6 +7,8 @@
 #include <windows.h>
 #include <stddef.h>
 
+#include <utility>
+
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
@@ -15,8 +17,6 @@
 #include "base/threading/scoped_blocking_call.h"
 #include "base/win/resource_util.h"
 
-using base::FilePath;
-
 namespace {
 
 struct LanguageAndCodePage {
@@ -24,26 +24,23 @@ struct LanguageAndCodePage {
   WORD code_page;
 };
 
-// Returns the \\VarFileInfo\\Translation value extracted from the
+// Returns the \VarFileInfo\Translation value extracted from the
 // VS_VERSION_INFO resource in |data|.
 LanguageAndCodePage* GetTranslate(const void* data) {
-  LanguageAndCodePage* translate = nullptr;
-  UINT length;
-  if (::VerQueryValue(data, L"\\VarFileInfo\\Translation",
-                      reinterpret_cast<void**>(&translate), &length)) {
-    return translate;
-  }
+  static constexpr wchar_t kTranslation[] = L"\\VarFileInfo\\Translation";
+  LPVOID translate = nullptr;
+  UINT dummy_size;
+  if (::VerQueryValue(data, kTranslation, &translate, &dummy_size))
+    return static_cast<LanguageAndCodePage*>(translate);
   return nullptr;
 }
 
-VS_FIXEDFILEINFO* GetVsFixedFileInfo(const void* data) {
-  VS_FIXEDFILEINFO* fixed_file_info = nullptr;
-  UINT length;
-  if (::VerQueryValue(data, L"\\", reinterpret_cast<void**>(&fixed_file_info),
-                      &length)) {
-    return fixed_file_info;
-  }
-  return nullptr;
+const VS_FIXEDFILEINFO& GetVsFixedFileInfo(const void* data) {
+  static constexpr wchar_t kRoot[] = L"\\";
+  LPVOID fixed_file_info = nullptr;
+  UINT dummy_size;
+  CHECK(::VerQueryValue(data, kRoot, &fixed_file_info, &dummy_size));
+  return *static_cast<VS_FIXEDFILEINFO*>(fixed_file_info);
 }
 
 }  // namespace
@@ -70,13 +67,13 @@ FileVersionInfo::CreateFileVersionInfoForModule(HMODULE module) {
 
 // static
 std::unique_ptr<FileVersionInfo> FileVersionInfo::CreateFileVersionInfo(
-    const FilePath& file_path) {
+    const base::FilePath& file_path) {
   return FileVersionInfoWin::CreateFileVersionInfoWin(file_path);
 }
 
 // static
 std::unique_ptr<FileVersionInfoWin>
-FileVersionInfoWin::CreateFileVersionInfoWin(const FilePath& file_path) {
+FileVersionInfoWin::CreateFileVersionInfoWin(const base::FilePath& file_path) {
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
                                                 base::BlockingType::MAY_BLOCK);
 
@@ -140,55 +137,46 @@ base::string16 FileVersionInfoWin::special_build() {
 }
 
 bool FileVersionInfoWin::GetValue(const base::char16* name,
-                                  base::string16* value_str) {
-  WORD lang_codepage[8];
-  size_t i = 0;
+                                  base::string16* value) const {
+  const struct LanguageAndCodePage lang_codepages[] = {
       // Use the language and codepage from the DLL.
-  lang_codepage[i++] = language_;
-  lang_codepage[i++] = code_page_;
+      {language_, code_page_},
       // Use the default language and codepage from the DLL.
-  lang_codepage[i++] = ::GetUserDefaultLangID();
-  lang_codepage[i++] = code_page_;
+      {::GetUserDefaultLangID(), code_page_},
       // Use the language from the DLL and Latin codepage (most common).
-  lang_codepage[i++] = language_;
-  lang_codepage[i++] = 1252;
+      {language_, 1252},
       // Use the default language and Latin codepage (most common).
-  lang_codepage[i++] = ::GetUserDefaultLangID();
-  lang_codepage[i++] = 1252;
+      {::GetUserDefaultLangID(), 1252},
+  };
 
-  i = 0;
-  while (i < base::size(lang_codepage)) {
+  for (const auto& lang_codepage : lang_codepages) {
     wchar_t sub_block[MAX_PATH];
-    WORD language = lang_codepage[i++];
-    WORD code_page = lang_codepage[i++];
     _snwprintf_s(sub_block, MAX_PATH, MAX_PATH,
-                 L"\\StringFileInfo\\%04x%04x\\%ls", language, code_page,
-                 base::as_wcstr(name));
-    LPVOID value = NULL;
+                 L"\\StringFileInfo\\%04x%04x\\%ls", lang_codepage.language,
+                 lang_codepage.code_page, base::as_wcstr(name));
+    LPVOID value_ptr = nullptr;
     uint32_t size;
-    BOOL r = ::VerQueryValue(data_, sub_block, &value, &size);
-    if (r && value) {
-      value_str->assign(static_cast<base::char16*>(value));
+    BOOL r = ::VerQueryValue(data_, sub_block, &value_ptr, &size);
+    if (r && value_ptr && size) {
+      value->assign(static_cast<base::char16*>(value_ptr), size - 1);
       return true;
     }
   }
   return false;
 }
 
-base::string16 FileVersionInfoWin::GetStringValue(const base::char16* name) {
+base::string16 FileVersionInfoWin::GetStringValue(
+    const base::char16* name) const {
   base::string16 str;
-  if (GetValue(name, &str))
+  GetValue(name, &str);
   return str;
-  else
-    return base::string16();
 }
 
 base::Version FileVersionInfoWin::GetFileVersion() const {
-  return base::Version(
-      std::vector<uint32_t>{HIWORD(fixed_file_info_->dwFileVersionMS),
-                            LOWORD(fixed_file_info_->dwFileVersionMS),
-                            HIWORD(fixed_file_info_->dwFileVersionLS),
-                            LOWORD(fixed_file_info_->dwFileVersionLS)});
+  return base::Version({HIWORD(fixed_file_info_.dwFileVersionMS),
+                        LOWORD(fixed_file_info_.dwFileVersionMS),
+                        HIWORD(fixed_file_info_.dwFileVersionLS),
+                        LOWORD(fixed_file_info_.dwFileVersionLS)});
 }
 
 FileVersionInfoWin::FileVersionInfoWin(std::vector<uint8_t>&& data,

base/file_version_info_win.h

@@ -38,12 +38,13 @@ class BASE_EXPORT FileVersionInfoWin : public FileVersionInfo {
   base::string16 file_description() override;
   base::string16 file_version() override;
 
-  // Lets you access other properties not covered above.
-  bool GetValue(const base::char16* name, base::string16* value);
+  // Lets you access other properties not covered above. |value| is only
+  // modified if GetValue() returns true.
+  bool GetValue(const base::char16* name, base::string16* value) const;
 
   // Similar to GetValue but returns a string16 (empty string if the property
   // does not exist).
-  base::string16 GetStringValue(const base::char16* name);
+  base::string16 GetStringValue(const base::char16* name) const;
 
   // Get file version number in dotted version format.
   base::Version GetFileVersion() const;
@@ -67,8 +68,8 @@ class BASE_EXPORT FileVersionInfoWin : public FileVersionInfo {
   const WORD language_;
   const WORD code_page_;
 
-  // This is a pointer into |data_| if it exists. Otherwise nullptr.
-  const VS_FIXEDFILEINFO* const fixed_file_info_;
+  // This is a reference for a portion of |data_|.
+  const VS_FIXEDFILEINFO& fixed_file_info_;
 
   DISALLOW_COPY_AND_ASSIGN(FileVersionInfoWin);
 };