Remove hard dependency on bundled python_26 in tests

net/test/spawned_test_server/local_test_server_win.cc

Index: net/test/spawned_test_server/local_test_server_win.cc
diff --git a/net/test/spawned_test_server/local_test_server_win.cc b/net/test/spawned_test_server/local_test_server_win.cc
index fd26483ed477b37698538f1f2b609e7d79b343f2..91b902f7e60e144af675b7f1ab8331896d7623ae 100644
--- a/net/test/spawned_test_server/local_test_server_win.cc
+++ b/net/test/spawned_test_server/local_test_server_win.cc
@@ -10,6 +10,7 @@
 #include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/environment.h"
 #include "base/files/file_path.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
@@ -85,6 +86,51 @@ bool ReadData(HANDLE read_fd, HANDLE write_fd,
 
 namespace net {
 
+class AddedPythonPath {
+private:
+  std::string old_path_;
+  scoped_ptr<base::Environment> environment_;
+  bool path_modified_;
+
+  DISALLOW_COPY_AND_ASSIGN(AddedPythonPath);
+
+public:

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: In Chromium style we usually put public: section first.

+  AddedPythonPath();
+  ~AddedPythonPath();
+};
+
+AddedPythonPath::AddedPythonPath()
+    : environment_(base::Environment::Create()),
+      path_modified_(false) {
+  // Retrieves the old path, adds third_party/python26 to the end of it and
+  // then restores the original path in the destructor.
+
+  (void)environment_->GetVar("PATH", &old_path_);

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: Are these (void) casts needed? Generally we don't do C-style casts. If you
need to ignore a result of function call, use ignore_result from basictypes.h.

[Daniel Bratell, 2013/08/20 14:33:09]

Not needed. Just an old convention to silence static analyzers (including the
ones in compilers) and to show readers of the code that ignoring the result of
the function was intentional. I think it's helpful since readers might otherwise
think it was an oversight.

I'm switching to ignore_result (though I'm not sure static analyzers will handle
that as well), but I can also remove it completely if you prefer that.

[Paweł Hajdan Jr., 2013/08/22 18:11:53]

If these ignore_results are not needed to get it to compile, please remove them
(along with any comments about them).

+
+  std::string new_value = old_path_;
+  if (new_value.length() > 0)

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: Why not do an .empty() check instead? The intent would be more obvious.

[Daniel Bratell, 2013/08/20 14:33:09]

Thanks! Just my string class confusion made me think there was no such method.
There are so many string classes (not just the ones in Chromium and STL. :-) )

+    new_value += ";"; // Path seperator.
+
+  // Add new path to the end so system pythons are used if available.

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: system pythons -> system python (plural -> singular)

+  base::FilePath python_path;
+  if (!PathService::Get(base::DIR_SOURCE_ROOT, &python_path))

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

This is a silent failure, and the function that calls it returns bool.

Let's propagate the failure there.

[Daniel Bratell, 2013/08/20 14:33:09]

This is the constructor so you mean adding/using an exception? What exception do
you suggest?

I didn't consider this a fatal problem since I'm assuming every developer has a
python installed in a system path anyway but I might be wrong.

[Paweł Hajdan Jr., 2013/08/22 18:11:53]

We disable exceptions in Chrome. This means you'd need to avoid doing work in
constructor, and instead have a method with bool return value for this work and
WARN_UNUSED_RESULT.
This is an error with getting path to source root, nothing to do with presence
of python. :) It can fail in unexpected ways, especially under e.g. isolated
testing.

+    return;
+  python_path = python_path.Append(FILE_PATH_LITERAL("third_party"))

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

nit: You can just use AppendASCII.

+                           .Append(FILE_PATH_LITERAL("python_26"));
+  new_value += python_path.AsUTF8Unsafe();

[Paweł Hajdan Jr., 2013/08/19 19:16:56]

Why the Unsafe method? :-/

Note that it may be needed to have an Environment that accepts native strings.

[Daniel Bratell, 2013/08/20 14:33:09]

The Environment class uses UTF-8 strings in the API. Apparently the "Unsafe"
part of the function name comes from it doing some magic stuff on some
non-Windows platforms and the function will be renamed as soon as that magic is
gone. Since this is pure Windows that doesn't apply here but yes, it looks a bit
bad. I will replace with WideToUTF8(python_path.value()).

[Paweł Hajdan Jr., 2013/08/22 18:11:53]

Sounds good!

+
+  path_modified_ = environment_->SetVar("PATH", new_value);
+}
+
+AddedPythonPath::~AddedPythonPath() {
+  if (path_modified_) {
+    if (old_path_.length() > 0)
+      (void)environment_->SetVar("PATH", old_path_);
+    else
+      (void)environment_->UnSetVar("PATH");
+  }
+}
+
 bool LocalTestServer::LaunchPython(const base::FilePath& testserver_path) {
   CommandLine python_command(CommandLine::NO_PROGRAM);
   if (!GetPythonCommand(&python_command))
@@ -134,6 +180,7 @@ bool LocalTestServer::LaunchPython(const base::FilePath& testserver_path) {
     return false;
   }
 
+  AddedPythonPath python_path;
   base::LaunchOptions launch_options;
   launch_options.inherit_handles = true;
   launch_options.job_handle = job_handle_.Get();