base: Add UnSetEnv function to EnvVarGetter API.

base: Add UnSetEnv function to EnvVarGetter API.

BUG=None
TEST=out/Debug/base_unittests --gtest_filter=EnvVarTest.UnSetEnvVar

Signed-off-by: Thiago Farina <tfarina@chromium.org>;

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

[Paweł Hajdan Jr., 2010-07-26 17:31:43]

Drive-by with some test comments and a suggestion. Is it time to rename
EnvVarGetter to something like Environment? It now does a lot more than getting
things.

http://codereview.chromium.org/3043018/diff/4001/5007
File chrome/browser/shell_integration_unittest.cc (right):

http://codereview.chromium.org/3043018/diff/4001/5007#newcode57
chrome/browser/shell_integration_unittest.cc:57: NOTIMPLEMENTED();
nit: This should be ADD_FAILURE(), see also other places in this file.

http://codereview.chromium.org/3043018/diff/4001/5010
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/3043018/diff/4001/5010#newcode123
net/proxy/proxy_config_service_linux_unittest.cc:123: NOTIMPLEMENTED();
Same here.

[Lei Zhang, 2010-07-26 20:43:06]

On 2010/07/26 17:31:43, Paweł Hajdan Jr. wrote:
> Drive-by with some test comments and a suggestion. Is it time to rename
> EnvVarGetter to something like Environment? It now does a lot more than
getting
> things.

I suppose it is time to bite the bullet and make this change.

[tfarina, 2010-07-29 23:34:49]

Sorry for the delay, I was pretty sick.

http://codereview.chromium.org/3043018/diff/4001/5007
File chrome/browser/shell_integration_unittest.cc (right):

http://codereview.chromium.org/3043018/diff/4001/5007#newcode57
chrome/browser/shell_integration_unittest.cc:57: NOTIMPLEMENTED();
On 2010/07/26 17:31:43, Paweł Hajdan Jr. wrote:
> nit: This should be ADD_FAILURE(), see also other places in this file.

Done.

http://codereview.chromium.org/3043018/diff/4001/5010
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/3043018/diff/4001/5010#newcode123
net/proxy/proxy_config_service_linux_unittest.cc:123: NOTIMPLEMENTED();
On 2010/07/26 17:31:43, Paweł Hajdan Jr. wrote:
> Same here.

Done.

[Lei Zhang, 2010-07-30 20:12:29]

LGTM with small modification below.

Glad you're feeling better.

http://codereview.chromium.org/3043018/diff/11001/12004
File base/env_var_unittest.cc (right):

http://codereview.chromium.org/3043018/diff/11001/12004#newcode53
base/env_var_unittest.cc:53: EXPECT_TRUE(env->UnSetEnv(kFooUpper));
Shouldn't you have a second call to HasEnv() to verify the variable has actually
been unset?

[tfarina, 2010-07-31 00:43:58]

http://codereview.chromium.org/3043018/diff/11001/12004
File base/env_var_unittest.cc (right):

http://codereview.chromium.org/3043018/diff/11001/12004#newcode53
base/env_var_unittest.cc:53: EXPECT_TRUE(env->UnSetEnv(kFooUpper));
On 2010/07/30 20:12:30, Lei Zhang wrote:
> Shouldn't you have a second call to HasEnv() to verify the variable has
actually
> been unset?

Yep, that would be good. Added!

[tfarina, 2010-07-31 00:47:14]

On 2010/07/30 20:12:29, Lei Zhang wrote:
> LGTM with small modification below.
> 
Committing now...

> Glad you're feeling better.
> 
:)

[viettrungluu, 2010-07-31 01:50:02]

Random after-the-fact drive-by (of only one file).

http://codereview.chromium.org/3043018/diff/22001/23009
File chrome/plugin/plugin_main_mac.mm (right):

http://codereview.chromium.org/3043018/diff/22001/23009#newcode28
chrome/plugin/plugin_main_mac.mm:28: int suffix_offset =
strlen(interpose_list.c_str()) -
No, don't do strlen(foo.c_str()). std::string has a method for that.

http://codereview.chromium.org/3043018/diff/22001/23009#newcode36
chrome/plugin/plugin_main_mac.mm:36: strcmp(interpose_list.c_str() +
suffix_offset,
You shouldn't do foo.c_str() + n either. There's also a better way to do that.

[viettrungluu, 2010-08-03 06:02:13]

http://codereview.chromium.org/3043018/diff/22001/23003
File base/env_var.h (right):

http://codereview.chromium.org/3043018/diff/22001/23003#newcode43
base/env_var.h:43: virtual bool UnSetEnv(const char* variable_name) = 0;
Even more after-the-fact drive-by nit: "unset" is a single word, so it should be
"UnsetEnv" (no capital 'S').