Proxy config for Linux (ProxyConfigServiceLinux)...

[This changelist has been replaced by http://codereview.chromium.org/60009 .]

Use GNOME proxy settings.  If that failed, use
http_proxy/ftp_proxy/no_proxy from the environment.

R=eroman,wtc
BUG=8143

[wtc, 2009-03-23 23:55:25]

Thanks a lot for the patch.

I didn't have time to review proxy_config_service_linux.cc
in detail (which is the meat of your changelist), so the
comments below are mostly nits and style issues.

http://codereview.chromium.org/49009/diff/2017/3013
File build/SConscript.main (right):

http://codereview.chromium.org/49009/diff/2017/3013#newcode597
Line 597: linux_env.ParseConfig( 'pkg-config --cflags --libs gconf-2.0' )
Nit: remove the spaces next to the parentheses.

http://codereview.chromium.org/49009/diff/2017/3014
File chrome/app/chrome_exe_main_gtk.cc (right):

http://codereview.chromium.org/49009/diff/2017/3014#newcode5
Line 5: #include <glib.h>
Your changes to chrome_exe_main_gtk.cc seem unrelated to
the rest of the changelist.

http://codereview.chromium.org/49009/diff/2017/3012
File net/net_lib.scons (left):

http://codereview.chromium.org/49009/diff/2017/3012#oldcode339
Line 339: 
Nit: add this blank line back.

http://codereview.chromium.org/49009/diff/2017/3012
File net/net_lib.scons (right):

http://codereview.chromium.org/49009/diff/2017/3012#newcode332
Line 332: env.ParseConfig( 'pkg-config --cflags --libs gconf-2.0' )
Nit: remove the spaces next to the parentheses.

http://codereview.chromium.org/49009/diff/2017/3015
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/49009/diff/2017/3015#newcode37
Line 37: const gchar *scheme;
Nit: our style guide allows placing * next to either the
type or the pointer variable, but it needs to be consistent.
I recommend placing the * next to the type.

http://codereview.chromium.org/49009/diff/2017/3015#newcode46
Line 46: namespace net {
Nit: add a blank line below this line.

http://codereview.chromium.org/49009/diff/2017/3015#newcode48
Line 48: bool isValidDomain(std::string str) {
Nit: isValidDomain => IsValidDomain

Input parameter str should be declared
  const std::string& str

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

http://codereview.chromium.org/49009/diff/2017/3015#newcode147
Line 147: const gchar *http_proxy = g_getenv("http_proxy");
Is it common to have lowercase environment variable names?

http://codereview.chromium.org/49009/diff/2017/3016
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/49009/diff/2017/3016#newcode1
Line 1: // Copyright (c) 2008 The Chromium Authors. All rights reserved.
Nit: use 2009 for this new file.

http://codereview.chromium.org/49009/diff/2017/3016#newcode19
Line 19: }  // namespace net
Nit: add a blank line above and below this line.

http://codereview.chromium.org/49009/diff/2017/3011
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/49009/diff/2017/3011#newcode23
Line 23: #if defined(OS_LINUX)
This line should be combined with the #endif line above
into "#elif defined(OS_LINUX)".

http://codereview.chromium.org/49009/diff/2017/3011#newcode211
Line 211: // TODO(port): implement ProxyConfigServiceLinux as well as make use
of
I guess this TODO comment block can be removed now?

[Maxim Ermilov, 2009-03-24 00:57:33]

> Line 5: #include <glib.h>
> Your changes to chrome_exe_main_gtk.cc seem unrelated to
> the rest of the changelist.

In this patch, I take information about proxy from gconf and In same time other
thread call to gtk/gdk functions. It is wrong. So I use
gdk_threads_enter/gdk_threads_leave. But before use this functions I should call
first gdk_threads_init. And It must be made before any use of the main loop from
GTK+. g_thread_init() must be called previous to gdk_threads_init.


> http://codereview.chromium.org/49009/diff/2017/3015#newcode147
> Line 147: const gchar *http_proxy = g_getenv("http_proxy");
> Is it common to have lowercase environment variable names?
Yes

[wtc, 2009-03-24 17:42:29]

Eric, you should look at this changelist, too.

[eroman, 2009-03-24 18:42:41]

thanks for the patch.

I have reviewed the non-gtk specific stuff (not familiar with GTK).

http://codereview.chromium.org/49009/diff/3008/2015
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/49009/diff/3008/2015#newcode25
Line 25: #ifndef NET_PROXY_PROXY_RESOLVER_LINUX_H_
nit: looks like this appears twice in the file.

http://codereview.chromium.org/49009/diff/3008/2010
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/49009/diff/3008/2010#newcode217
Line 217: NULL);
NULL isn't valid here, use new ProxyResolverV8() instead.

http://codereview.chromium.org/49009/diff/2017/3015
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/49009/diff/2017/3015#newcode8
Line 8: #include <string.h>
is both <string> and <string.h> needed?

http://codereview.chromium.org/49009/diff/2017/3015#newcode14
Line 14: #include <sstream>
to maintain sorted, order, <vector> should be after <sstream>

http://codereview.chromium.org/49009/diff/2017/3015#newcode58
Line 58: int GetProxyConfigGCONF(ProxyConfig* config) {
how about just returning bool from this function (doesn't seem there are any
finer granularity errors it returns).

http://codereview.chromium.org/49009/diff/2017/3015#newcode65
Line 65: if (mode != NULL) {
typically our code omits omits the != NULL and just writes "if (mode)"

http://codereview.chromium.org/49009/diff/2017/3015#newcode70
Line 70: } else {res = ERR_FAILED;}
this should be formatted as:
else {
  res = ERR_FAILED
}

would be nice if this could do an early return, but i guess still need to call
g_free(mode)

http://codereview.chromium.org/49009/diff/2017/3015#newcode84
Line 84: proxy = gconf_client_get_string(client, GCONF_HTTP_PROXY_KEY, NULL);
shouldnt there be a corresponding g_free(proxy) -- there is in one of the other
cases.

http://codereview.chromium.org/49009/diff/2017/3015#newcode90
Line 90: ss << proxy << ":" << port;
I would suggest organizing this similarly to [proxy_resolver.mac.cc] which has
the helper function:

  GetProxyServerFromDictionary(scheme, dict, host_key, port_key)

This would then encapsulate the repeated "port != 0 && proxy != NULL &&
isValidDomain(proxy)" checks, and the freeing of |proxy|.

I am picturing something along the lines of:

ProxyServer GetProxyServerFromGconf(
    GConfClient* client,
    ProxyServer::Scheme scheme,
    const char* host_key,
    const char* port_key) {
  gchar* proxy_host = gconf_client_get_string(client, host_key, NULL);
  int port = gconf_client_get_int(client, port_key, NULL);

  ProxyServer result;  // Initialized to invalid.

  if (port != 0 && proxy_host != NULL && IsValidDomain(proxy_host)) {
    result = ProxyServer(scheme, proxy_host, proxy_port);
  }

  g_free(proxy_host);
  return result;
}

Which is then used as:

ProxyServer proxy_server =
    GetProxyServerFromGconf(
      client,
      ProxyServer::SCHEME_HTTP,
      GCONF_HTTP_PROXY_KEY,
      GCONF_HTTP_PROXY_PORT_KEY);

if (proxy_server.is_valid()) {
  config->proxy_rules += proxy_server.ToURI();
}

http://codereview.chromium.org/49009/diff/2017/3015#newcode102
Line 102: if (port != 0 && proxy != NULL && isValidDomain(proxy)) {
Same comment to above, consider using something like |GetProxyServerFromGconf|

http://codereview.chromium.org/49009/diff/2017/3015#newcode117
Line 117: if (port != 0 && proxy != NULL && isValidDomain(proxy)) {
Same comment to above, consider using something like |GetProxyServerFromGconf|,
this time passing ProxyServer::SCHEME_SOCKS4.

http://codereview.chromium.org/49009/diff/2017/3015#newcode131
Line 131: list = gconf_client_get_list(client, GCONF_PROXY_BYPASS_LIST_KEY,
if (res == ERR_FAILED), maybe we don't want to bother retrieving the proxy
bypass list...

http://codereview.chromium.org/49009/diff/2017/3015#newcode153
Line 153: GURL http = GURL(http_proxy);
I don't actually know anything about convention of these environment variables,
but is it expected that server would be expressed as a fully qualified URL as
opposed to simply a host:port ?

http://codereview.chromium.org/49009/diff/2017/3015#newcode154
Line 154: if (http.has_host()) {
better off using "is_valid()" here, since GURL will expect to have a scheme and
host to be valid.

http://codereview.chromium.org/49009/diff/2017/3015#newcode170
Line 170: if (ftp_proxy != NULL && strlen(http_proxy) > 3) {
shouldnt this be ftp_proxy?

http://codereview.chromium.org/49009/diff/2017/3015#newcode171
Line 171: GURL ftp = GURL(ftp_proxy);
Same comment as above -- is ftp_proxy supposed to be a host:port or a
scheme://host:port

http://codereview.chromium.org/49009/diff/2017/3015#newcode174
Line 174: ss += "http=";
shouldnt this be "ftp=" ?

http://codereview.chromium.org/49009/diff/2017/3015#newcode191
Line 191: std::string bypass_url_domain = proxy_server_bypass_list.token();
might want to strip whitespace too.

http://codereview.chromium.org/49009/diff/2017/3015#newcode199
Line 199: gdk_threads_enter();
just to check, is this going to be pretty fast?

GetProxyConfig() is called in a polling fashion every 5 seconds (from IO
thread), so we want to be sure it won't cause the UI thread to block.

[Maxim Ermilov, 2009-03-24 23:18:20]

On 2009/03/24 18:42:41, eroman wrote:
> http://codereview.chromium.org/49009/diff/2017/3015#newcode8
> Line 8: #include <string.h>
> is both <string> and <string.h> needed?
Yes I use strcmp.

 
> http://codereview.chromium.org/49009/diff/2017/3015#newcode153
> Line 153: GURL http = GURL(http_proxy);
> I don't actually know anything about convention of these environment
variables,
> but is it expected that server would be expressed as a fully qualified URL as
> opposed to simply a host:port ?
> 
Yes, It is.


> http://codereview.chromium.org/49009/diff/2017/3015#newcode199
> Line 199: gdk_threads_enter();
> just to check, is this going to be pretty fast?
> 
> GetProxyConfig() is called in a polling fashion every 5 seconds (from IO
> thread), so we want to be sure it won't cause the UI thread to block.

Execution time between gdk_threads_enter - gdk_threads_leave inclusive = 0ms

But some times it wait UI thread and execution time grow to 2ms

[sdoyon, 2009-03-25 18:50:52]

OK, I'd been working on this last week. Sorry I was unavailable for
the last couple of days. It's great to get external contributions. But
had you not seen my name on the bug?

I guess I'll throw away the code I had now, but I still have comments
based on the research I did.

I'm no glib/gtk/gconf expert. I've seen some code checking against a
NULL return from gconf_client_get_default(). The doc doesn't mention
possible failures though, but shouldn't that be done?

I must admit I was unaware of the threading issues. Can you educate me
a bit about that?

Is it generally worth passing in a GError and logging any reported
errors?

OK now onto proxy stuff.

I think that
   /system/proxy/mode == auto && empty /system/proxy/autoconfig_url
=> means we should set config->auto_detect = true; and consider it a success.

There is a /system/http_proxy/use_http_proxy boolean setting,
apparently a predecessor to the mode setting. Choosing manual mode
from the GUI causes both mode to be set to manual and use_http_proxy
to be set to true. I'm not sure in what situation those two might
become unsynchronized and whether we need to check both, but the
Mozilla code is careful not to use the specific proxy settings unless
both mode is manual and use_http_proxy is true, and googling turned up
some scripts that seem to be careful to set both.

kSchemeInGCONF has the string "shttp", which is used as a scheme
specifier for config->proxy_rules. Doesn't this need to be "https"?

I'm not very familiar with socks, but I am under the impression that
socks is generally not intended to coexist with the other
protocols. Looks like the code as it is might put two "http=" entries
in config->proxy_rules. What the Mozilla code does is, if
use_same_proxy is false, then check first for socks. If that is set
then use that and don't look at http/secure/ftp.

Now about environment variables.

"https_proxy" and "all_proxy" appear to be commonly used and should be
supported too.

"auto_proxy" is much less common. I've seen it used in the
"environment_proxy" Firefox extension. But it sounds like a good idea
to me and I'd say we should have it too. It should specify the
pac_url, but I would add this twist that if "auto_proxy" is set to the
empty string, then we should set config->auto_detect = true.

There are environment variables commonly used to specify a socks
proxy. They are: "SOCKS_SERVER" and "SOCKS_VERSION". Yes those are all
caps and this is somewhat inconsistent. I think we ought to support
those too. Again I don't think both http_proxy and SOCKS_SERVER are
meant to coexist. I don't know that it makes much difference which is
tried first, but if found then the other should probably be ignored.

I'm looking at the way you're parsing the server:port specifications
for http_proxy and friends. Can a GURL have no scheme and still be
valid? Could use ProxyServer instead...

The "no_proxy" entry has somewhat strange semantics. I don't think we
can just stuff it into config->proxy_bypass. The traditional semantics
appear to be that the no_proxy entries match the trailing part of the
domain name.
http://lynx.isc.org/lynx2.8.6/lynx2-8-6/lynx_help/keystrokes/environments.html

So a single "*" entry is allowed, but otherwise, entries are assumed
to be a simple trailing match. This seems to be the definition adopted
by curl and Mozilla. I would have thought it would make more sense to
match on domain boundaries, so that example.com wouldn't match
abc.thisexample.com. You can indeed Google and find advice about
putting a dot ahead of your pattern.

We would get that behavior by inserting an '*' at the head of each
pattern. MatchPattern() called from
ProxyService::ShouldBypassProxyForURL() would do the rest. no_proxy
entries can also be a CIDR IP/mask pattern. There is a TODO about
supporting that in proxy_service.cc. I'm mentioning it because we
shouldn't insert a '*' wildcard in the case of an IP address.

no_proxy entries may also specify a port number, as in example.com:99,
in which case only connections in the given port are excluded from
proxying. I had intended to add support for that in
ProxyService::ShouldBypassProxyForURL().

gconf vs env vars:

When do we use gconf and when do we use env vars? AFAICT, you fallback
to env vars if either gconf specifies no proxy, or if you get
something invalid from gconf.

It was suggested to me that I should try to figure out whether the
user is running GNOME before consulting gconf, probably by checking
for the existance of GNOME_DESKTOP_SESSION_ID.

Initially I had intended to check for env vars first. That way you
would have been able to override proxy settings for one particular
invocation just by opening a terminal and doing:

    all_proxy=abc.example.com:3128 .../chrome

But I found out that gnome-terminal sets some environment variables
for you. So you can't distinguish whether they were set by the user or
gnome-terminal. And we can't trust the env vars set by gnome-terminal
because it appears that it may set http_proxy even when auto
configuration is selected in gconf.

And so I think we will indeed have to try gconf first.

Authentication:

Both gconf and env vars can specify a username and password for
accessing the proxy. I'm not sure we need to support that. Should we
put out a warning?

Being polled:

I must admit I was surprised to read that this piece of code will be
polled every 5 seconds. I wonder... I read that gconf has a
notification mechanism, I don't know how hard it is to use. And the
environment variables cannot change from under us. Calling this every
5 seconds shouldn't be a big deal, but it may not be necessary.


Please let me know what you think about these comments, and any
insights you might have.

[Maxim Ermilov, 2009-03-25 21:34:37]

On 2009/03/25 18:50:52, sdoyon wrote:
> OK, I'd been working on this last week. Sorry I was unavailable for
> the last couple of days. It's great to get external contributions. But
> had you not seen my name on the bug?
Sorry...


> I'm no glib/gtk/gconf expert. I've seen some code checking against a
> NULL return from gconf_client_get_default(). The doc doesn't mention
> possible failures though, but shouldn't that be done?

No, error message will be put on console and every calls to gconf_client_get_*
will put error message too.

> I must admit I was unaware of the threading issues. Can you educate me
> a bit about that?

GConfClient — GObject-based wrapper(Global for programm).
If you use GLib from more than one thread, you must initialize the thread system
by calling g_thread_init().


> Is it generally worth passing in a GError and logging any reported
> errors?

If something goes wrong, env variable will use instead. Should I print error in
this situation?


> But I found out that gnome-terminal sets some environment variables
> for you. So you can't distinguish whether they were set by the user or
> gnome-terminal. And we can't trust the env vars set by gnome-terminal
> because it appears that it may set http_proxy even when auto
> configuration is selected in gconf.

gnome-terminal set this variable based on gconf.


I will correct my patch in next 14 hours

[Maxim Ermilov, 2009-03-27 01:24:03]

Add support for "https_proxy" and "all_proxy","auto_proxy","SOCKS_SERVER" and
"SOCKS_VERSION". Correct code, that work with "no_proxy". Use
gconf_key:/system/http_proxy/use_http_proxy. Checking for the value of
DESKTOP_SESSION,before consulting gconf.

[sdoyon, 2009-03-27 20:41:31]

Thank you for your changes.

Some more comments:

IsValidDomain() will reject IPv6 addresses. I guess you'd need to add
':' '[' ']'.

If a gconf proxy entry has the port value unset or set to 0: you're
having it fail GetProxyConfigGCONF() entirely. Would it make sense to
plug in a default port number instead: from
ProxyServer::GetDefaultPortForScheme()?

When use_http_proxy is true: you should add a rule for socks only if
no other rules were added. Or at any rate, don't add two http= rules.

The "return false" statement inside the handling for mode: looks like
it'll leak the mode string.

I believe you should only get ignore_hosts and specify a non-empty
config->proxy_bypass when mode is "manual" and use_http_proxy is true.

The documentation for g_getenv says "The returned string may be
overwritten by the next call to g_getenv() [...]" I'm not clear why
the buffer would change if you're not modifying the environment, but
that seems to be the contract.

"SOCKS_VERSION" should probably be optional and defaulting to "4".

I'm new to Chrome myself, but perhaps IntToString() is preferred over
sscanf("%d")? Or strtol...

The delimiter for config->proxy_rules should be ';', not ',' (in
GetProxyConfigEnv()).

About parsing no_proxy: you should strip whitespace around each
token. The IP address recognition will not suffice: it should handle
1.2.3.4:99 and 1.2.3.4/8 and IPv6, possibly with brackets and a
:port... There are even legacy IPv4 representations with hex... This
is surely not commonly used, but AFAICT Mozilla and libproxy do handle
it. We should probably parse it right and then we'll be able to do the
CIDR comparison that is a TODO in proxy_service.cc. googleurl has code
to validate IP address strings internally that we can perhaps
re-use. Perhaps we could get this patch in first and leave the
no_proxy IP address checking as a TODO.

You're also missing port matching in
ProxyService::ShouldBypassProxyForURL(). I can submit a patch for that
separately if you want to leave it as a TODO.

GetProxyConfig(): "DESKTOP_SESSION" may not be a reliable way to
determine whether we are running GNOME. On my machine it's value is
"default", and I think this variable depends on your window
manager. The one I had recommended earlier is
"GNOME_DESKTOP_SESSION_ID": the value doesn't matter, just that it is
defined. You could always have both with an OR condition if you like.

Do you really need the pkg-config for gconf in both net/net_lib.scons
and build/SConscript.main?
This doesn't appear to build for me: missing some symbols from
proxy_resolver_v8.

[Maxim Ermilov, 2009-03-28 00:38:28]

I correct my patch.

On 2009/03/27 20:41:31, sdoyon wrote:
> Perhaps we could get this patch in first and leave the
> no_proxy IP address checking as a TODO.

 May be leave it as a TODO.

[eroman, 2009-03-30 21:25:08]

Thanks for making those changes.

I have a couple more style nits comments.

Also, it would be great to have some unit-test coverage. I recognize it is a bit
of a pain to isolate the dependencies. As part of "taking my own medicine" I
just added such a unit-test for the windows version [see
http://codereview.chromium.org/55001].

Lastly:

Just a heads up, but I am making some changes to |proxy_rules|, in order to
avoid needing to create a string here (http://codereview.chromium.org/57011). If
I end up getting this into repository soonish, it will require some extra
modifications in this CL to match.

http://codereview.chromium.org/49009/diff/5002/5007
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/49009/diff/5002/5007#newcode50
Line 50: bool IsValidDomain(const std::string& str) {
nit: make this "static".

http://codereview.chromium.org/49009/diff/5002/5007#newcode54
Line 54: if ( !(isalnum(str[i]) || str[i] == '.' || str[i] == '-' ||
nit: remove the space here "( !"

http://codereview.chromium.org/49009/diff/5002/5007#newcode61
Line 61: ProxyServer GetProxyServerFromGconf(
nit: make this "static".

http://codereview.chromium.org/49009/diff/5002/5007#newcode69
Line 69: ProxyServer result; // Initialized to invalid.
nit: two spaces between ";" and "/"

http://codereview.chromium.org/49009/diff/5002/5007#newcode73
Line 73: proxy_port = ProxyServer::GetDefaultPortForScheme (scheme);
nit: no space between "GetDefaultPortForScheme" and "("

http://codereview.chromium.org/49009/diff/5002/5007#newcode81
Line 81: bool GetProxyConfigGCONF(ProxyConfig* config) {
nit: make this "static".

http://codereview.chromium.org/49009/diff/5002/5007#newcode82
Line 82: bool res = true;
nit: I would suggest a more descriptive name like "result" or "ok".

http://codereview.chromium.org/49009/diff/5002/5007#newcode110
Line 110: client,
nit: indent these parameters by 4 spaces instead of 2.

http://codereview.chromium.org/49009/diff/5002/5007#newcode125
Line 125: client,
nit: indent these parameters by 4 spaces instead of 2.

http://codereview.chromium.org/49009/diff/5002/5007#newcode138
Line 138: client,
nit: indent these parameters by 4 spaces instead of 2.

http://codereview.chromium.org/49009/diff/5002/5007#newcode169
Line 169: GCONF_VALUE_STRING, NULL);
nit: two more spaces to line this up.

http://codereview.chromium.org/49009/diff/5002/5007#newcode193
Line 193: } kSchemeInEnv[] = {
nit: can you make this "static" ?

http://codereview.chromium.org/49009/diff/5002/5007#newcode208
Line 208: if (auto_proxy != NULL) {
nit: for consistency, consider "if (auto_proxy)"

http://codereview.chromium.org/49009/diff/5002/5007#newcode215
Line 215: if (socks_ver != NULL && socks_server != NULL) {
nit: for consistency consider omitting "!= NULL"

http://codereview.chromium.org/49009/diff/5002/5007#newcode237
Line 237: if (all_proxy != NULL) {
nit: for consistency consider omitting "!= NULL".

http://codereview.chromium.org/49009/diff/5002/5007#newcode246
Line 246: if (proxy != NULL) {
nit: for consistency consider omitting "!= NULL".

http://codereview.chromium.org/49009/diff/5002/5007#newcode249
Line 249: if (config->proxy_rules.size() > 1)
I believe you can just test > 0 here (i.e. not empty).

http://codereview.chromium.org/49009/diff/5002/5007#newcode266
Line 266: str += no_proxy[i];
instead of stripping whitespace here, I would suggest stripping the whitespace
around each token within the loop:

std::string bypass_url_domain;
TrimWhitespaceASCII(proxy_server_bypass_list.token(), TRIM_ALL, &url_domain);

TrimWhitespaceASCII() is defined in base/string_util.h. I encourage checking it
out to see if it can meet your needs (strips more than just SP).

http://codereview.chromium.org/49009/diff/5002/5007#newcode272
Line 272: if (sscanf(bypass_url_domain.c_str(), "%d.%d.%d.%d",
I recommend using GURL to test if it is an IP address or not (that way it should
work with IPv6 too).

I believe the following snippet should work (typing it out using
GURL::HostIsIPAddress() as a guide):



#include "googleurl/src/url_canon.h"

...

bool IsIpAddress(const std::string& domain) {
  url_canon::RawCanonOutputT<char, 128> ignored_output;
  url_parse::Component ignored_component;
  url_parse::Component domain_comp(0, domain.size());
  return url_canon::CanonicalizeIPAddress(domain.c_str(), domain_comp,
                                          &ignored_output, &ignored_component);
}

http://codereview.chromium.org/49009/diff/5002/5008
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/49009/diff/5002/5008#newcode14
Line 14: ProxyConfigServiceLinux() {
minor nit: can leave this off since it's empty.