Move AlterEnvironment to base/environment.h, implement on Windows.

Move AlterEnvironment to base/environment.h, implement on Windows. This re-implements the Posix version to be more like the Windows version and to share some parsing code. The new version will be somewhat slower due to some extra mallocs, but is shorter and more clear. I didn't want to implement a super optimized version on Windows, and the alternative would be to keep the new Windows version and the old Posix version in parallel which seemed less desirable.

This changes the input from a vector to a map and just adds the map on the LaunchOptions rather than requiring the caller to set a pointer. This cleans up the callsites somewhat.


BUG=
R=viettrungluu@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220608

[viettrungluu, 2013-08-09 19:27:11]

Note: I've only reviewed environment.{cc,h} so far.

https://codereview.chromium.org/22750002/diff/24001/base/environment.cc
File base/environment.cc (right):

https://codereview.chromium.org/22750002/diff/24001/base/environment.cc#newcode7
base/environment.cc:7: #include <map>
I don't think this include is needed (given that it's included and required by
the header, and you have no standalone uses of std::map).

On the other hand, you should probably include <vector>.

https://codereview.chromium.org/22750002/diff/24001/base/environment.cc#newco...
base/environment.cc:217: scoped_ptr<char*[]>
result(reinterpret_cast<char**>(buffer));
Hrm, putting a char[] into a scoped_ptr<char*[]> seems dubious to me. In
particular, it seems that on destruction of the scoped_ptr, the wrong destructor
is called.

Also, at least theoretically, buffer may not be compatible with the alignment
requirements of char*. (In C++11, you might DCHECK it versus alignof(char*).)

On the other hand, allocating the buffer as an array of char*s (big enough to
contain that chars at the end) is probably kosher.

https://codereview.chromium.org/22750002/diff/24001/base/environment.cc#newco...
base/environment.cc:222: memcpy(storage_data, value_storage.c_str(),
value_storage.size());
c_str -> data

https://codereview.chromium.org/22750002/diff/24001/base/environment.h
File base/environment.h (right):

https://codereview.chromium.org/22750002/diff/24001/base/environment.h#newcode77
base/environment.h:77: // This Posix version takes and returns a Posix-stle
environment block, which
s/stle/style/

https://codereview.chromium.org/22750002/diff/24001/base/environment.h#newcode83
base/environment.h:83: const char* const* const env,
Isn't the last (rightmost) const rather unusual/pointless?

[brettw, 2013-08-09 20:08:56]

https://codereview.chromium.org/22750002/diff/24001/base/environment.cc
File base/environment.cc (right):

https://codereview.chromium.org/22750002/diff/24001/base/environment.cc#newco...
base/environment.cc:217: scoped_ptr<char*[]>
result(reinterpret_cast<char**>(buffer));
On 2013/08/09 19:27:11, viettrungluu wrote:
> Hrm, putting a char[] into a scoped_ptr<char*[]> seems dubious to me. In
> particular, it seems that on destruction of the scoped_ptr, the wrong
destructor
> is called.
> 
> Also, at least theoretically, buffer may not be compatible with the alignment
> requirements of char*. (In C++11, you might DCHECK it versus alignof(char*).)
> 
> On the other hand, allocating the buffer as an array of char*s (big enough to
> contain that chars at the end) is probably kosher.

Done.

https://codereview.chromium.org/22750002/diff/24001/base/environment.cc#newco...
base/environment.cc:222: memcpy(storage_data, value_storage.c_str(),
value_storage.size());
I did this change but I added an if(). I did the original deliberately because
data() asserts if it's empty, and c_str doesn't.

[viettrungluu, 2013-08-13 17:49:40]

LGTM. You break it, you buy it.

https://codereview.chromium.org/22750002/diff/30001/base/environment.h
File base/environment.h (right):

https://codereview.chromium.org/22750002/diff/30001/base/environment.h#newcode1
base/environment.h:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
I feel like environment.{cc,h} should be in base/process instead of just being
in base. <shrug>

[commit-bot: I haz the power, 2013-08-29 20:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brettw@chromium.org/22750002/117001

[commit-bot: I haz the power, 2013-08-29 20:34:19]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[brettw, 2013-08-30 18:00:45]

Committed patchset #13 manually as r220608.