Properly detect KDE4 on newer systems (e.g. [K]Ubuntu 9.04).

Properly detect KDE4 on newer systems (e.g. [K]Ubuntu 9.04).
BUG=25938
TEST=try to configure proxy settings on KDE4 when $DESKTOP_SESSION=kde, it should work

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

[Evan Martin, 2009-11-21 18:31:58]

I'd like to see a test that covers the regression; this detection code is really
subtle and the existing test coverage is quite good.

http://codereview.chromium.org/427013/diff/1005/2005
File base/linux_util.cc (right):

http://codereview.chromium.org/427013/diff/1005/2005#newcode233
Line 233: if (env->Getenv("KDE_SESSION_VERSION", &dummy))
We should really fix Getenv to accept a NULL argument so we don't need all of
these "dummy" variables.  :)

http://codereview.chromium.org/427013/diff/1005/2006
File net/proxy/proxy_config_service_linux_unittest.cc (left):

http://codereview.chromium.org/427013/diff/1005/2006#oldcode1195
Line 1195: env_getter->values.DESKTOP_SESSION = "kde";
Is it really never the case that DESKTOP_SESSION=kde?

If not, don't we need tests covering both cases?

[Mike Mammarella, 2009-11-22 01:28:59]

On 2009/11/21 18:31:58, Evan Martin wrote:
> I'd like to see a test that covers the regression; this detection code is
really
> subtle and the existing test coverage is quite good.

Well, it's not that Chromium's behavior regressed - it's that the behavior of
newer desktop systems is different, and the old logic no longer correctly
determined the running desktop environment. I don't see how we can test that,
since any mockup simulation of the desktop environment's behavior (i.e. how it
sets DESKTOP_SESSION) would defeat the point.

> http://codereview.chromium.org/427013/diff/1005/2005
> File base/linux_util.cc (right):
> 
> http://codereview.chromium.org/427013/diff/1005/2005#newcode233
> Line 233: if (env->Getenv("KDE_SESSION_VERSION", &dummy))
> We should really fix Getenv to accept a NULL argument so we don't need all of
> these "dummy" variables.  :)

Or perhaps add a new method like Contains() that just returns the bool.

> http://codereview.chromium.org/427013/diff/1005/2006
> File net/proxy/proxy_config_service_linux_unittest.cc (left):
> 
> http://codereview.chromium.org/427013/diff/1005/2006#oldcode1195
> Line 1195: env_getter->values.DESKTOP_SESSION = "kde";
> Is it really never the case that DESKTOP_SESSION=kde?
> 
> If not, don't we need tests covering both cases?

I changed the test case to refer to kde4 rather than plain kde since that makes
the desktop environment detection code not check for KDE_SESSION_VERSION. The
mock environment variable getter asserts that we never ask for a variable it
doesn't at least know about, so I'd have to add that variable to it and change
all the test cases to have an extra NULL placeholder in them to add it. But see
above about testing this particular functionality; I don't think it can really
be tested the way you'd like.

[Mike Mammarella, 2009-11-25 00:43:45]

Ping. Thoughts on this?

By the way it looks like we might need some new way to detect GNOME as well:
http://code.google.com/p/chromium/issues/detail?id=27106

[Evan Martin, 2009-11-25 01:07:06]

I guess my original question was: does this continue working correctly on older
systems?  LGTM if so

[Lei Zhang, 2009-11-25 03:07:52]

This should still work on older systems.

http://codereview.chromium.org/427013/diff/1005/2005
File base/linux_util.cc (right):

http://codereview.chromium.org/427013/diff/1005/2005#newcode233
base/linux_util.cc:233: if (env->Getenv("KDE_SESSION_VERSION", &dummy))
On my Kubuntu 9.04 box, KDE_SESSION_VERSION=4. Shouldn't we check that? Same
below on line 245. I realize that variable doesn't exist on KDE 3.

[Mike Mammarella, 2009-11-25 09:35:53]

http://codereview.chromium.org/427013/diff/1005/2005
File base/linux_util.cc (right):

http://codereview.chromium.org/427013/diff/1005/2005#newcode233
base/linux_util.cc:233: if (env->Getenv("KDE_SESSION_VERSION", &dummy))
On 2009/11/25 03:07:52, Lei Zhang wrote:
> On my Kubuntu 9.04 box, KDE_SESSION_VERSION=4. Shouldn't we check that? Same
> below on line 245. I realize that variable doesn't exist on KDE 3.

My reasoning was that if we check for its value, then we'll start detecting KDE3
again if they change that value (e.g. to 4.4, 5, 4-dfsg1-10ubuntu8, etc.) rather
than KDE4. The presence of the variable indicates KDE >= 4 which is all we care
about currently.

[Lei Zhang, 2009-11-25 19:22:32]

On 2009/11/25 09:35:53, Mike Mammarella wrote:
> http://codereview.chromium.org/427013/diff/1005/2005
> File base/linux_util.cc (right):
> 
> http://codereview.chromium.org/427013/diff/1005/2005#newcode233
> base/linux_util.cc:233: if (env->Getenv("KDE_SESSION_VERSION", &dummy))
> On 2009/11/25 03:07:52, Lei Zhang wrote:
> > On my Kubuntu 9.04 box, KDE_SESSION_VERSION=4. Shouldn't we check that? Same
> > below on line 245. I realize that variable doesn't exist on KDE 3.
> 
> My reasoning was that if we check for its value, then we'll start detecting
KDE3
> again if they change that value (e.g. to 4.4, 5, 4-dfsg1-10ubuntu8, etc.)
rather
> than KDE4. The presence of the variable indicates KDE >= 4 which is all we
care
> about currently.

I suppose even if we're able to detect KDE > 4, we wouldn't know what the right
thing to do is. We'd just deal with KDE > 4 when that day arrives.

LGTM then.

[Lei Zhang, 2009-11-25 19:27:28]

BTW, I think this should be cherry-picked into the 249 branch. It's a small, low
risk change that'll help out a lot of KDE users. I'm surprised we didn't find
and fix this earlier, since most Ubuntu users are on Jaunty/Karmic now.

[Evan Martin, 2009-11-25 19:37:43]

I defer to you guys on this; I know so little about KDE it's embarrassing.