Use Environment::SetVar in more places.

Use Environment::SetVar in more places.

Make use of Environment::SetVar API to replace the calls to the
Windows function SetEnvironmentVariable.

BUG=None
TEST=trybots

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55993

[tfarina, 2010-08-12 02:58:20]

On 2010/08/07 04:45:35, tfarina wrote:
> 

friendly ping.

[Lei Zhang, 2010-08-12 21:05:01]

Whoops, missed this one.

http://codereview.chromium.org/3076042/diff/22001/23006
File chrome_frame/crash_reporting/crash_dll.cc (right):

http://codereview.chromium.org/3076042/diff/22001/23006#newcode26
chrome_frame/crash_reporting/crash_dll.cc:26: DWORD ret =
::GetEnvironmentVariable(name, value, ARRAYSIZE(value));
and change this while you're at it?

http://codereview.chromium.org/3076042/diff/22001/23009
File chrome_frame/crash_reporting/vectored_handler_unittest.cc (right):

http://codereview.chromium.org/3076042/diff/22001/23009#newcode145
chrome_frame/crash_reporting/vectored_handler_unittest.cc:145:
EXPECT_TRUE(env->SetVar(WideToUTF8(kCrashOnLoadMode).c_str(), "1"));
You should just change |kCrashOnLoadMode| and friends to char [].

[amit, 2010-08-12 21:14:13]

http://codereview.chromium.org/3076042/diff/22001/23001
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/3076042/diff/22001/23001#newcode209
chrome/app/breakpad_win.cc:209: if (!env->HasVar(env_vars::kRestartInfo)) {
This looks odd and expensive, considering that this is just a replacement for a
single ::GetEnvironmentVariableW (or similar flavor on other OSs) API. 
Is there a better way to write this without having to incur
allocation/deallocation? Perhaps as EnvironmentTraits with static methods like
the following:

class Win32EnvTraits {
  static bool HasVar(...);
  static bool SetVar(...);
  static bool GetVar(...);
};

class OSXEnvTraits {
...
};

#ifdef WIN32
typedef Win32EnvTraits Environment;
#else if ....
...
#endif

[Lei Zhang, 2010-08-12 21:27:10]

http://codereview.chromium.org/3076042/diff/22001/23001
File chrome/app/breakpad_win.cc (right):

http://codereview.chromium.org/3076042/diff/22001/23001#newcode209
chrome/app/breakpad_win.cc:209: if (!env->HasVar(env_vars::kRestartInfo)) {
On 2010/08/12 21:14:13, amit wrote:
> This looks odd and expensive, considering that this is just a replacement for
a
> single ::GetEnvironmentVariableW (or similar flavor on other OSs) API. 
> Is there a better way to write this without having to incur
> allocation/deallocation? Perhaps as EnvironmentTraits with static methods like
> the following:

In this particular case, does it matter? We only reach this code when the
browser process crashes.

In general, does it matter? None of these calls are in a loop so they're just
one time costs.

Having an Environment class lets us create MockEnvironments to make testing
easier.

[amit, 2010-08-12 21:52:18]

On Thu, Aug 12, 2010 at 2:27 PM, <thestig@chromium.org> wrote:

>
> http://codereview.chromium.org/3076042/diff/22001/23001
> File chrome/app/breakpad_win.cc (right):
>
> http://codereview.chromium.org/3076042/diff/22001/23001#newcode209
> chrome/app/breakpad_win.cc:209: if
> (!env->HasVar(env_vars::kRestartInfo)) {
> On 2010/08/12 21:14:13, amit wrote:
>
>> This looks odd and expensive, considering that this is just a
>>
> replacement for a
>
>> single ::GetEnvironmentVariableW (or similar flavor on other OSs) API.
>>
>
>  Is there a better way to write this without having to incur
>> allocation/deallocation? Perhaps as EnvironmentTraits with static
>>
> methods like
>
>> the following:
>>
>
> In this particular case, does it matter? We only reach this code when
> the browser process crashes.
>
> In general, does it matter? None of these calls are in a loop so they're
> just one time costs.
>
I was tentative about this considering that this patch mostly updates unit
tests :)
So optimizing 2 allocations is a micro-optimization. However, having to
allocate an instance just to abstract out an API feels awkward and if that's
the standard way of doing things then what if in future, I have to do this
in a right loop?.

>
> Having an Environment class lets us create MockEnvironments to make
> testing easier.
>
>
> http://codereview.chromium.org/3076042/show
>

[Lei Zhang, 2010-08-12 22:02:49]

On 2010/08/12 21:52:18, amit wrote:
> > In general, does it matter? None of these calls are in a loop so they're
> > just one time costs.
> >
> I was tentative about this considering that this patch mostly updates unit
> tests :)
> So optimizing 2 allocations is a micro-optimization. However, having to
> allocate an instance just to abstract out an API feels awkward and if that's
> the standard way of doing things then what if in future, I have to do this
> in a right loop?.

If you got a tight loop that calls GetEnv repeatedly, then just allocate the
Environment once outside of the loop.

I'm all for faster code, but I think the gains in testability out weighs the
small loss in performance.

I don't know how getenv/setenv is implemented on all platforms, but adding one
more memory allocation/deallocation may not be that more expensive when added to
the getenv/setenv call itself.

[amit, 2010-08-12 22:20:28]

On Thu, Aug 12, 2010 at 3:02 PM, <thestig@chromium.org> wrote:

> On 2010/08/12 21:52:18, amit wrote:
>
>> > In general, does it matter? None of these calls are in a loop so they're
>> > just one time costs.
>> >
>> I was tentative about this considering that this patch mostly updates unit
>> tests :)
>> So optimizing 2 allocations is a micro-optimization. However, having to
>> allocate an instance just to abstract out an API feels awkward and if
>> that's
>> the standard way of doing things then what if in future, I have to do this
>> in a right loop?.
>>
>
> If you got a tight loop that calls GetEnv repeatedly, then just allocate
> the
> Environment once outside of the loop.
>
> I'm all for faster code, but I think the gains in testability out weighs
> the
> small loss in performance.
>
My un-ease is more about awkwardness of use than performance. The new
standard way of doing things is now allocate, invoke and release as opposed
to a straightforward API call.  Things look slightly better with syntactic
sugar coating of scoped_ptr.
I am with you on the testability-perf trade off. So if this is the best we
could do then I am OK with it and consider this an LGTM.

>
> I don't know how getenv/setenv is implemented on all platforms, but adding
> one
> more memory allocation/deallocation may not be that more expensive when
> added to
> the getenv/setenv call itself.


>
> http://codereview.chromium.org/3076042/show
>

[tfarina, 2010-08-13 01:18:42]

http://codereview.chromium.org/3076042/diff/22001/23009
File chrome_frame/crash_reporting/vectored_handler_unittest.cc (right):

http://codereview.chromium.org/3076042/diff/22001/23009#newcode145
chrome_frame/crash_reporting/vectored_handler_unittest.cc:145:
EXPECT_TRUE(env->SetVar(WideToUTF8(kCrashOnLoadMode).c_str(), "1"));
On 2010/08/12 21:05:01, Lei Zhang wrote:
> You should just change |kCrashOnLoadMode| and friends to char [].

I did this and I wasn't able to fix the compile failures in other way. See the
previous patchsets.

[Lei Zhang, 2010-08-13 01:38:43]

On 2010/08/13 01:18:42, tfarina wrote:
> http://codereview.chromium.org/3076042/diff/22001/23009
> File chrome_frame/crash_reporting/vectored_handler_unittest.cc (right):
> 
> http://codereview.chromium.org/3076042/diff/22001/23009#newcode145
> chrome_frame/crash_reporting/vectored_handler_unittest.cc:145:
> EXPECT_TRUE(env->SetVar(WideToUTF8(kCrashOnLoadMode).c_str(), "1"));
> On 2010/08/12 21:05:01, Lei Zhang wrote:
> > You should just change |kCrashOnLoadMode| and friends to char [].
> 
> I did this and I wasn't able to fix the compile failures in other way. See the
> previous patchsets.

You had link failures because crash_dll doesn't depend on base and
IgnoreAllDefaultLibraries is set. I guess this is the best you can do, so LGTM.

[tfarina, 2010-08-13 01:44:51]

On Thu, Aug 12, 2010 at 10:38 PM,  <thestig@chromium.org> wrote:
> On 2010/08/13 01:18:42, tfarina wrote:
>>
>> http://codereview.chromium.org/3076042/diff/22001/23009
>> File chrome_frame/crash_reporting/vectored_handler_unittest.cc (right):
>
>> http://codereview.chromium.org/3076042/diff/22001/23009#newcode145
>> chrome_frame/crash_reporting/vectored_handler_unittest.cc:145:
>> EXPECT_TRUE(env->SetVar(WideToUTF8(kCrashOnLoadMode).c_str(), "1"));
>> On 2010/08/12 21:05:01, Lei Zhang wrote:
>> > You should just change |kCrashOnLoadMode| and friends to char [].
>
>> I did this and I wasn't able to fix the compile failures in other way. See
>> the
>> previous patchsets.
>
> You had link failures because crash_dll doesn't depend on base and
> IgnoreAllDefaultLibraries is set.
Yeah, I looked again at the gyp file and I was going to add base as a
dependency.

> I guess this is the best you can do, so
> LGTM.
>




-- 
A: Because it messes up the order in which people normally read text.
 See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

-Thiago

[Lei Zhang, 2010-08-13 01:46:30]

On 2010/08/13 01:44:51, tfarina wrote:
> Yeah, I looked again at the gyp file and I was going to add base as a
> dependency.

That's not going to work because of IgnoreAllDefaultLibraries. Patchset 8 is as
good as it gets.

[tfarina, 2010-08-13 01:48:32]

On Thu, Aug 12, 2010 at 10:46 PM,  <thestig@chromium.org> wrote:
> On 2010/08/13 01:44:51, tfarina wrote:
>>
>> Yeah, I looked again at the gyp file and I was going to add base as a
>> dependency.
>
> That's not going to work because of IgnoreAllDefaultLibraries. Patchset 8 is
> as
> good as it gets.
>

Cool, thanks. I'll you send to the trybots and commit the patchset 8
when they passes.

-- 
A: Because it messes up the order in which people normally read text.
 See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

-Thiago

[tfarina, 2010-08-13 19:25:00]

On Thu, Aug 12, 2010 at 6:14 PM,  <amit@chromium.org> wrote:
>
> http://codereview.chromium.org/3076042/diff/22001/23001
> File chrome/app/breakpad_win.cc (right):
>
> http://codereview.chromium.org/3076042/diff/22001/23001#newcode209
> chrome/app/breakpad_win.cc:209: if
> (!env->HasVar(env_vars::kRestartInfo)) {
> This looks odd and expensive, considering that this is just a
> replacement for a single ::GetEnvironmentVariableW (or similar flavor on
> other OSs) API.
> Is there a better way to write this without having to incur
> allocation/deallocation? Perhaps as EnvironmentTraits with static
> methods like the following:
>
> class Win32EnvTraits {
>  static bool HasVar(...);
>  static bool SetVar(...);
>  static bool GetVar(...);
> };
>
> class OSXEnvTraits {
> ...
> };
>
> #ifdef WIN32
> typedef Win32EnvTraits Environment;
> #else if ....
> ...
> #endif
>

I like this idea, that could help in situations like this (having to
alloc each time doesn't make much sense in these cases):

http://git.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=net/test/t...
http://git.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=media/audi...

-- 
A: Because it messes up the order in which people normally read text.
 See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

-Thiago