Move EnvironmentVariableGetter from base/linux_util.h to base/env_var.h. Labe...

Move EnvironmentVariableGetter from base/linux_util.h to base/env_var.h and rename it EnvVarGetter. Label base::SysInfo::{Get,Has}EnvVar as deprecated.

BUG=none
TEST=none

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

[Lei Zhang, 2010-04-02 21:17:15]

Matt: I think more places can benefit from EnvironmentVariableGetter, so I'm
moving it out of linux_util.h. I also wrote the Windows implementation. Also,
maybe we want to rename it to EnvVarGetter? We have a 80 column limit...

Mark: I think we should deprecate base::SysInfo::GetEnvVar in favor of
EnvironmentVariableGetter. What do you think?

[Mark Mentovai, 2010-04-02 21:39:35]

EnvironmentVariableGetter is kind of a mouthful, innit?

[Mark Mentovai, 2010-04-02 21:45:11]

LGTM as far as the move goes.

http://codereview.chromium.org/1606007/diff/20001/15009
File base/env_var.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15009#newcode29
base/env_var.cc:29: // are lowercase, which is inconsistent. Let's try to be
helpful
"Helpful?"

Tangibly, what does this gain?

http://codereview.chromium.org/1606007/diff/20001/15009#newcode57
base/env_var.cc:57: scoped_array<wchar_t> value(new wchar_t[value_length]);
scoped_array<wchar_t> is really either a wstring or a vector<wchar_t>. 
scoped_array is really no different from vector, why not use the standard form?

[tfarina (gmail-do not use), 2010-04-02 21:57:41]

http://codereview.chromium.org/1606007/diff/20001/15011
File base/base.gypi (right):

http://codereview.chromium.org/1606007/diff/20001/15011#newcode72
base/base.gypi:72: 'env_var.cc',
nit: shouldn't be above of event_*?

http://codereview.chromium.org/1606007/diff/20001/15012
File base/base_paths_posix.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15012#newcode1
base/base_paths_posix.cc:1: // Copyright (c) 2006-2008 The Chromium Authors. All
rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15010
File base/linux_util.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15010#newcode1
base/linux_util.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15008
File base/linux_util.h (right):

http://codereview.chromium.org/1606007/diff/20001/15008#newcode1
base/linux_util.h:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15007
File base/sys_info.h (right):

http://codereview.chromium.org/1606007/diff/20001/15007#newcode1
base/sys_info.h:1: // Copyright (c) 2008 The Chromium Authors. All rights
reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15013
File chrome/browser/shell_integration.h (right):

http://codereview.chromium.org/1606007/diff/20001/15013#newcode1
chrome/browser/shell_integration.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15015
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15015#newcode1
chrome/browser/shell_integration_linux.cc:1: // Copyright (c) 2006-2009 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15014
File chrome/browser/shell_integration_unittest.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15014#newcode1
chrome/browser/shell_integration_unittest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15016
File chrome/browser/web_applications/web_app.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15016#newcode1
chrome/browser/web_applications/web_app.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15016#newcode767
chrome/browser/web_applications/web_app.cc:767: };  // namespace web_app
nit: remove ;

http://codereview.chromium.org/1606007/diff/20001/15020
File chrome/common/chrome_paths_linux.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15020#newcode1
chrome/common/chrome_paths_linux.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15003
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15003#newcode1
net/proxy/proxy_config_service_linux.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15004
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/1606007/diff/20001/15004#newcode1
net/proxy/proxy_config_service_linux.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1606007/diff/20001/15005
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15005#newcode1
net/proxy/proxy_config_service_linux_unittest.cc:1: // Copyright (c) 2009 The
Chromium Authors. All rights reserved.
nit: 2010

[Lei Zhang, 2010-04-02 22:18:28]

http://codereview.chromium.org/1606007/diff/20001/15011
File base/base.gypi (right):

http://codereview.chromium.org/1606007/diff/20001/15011#newcode72
base/base.gypi:72: 'env_var.cc',
On 2010/04/02 21:57:42, tfarina wrote:
> nit: shouldn't be above of event_*?

!sort took care of the entire list.

http://codereview.chromium.org/1606007/diff/20001/15009
File base/env_var.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15009#newcode29
base/env_var.cc:29: // are lowercase, which is inconsistent. Let's try to be
helpful
On 2010/04/02 21:45:11, Mark Mentovai wrote:
> "Helpful?"
> 
> Tangibly, what does this gain?

Deferring to Matt.

http://codereview.chromium.org/1606007/diff/20001/15009#newcode57
base/env_var.cc:57: scoped_array<wchar_t> value(new wchar_t[value_length]);
On 2010/04/02 21:45:11, Mark Mentovai wrote:
> scoped_array<wchar_t> is really either a wstring or a vector<wchar_t>. 
> scoped_array is really no different from vector, why not use the standard
form?

I don't think you can pass a vector to GetEnvironmentVariable. Also, this is
code copied from sys_info_win.cc, which you reviewed the first time around. :)

http://codereview.chromium.org/1606007/diff/20001/15010
File base/linux_util.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15010#newcode1
base/linux_util.cc:1: // Copyright (c) 2009 The Chromium Authors. All rights
reserved.
On 2010/04/02 21:57:42, tfarina wrote:
> nit: 2010

You don't have to point this out for every file. It is sufficient to just say it
once and say "same for all other files". The excessive number of comments just
makes the review harder to read.

[mattm, 2010-04-02 22:28:55]

http://codereview.chromium.org/1606007/diff/20001/15009
File base/env_var.cc (right):

http://codereview.chromium.org/1606007/diff/20001/15009#newcode29
base/env_var.cc:29: // are lowercase, which is inconsistent. Let's try to be
helpful
On 2010/04/02 22:18:28, Lei Zhang wrote:
> On 2010/04/02 21:45:11, Mark Mentovai wrote:
> > "Helpful?"
> > 
> > Tangibly, what does this gain?
> 
> Deferring to Matt.


There are some vars like http_proxy which are usually lowercase, but I guess
some other apps may want it uppercase or users might make it uppercase to
conform with normal env vars, so we accept both.  (eg, wget manual also mentions
it in both)

So being "helpful" I guess means just saving the caller from having to do both
calls themselves in those cases.

[Lei Zhang, 2010-04-02 22:42:19]

On 2010/04/02 22:28:55, mattm wrote:
> http://codereview.chromium.org/1606007/diff/20001/15009
> File base/env_var.cc (right):
> 
> http://codereview.chromium.org/1606007/diff/20001/15009#newcode29
> base/env_var.cc:29: // are lowercase, which is inconsistent. Let's try to be
> helpful
> On 2010/04/02 22:18:28, Lei Zhang wrote:
> > On 2010/04/02 21:45:11, Mark Mentovai wrote:
> > > "Helpful?"
> > > 
> > > Tangibly, what does this gain?
> > 
> > Deferring to Matt.
> 
> 
> There are some vars like http_proxy which are usually lowercase, but I guess
> some other apps may want it uppercase or users might make it uppercase to
> conform with normal env vars, so we accept both.  (eg, wget manual also
mentions
> it in both)
> 
> So being "helpful" I guess means just saving the caller from having to do both
> calls themselves in those cases.

Ok, I added that as an example of helpfulness in patch set 4.