Remove FilePath usage from the CrashReporterClient interface on Windows.

chrome/app/chrome_crash_reporter_client_win.cc

Index: chrome/app/chrome_crash_reporter_client_win.cc
diff --git a/chrome/app/chrome_crash_reporter_client_win.cc b/chrome/app/chrome_crash_reporter_client_win.cc
index edc2551c74cb4f00c6f92b08ad03be608d4da2a6..3a34b5384b637d1eeb9beb0c6e383f0d78aa03d6 100644
--- a/chrome/app/chrome_crash_reporter_client_win.cc
+++ b/chrome/app/chrome_crash_reporter_client_win.cc
@@ -6,24 +6,16 @@
 // Add test coverage for Crashpad.
 #include "chrome/app/chrome_crash_reporter_client_win.h"
 
+#include <assert.h>
 #include <windows.h>
 
 #include <memory>
 #include <string>
 
-#include "base/command_line.h"
-#include "base/files/file_path.h"
-#include "base/logging.h"
-#include "base/path_service.h"
-#include "base/strings/string_split.h"
-#include "base/strings/utf_string_conversions.h"
-#include "base/version.h"
 #include "build/build_config.h"
 #include "chrome/common/chrome_result_codes.h"
 #include "chrome/common/crash_keys.h"
-#include "chrome/common/env_vars.h"
 #include "chrome/install_static/install_util.h"
-#include "content/public/common/content_switches.h"
 
 namespace {
 
@@ -38,78 +30,81 @@ ChromeCrashReporterClient::ChromeCrashReporterClient() {}
 ChromeCrashReporterClient::~ChromeCrashReporterClient() {}
 
 bool ChromeCrashReporterClient::GetAlternativeCrashDumpLocation(
-    base::FilePath* crash_dir) {
+    base::string16* crash_dir) {
   // By setting the BREAKPAD_DUMP_LOCATION environment variable, an alternate
   // location to write breakpad crash dumps can be set.
   std::string alternate_crash_dump_location =
       install_static::GetEnvironmentString("BREAKPAD_DUMP_LOCATION");
   if (!alternate_crash_dump_location.empty()) {
-    *crash_dir = base::FilePath::FromUTF8Unsafe(alternate_crash_dump_location);
+    *crash_dir = install_static::UTF8ToUTF16(alternate_crash_dump_location);

[grt (UTC plus 2), 2016/06/02 12:55:33]

GetEnvironmentString does a UTF16ToUTF8 under the covers. how about making it
return a base::string16 from the get-go on windows, and require that other
consumers do the conversion? or add a second GetEnvironmentString that doesn't
do the conversion

[ananta, 2016/06/02 20:34:20]

Done. Added GetEnvironmentString16, SetEnvironmentString16 and
HasEnvironmentVariable16 for symmetry.

     return true;
   }
   return false;
 }
 
 void ChromeCrashReporterClient::GetProductNameAndVersion(
-    const base::FilePath& exe_path,
+    const base::string16& exe_path,
     base::string16* product_name,
     base::string16* version,
     base::string16* special_build,
     base::string16* channel_name) {
-  DCHECK(product_name);
-  DCHECK(version);
-  DCHECK(special_build);
-  DCHECK(channel_name);
+  assert(product_name);
+  assert(version);
+  assert(special_build);
+  assert(channel_name);
 
   install_static::GetExecutableVersionDetails(
-      exe_path.value(), product_name, version, special_build, channel_name);
+      exe_path, product_name, version, special_build, channel_name);
 }
 
 bool ChromeCrashReporterClient::ShouldShowRestartDialog(base::string16* title,
                                                         base::string16* message,
                                                         bool* is_rtl_locale) {
-  if (!install_static::HasEnvironmentVariable(env_vars::kShowRestart) ||
-      !install_static::HasEnvironmentVariable(env_vars::kRestartInfo) ||
-      install_static::HasEnvironmentVariable(env_vars::kMetroConnected)) {
+  if (!install_static::HasEnvironmentVariable(install_static::kShowRestart) ||
+      !install_static::HasEnvironmentVariable(install_static::kRestartInfo)) {
     return false;
   }
 
   std::string restart_info =
-      install_static::GetEnvironmentString(env_vars::kRestartInfo);
+      install_static::GetEnvironmentString(install_static::kRestartInfo);

[grt (UTC plus 2), 2016/06/02 12:55:33]

another spot where a string16 GetEnvironmentString makes sense

[ananta, 2016/06/02 20:34:20]

Done.

 
   // The CHROME_RESTART var contains the dialog strings separated by '|'.
   // See ChromeBrowserMainPartsWin::PrepareRestartOnCrashEnviroment()
   // for details.
-  std::vector<std::string> dlg_strings = base::SplitString(
-      restart_info, "|", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  std::vector<std::string> dlg_strings = install_static::TokenizeString(
+      restart_info, '|', true); // true = Trim shitespace.
 
   if (dlg_strings.size() < 3)
     return false;
 
-  *title = base::UTF8ToUTF16(dlg_strings[0]);
-  *message = base::UTF8ToUTF16(dlg_strings[1]);
-  *is_rtl_locale = dlg_strings[2] == env_vars::kRtlLocale;
+  *title = install_static::UTF8ToUTF16(dlg_strings[0]);
+  *message = install_static::UTF8ToUTF16(dlg_strings[1]);
+  *is_rtl_locale = dlg_strings[2] == install_static::kRtlLocale;
   return true;
 }
 
 bool ChromeCrashReporterClient::AboutToRestart() {
-  if (!install_static::HasEnvironmentVariable(env_vars::kRestartInfo))
+  if (!install_static::HasEnvironmentVariable(install_static::kRestartInfo))
     return false;
 
-  install_static::SetEnvironmentString(env_vars::kShowRestart, "1");
+  install_static::SetEnvironmentString(install_static::kShowRestart, "1");
   return true;
 }
 
 bool ChromeCrashReporterClient::GetDeferredUploadsSupported(

[grt (UTC plus 2), 2016/06/02 12:55:33]

i was under the impression that this feature never launched. i can find no code
that calls this function. could you make this "return false;" and then remove
the function from the interface and all impls in a different CL?

[ananta, 2016/06/02 20:34:20]

Done.

[grt (UTC plus 2), 2016/06/02 22:57:44]

Thanks. scottmg: could you confirm that my read is correct?

[scottmg, 2016/06/02 23:01:09]

Yeah, I agree. I have no idea what that is, and it doesn't seem to be used.

     bool is_per_user_install) {
-  Version update_version(install_static::GetGoogleUpdateVersion());
-  return update_version.IsValid() &&
-      update_version >= base::Version(kMinUpdateVersion);
+  std::string update_version = install_static::GetGoogleUpdateVersion();
+  if (update_version.empty())
+    return false;
+  int result = 0;
+  install_static::CompareVersionStrings(update_version,
+      kMinUpdateVersion, &result);
+  return result >= 0;
 }
 
 bool ChromeCrashReporterClient::GetIsPerUserInstall(
-    const base::FilePath& exe_path) {
-  return !install_static::IsSystemInstall(exe_path.value().c_str());
+    const base::string16& exe_path) {
+  return !install_static::IsSystemInstall(exe_path.c_str());
 }
 
 bool ChromeCrashReporterClient::GetShouldDumpLargerDumps(
@@ -143,7 +138,7 @@ bool ChromeCrashReporterClient::ReportingIsEnforcedByPolicy(
 
 
 bool ChromeCrashReporterClient::GetCrashDumpLocation(
-    base::FilePath* crash_dir) {
+    base::string16* crash_dir) {
   // By setting the BREAKPAD_DUMP_LOCATION environment variable, an alternate
   // location to write breakpad crash dumps can be set.
   // If this environment variable exists, then for the time being,
@@ -155,11 +150,7 @@ bool ChromeCrashReporterClient::GetCrashDumpLocation(
 
   // TODO(scottmg): Consider supporting --user-data-dir. See
   // https://crbug.com/565446.
-  base::string16 crash_dump_location;
-  if (!install_static::GetDefaultCrashDumpLocation(&crash_dump_location))
-    return false;
-  *crash_dir = base::FilePath(crash_dump_location);
-  return true;
+  return install_static::GetDefaultCrashDumpLocation(crash_dir);
 }
 
 size_t ChromeCrashReporterClient::RegisterCrashKeys() {
@@ -167,7 +158,7 @@ size_t ChromeCrashReporterClient::RegisterCrashKeys() {
 }
 
 bool ChromeCrashReporterClient::IsRunningUnattended() {
-  return install_static::HasEnvironmentVariable(env_vars::kHeadless);
+  return install_static::HasEnvironmentVariable(install_static::kHeadless);
 }
 
 bool ChromeCrashReporterClient::GetCollectStatsConsent() {
@@ -176,8 +167,8 @@ bool ChromeCrashReporterClient::GetCollectStatsConsent() {
 
 bool ChromeCrashReporterClient::EnableBreakpadForProcess(
     const std::string& process_type) {
-  return process_type == switches::kRendererProcess ||
-         process_type == switches::kPpapiPluginProcess ||
-         process_type == switches::kZygoteProcess ||
-         process_type == switches::kGpuProcess;
+  return process_type == install_static::kRendererProcess ||
+         process_type == install_static::kPpapiPluginProcess ||
+         process_type == install_static::kZygoteProcess ||
+         process_type == install_static::kGpuProcess;
 }