ProxyConfigService for Linux....

ProxyConfigService for Linux.

Establishes a ProxyConfig by reading settings from gconf or consulting
environment variables.

BUG=8143
Thanks to ermilov.maxim@gmail.com for his contribution: some ideas<
and code snippets from his patch were folded into this one.
(See http://codereview.chromium.org/49009)


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

[eroman, 2009-04-02 21:37:48]

I have done a quick first pass for some nits.

Will follow up with more logic comments next.

I am happy to see there is a substantial unit test! (but a bit skeptical to have
glanced macros).

http://codereview.chromium.org/60009/diff/1/13
File chrome/app/chrome_exe_main_gtk.cc (right):

http://codereview.chromium.org/60009/diff/1/13#newcode36
Line 36: g_type_init();
nit: can the need for this be clarified by either:

(a) adding a comment that ProxyConfigServiceLinux depends on this because it
accesses through the IO thread.

(b) extract this initialization to a static method of ProxyConfigService so the
dependency is explicit.

http://codereview.chromium.org/60009/diff/1/7
File net/net.gyp (right):

http://codereview.chromium.org/60009/diff/1/7#newcode431
Line 431: 'proxy/proxy_config_service_linux_unittest.cc',
Have you verified that this file does NOT get built on windows?

I was glancing through the |conditions| section of |net_unittests|, and I don't
think there is a filter to remove "*_linux_unittest.cc" from the non-linux
builds.

Whereas by comparison, there is a filter for *_linux.cc:

'OS == "win"', {
     'sources/': [ ['exclude', '_(mac|linux|posix)\\.cc$'] ],

http://codereview.chromium.org/60009/diff/1/3
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/60009/diff/1/3#newcode18
Line 18: #include "googleurl/src/url_canon.h"
this needs to be higher up in the list to maintain sorted order.

http://codereview.chromium.org/60009/diff/1/3#newcode51
Line 51: // Given a proxy hostname from a setting, return that hostname with
"return" --> "returns" (passive tense).

http://codereview.chromium.org/60009/diff/1/3#newcode61
Line 61: scheme = ProxyServer::SCHEME_SOCKS5;
Can you use curly braces on this "if", as it is a bit hard to spot the
indentation. On my initial read I thought it included more in the block.

http://codereview.chromium.org/60009/diff/1/3#newcode97
Line 97: // Obtain an environment variable's value. Parse a proxy server
"Obtains".

http://codereview.chromium.org/60009/diff/1/3#newcode457
Line 457: // Check for autyhentication, just so we can warn.
typo

http://codereview.chromium.org/60009/diff/1/4
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/60009/diff/1/4#newcode13
Line 13: #include "net/proxy/proxy_config_service.h"
This should be first, since it is the associated .h for this .cc. See:

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

http://codereview.chromium.org/60009/diff/1/4#newcode21
Line 21: // Get an environment variable's value and store it in
Use passive tense for the comments. "Gets an environment ...".

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

http://codereview.chromium.org/60009/diff/1/4#newcode28
Line 28: virtual bool is_valid() = 0;
Style convention is to only use c hacker notation (lowercase_with_underscore)
for const accessors.

Since this is a virtual function, use Upper camel case as in:

IsValid()

http://codereview.chromium.org/60009/diff/1/4#newcode33
Line 33: // Get a string type value from gconf and store it in result. Returns
Use passive tense for function comments instead, as in "Gets a string".

http://codereview.chromium.org/60009/diff/1/4#newcode57
Line 57: // ProxyConfigService methods.
ultra minor nit: can you use a colon instead of a period? (I think the majority
of our code uses template "XXX methods:")

http://codereview.chromium.org/60009/diff/1/6
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/60009/diff/1/6#newcode11
Line 11: #include "net/proxy/proxy_config_service_linux.h"
Make this the first one.

http://codereview.chromium.org/60009/diff/1/6#newcode17
Line 17: // Vset of values for all environment variables that we might
Typo

[eroman, 2009-04-02 23:58:33]

wee, got through it all!

generally looks good, and you are considering a lot of details.

i have some comments on the implementation details.

http://codereview.chromium.org/60009/diff/1/3
File net/proxy/proxy_config_service_linux.cc (right):

http://codereview.chromium.org/60009/diff/1/3#newcode180
Line 180: // Look for the proxy bypass list.
suggestions: since this function is quite large, how about breaking the proxy
bypass extraction into its own helper?

http://codereview.chromium.org/60009/diff/1/3#newcode193
Line 193: // supported. We will support them, as we Get * wildcards for free
"Get" --> "get".

http://codereview.chromium.org/60009/diff/1/3#newcode194
Line 194: // (see MatchPattern() called from
ProxyService::ShouldBypassProxyForURL().
unmatched close paren :-)

http://codereview.chromium.org/60009/diff/1/3#newcode202
Line 202: // to do this in proxy_service.cc, but it's currently a TODO.
Feel free to file a bug on this, and reference the bug id here as
"http://crbug.com/XXX".

http://codereview.chromium.org/60009/diff/1/3#newcode219
Line 219: potential_ip = std::string(begin, scan-1);
nit: "scan - 1"  (space around binary operators).

http://codereview.chromium.org/60009/diff/1/3#newcode224
Line 224: bypass_entry.insert(0, "*");
nit: please use a curly brace here, since multi-line statements on "if" can be
hard to read.

http://codereview.chromium.org/60009/diff/1/3#newcode236
Line 236: class GConfSettingGetterImpl : public GConfSettingGetterBase {
this is sexy.
seems gdk api needs a surprising amount of setup!

http://codereview.chromium.org/60009/diff/1/3#newcode274
Line 274: GConfValue* gconf_value = gconf_client_get(client_, key, &error);
nit: in the other Get* this is called |value|, but here it is called
|gconf_value|.

Also, can you not use gconf_client_get_bool() here?

http://codereview.chromium.org/60009/diff/1/3#newcode278
Line 278: // Unset.
nit: for multiline ifs, please use braces.

http://codereview.chromium.org/60009/diff/1/3#newcode338
Line 338: // Obtain host and port gconf settings and parse a proxy server
"obtains", "parses".

http://codereview.chromium.org/60009/diff/1/3#newcode347
Line 347: // Unset or empty.
please enclose with curly braces.

http://codereview.chromium.org/60009/diff/1/3#newcode354
Line 354: host += ":" + IntToString(port);
curly braces please.

http://codereview.chromium.org/60009/diff/1/3#newcode377
Line 377: return false;
curly braces please.

http://codereview.chromium.org/60009/diff/1/3#newcode382
Line 382: if (mode.compare("auto") == 0) {
just curious, don't need to be case-insensitive?

http://codereview.chromium.org/60009/diff/1/3#newcode386
Line 386: &pac_url_str)) {
nit: indentation is a bit off. either line up with the second parens, or indent
by 4.

http://codereview.chromium.org/60009/diff/1/3#newcode401
Line 401: return false;
curly braces please.

http://codereview.chromium.org/60009/diff/1/3#newcode404
Line 404: &use_http_proxy)
nit: indentation.

http://codereview.chromium.org/60009/diff/1/3#newcode415
Line 415: &same_proxy);
nit: indentation.

http://codereview.chromium.org/60009/diff/1/3#newcode455
Line 455: return false;
nit: use curly braces.

http://codereview.chromium.org/60009/diff/1/3#newcode463
Line 463: "not supported";
nit: indent by 4 or lineup with one of the <<

http://codereview.chromium.org/60009/diff/1/3#newcode497
Line 497: // gnome-terminal "helpfully" sets http_proxy and no_proxy, and it
hehehe.

http://codereview.chromium.org/60009/diff/1/3#newcode506
Line 506: "from gconf";
nit: indentation.

http://codereview.chromium.org/60009/diff/1/3#newcode522
Line 522: "from environment variables";
nit: indentation.

http://codereview.chromium.org/60009/diff/1/3#newcode525
Line 525: return OK;
nit: consider a one liner
return ok ? OK : ERR_FAILED;

http://codereview.chromium.org/60009/diff/1/4
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/60009/diff/1/4#newcode19
Line 19: class EnvironmentVariableGetterBase {
I recommend making this an inner class of ProxyConfigServiceLinux to cut back on
names added to net::

Secondly, I would omit "Base" in the name -- since it is abstract there
shouldn't be any confusion on using it incorrectly. By convention our code
doesn't label interfaces/base classes.

http://codereview.chromium.org/60009/diff/1/4#newcode24
Line 24: };
Please add an empty virtual destructor here. It is part of our style to *always*
define a virtual destructor on abstract classes, so it doesn't get forgotten and
introduce a leak.

This has saved my bacon in the past :-)

http://codereview.chromium.org/60009/diff/1/4#newcode26
Line 26: class GConfSettingGetterBase {
btw i like these abstractions.
i guess i am a fanboi of dependency injection :)

http://codereview.chromium.org/60009/diff/1/4#newcode29
Line 29: // gdk thread-safety
Can you expand this a bit to say what the critical section it protects is (i.e.
when enter is called, when leave is called).

http://codereview.chromium.org/60009/diff/1/4#newcode43
Line 43: };
Same comment as above, please define an (empty) virtual destructor for this
interface.

http://codereview.chromium.org/60009/diff/1/4#newcode68
Line 68: ProxyServer* result_server);
nit: in GetProxyFromEnvVar() output is named |result|, and here is is named
|result_server| -- try to use just one or the other throughout.

http://codereview.chromium.org/60009/diff/1/6
File net/proxy/proxy_config_service_linux_unittest.cc (right):

http://codereview.chromium.org/60009/diff/1/6#newcode20
Line 20: const char *GNOME_DESKTOP_SESSION_ID, *DESKTOP_SESSION,
I have some comments regarding the macros that consume this... if you do keep
the macros, please add a comment here explaining the UPPERCASING is intentional.

http://codereview.chromium.org/60009/diff/1/6#newcode30
Line 30: UNSET = 0, TRUE, FALSE
nit: Can you leave off the "= 0"? I don't believe the start value actually
matters for us here.

http://codereview.chromium.org/60009/diff/1/6#newcode50
Line 50: typedef std::map<std::string, value_type*> map_type;
Why not just store the value directly in SettingsTable? I found the
{GConfValues,EnvVarValues} / SettingsTable split confusing.

You probably only need GconfValues as an intermediary representation to build up
the tests[] data, and then just *copy* it into a Settings table

http://codereview.chromium.org/60009/diff/1/6#newcode51
Line 51: map_type settings;
list this after the methods. See:

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

http://codereview.chromium.org/60009/diff/1/6#newcode53
Line 53: // Get the value from its location
Passive tense -- "Gets".

http://codereview.chromium.org/60009/diff/1/6#newcode68
Line 68: #define ENTRY(x) table.settings[#x] = &values.x
Same comment as earlier -- seems if SettingsTable and EnvVarValues were
consolidated, there wouldn't be the need for this pointing.

http://codereview.chromium.org/60009/diff/1/6#newcode96
Line 96: SettingsTable<const char*> stringsTable;
name_variables_like_this.

Also, move the member data declarations after the methods.
Lastly, can these be private?

http://codereview.chromium.org/60009/diff/1/6#newcode174
Line 174: // From proxy_config_service_win_unittest.cc
Can you extract these three helper functions rather than duplicate?

Perhaps create a file |proxy_config_service_unittest.h|, and just include them
inline there.

http://codereview.chromium.org/60009/diff/1/6#newcode205
Line 205: new MockEnvironmentVariableGetter;
indent continuations by 4.

http://codereview.chromium.org/60009/diff/1/6#newcode440
Line 440: "*.google.com\n",             // proxy_bypass_list
minor nit: consider using consitent indentation for these (some but not all are
aligned).

http://codereview.chromium.org/60009/diff/1/6#newcode456
Line 456: for (BypassList::const_iterator it = config.proxy_bypass.begin();
Since this is also common with proxy_config_service_win_unittest.cc, it could
also be one of the functions extracted to hypothetical file
|proxy_config_service_unittest.h|.

http://codereview.chromium.org/60009/diff/1/6#newcode467
Line 467: TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
You may want to throw in a test for the upper-case/lower-case $VARIABLE_NAME
fallback scheme.

If it requires too much setup work, don't worry about it.

http://codereview.chromium.org/60009/diff/1/6#newcode469
Line 469: new MockEnvironmentVariableGetter;
indent by 4.

http://codereview.chromium.org/60009/diff/1/6#newcode727
Line 727: typedef std::vector<std::string> BypassList;
Same comment as above -- extract this to its own function (shared with
proxy_config_service_win_unittest.cc).

http://codereview.chromium.org/60009/diff/1/2
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/60009/diff/1/2#newcode570
Line 570: std::string url_domain_and_port = url_domain + ":"
Please split off the ProxyService::ShouldBypassProxyForURL() changes into a
separate CL.

I feel this will make for nicer changelog history, as this part isn't
linux-specific.

http://codereview.chromium.org/60009/diff/1/2#newcode610
Line 610: // proxy bypass for IP-specified hosts (e.g.  "10.0.0.0/8"; see
Just a heads up -- I noticed your unit-test uses this format too. The test is
good, just pointing out that it won't actually work quite right yet.

[sdoyon, 2009-04-03 19:35:37]

> http://codereview.chromium.org/60009/diff/1/13#newcode36
> Line 36: g_type_init();
> nit: can the need for this be clarified by either:

I have added a comment. But I would like this aspect checked by someone
familiar with glib/gdk. I have re-pinged a couple of team members about
this.

> http://codereview.chromium.org/60009/diff/1/7#newcode431
> Line 431: 'proxy/proxy_config_service_linux_unittest.cc',
> Have you verified that this file does NOT get built on windows?

Had escaped me, fixed.

> http://codereview.chromium.org/60009/diff/1/3#newcode18
> Line 18: #include "googleurl/src/url_canon.h"
> this needs to be higher up in the list to maintain sorted order.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode51
> Line 51: // Given a proxy hostname from a setting, return that hostname
> with
> "return" --> "returns" (passive tense).

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode61
> Line 61: scheme = ProxyServer::SCHEME_SOCKS5;
> Can you use curly braces on this "if", as it is a bit hard to spot the

If you like.
Don't see why I put the comment inside the condition... moving it out.

> http://codereview.chromium.org/60009/diff/1/3#newcode97
> Line 97: // Obtain an environment variable's value. Parse a proxy server
> "Obtains".

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode457
> Line 457: // Check for autyhentication, just so we can warn.
> typo

Done.

> http://codereview.chromium.org/60009/diff/1/4
> File net/proxy/proxy_config_service_linux.h (right):
>
> http://codereview.chromium.org/60009/diff/1/4#newcode13
> Line 13: #include "net/proxy/proxy_config_service.h"
> This should be first, since it is the associated .h for this .cc. See:

Don't think so, this is a .h file.

You could argue for that include to have some precedence as it's the class
we're going to subclass, but probably not.

> http://codereview.chromium.org/60009/diff/1/4#newcode21
> Line 21: // Get an environment variable's value and store it in
> Use passive tense for the comments. "Gets an environment ...".

Done.

> http://codereview.chromium.org/60009/diff/1/4#newcode28
> Line 28: virtual bool is_valid() = 0;
> Style convention is to only use c hacker notation
> (lowercase_with_underscore) for const accessors.
>
> Since this is a virtual function, use Upper camel case as in:
>
> IsValid()

Funny, I had unconciously copied the is_valid() from ProxyServer and
GURL. Of course GURL's is an accessor. Is ProxyServer's OK because
is not virtual?  Anyway fixed.

> http://codereview.chromium.org/60009/diff/1/4#newcode33
> Line 33: // Get a string type value from gconf and store it in result.
> Returns
> Use passive tense for function comments instead, as in "Gets a string".

Done.

> http://codereview.chromium.org/60009/diff/1/4#newcode57
> Line 57: // ProxyConfigService methods.
> ultra minor nit: can you use a colon instead of a period? (I think the
> majority of our code uses template "XXX methods:")

Sure, done. Although this is copied from proxy_config_service_win.h which
uses a dot.


> http://codereview.chromium.org/60009/diff/1/6#newcode11
> Line 11: #include "net/proxy/proxy_config_service_linux.h"
> Make this the first one.

Done.

> http://codereview.chromium.org/60009/diff/1/6#newcode17
> Line 17: // Vset of values for all environment variables that we might
> Typo


Done.

Later for the more substantial comments.

Thanks

[sdoyon, 2009-04-10 05:30:21]

On Thu, 2 Apr 2009, eroman@chromium.org wrote:

> http://codereview.chromium.org/60009/diff/1/3
> File net/proxy/proxy_config_service_linux.cc (right):
>
> http://codereview.chromium.org/60009/diff/1/3#newcode180
> Line 180: // Look for the proxy bypass list.
> suggestions: since this function is quite large, how about breaking the
> proxy bypass extraction into its own helper?

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode193
> Line 193: // supported. We will support them, as we Get * wildcards for
> free
> "Get" --> "get".

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode194
> Line 194: // (see MatchPattern() called from
> ProxyService::ShouldBypassProxyForURL().
> unmatched close paren :-)

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode202
> Line 202: // to do this in proxy_service.cc, but it's currently a TODO.
> Feel free to file a bug on this, and reference the bug id here as
> "http://crbug.com/XXX".

OK, done.

> http://codereview.chromium.org/60009/diff/1/3#newcode219
> Line 219: potential_ip = std::string(begin, scan-1);
> nit: "scan - 1"  (space around binary operators).

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode224
> Line 224: bypass_entry.insert(0, "*");
> nit: please use a curly brace here, since multi-line statements on "if"
> can be hard to read.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode236
> Line 236: class GConfSettingGetterImpl : public GConfSettingGetterBase {
> this is sexy.
> seems gdk api needs a surprising amount of setup!

Yeah... I'm still investigating the GDK thread safety aspect. More on
that below.

> Line 274: GConfValue* gconf_value = gconf_client_get(client_, key,
> &error);
> nit: in the other Get* this is called |value|, but here it is called
> |gconf_value|.
>
> Also, can you not use gconf_client_get_bool() here?

Being perhaps slightly pedantic, I preferred to distinguish unset
keys, vs being returned a default value of false. For strings it's
easier: gconf_client_get_string() returns NULL when the key is unset,
and an empty string when it's set to empty. For booleans, we need to
fallback to the type-generic gconf_client_get(), which returns a
GConfValue*, which contains a typed value: from it we can check the
type and extract our bool. Ints and lists have the same issue, but I
didn't bother with the distinction for them because it's clunky and as
it happens we don't care in those cases.

I've added a comment to clarify.

> http://codereview.chromium.org/60009/diff/1/3#newcode278
> Line 278: // Unset.
> nit: for multiline ifs, please use braces.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode338
> Line 338: // Obtain host and port gconf settings and parse a proxy
> server
> "obtains", "parses".

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode347
> Line 347: // Unset or empty.
> please enclose with curly braces.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode354
> Line 354: host += ":" + IntToString(port);
> curly braces please.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode377
> Line 377: return false;
> curly braces please.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode382
> Line 382: if (mode.compare("auto") == 0) {
> just curious, don't need to be case-insensitive?

Not really necessary. The possible values "none","manual","auto" are
plainly documented, there are no capitalized keys or values in gconf
to induce confusion, and most of the time this will be set indirectly
through a GUI preference editor. Mozilla and libproxy don't do a case
insensitive compare either.

> http://codereview.chromium.org/60009/diff/1/3#newcode386
> Line 386: &pac_url_str)) {
> nit: indentation is a bit off. either line up with the second parens, or
> indent by 4.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode401
> Line 401: return false;
> curly braces please.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode404
> Line 404: &use_http_proxy)
> nit: indentation.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode415
> Line 415: &same_proxy);
> nit: indentation.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode455
> Line 455: return false;
> nit: use curly braces.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode463
> Line 463: "not supported";
> nit: indent by 4 or lineup with one of the <<

Oh right... Note to self: this one must be due to the C++ emacs mode
on glaptop not being up to google style...

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode506
> Line 506: "from gconf";
> nit: indentation.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode522
> Line 522: "from environment variables";
> nit: indentation.

Done.

> http://codereview.chromium.org/60009/diff/1/3#newcode525
> Line 525: return OK;
> nit: consider a one liner
> return ok ? OK : ERR_FAILED;

Indeed.

> http://codereview.chromium.org/60009/diff/1/4
> File net/proxy/proxy_config_service_linux.h (right):
>
> http://codereview.chromium.org/60009/diff/1/4#newcode19
> Line 19: class EnvironmentVariableGetterBase {
> I recommend making this an inner class of ProxyConfigServiceLinux to cut
> back on names added to net::

Oh indeed. And I probably should be using anonymous namespaces for the
subclasses. Did that.

> Secondly, I would omit "Base" in the name -- since it is abstract there
> shouldn't be any confusion on using it incorrectly. By convention our
> code doesn't label interfaces/base classes.

Ah right. Done.

> http://codereview.chromium.org/60009/diff/1/4#newcode24
> Line 24: };
> Please add an empty virtual destructor here. It is part of our style to
> *always* define a virtual destructor on abstract classes, so it doesn't
> get forgotten and introduce a leak.
>
> This has saved my bacon in the past :-)

Indeed. Done.

> http://codereview.chromium.org/60009/diff/1/4#newcode26
> Line 26: class GConfSettingGetterBase {
> btw i like these abstractions.
> i guess i am a fanboi of dependency injection :)

Thanks. I haven't exactly done this often, but that's the only way I
could think to do it.

> http://codereview.chromium.org/60009/diff/1/4#newcode29
> Line 29: // gdk thread-safety
> Can you expand this a bit to say what the critical section it protects
> is (i.e. when enter is called, when leave is called).

Right. I've been researching the threading issues, as you know. Still
pending as to whether this is really absolutely necessary.

Assuming the worse case in the meantime, I need to make changes to get
the global lock also when initializing the gconf_client. I added an
init method such that the global lock can be held while calling
gconf_client_get_default(). (It replaces IsValid()). I've added a
comment.

I moved the glib/gdk threading initializations to
app/chrome_dll_main.cc (from chrome_exe_main_gtk.cc). This is where we
already have the gtk_init(). estade@ confirmed this choice.

If GDK locking is needed for gconf, then we need to get the global
lock in the main loop too. Added a comment about that.

> http://codereview.chromium.org/60009/diff/1/4#newcode43
> Line 43: };
> Same comment as above, please define an (empty) virtual destructor for
> this interface.

Done.

> http://codereview.chromium.org/60009/diff/1/4#newcode68
> Line 68: ProxyServer* result_server);
> nit: in GetProxyFromEnvVar() output is named |result|, and here is is
> named |result_server| -- try to use just one or the other throughout.

Indeed.

Ouch...: my original reply appears to bust Rietveld's max length for a
message... More in a subsequent comment.

[sdoyon, 2009-04-10 05:31:15]

On Thu, 2 Apr 2009, eroman@chromium.org wrote:

> http://codereview.chromium.org/60009/diff/1/6
> File net/proxy/proxy_config_service_linux_unittest.cc (right):
>
> http://codereview.chromium.org/60009/diff/1/6#newcode20
> Line 20: const char *GNOME_DESKTOP_SESSION_ID, *DESKTOP_SESSION,
> I have some comments regarding the macros that consume this... if you do
> keep the macros, please add a comment here explaining the UPPERCASING is
> intentional.

Right.

> http://codereview.chromium.org/60009/diff/1/6#newcode30
> Line 30: UNSET = 0, TRUE, FALSE
> nit: Can you leave off the "= 0"? I don't believe the start value
> actually matters for us here.

It is so that struct GConfValues my_values = { 0 }; will initialize
the boolean fields to UNSET. Added a comment.

> http://codereview.chromium.org/60009/diff/1/6#newcode50
> Line 50: typedef std::map<std::string, value_type*> map_type;
> Why not just store the value directly in SettingsTable? I found the
> {GConfValues,EnvVarValues} / SettingsTable split confusing.

My initial plan was that you would be able to just write:
  env_getter->values.http_proxy = "blabla";
and then env_getter->GetString("http_proxy", ...) would get the
updated value. So you could mutate one test input struct into the
next, when you want to change only one or two fields.

In the end though I mostly re-used the test structure from the windows
unit tests and did not actually make use of this pattern.

To illustrate, here's a small cleanup: I've removed the env_values
variable from ProxyConfigServiceLinuxTest. Instead I just do:
  env_getter->values.GNOME_DESKTOP_SESSION_ID = "defined";
And conversely for the other test.

(I've also added functionality to the mocks so they can self
initialize/reset to zero values.)

This seems reasonably useful to me, although admittedly I'm not really
making much use of that flexibility at this point.

I'm still ambivalent about the tests[] array formulation. It looks
pretty neat, but I have had two practical issues while working with
it:

1) get one field wrong in the middle of it, and the compiler complains
obscurely, pointing to the closing brace at the very end of the entire
sequence.

2) When you get a test failure, there's practically no indication of
which test failed.

I'm not sure what to do about this, short of "unrolling" the whole thing.

**Update: After asking and looking around, it seems one can simply put
in a custom message with << on an EXPECT statement. So we could add a
description string to each test and have that output on failure. I'll
put it in as a TODO for now.

> http://codereview.chromium.org/60009/diff/1/6#newcode51
> Line 51: map_type settings;
> list this after the methods. See:

OK.

> http://codereview.chromium.org/60009/diff/1/6#newcode53
> Line 53: // Get the value from its location
> Passive tense -- "Gets".

Done.

> http://codereview.chromium.org/60009/diff/1/6#newcode68
> Line 68: #define ENTRY(x) table.settings[#x] = &values.x
> Same comment as earlier -- seems if SettingsTable and EnvVarValues were
> consolidated, there wouldn't be the need for this pointing.

There wouldn't be pointing, but there would still be the copying
instead: translating from setting variable name to some field with the
value. If the settings weren't pointers but values directly, then I
think the macro would just lose the &.

Variants might be to do the translation at Get time, or perhaps have a
constant settings table with 0-based pointers into the values struct.
But it'd be similarly cumbersome either way and the macros would still
seem helpful.

Or am I missing something?

> http://codereview.chromium.org/60009/diff/1/6#newcode96
> Line 96: SettingsTable<const char*> stringsTable;
> name_variables_like_this.

Ah yes, sorry, I knew that... 

> Also, move the member data declarations after the methods.

Right.

> Lastly, can these be private?

Yes, except the "values" field that I meant intentionally public so I
can conveniently assign to it in setting up a test.
Fixed.

> http://codereview.chromium.org/60009/diff/1/6#newcode174
> Line 174: // From proxy_config_service_win_unittest.cc
> Can you extract these three helper functions rather than duplicate?
>
> Perhaps create a file |proxy_config_service_unittest.h|, and just
> include them inline there.

OK done. I went the whole .cc way, so someone else won't complain that
I have lots of code in a header file and a for loop in an inline
function.

> http://codereview.chromium.org/60009/diff/1/6#newcode205
> Line 205: new MockEnvironmentVariableGetter;
> indent continuations by 4.

Done. (glaptop again... ahh.)

> http://codereview.chromium.org/60009/diff/1/6#newcode440
> Line 440: "*.google.com\n",             // proxy_bypass_list
> minor nit: consider using consitent indentation for these (some but not
> all are aligned).

Prettified.

> http://codereview.chromium.org/60009/diff/1/6#newcode456
> Line 456: for (BypassList::const_iterator it =
> config.proxy_bypass.begin();
> Since this is also common with proxy_config_service_win_unittest.cc, it
> could also be one of the functions extracted to hypothetical file
> |proxy_config_service_unittest.h|.

Done.

> http://codereview.chromium.org/60009/diff/1/6#newcode467
> Line 467: TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
> You may want to throw in a test for the upper-case/lower-case
> $VARIABLE_NAME fallback scheme.
>
> If it requires too much setup work, don't worry about it.

Yeah, it's awkward: it would mean adding some fields with different
casing, or flipping the case in the getter method, which feels too
much like replicating the code I'm trying to test.

> http://codereview.chromium.org/60009/diff/1/6#newcode469
> Line 469: new MockEnvironmentVariableGetter;
> indent by 4.

Done.

> http://codereview.chromium.org/60009/diff/1/6#newcode727
> Line 727: typedef std::vector<std::string> BypassList;
> Same comment as above -- extract this to its own function (shared with
> proxy_config_service_win_unittest.cc).

Done.

> http://codereview.chromium.org/60009/diff/1/2
> File net/proxy/proxy_service.cc (right):
>
> http://codereview.chromium.org/60009/diff/1/2#newcode570
> Line 570: std::string url_domain_and_port = url_domain + ":"
> Please split off the ProxyService::ShouldBypassProxyForURL() changes
> into a separate CL.
>
> I feel this will make for nicer changelog history, as this part isn't
> linux-specific.

Well it is linux-specific, in the sense that only Linux uses that
functionality. There's also the nastiness that the two CLs would have
proxy_service.cc in common: the instantiation of
ProxyConfigServiceLinux is in there too. Do you still want it split?
Which one would you want to see land first?

> http://codereview.chromium.org/60009/diff/1/2#newcode610
> Line 610: // proxy bypass for IP-specified hosts (e.g.  "10.0.0.0/8";
> see
> Just a heads up -- I noticed your unit-test uses this format too. The
> test is good, just pointing out that it won't actually work quite right
> yet.

The tests are currently not exercizing the missing CIDR matching
function.

> Yeah, proxy_config_service_win.h is wrong, please update its comment
> to end in a colon instead.
> Empirically looking through net/ we can see it is inconsistent (and
> only instance using a period):

Really? OK done.

I also made adjustments to the build files for the move to gyp.

Thanks

[eroman, 2009-04-10 08:41:26]

Excellent work!

LGTM after your review these comments.

> 2) When you get a test failure, there's practically no indication of
> which test failed.
> 
> I'm not sure what to do about this, short of "unrolling" the whole
> thing.
> 
> **Update: After asking and looking around, it seems one can simply put
> in a custom message with << on an EXPECT statement. So we could add a
> description string to each test and have that output on failure. I'll
> put it in as a TODO for now.

I reckon you could also solve this problem by using SCOPED_TRACE().

I haven't tried this code, but I am thinking something along these lines:

for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
   SCOPED_TRACE(StringPrintf("Running test i = %d", i));

   ...

   EXPECT_EQ(...)

   ...
}


Now when EXPECT_EQ() fails, it will additionally print out which iteration of
the loop we were on (and less annoying than having to add a message to every
single EXPECT_EQ).

For more info on SCOPED_TRACE check out: 

http://code.google.com/p/googletest/wiki/GoogleTestAdvancedGuide#Adding_Trace...

If you wanted to go overboard, it should also be possible to use a macro when
defining the tests[] data, such that it saves the __LINE number of the test
case, so it can be printed out on failure as well. (at which point you have
parity with an expanded test format).

> Well it is linux-specific, in the sense that only Linux uses that
> functionality. There's also the nastiness that the two CLs would have
> proxy_service.cc in common: the instantiation of
> ProxyConfigServiceLinux is in there too. Do you still want it split?
> Which one would you want to see land first?

Yes, please make this a separate CL (preferably *after* landing the linux
change).

I disagree that it is Linux specific, since the change does afterall affect all
the other platforms.

Feel free to ping me on IM if you want to talk about this further.

http://codereview.chromium.org/60009/diff/4001/5011
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/60009/diff/4001/5011#newcode429
Line 429: // TODO(sdoyon): confirm whether gconf truely needs this. If so,
not sure, but I think this is spelled "truly"

http://codereview.chromium.org/60009/diff/4001/5006
File net/net.gyp (right):

http://codereview.chromium.org/60009/diff/4001/5006#newcode313
Line 313: '../build/linux/system.gyp:gdk',
minor nit: sort the dependencies

http://codereview.chromium.org/60009/diff/4001/5006#newcode434
Line 434: 'proxy/proxy_config_service_common_unittest.cc',
add the .h file here too

http://codereview.chromium.org/60009/diff/4001/5012
File net/proxy/proxy_config_service_common_unittest.cc (right):

http://codereview.chromium.org/60009/diff/4001/5012#newcode42
Line 42: // Join the proxy bypass list using "\n" to make it into a single
string.
Can you move this into the header file (we generally keep the function
documentations with the declarations).

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

http://codereview.chromium.org/60009/diff/4001/5002#newcode60
Line 60: static std::string FixupProxyHostScheme(ProxyServer::Scheme scheme,
nit: can probably move the file-static function helpers into anonymous block,
since both styles are doing the same thing.

http://codereview.chromium.org/60009/diff/4001/5002#newcode77
Line 77: "not supported";
nit: 4 spaces

http://codereview.chromium.org/60009/diff/4001/5002#newcode91
Line 91: // Obtains an environment variable's value. Parse a proxy server
Parse --> Parses.

http://codereview.chromium.org/60009/diff/4001/5002#newcode107
Line 107: << "environment variable " << variable;
nit: delete two whitespaces here, so the << line up.

http://codereview.chromium.org/60009/diff/4001/5002#newcode127
Line 127: return url_canon::CanonicalizeIPAddress(domain.c_str(), domain_comp,
minor nit: you can use domain.data() here, since we don't require a
NULL-termination. I seem to remember that calling c_str() can sometimes
re-allocate the buffer to add the terminator, but I could be crazy.

http://codereview.chromium.org/60009/diff/4001/5002#newcode184
Line 184: // Fill proxy config from environment variables. Returns true if
"Fill" --> "Fills"

http://codereview.chromium.org/60009/diff/4001/5002#newcode194
Line 194: // Defined and empty => autodetect
please curly brace.

http://codereview.chromium.org/60009/diff/4001/5002#newcode198
Line 198: config->pac_url = GURL(auto_proxy);
ditto.

http://codereview.chromium.org/60009/diff/4001/5002#newcode223
Line 223: config->proxy_rules.type =
ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME;
ditto.

http://codereview.chromium.org/60009/diff/4001/5002#newcode245
Line 245: return !no_proxy.empty();
curly braces.

http://codereview.chromium.org/60009/diff/4001/5002#newcode256
Line 256: : client_(NULL) {}
This probably can fit on the previous line. Same question as earlier about
whether "explicit" has an effect here.

http://codereview.chromium.org/60009/diff/4001/5002#newcode284
Line 284: CHECK(client_);
nit: for consistency, move this up 1 line (similar to GetBoolean)

http://codereview.chromium.org/60009/diff/4001/5002#newcode288
Line 288: if (NULL == value)
if (!value) for consistency

http://codereview.chromium.org/60009/diff/4001/5002#newcode303
Line 303: if (NULL == gconf_value) {
ditto.

http://codereview.chromium.org/60009/diff/4001/5002#newcode312
Line 312: *result = bool_value ? true : false;
how about
*result = static_cast<bool>(bool_value);

http://codereview.chromium.org/60009/diff/4001/5002#newcode349
Line 349: // (GError is NULL).
i think this would read better as "error is NULL" as GError is the type.

http://codereview.chromium.org/60009/diff/4001/5002#newcode352
Line 352: LOG(ERROR) << "ProxyConfigServiceLinux: error Getting gconf value for
"
"Getting" should probably be lowercased.

http://codereview.chromium.org/60009/diff/4001/5002#newcode367
Line 367: // Obtains host and port gconf settings and parses a proxy server
Generally these method descriptions (explaining how to use, what the
inputs/outputs are) should go beside the declaration (header file).

This applies in other places in this file.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/60009/diff/4001/5002#newcode368
Line 368: // specification from it and puts it in result. Returns true if the
"puts it in result" --> "puts it in result_server" or "puts it in the result".

http://codereview.chromium.org/60009/diff/4001/5002#newcode397
Line 397: // Fill proxy config from gconf. Returns true if settings were found
"Fill" --> "Fills".

http://codereview.chromium.org/60009/diff/4001/5002#newcode409
Line 409: return true;
curly brace.

http://codereview.chromium.org/60009/diff/4001/5002#newcode449
Line 449: // try socks
minor nit: end comments with period.

http://codereview.chromium.org/60009/diff/4001/5002#newcode466
Line 466: // protocol specific settings
minor nit: end comments with period.

http://codereview.chromium.org/60009/diff/4001/5002#newcode496
Line 496: // Now the bypass list
period.

http://codereview.chromium.org/60009/diff/4001/5002#newcode508
Line 508: : env_var_getter_(env_var_getter), gconf_getter_(gconf_getter) {
indent by 4.

http://codereview.chromium.org/60009/diff/4001/5002#newcode517
Line 517: // GNOME_DESKTOP_SESSION_ID being defined is a good infication that
"infication" --> "indication".

http://codereview.chromium.org/60009/diff/4001/5003
File net/proxy/proxy_config_service_linux.h (right):

http://codereview.chromium.org/60009/diff/4001/5003#newcode18
Line 18: // These are used to derive mocks for unittests.
This comment seems misplaced.

http://codereview.chromium.org/60009/diff/4001/5003#newcode27
Line 27: // Gets an environment variable's value and store it in
nit: "store" --> "stores".

http://codereview.chromium.org/60009/diff/4001/5003#newcode43
Line 43: virtual void enter() = 0;
Upper case "Enter".

http://codereview.chromium.org/60009/diff/4001/5003#newcode44
Line 44: virtual void leave() = 0;
Upper case "Leave".

http://codereview.chromium.org/60009/diff/4001/5003#newcode53
Line 53: // Gets a string type value from gconf and store it in result. Returns
nit: "store" --> "stores"

http://codereview.chromium.org/60009/diff/4001/5003#newcode66
Line 66: explicit ProxyConfigServiceLinux();
what does "explicit" do for 0-arg constructor?

http://codereview.chromium.org/60009/diff/4001/5003#newcode79
Line 79: ProxyServer* result);
for consistency with the definition, this should be named |result_server|.

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

http://codereview.chromium.org/60009/diff/4001/5005#newcode24
Line 24: // The strange capitalization is so that the field matchers the
"matchers" --> "matches"

http://codereview.chromium.org/60009/diff/4001/5005#newcode27
Line 27: *auto_proxy, *all_proxy,
indent continued lines by 4.

http://codereview.chromium.org/60009/diff/4001/5005#newcode43
Line 43: *http_host, *secure_host, *ftp_host, *socks_host;
ditto.

http://codereview.chromium.org/60009/diff/4001/5005#newcode149
Line 149: virtual void enter() {
nit: put it all on one line, as in:
virtual void enter() {}

http://codereview.chromium.org/60009/diff/4001/5005#newcode183
Line 183: };
unnecessary semicolon.

http://codereview.chromium.org/60009/diff/4001/5001
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/60009/diff/4001/5001#newcode210
Line 210: new ProxyResolverV8());
Actually, this is best left as CreateNull() for now.

We will rely exclusively on the CreateUsingV8Resolver() method for Linux.

This one is really just a fallback for when CreateUsingV8Resolver() cannot be
used. Since we have no fallback implementation on linux, it will just give a
dummy service.

Selecting ProxyResolverV8 here wouldn't work, since there is no corresponding
call to SetProxyScriptFetcher(). Moreover we wouldn't want it to work, since
there are threadsafety issues with using V8 pac resolver in single-process mode
and in test_shell. (if you are interested, see:
http://chrome-svn/viewvc/chrome/trunk/src/chrome/browser/net/chrome_url_reque...)

http://codereview.chromium.org/60009/diff/4001/5001#newcode594
Line 594: size_t colon = bypass_url_domain.rfind(":");
This seems like it will be problematic for IPv6 literals, which use colons
inside of [].

http://codereview.chromium.org/60009/diff/4001/5001#newcode598
Line 598: std::string::const_iterator port = bypass_url_domain.begin() + colon;
I agree with your general idea in issue 9835 of moving the parsing of bypass
list earlier. That model would then also lend itself to this change more
elegantly, since the scanning for the port in proxy bypass could be done once
rather than on each ProxyResolve.

(Note I am not suggesting you make that change here and now; just thinking out
loud).

http://codereview.chromium.org/60009/diff/4001/5004
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/60009/diff/4001/5004#newcode795
Line 795: new MockProxyResolver);
one more space.

http://codereview.chromium.org/60009/diff/4001/5004#newcode806
Line 806: new MockProxyResolver);
one more space.

[sdoyon, 2009-04-14 15:11:35]

On Fri, 10 Apr 2009, eroman@chromium.org wrote:

>> 2) When you get a test failure, there's practically no indication of
>> which test failed.
>
>> I'm not sure what to do about this, short of "unrolling" the whole
>> thing.
>
>> **Update: After asking and looking around, it seems one can simply put
>> in a custom message with << on an EXPECT statement. So we could add a
>> description string to each test and have that output on failure. I'll
>> put it in as a TODO for now.
>
> I reckon you could also solve this problem by using SCOPED_TRACE().
>
> I haven't tried this code, but I am thinking something along these
> lines:
>
> for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
>  SCOPED_TRACE(StringPrintf("Running test i = %d", i));

Ah that is pretty neat.

> If you wanted to go overboard, it should also be possible to use a macro
> when defining the tests[] data, such that it saves the __LINE number of
> the test case, so it can be printed out on failure as well. (at which
> point you have parity with an expanded test format).

Alright, let's go overboard and do all that. Turned the comments at
the head of each test into a macro call that captures that short
description and the line number, and added it as a SCOPED_TRACE with
the iteration number.

Actually for this to be ultimate, we'd want a ToString() function on
proxy rules, because now when == returns false you still get to break
out the debugger or add messages to figure out in what way the rules
don't match. But enough for this time, this is good enough.

>> Well it is linux-specific, in the sense that only Linux uses that
>> functionality. There's also the nastiness that the two CLs would have
>> proxy_service.cc in common: the instantiation of
>> ProxyConfigServiceLinux is in there too. Do you still want it split?
>> Which one would you want to see land first?
>
> Yes, please make this a separate CL (preferably *after* landing the
> linux change).
>
> I disagree that it is Linux specific, since the change does afterall
> affect all the other platforms.

OK then.
http://codereview.chromium.org/73038

> http://codereview.chromium.org/60009/diff/4001/5011
> File chrome/app/chrome_dll_main.cc (right):
>
> http://codereview.chromium.org/60009/diff/4001/5011#newcode429
> Line 429: // TODO(sdoyon): confirm whether gconf truely needs this. If
> so,
> not sure, but I think this is spelled "truly"

Quite.

> http://codereview.chromium.org/60009/diff/4001/5006
> File net/net.gyp (right):
>
> http://codereview.chromium.org/60009/diff/4001/5006#newcode313
> Line 313: '../build/linux/system.gyp:gdk',
> minor nit: sort the dependencies

They weren't sorted before... and the order of libs in a link command
sometimes matter... OK sorting works fine, done.

> http://codereview.chromium.org/60009/diff/4001/5006#newcode434
> Line 434: 'proxy/proxy_config_service_common_unittest.cc',
> add the .h file here too

Done.

> http://codereview.chromium.org/60009/diff/4001/5012
> File net/proxy/proxy_config_service_common_unittest.cc (right):
>
> http://codereview.chromium.org/60009/diff/4001/5012#newcode42
> Line 42: // Join the proxy bypass list using "\n" to make it into a
> single string.
> Can you move this into the header file (we generally keep the function
> documentations with the declarations).

Done.

> http://codereview.chromium.org/60009/diff/4001/5002
> File net/proxy/proxy_config_service_linux.cc (right):
>
> http://codereview.chromium.org/60009/diff/4001/5002#newcode60
> Line 60: static std::string FixupProxyHostScheme(ProxyServer::Scheme
> scheme,
> nit: can probably move the file-static function helpers into anonymous
> block, since both styles are doing the same thing.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode77
> Line 77: "not supported";
> nit: 4 spaces

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode91
> Line 91: // Obtains an environment variable's value. Parse a proxy
> server
> Parse --> Parses.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode107
> Line 107: << "environment variable " << variable;
> nit: delete two whitespaces here, so the << line up.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode127
> Line 127: return url_canon::CanonicalizeIPAddress(domain.c_str(),
> domain_comp,
> minor nit: you can use domain.data() here, since we don't require a
> NULL-termination. I seem to remember that calling c_str() can sometimes
> re-allocate the buffer to add the terminator, but I could be crazy.

Uh let's keep it the same as in googleurl, if you don't mind.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode184
> Line 184: // Fill proxy config from environment variables. Returns true
> if
> "Fill" --> "Fills"

I really have a hard time with this don't I :-).

> http://codereview.chromium.org/60009/diff/4001/5002#newcode194
> Line 194: // Defined and empty => autodetect
> please curly brace.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode198
> Line 198: config->pac_url = GURL(auto_proxy);
> ditto.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode223
> Line 223: config->proxy_rules.type =
> ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME;
> ditto.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode245
> Line 245: return !no_proxy.empty();
> curly braces.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode256
> Line 256: : client_(NULL) {}
> This probably can fit on the previous line. Same question as earlier
> about whether "explicit" has an effect here.

Earlier? Oh! Rietveld doesn't build the message in the order you write
the comments... interesting.

Err the explicit is a left-over that I wasn't careful enough to remove
when I modified the constructor. Removed.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode284
> Line 284: CHECK(client_);
> nit: for consistency, move this up 1 line (similar to GetBoolean)

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode288
> Line 288: if (NULL == value)
> if (!value) for consistency

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode303
> Line 303: if (NULL == gconf_value) {
> ditto.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode312
> Line 312: *result = bool_value ? true : false;
> how about
> *result = static_cast<bool>(bool_value);

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode349
> Line 349: // (GError is NULL).
> i think this would read better as "error is NULL" as GError is the type.

Indeed.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode352
> Line 352: LOG(ERROR) << "ProxyConfigServiceLinux: error Getting gconf
> value for "
> "Getting" should probably be lowercased.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode367
> Line 367: // Obtains host and port gconf settings and parses a proxy
> server
> Generally these method descriptions (explaining how to use, what the
> inputs/outputs are) should go beside the declaration (header file).
>
> This applies in other places in this file.

It's never been clear to me why private methods should be documented
along the public interface of a class... Anyway, I moved the comments.

>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
>
> http://codereview.chromium.org/60009/diff/4001/5002#newcode368
> Line 368: // specification from it and puts it in result. Returns true
> if the
> "puts it in result" --> "puts it in result_server" or "puts it in the
> result".

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode397
> Line 397: // Fill proxy config from gconf. Returns true if settings were
> found
> "Fill" --> "Fills".

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode409
> Line 409: return true;
> curly brace.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode449
> Line 449: // try socks
> minor nit: end comments with period.

Right.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode466
> Line 466: // protocol specific settings
> minor nit: end comments with period.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode496
> Line 496: // Now the bypass list
> period.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode508
> Line 508: : env_var_getter_(env_var_getter), gconf_getter_(gconf_getter)
> {
> indent by 4.

Done.

> http://codereview.chromium.org/60009/diff/4001/5002#newcode517
> Line 517: // GNOME_DESKTOP_SESSION_ID being defined is a good infication
> that
> "infication" --> "indication".

Waw...

[sdoyon, 2009-04-14 15:12:32]

On Fri, 10 Apr 2009, eroman@chromium.org wrote:

> http://codereview.chromium.org/60009/diff/4001/5003
> File net/proxy/proxy_config_service_linux.h (right):
>
> http://codereview.chromium.org/60009/diff/4001/5003#newcode18
> Line 18: // These are used to derive mocks for unittests.
> This comment seems misplaced.

Sorry about that!

> http://codereview.chromium.org/60009/diff/4001/5003#newcode27
> Line 27: // Gets an environment variable's value and store it in
> nit: "store" --> "stores".

Right.

> http://codereview.chromium.org/60009/diff/4001/5003#newcode43
> Line 43: virtual void enter() = 0;
> Upper case "Enter".

Oh! Fixed.

> http://codereview.chromium.org/60009/diff/4001/5003#newcode44
> Line 44: virtual void leave() = 0;
> Upper case "Leave".

Sure.

> http://codereview.chromium.org/60009/diff/4001/5003#newcode53
> Line 53: // Gets a string type value from gconf and store it in result.
> Returns
> nit: "store" --> "stores"

Done.

> http://codereview.chromium.org/60009/diff/4001/5003#newcode66
> Line 66: explicit ProxyConfigServiceLinux();
> what does "explicit" do for 0-arg constructor?

Removed.

> http://codereview.chromium.org/60009/diff/4001/5003#newcode79
> Line 79: ProxyServer* result);
> for consistency with the definition, this should be named
> |result_server|.

Done.

> http://codereview.chromium.org/60009/diff/4001/5005
> File net/proxy/proxy_config_service_linux_unittest.cc (right):
>
> http://codereview.chromium.org/60009/diff/4001/5005#newcode24
> Line 24: // The strange capitalization is so that the field matchers the
> "matchers" --> "matches"

Done.

> http://codereview.chromium.org/60009/diff/4001/5005#newcode27
> Line 27: *auto_proxy, *all_proxy,
> indent continued lines by 4.

Done.

> http://codereview.chromium.org/60009/diff/4001/5005#newcode43
> Line 43: *http_host, *secure_host, *ftp_host, *socks_host;
> ditto.

Done

> http://codereview.chromium.org/60009/diff/4001/5005#newcode149
> Line 149: virtual void enter() {
> nit: put it all on one line, as in:
> virtual void enter() {}

Done.

> http://codereview.chromium.org/60009/diff/4001/5005#newcode183
> Line 183: };
> unnecessary semicolon.

Right.

> http://codereview.chromium.org/60009/diff/4001/5001
> File net/proxy/proxy_service.cc (right):
>
> http://codereview.chromium.org/60009/diff/4001/5001#newcode210
> Line 210: new ProxyResolverV8());
> Actually, this is best left as CreateNull() for now.
>
> We will rely exclusively on the CreateUsingV8Resolver() method for
> Linux.
>
> This one is really just a fallback for when CreateUsingV8Resolver()
> cannot be used. Since we have no fallback implementation on linux, it
> will just give a dummy service.
>
> Selecting ProxyResolverV8 here wouldn't work, since there is no
> corresponding call to SetProxyScriptFetcher(). Moreover we wouldn't want
> it to work, since there are threadsafety issues with using V8 pac
> resolver in single-process mode and in test_shell. (if you are
> interested, see:
>
http://chrome-svn/viewvc/chrome/trunk/src/chrome/browser/net/chrome_url_reque...)

Ah... Thanks, that clarifies things a good deal.

> http://codereview.chromium.org/60009/diff/4001/5001#newcode594
> Line 594: size_t colon = bypass_url_domain.rfind(":");
> This seems like it will be problematic for IPv6 literals, which use
> colons inside of [].

Right. Actually, I guess an IPv6 litteral without port might appear in
this list without the [] around it, which is even more confusing. OK
I've put in an interim fix, but that ought to be straightened out with
the other TODO in that function.

> http://codereview.chromium.org/60009/diff/4001/5001#newcode598
> Line 598: std::string::const_iterator port = bypass_url_domain.begin() +
> colon;
> I agree with your general idea in issue 9835 of moving the parsing of
> bypass list earlier. That model would then also lend itself to this
> change more elegantly, since the scanning for the port in proxy bypass
> could be done once rather than on each ProxyResolve.

Exactly.

> http://codereview.chromium.org/60009/diff/4001/5004
> File net/proxy/proxy_service_unittest.cc (right):
>
> http://codereview.chromium.org/60009/diff/4001/5004#newcode795
> Line 795: new MockProxyResolver);
> one more space.

Actually, I've added a unit test with IPv6 litterals. And twice I
wasted some minutes debugging because I'd gotten one of the numbered
variables wrong, so in frustration I redid the function with scopes.

[eroman, 2009-04-14 20:04:55]

LGTM.


> Actually for this to be ultimate, we'd want a ToString()
> function on proxy rules, because now when == returns false
> you still get to break out the debugger or add messages to
> figure out in what way the rules don't match. But enough
> for this time, this is good enough.

Agreed this is good enough as is.

Just as an FYI, I have added an operator<< overload for ProxyRules into
proxy_config.cc. All it would take now to get EXPECT_EQ(ProxyRules, ProxyRules)
to work now is to expose its function prototype in proxy_config.h. (I only
exposed the ProxyConfig overload).

[sdoyon, 2009-04-16 14:52:05]

On Tue, 14 Apr 2009, eroman@chromium.org wrote:

>> Actually for this to be ultimate, we'd want a ToString()
>> function on proxy rules, because now when == returns false
>> you still get to break out the debugger or add messages to
>> figure out in what way the rules don't match. But enough
>> for this time, this is good enough.
>
> Agreed this is good enough as is.
>
> Just as an FYI, I have added an operator<< overload for ProxyRules into
> proxy_config.cc. All it would take now to get EXPECT_EQ(ProxyRules,
> ProxyRules) to work now is to expose its function prototype in
> proxy_config.h. (I only exposed the ProxyConfig overload).

Oh alright :-). I exposed operator<< for ProxyRules and changed the
test to EXPECT_EQ. Plenty of context on test failure now.

Holding the commit because of developments on the gconf thread-safety
front.

This change introduces a new build dependency on Linux. I've updated
the wiki, and also the build slaves (thanks to maruel@). I will send a
heads-up on chromium0dev before committing.

[eroman, 2009-04-16 19:38:22]

still LGTM.

> Holding the commit because of developments on the gconf 
> thread-safety front.

I recommend you check this in now.
Any gconf thread safety changes can be tackled as a subsequent iteration. That
will make the reviewing easier, and get your unit tests running sooner (so ppl
don't break your work).

http://codereview.chromium.org/60009/diff/7001/4036
File net/proxy/proxy_config.cc (right):

http://codereview.chromium.org/60009/diff/7001/4036#newcode96
Line 96: // Helper to stringize a ProxyRules.
delete, as the documentation is now in header.

http://codereview.chromium.org/60009/diff/7001/4037
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/60009/diff/7001/4037#newcode114
Line 114: // Dumps a human-readable string representation of the proxy_rules to
|out|;
"the proxy_rules" --> either "the proxy rules", "|rules|", "the ProxyRules".

http://codereview.chromium.org/60009/diff/7001/4031
File net/proxy/proxy_config_service_win_unittest.cc (right):

http://codereview.chromium.org/60009/diff/7001/4031#newcode172
Line 172: EXPECT_TRUE(tests[i].proxy_rules == config.proxy_rules);
would be super to update this too.

[sdoyon, 2009-04-17 14:22:04]

On Thu, 16 Apr 2009, eroman@chromium.org wrote:

> still LGTM.
>
>> Holding the commit because of developments on the gconf
>> thread-safety front.
>
> I recommend you check this in now.
> Any gconf thread safety changes can be tackled as a subsequent
> iteration. That will make the reviewing easier, and get your unit tests
> running sooner (so ppl don't break your work).

OK then. I'll send a heads-up for the new build dependency.

> http://codereview.chromium.org/60009/diff/7001/4036
> File net/proxy/proxy_config.cc (right):
>
> http://codereview.chromium.org/60009/diff/7001/4036#newcode96
> Line 96: // Helper to stringize a ProxyRules.
> delete, as the documentation is now in header.

Done.

> http://codereview.chromium.org/60009/diff/7001/4037
> File net/proxy/proxy_config.h (right):
>
> http://codereview.chromium.org/60009/diff/7001/4037#newcode114
> Line 114: // Dumps a human-readable string representation of the
> proxy_rules to |out|;
> "the proxy_rules" --> either "the proxy rules", "|rules|", "the
> ProxyRules".

Done.

> http://codereview.chromium.org/60009/diff/7001/4031
> File net/proxy/proxy_config_service_win_unittest.cc (right):
>
> http://codereview.chromium.org/60009/diff/7001/4031#newcode172
> Line 172: EXPECT_TRUE(tests[i].proxy_rules == config.proxy_rules);
> would be super to update this too.

Oh sure.