Adds support for the DHCP portion of the WPAD (proxy auto-discovery) protocol.

Adds support for the DHCP portion of the WPAD (proxy auto-discovery) protocol.

This is Windows-only for now, and is disabled by default.  Start
Chrome with the flag --enable-dhcp-wpad to enable the feature.  See
discussion in comment on DhcpProxyScriptFetcherFactory for why this
needs to be done in a per-platform way rather than cross-platform.
The code is factored so that adding other platform implementations
will be straight forward.

Most of the implementation is stand-alone and extends the
ScriptProxyFetcher class hierarchy (and makes its interface slightly
more generic).  The integration point into existing code is in
InitProxyResolver, which previously handled fallback from DNS
auto-detect to custom PAC URL and now does fallback from DHCP to DNS
to custom PAC URL.

BUG=18575
TEST=net_unittests has good coverage for the new and changed code, but
manual tests on a network with a PAC URL configured in DHCP are also
needed.

Original commit r85646.
Reverted (test failures on some release bots) r85648.
Will reland with fix.

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

[Jói, 2011-04-14 20:23:53]

Hi Eric,

This is a bit big; apologies but there weren't really good milestones along the
way (and if I had sent it to you at what I thought for a bit was the most
appropriate milestone, you would have ended up reviewing a much larger delta).

A couple of caveats:

a) Look for my TODO(joi) comment in init_proxy_resolver.cc, there was existing
code there that I didn't quite understand.

b) I am still looking for a physical network to test this on, and I may end up
having to set up a couple of KVM instances with a private virtual bridge
network.  The code as it stands sends the exact same DHCP query as IE does, so
I'm pretty sure it's correct, but I have not yet verified the code that actually
retrieves the output from the Win32 DHCP API's data structures.

Cheers,
Jói

[Jói, 2011-04-14 20:25:44]

One more thing:  I haven't added NetLog logging to the DHCP fetcher classes yet.
 I wanted your opinion on whether this would be desired; I thought perhaps not
since ProxyScriptFetcherImpl does not use NetLog, but I would be happy to add it
if you think it is appropriate.

Cheers,
Jói


On 2011/04/14 20:23:53, Jói wrote:
> Hi Eric,
> 
> This is a bit big; apologies but there weren't really good milestones along
the
> way (and if I had sent it to you at what I thought for a bit was the most
> appropriate milestone, you would have ended up reviewing a much larger delta).
> 
> A couple of caveats:
> 
> a) Look for my TODO(joi) comment in init_proxy_resolver.cc, there was existing
> code there that I didn't quite understand.
> 
> b) I am still looking for a physical network to test this on, and I may end up
> having to set up a couple of KVM instances with a private virtual bridge
> network.  The code as it stands sends the exact same DHCP query as IE does, so
> I'm pretty sure it's correct, but I have not yet verified the code that
actually
> retrieves the output from the Win32 DHCP API's data structures.
> 
> Cheers,
> Jói

[eroman, 2011-04-14 20:54:31]

Thanks Joi!

I am a little backlogged on codereviews, but I will try to get at least some
initial comments to you by the end of day.

[Jói, 2011-04-14 20:59:37]

Thanks a lot Eric.  I'll mention two things, since you are busy:

a) If you would like me to ask somebody else to review, I would be
absolutely fine with that, but I figured you might want the review
since the proxy stuff is your baby.

b) I still have some more manual testing to do on this and after that,
I'm going to switch to some unrelated work, so there is no huge
urgency to complete the review.  Much of the code is new, changes are
primarily touching InitProxyResolver, so I don't think it will be hard
to keep this change alive without major merge conflicts for a few days
at least.

Cheers,
Jói


On Thu, Apr 14, 2011 at 4:54 PM,  <eroman@chromium.org> wrote:
> Thanks Joi!
>
> I am a little backlogged on codereviews, but I will try to get at least some
> initial comments to you by the end of day.
>
> http://codereview.chromium.org/6831025/
>

[eroman, 2011-04-14 21:19:30]

> a) If you would like me to ask somebody else to review, I 
> would be absolutely fine with that, but I figured you might
> want the review since the proxy stuff is your baby.

No worries, I am the best person to reviews this :)

I just want to set your expectations accordingly, that it might take me a day or
2 to get through this.

http://codereview.chromium.org/6831025/diff/1/net/proxy/init_proxy_resolver.cc
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/6831025/diff/1/net/proxy/init_proxy_resolver.c...
net/proxy/init_proxy_resolver.cc:271: // TODO(joi): Not sure this is and the
next statement are correct; seems
I can give you some quick feedback on what this is all about.

Ultimately InitProxyResolver initializes the ProxyResolver. It does this by
trying a number of PAC options, in a particular order, until identifying one
that works.

For diagnostic purposes, I have the InitProxyResolver tell the caller what the
final PAC URL that it ended up using was.

You can see this on the about:net-internals page:

http://i.imgur.com/D88Dj.png

In particular, look at the "Original settings" column, versus the "Effective
settings".

The "Original settings" shows the input that was passed into InitProxyResolver.
Whereas the "Effective settings" is displaying the URL that we were successful
in configuring against, which in this case is http://wpad/wpad.dat (the result
of our "auto detect"). It could have been possible that "auto detect" failed and
we instead used the custom PAC URL, in which case this panel would have shown
"http://my-custom-script/sup.pac" for the effective settings.

So now what DHCP is also being tested, we should plumb through the particular
PAC URL that we determined via DHCP, if that is what we ended up using.

So in other words, the "CreateFromCustomPacURL(GURL(kWpadUrl)) is no longer
correct in the case of auto-detect via DHCP. (It was valid before since that was
the full extent of our autodetect test).

Regarding the "expects_pac_bytes()" check: that is a bit of a hack to accomodate
our ProxyResolverMac/ProxyResolverWin implementations. In case we used one of
those guys, we can't be sure that a successful attempt using pac_source of
"autodetect" actually got the script from http://wpad/wpad.dat, since they could
be doing DHCP wpad. So in that case I just left the result URL unresolved, which
isn't particularly useful for diagnostics :)   (However that isn't the normal
configuration to be using in Chrome anyway).

Hope that makes sense, if not we can chat some more.

[Jói, 2011-04-14 23:37:02]

> I just want to set your expectations accordingly, that it might take me a
> day or
> 2 to get through this.

Thanks, that's absolutely fine.

> For diagnostic purposes, I have the InitProxyResolver tell the caller
> what the final PAC URL that it ended up using was.

Got it.  I will add the necessary plumbing to get the URL in addition
to the PAC text when DHCP auto-detect is successful.

> Regarding the "expects_pac_bytes()" check: that is a bit of a hack to
> accomodate our ProxyResolverMac/ProxyResolverWin implementations. In
> case we used one of those guys, we can't be sure that a successful
> attempt using pac_source of "autodetect" actually got the script from
> http://wpad/wpad.dat, since they could be doing DHCP wpad. So in that
> case I just left the result URL unresolved, which isn't particularly
> useful for diagnostics :)   (However that isn't the normal configuration
> to be using in Chrome anyway).

OK, understood.

Cheers,
Jói


On Thu, Apr 14, 2011 at 5:19 PM,  <eroman@chromium.org> wrote:
>> a) If you would like me to ask somebody else to review, I
>> would be absolutely fine with that, but I figured you might
>> want the review since the proxy stuff is your baby.
>
> No worries, I am the best person to reviews this :)
>
> I just want to set your expectations accordingly, that it might take me a
> day or
> 2 to get through this.
>
>
> http://codereview.chromium.org/6831025/diff/1/net/proxy/init_proxy_resolver.cc
> File net/proxy/init_proxy_resolver.cc (right):
>
>
http://codereview.chromium.org/6831025/diff/1/net/proxy/init_proxy_resolver.c...
> net/proxy/init_proxy_resolver.cc:271: // TODO(joi): Not sure this is and
> the next statement are correct; seems
> I can give you some quick feedback on what this is all about.
>
> Ultimately InitProxyResolver initializes the ProxyResolver. It does this
> by trying a number of PAC options, in a particular order, until
> identifying one that works.
>
> For diagnostic purposes, I have the InitProxyResolver tell the caller
> what the final PAC URL that it ended up using was.
>
> You can see this on the about:net-internals page:
>
> http://i.imgur.com/D88Dj.png
>
> In particular, look at the "Original settings" column, versus the
> "Effective settings".
>
> The "Original settings" shows the input that was passed into
> InitProxyResolver. Whereas the "Effective settings" is displaying the
> URL that we were successful in configuring against, which in this case
> is http://wpad/wpad.dat (the result of our "auto detect"). It could have
> been possible that "auto detect" failed and we instead used the custom
> PAC URL, in which case this panel would have shown
> "http://my-custom-script/sup.pac" for the effective settings.
>
> So now what DHCP is also being tested, we should plumb through the
> particular PAC URL that we determined via DHCP, if that is what we ended
> up using.
>
> So in other words, the "CreateFromCustomPacURL(GURL(kWpadUrl)) is no
> longer correct in the case of auto-detect via DHCP. (It was valid before
> since that was the full extent of our autodetect test).
>
> Regarding the "expects_pac_bytes()" check: that is a bit of a hack to
> accomodate our ProxyResolverMac/ProxyResolverWin implementations. In
> case we used one of those guys, we can't be sure that a successful
> attempt using pac_source of "autodetect" actually got the script from
> http://wpad/wpad.dat, since they could be doing DHCP wpad. So in that
> case I just left the result URL unresolved, which isn't particularly
> useful for diagnostics :)   (However that isn't the normal configuration
> to be using in Chrome anyway).
>
> Hope that makes sense, if not we can chat some more.
>
> http://codereview.chromium.org/6831025/
>

[Jói, 2011-04-15 18:57:18]

FYI, I have uploaded a version that sets the effective proxy configuration
correctly, and I added test code to init_proxy_config_unittest.cc that checks
each of the different cases for the effective proxy config.

Cheers,
Jói

[Jói, 2011-04-19 19:12:38]

Hi Eric,

Manual testing revealed that the Win32 DHCP API can take an extremely long time
(like 20-30s) to complete in certain edge-case network configurations.  I've
uploaded a new version that adds a timeout; this affects
net/proxy/dhcp_proxy_script_adapter_fetcher_win* files (other deltas from patch
set 4 are just from merging to yesterday's LKGR).

Cheers,
Jói


On 2011/04/15 18:57:18, Jói wrote:
> FYI, I have uploaded a version that sets the effective proxy configuration
> correctly, and I added test code to init_proxy_config_unittest.cc that checks
> each of the different cases for the effective proxy config.
> 
> Cheers,
> Jói

[eroman, 2011-04-21 05:22:48]

Thanks Joi, there is a lot of good stuff in here!

Let me start with a couple of high level comments:

(1) I would prefer having completely separate interfaces for
URLProxyScriptFetcher and DhcpProxyScriptFetcher (as opposed to generalizing
them to a base ProxyScriptFetcher).

Since we already have logic to switch the call modes in InitProxyResolver, there
isn't really an advantage to having a shared interface by the two (we don't pass
in implementations where that level of generality is useful).

On the other hand, having distinct interfaces means we can make the API more
specialized and appropriate to the use case. (I don't like the auxiliary
GetUrl() and SetUrl() methods).

I propose tweaking it to something more along these lines:

UrlProxyScriptFetcher {
  virtual int Fetch(const GURL& url, string16* script_data, CompletionCallback*
callback);
}

DhcpProxyScriptFetcher {
  virtual int Fetch(GURL* url, string16* script_data, CompletionCallback*
callback);
}

In the case of DhcpProxyScriptFetcher, we change the |url| field to being an
out-parameter rather than an in parameter.

(2) Throughout the network code, we use straight-up dependency injection for
testing, rather than subclassing implementation classes. I feel very strongly
about this pattern, and can discuss further the pros and cons :)

But for the purpose of this CL, I strongly encourage you to remove the various
virtuals, and instead pass those dependencies explicitly via the constructor (or
in some cases, setters are appropriate too).

For instance, in the case of InitProxyResolver, you should make
DhcpProxyScriptFetcher* a (non-owning) dependency that is passed in the
constructor. This makes it follow the same model as with ProxyScriptFetcher.
This does mean you will have to update more allsites (which can be annoying),
but it is better documenting as to what the dependencies being used actually
are, and forces the caller to make appropriate selections.

Similarly in the case of the "adapter fetcher", I would like for there to be a
UrlProxyScriptFetcher "factory" dependency which is explicit in the constructor,
rather than hiding it away in the virtual method (and constructing a fetcher
based on the context).

(3) The general design looks good to me --> you query the DHCP 252 option for
each adapter from a worker thread, and then fetch each resulting URL in
parallel, until you get success on something with a high enough score. (It would
be nice if we only had to check one adapter though, since then we could leave
the fetching logic as part of InitProxyResolver, but I understand why you have
combined them). 

Could you give this sort of high level description at the top of
dhcp_proxy_fetcher_win.h, so it is easier to see how the adapter fetcher /
urlproxyscriptfetcher / etc fit together?

(4) I propose getting rid of DhcpProxyScriptFetcherFactory and its singleton.
This goes hand-in-hand with the dependency injection comment; if we are passing
a DhcpProxyScriptFetcher* directly to the ProxyService, then we will end up
configuring the appropriate one at the time of construction (deep in the bowels
of the chrome url request context code). We do this for a number of other
network dependencies too, and end up testing chrome's command line flags at that
point.

http://codereview.chromium.org/6831025/diff/14001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/6831025/diff/14001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:403: const char kEnableDhcpPac[]               
 = "enable-dhcp-wpad";
nit: how about calling this kEnableDhcpWpad (to mirror the command line value).

http://codereview.chromium.org/6831025/diff/14001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/base/net_error_list.h#n...
net/base/net_error_list.h:447: // No PAC URL configuration could be retrieved
from DHCP.
nit: I suggest wording this a little more concretely.

"could not be retrieved from DHCP" is ambiguous as to whether this was a
failure, or whether we succeeded talking DHCP but there was no PAC URL being
advertised. I expect you are in fact lumping these two together, but could be
helpful for future readers to make the explicit.

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:122: //     "source": <String describing
where PAC script comes from>,
Cool, thanks for remembering to updated this!

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:127: //      "error_code": <Net error code
integer>,
While you are here updating stuff, could you fix this to read "net_error" ?

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:136: //      "error_code": <Net error code
integer>,
Ditto to above.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:23: const int kTimeoutMs =
2000;
What is the rationale for 2seconds?

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:48: DCHECK(state_ ==
STATE_START);
nit: DCHECK_EQ()

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:138: pac_url_ = GURL(url);
One possibility to consider is whether the URL string parsed to a valid URL.

It probably doesn't matter, since if you call script_fetcher_ with an invalid
URL it will probably fail anyway.

Shrug. If you wanted to check for this, there is an is_valid() or something on
GURL.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:177: new
ProxyScriptFetcherImpl(url_request_context_);
This isn't really desirable to be instantiating specific implementations of
URLProxyScriptFetcher here, whereas the other consumer (InitProxyResolver) is
passed in the dependency. It would be better to pass some URLProxyScriptFetcher
factory in as a dependency, so the caller can decide how to do the fetching.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:182: int
DhcpProxyScriptAdapterFetcher::ImplGetTimeoutMs() const {
I don't much care for the use of virtuals. I would prefer dependency injection
pattern. See my meta comment for alternatives.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:223: // chances on
non-standard, poorly documented usage.
Nice comment block!

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:235: NOTREACHED();
nit: I suggest adding return "" here, and removing the "else" below.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:238:
DCHECK(strlen(reinterpret_cast<const char*>(wpad_params.Data)) + 1 ==
nit: DCHECK_EQ()

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:12: #include
"net/proxy/proxy_script_fetcher_impl.h"
Is this header necessary? Could we just include proxy_script_fetcher.h (the
non-impl version), or forward-declare ProxyScriptFetcher

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:26: public base::NonThreadSafe
{
Feels a bit weird to call this nonthreadsafe, since it calls virtuals on the
worker thread too.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:31:
~DhcpProxyScriptAdapterFetcher();
I always recommend making destructors virtual any time you have virtual methods.
(otherwise it is easy to accidentally end up leaking stuff in your derived class
when deleting via a base pointer).

That said, see my meta-comment about avoiding the virtuals in the first place.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:34: // |adapter_name|. On
completion (but not cancellation), |callback|
what does it mean to take ownership of |adapter_name|? (It is a |const string&|)

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:56: virtual string16
pac_script() const;
Don't use hacker_notation() for virtuals. (I know some parts of chrome disobey
this... but they are wrong IMO)

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:63: virtual GURL pac_url()
const;
ditto.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:110: STATE_CANCEL,
nit: how about STATE_CANCELLED

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:6: #include
"base/perftimer.h"
nit: order header lexicographically.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:8: #include
"net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
nit: it isn't explicit in the style guide, but I assume it is consistent to put
the header of the file under test at the top (similar to what we do for .cc
files).

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:16: const char*
const kPacUrl = "http://pacserver/script.pac";
can this go into the anonymous namespace?

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:30: explicit
MockDhcpProxyScriptAdapterFetcher()
nit: omit explicit on 0-arg constructors.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:31: :
DhcpProxyScriptAdapterFetcher(new TestURLRequestContext()),
won't this leak? (I don't believe the adapter holds any references).

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:92: public:
nit: indent by 1.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:128:
EXPECT_EQ(string16(L""), client.fetcher_->pac_script());
nit: i believe you can leave of the string16() in cases like this and rely on
implicit constructor. Up to you.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:149:
client.fetcher_->dhcp_delay_ms_ = 20 * 1000;
One thing to be aware of is valgrind may end up reporting leaks for this sort of
test (since the worker thread may be keeping objects alive past the test).

One approach to mitigate that, is to set a lower timeout (which we are willing
to wait for), and then wait for it to complete before exiting the test.
Otherwise be prepared to add some valgrind suppressions :)

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:151: PerfTimer
timer;
What is the perf timer being used for?

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:214: public:
extra space.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher.cc:29: *utf16_text = string16();
no need to set anything (callers should not be checking the result of text on
error)

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_factory.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_factory.h:35: static
DhcpProxyScriptFetcherFactory* GetInstance();
We can avoid singletons here by passing this as a dependency to
InitProxyResolver, when it is constructed on the chrome side of things
(similarly, we can test the command line flags to decide whether it should be
enabled or not at construction time).

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:48: if
(!ImplGetCandidateAdapterNames(&adapter_names)) {
Is this a fast operation? (in other words, is it OK to be enumerating the
adapter names on the IO thread).

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:52: *utf16_text = L"";
no need to set this.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:141: wait_timer_.Start(
isn't there another timer already?

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:14: #include <list>
These go at the top.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:42: // Adds to |adapter_names| the
name of each network adapter on this machine
Rather than adds, what about "sets" ? (be sure to clear it at the start).
Otherwise this would be better name AppendCandiateAdapterNames()

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:15: #include <vector>
move up.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcpcsvc_init_win.cc
File net/proxy/dhcpcsvc_init_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcpcsvc_init_win...
net/proxy/dhcpcsvc_init_win.cc:24: // Worker pool threads that use the DHCP API
may still be running, so skip
Indeed :(

I hate worker threads.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:191: std::string source_field;
I suggest moving this code into a helper function.

Perhaps something like this:

NetLogParameter* CreateNetLogParameterForSource(const PacSource& pac_source) {
 <This code block>
}

And then just call that from the BeginEvent() below.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:219:
dhcp_proxy_script_fetcher_.reset(ImplCreateDhcpProxyScriptFetcher(
Can this instead be a dependency to InitProxyResolver?

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:223:
proxy_script_fetcher_->SetURL(effective_pac_url);
Why split off SetURL() as separate from Fetch() ?

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.h:70: virtual DhcpProxyScriptFetcher*
ImplCreateDhcpProxyScriptFetcher(
I don't like using virtuals in this manner, can you use the dependency injection
pattern instead, which this class is already using (for the
proxy_script_fetcher, resolver, and net_log).

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:70: if
(script_fetcher_.get()) {
nit: don't need this if, can just reset unconditionally.

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:67:
fetchers_.push_back(fetcher.get());
nit: why the .get() ?

[Jói, 2011-04-21 15:57:38]

Hi Eric,

Thanks a lot for doing this long review.  Let's discuss your high-level comments
first, before I dive into making changes.  If I do everything you suggested, I
think it's maybe a good day's worth of work or more for primarily stylistic
improvements on code that I don't expect us to touch much in the future.  I have
no problem making the effort to conform to the style that is preferred in net/
and I appreciate the education on it, I just want to check how strongly some of
the changes are required before spending the time :)

(1) Separate interfaces for URLProxyScriptFetcher and DhcpProxyScriptFetcher.

I started with separate interfaces, but combined them for two main reasons:

a) If it's a general-purpose interface for fetching PAC scripts, making the
top-level interface take the URL as a parameter limits its generality (whereas
taking the location to output to and a callback is pretty general).  I realize
after reading your comments that you probably introduced the interface only to
enable dependency injection, rather than as a way to start a class hierarchy,
which makes this less of a good point :)

b) We had discussed eventually doing the DHCP and DNS queries in parallel.  If
we do this, I had envisioned switching DhcpProxyScriptAdapterFetcher to also use
the ProxyScriptFetcher interface, and generalizing the logic that is currently
in WindowsDhcpProxyScriptFetcher that starts several fetches in parallel and
implements a timeout after the first fetch completes.  This "parallel fetcher"
would be used (a) for WindowsDhcpProxyScriptFetcher where it would be
initialized with one DhcpProxyScriptAdapterFetcher per adapter, and (b) for
something like a ParallelDnsDhcpProxyScriptFetcher initialized with one
URLProxyScriptFetcher and one DhcpProxyScriptAdapterFetcher (combining DNS and
DHCP in just one parallel fetcher would be a layering violation and would make
the logic around whether fetching PAC from DHCP is available and enabled on your
platform more tricky).  However, since I haven't switched the
DhcpProxyScriptAdapterFetcher to use the interface, and I'm not generalizing the
parallel fetch logic in the current change, this is not a very strong reason to
do the refactoring in the current change (doing it if/when I extract the
parallel fetch logic would be just as easy). But if we assume we do end up with
a parallel DNS/DHCP fetcher, you may or may not be swayed by this point.

Bottom line:  If the above arguments have not changed your mind, I have no
problem reverting back to separate interfaces for URL vs. DHCP-based fetchers.

(2) Use dependency injection instead of virtual methods to enable testability.

I'd like to propose a middle ground here:  That I switch to dependency injection
in InitProxyResolver to match what is already there, but keep my existing
approach for the new code.  This is both to save time re-working code that I
doubt will be touched much, but also because actually did consider dependency
injection for testability of these classes and rejected it in favor of the
virtual methods approach; I'll explain below.

I've done a lot of unit testing (the last project I led had something like 70%
unit test coverage), and I'm a big fan of dependency injection when the
production code actually needs to be able to instantiate a class with different
dependencies.  I'm not such a big fan of it when the production code will only
ever instantiate the class with exactly one version of these dependencies.  It
adds complexity to call sites, as you mentioned, and it slightly obfuscates the
production code (you read the class implementation, you notice the injection,
and you need to search all call sites or implementations of the injected
interfaces to see whether the production code actually uses multiple versions of
the dependencies or not).  So in this particular case, where in production code
I don't think it's likely there would ever be different dependencies injected, I
chose the virtual methods approach.

I'm happy to discuss this further if my proposed middle ground does not make you
happy.  It's not a religious thing for me, and I hope I don't come off as
confrontational - it's just that the different methods for adding testability
have pros and cons and I made what I think was a reasoned decision to go this
route, so I'd at least like to chat before we do something different.

I think you may have inspired me to do a blog post on the various methods for
introducing testability to contrast their pros and cons.  There are a couple of
other methods (one template-based, one preprocessor based) that are mostly of
interest to performance-critical code, and contrasting those two with the
virtual methods approach and the dependency injection approach could be
interesting.

(3) On general design and adding a description of it at the top of
dhcp_proxy_script_fetcher.win.

Definitely, will document it there.  The adapter fetcher started its life as a
sub-statemachine of WindowsDhcpProxyScriptFetcher but became big enough that it
deserved top-level existence; the documentation was perhaps clearer when it was
still an inner class, as then you could see the outer and inner state machines
documented side by side.

(4) Proposal to get rid of DhcpProxyScriptFetcherFactory and its singleton.

The singleton could go and the factory could be injected as a dependency
instead, or the class constructed by the factory could be injected as a
dependency, but I believe the factory itself should stay as it localizes the
logic about which factory to choose for your platform and configuration.  I
don't think it would make sense to explode that logic to each call-site which is
probably a class that has nothing to do with DHCP WPAD.

The reason I chose to use a factory singleton is so that Chrome can set the
configuration easily on the singleton, which again simplifies callers.  I still
feel it is simpler to leave it as is but if you feel strongly about it I don't
object to making it non-singleton.

I've responded to a few other higher-level comments of yours below.  I will
address all remaining comments in the next go-around after we finish discussing
the high level points.

Cheers,
Jói

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:23: const int kTimeoutMs =
2000;
On 2011/04/21 05:22:48, eroman wrote:
> What is the rationale for 2seconds?

This is mostly a guess.  DHCP should normally be very quick; it's (almost?)
always on the LAN, so roundtrip times are a couple of milliseconds or less and
there are something like two roundtrips for an initial query.  DHCP servers are
probably usually fairly fast.  I could make it longer but that would, in certain
unlikely edge-cases, increase the time we block on attempting lookup of the PAC
URL from DHCP before trying DNS.

If/when we switch to doing DHCP and DNS in parallel, this won't be so much of an
issue - we'll cancel the DHCP queries after some fairly short timeout if DNS
comes in first.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:177: new
ProxyScriptFetcherImpl(url_request_context_);
On 2011/04/21 05:22:48, eroman wrote:
> This isn't really desirable to be instantiating specific implementations of
> URLProxyScriptFetcher here, whereas the other consumer (InitProxyResolver) is
> passed in the dependency. It would be better to pass some
URLProxyScriptFetcher
> factory in as a dependency, so the caller can decide how to do the fetching.

I don't see the benefit of that if we are only trying to achieve testability,
since there is only the one URL-fetching implementation of ProxyScriptFetcher,
and that seems unlikely to change.  However, this is one of those things I will
change if in our discussion we conclude that dependency injection must be used
as the method of introducing testability.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:26: public base::NonThreadSafe
{
On 2011/04/21 05:22:48, eroman wrote:
> Feels a bit weird to call this nonthreadsafe, since it calls virtuals on the
> worker thread too.

The public interface is non-threadsafe, so I think it makes sense.  There is a
comment right above explaining this.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:34: // |adapter_name|. On
completion (but not cancellation), |callback|
On 2011/04/21 05:22:48, eroman wrote:
> what does it mean to take ownership of |adapter_name|? (It is a |const
string&|)

Sorry, will fix the comment, that is a holdover from when I was putting string*
instead of string into the set returned when enumerating adapters, to avoid a
string copy when passing the string to the worker thread (which happens within
Fetch().  I fixed that when I realized I needed a string conversion anyway.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:48: if
(!ImplGetCandidateAdapterNames(&adapter_names)) {
On 2011/04/21 05:22:48, eroman wrote:
> Is this a fast operation? (in other words, is it OK to be enumerating the
> adapter names on the IO thread).

I believe so.  It calls the GetAdaptersAddresses Win32 API, which I don't
believe does any I/O.  I did a quick search online and I can't find any
information about "slow" or "hangs" or such related to the function.  If you
want me to make sure, I could add a UMA timer around use of the function.

In Chrome code, it is already used on the UI thread, via
content/browser/renderer_host/p2p/socket_dispatcher_host.cc (which calls
net::GetNetworkList()).  So if you think there might be a problem it would need
to be fixed there too.

[cpu_(ooo_6.6-7.5), 2011-04-22 20:54:59]

drive-by

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:19: 
can this dhcp dll be delay-loaded?

[Jói, 2011-05-03 15:53:06]

Eric, this is just to document the decisions we made when we spoke yesterday:

a) I will split the ProxyScriptFetcher and DhcpProxyScriptFetcher
interfaces as you suggested. If/when we do the extraction of parallel
fetching logic that I suggested we might, then we can again merge
them, but for this change they will be separate.

b) I will switch to using dependency injection for unit testing
existing classes that were already using it.

c) I will look into making the DhcpProxyScriptFetcherFactory a
non-singleton but we will leave the factory to encapsulate the
creation logic.

You can expect an update hopefully later today.

Cheers,
Jói


On Fri, Apr 22, 2011 at 4:54 PM,  <cpu@chromium.org> wrote:
> drive-by
>
>
>
http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
> File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):
>
>
http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
> net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:19:
> can this dhcp dll be delay-loaded?
>
> http://codereview.chromium.org/6831025/
>

[Jói, 2011-05-03 21:20:59]

Please take another look.

Cheers,
Jói

http://codereview.chromium.org/6831025/diff/14001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/6831025/diff/14001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:403: const char kEnableDhcpPac[]               
 = "enable-dhcp-wpad";
On 2011/04/21 05:22:48, eroman wrote:
> nit: how about calling this kEnableDhcpWpad (to mirror the command line
value).

Done.

http://codereview.chromium.org/6831025/diff/14001/net/base/net_error_list.h
File net/base/net_error_list.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/base/net_error_list.h#n...
net/base/net_error_list.h:447: // No PAC URL configuration could be retrieved
from DHCP.
On 2011/04/21 05:22:48, eroman wrote:
> nit: I suggest wording this a little more concretely.
> 
> "could not be retrieved from DHCP" is ambiguous as to whether this was a
> failure, or whether we succeeded talking DHCP but there was no PAC URL being
> advertised. I expect you are in fact lumping these two together, but could be
> helpful for future readers to make the explicit.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
File net/base/net_log_event_type_list.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:122: //     "source": <String describing
where PAC script comes from>,
On 2011/04/21 05:22:48, eroman wrote:
> Cool, thanks for remembering to updated this!

np

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:127: //      "error_code": <Net error code
integer>,
On 2011/04/21 05:22:48, eroman wrote:
> While you are here updating stuff, could you fix this to read "net_error" ?

Done.

http://codereview.chromium.org/6831025/diff/14001/net/base/net_log_event_type...
net/base/net_log_event_type_list.h:136: //      "error_code": <Net error code
integer>,
On 2011/04/21 05:22:48, eroman wrote:
> Ditto to above.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:48: DCHECK(state_ ==
STATE_START);
On 2011/04/21 05:22:48, eroman wrote:
> nit: DCHECK_EQ()

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:138: pac_url_ = GURL(url);
On 2011/04/21 05:22:48, eroman wrote:
> One possibility to consider is whether the URL string parsed to a valid URL.
> 
> It probably doesn't matter, since if you call script_fetcher_ with an invalid
> URL it will probably fail anyway.
> 
> Shrug. If you wanted to check for this, there is an is_valid() or something on
> GURL.

I now have
  pac_url_ = GURL(url);
  if (pac_url_.is_empty() || !pac_url_.is_valid()) {
where previously there was
  if (url.empty())

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:182: int
DhcpProxyScriptAdapterFetcher::ImplGetTimeoutMs() const {
On 2011/04/21 05:22:48, eroman wrote:
> I don't much care for the use of virtuals. I would prefer dependency injection
> pattern. See my meta comment for alternatives.

As discussed, I changed InitProxyResolver and its tests and users to use
dependency injection for the DHCP WPAD stuff, but am leaving the DHCP classes as
is.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:223: // chances on
non-standard, poorly documented usage.
On 2011/04/21 05:22:48, eroman wrote:
> Nice comment block!

thx

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:235: NOTREACHED();
On 2011/04/21 05:22:48, eroman wrote:
> nit: I suggest adding return "" here, and removing the "else" below.

It's an else if; there can be NO_ERROR and wpad_params.nBytesData can be 0.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:238:
DCHECK(strlen(reinterpret_cast<const char*>(wpad_params.Data)) + 1 ==
On 2011/04/21 05:22:48, eroman wrote:
> nit: DCHECK_EQ()

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:12: #include
"net/proxy/proxy_script_fetcher_impl.h"
On 2011/04/21 05:22:48, eroman wrote:
> Is this header necessary? Could we just include proxy_script_fetcher.h (the
> non-impl version), or forward-declare ProxyScriptFetcher

Moved to .cc file.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:31:
~DhcpProxyScriptAdapterFetcher();
On 2011/04/21 05:22:48, eroman wrote:
> I always recommend making destructors virtual any time you have virtual
methods.
> (otherwise it is easy to accidentally end up leaking stuff in your derived
class
> when deleting via a base pointer).
> 
> That said, see my meta-comment about avoiding the virtuals in the first place.

Now virtual, thanks for catching that.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:56: virtual string16
pac_script() const;
On 2011/04/21 05:22:48, eroman wrote:
> Don't use hacker_notation() for virtuals. (I know some parts of chrome disobey
> this... but they are wrong IMO)

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:63: virtual GURL pac_url()
const;
On 2011/04/21 05:22:48, eroman wrote:
> ditto.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:110: STATE_CANCEL,
On 2011/04/21 05:22:48, eroman wrote:
> nit: how about STATE_CANCELLED

I considered that but then the others should change to STARTED, WAITING,
FINISHED and that feels wrong to me.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:6: #include
"base/perftimer.h"
On 2011/04/21 05:22:48, eroman wrote:
> nit: order header lexicographically.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:8: #include
"net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
On 2011/04/21 05:22:48, eroman wrote:
> nit: it isn't explicit in the style guide, but I assume it is consistent to
put
> the header of the file under test at the top (similar to what we do for .cc
> files).

Looking at _unittest.cc files under net/ it seems there may be a slight
preference for doing what you suggested, so I have switched to be consistent. 
Looking at all of the Chrome code base, the preference seems to be for what I
had before switching.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:16: const char*
const kPacUrl = "http://pacserver/script.pac";
On 2011/04/21 05:22:48, eroman wrote:
> can this go into the anonymous namespace?

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:30: explicit
MockDhcpProxyScriptAdapterFetcher()
On 2011/04/21 05:22:48, eroman wrote:
> nit: omit explicit on 0-arg constructors.

Thanks (it was 1-arg at some point).  It is now 1-arg again :)

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:31: :
DhcpProxyScriptAdapterFetcher(new TestURLRequestContext()),
On 2011/04/21 05:22:48, eroman wrote:
> won't this leak? (I don't believe the adapter holds any references).

You're right; I was thinking it held it via scoped_refptr but it doesn't. Fixed
the test code.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:92: public:
On 2011/04/21 05:22:48, eroman wrote:
> nit: indent by 1.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:128:
EXPECT_EQ(string16(L""), client.fetcher_->pac_script());
On 2011/04/21 05:22:48, eroman wrote:
> nit: i believe you can leave of the string16() in cases like this and rely on
> implicit constructor. Up to you.

Leaving as is.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:149:
client.fetcher_->dhcp_delay_ms_ = 20 * 1000;
On 2011/04/21 05:22:48, eroman wrote:
> One thing to be aware of is valgrind may end up reporting leaks for this sort
of
> test (since the worker thread may be keeping objects alive past the test).
> 
> One approach to mitigate that, is to set a lower timeout (which we are willing
> to wait for), and then wait for it to complete before exiting the test.
> Otherwise be prepared to add some valgrind suppressions :)

I added some granularity to the wait loop so that the test can stop the fake
worker thread soon when the test is done, and used that to exit as soon as
possible from the test after the thread is gone.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:151: PerfTimer
timer;
On 2011/04/21 05:22:48, eroman wrote:
> What is the perf timer being used for?

See the call to timer.Elapsed() within the ASSERT_GT a few lines further down.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:214: public:
On 2011/04/21 05:22:48, eroman wrote:
> extra space.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher.cc:29: *utf16_text = string16();
On 2011/04/21 05:22:48, eroman wrote:
> no need to set anything (callers should not be checking the result of text on
> error)

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_factory.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_factory.h:35: static
DhcpProxyScriptFetcherFactory* GetInstance();
On 2011/04/21 05:22:48, eroman wrote:
> We can avoid singletons here by passing this as a dependency to
> InitProxyResolver, when it is constructed on the chrome side of things
> (similarly, we can test the command line flags to decide whether it should be
> enabled or not at construction time).

I've changed this class to just have the static flag and static functions,
rather than being a singleton.  After looking at what it would take to provide
the configuration in URLRequestContext, I disagree that every callsite should
pass the flag.  I think it localizes responsibilities better to have flag
parsing outside of chrome/net, and receiving configuration via browser_main is
already done for many components of net in InitializeNetworkOptions().  So for
now, I'm leaving initialization of the "feature enabled" flag in
InitializeNetworkOptions.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:52: *utf16_text = L"";
On 2011/04/21 05:22:48, eroman wrote:
> no need to set this.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:141: wait_timer_.Start(
On 2011/04/21 05:22:48, eroman wrote:
> isn't there another timer already?

There is a separate timer in the AdapterFetcher.  This timer is from when the
first AdapterFetcher returns, if the timer fires we stop waiting for slower
AdapterFetcher instances.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:14: #include <list>
On 2011/04/21 05:22:48, eroman wrote:
> These go at the top.

I've moved them to the top.

I checked and this appears to be the most common way of doing it in the Chrome
codebase.  Just to rant though, I disagree with the approach as it makes
"include what you use" tougher (if you include after other headers, it makes it
harder for those headers to forget to include what they use).  I'm sure it's
been argued to death already though.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:42: // Adds to |adapter_names| the
name of each network adapter on this machine
On 2011/04/21 05:22:48, eroman wrote:
> Rather than adds, what about "sets" ? (be sure to clear it at the start).
> Otherwise this would be better name AppendCandiateAdapterNames()

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:15: #include <vector>
On 2011/04/21 05:22:48, eroman wrote:
> move up.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcpcsvc_init_win.cc
File net/proxy/dhcpcsvc_init_win.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcpcsvc_init_win...
net/proxy/dhcpcsvc_init_win.cc:24: // Worker pool threads that use the DHCP API
may still be running, so skip
On 2011/04/21 05:22:48, eroman wrote:
> Indeed :(
> 
> I hate worker threads.

Yep.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:191: std::string source_field;
On 2011/04/21 05:22:48, eroman wrote:
> I suggest moving this code into a helper function.
> 
> Perhaps something like this:
> 
> NetLogParameter* CreateNetLogParameterForSource(const PacSource& pac_source) {
>  <This code block>
> }
> 
> And then just call that from the BeginEvent() below.

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:219:
dhcp_proxy_script_fetcher_.reset(ImplCreateDhcpProxyScriptFetcher(
On 2011/04/21 05:22:48, eroman wrote:
> Can this instead be a dependency to InitProxyResolver?

Done.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:223:
proxy_script_fetcher_->SetURL(effective_pac_url);
On 2011/04/21 05:22:48, eroman wrote:
> Why split off SetURL() as separate from Fetch() ?

Reverted as discussed.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.h:70: virtual DhcpProxyScriptFetcher*
ImplCreateDhcpProxyScriptFetcher(
On 2011/04/21 05:22:48, eroman wrote:
> I don't like using virtuals in this manner, can you use the dependency
injection
> pattern instead, which this class is already using (for the
> proxy_script_fetcher, resolver, and net_log).

I've switched to using dependency injection in InitProxyResolver, as discussed.

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:19: 
On 2011/04/22 20:54:59, cpu wrote:
> can this dhcp dll be delay-loaded?

Done, please take a look to make sure I did it correctly (by adding to the list
in chrome/chrome_dll.gypi, which I sorted alphabetically while I was in there).

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:70: if
(script_fetcher_.get()) {
On 2011/04/21 05:22:48, eroman wrote:
> nit: don't need this if, can just reset unconditionally.

Done.

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/17001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:67:
fetchers_.push_back(fetcher.get());
On 2011/04/21 05:22:48, eroman wrote:
> nit: why the .get() ?

Done.

[Jói, 2011-05-05 14:58:42]

FYI - nothing important, just a couple of things I noticed today.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:8: #include
"net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
On 2011/04/21 05:22:48, eroman wrote:
> nit: it isn't explicit in the style guide, but I assume it is consistent to
put
> the header of the file under test at the top (similar to what we do for .cc
> files).

I was just looking at include order in the style guide again.  The Google style
actually does make this explicit, and the Chromium style guide refers to it.  So
thanks for pointing this out to me, I had been using the wrong approach for
quite a while.

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:14: #include <list>
On 2011/05/03 21:20:59, Jói wrote:
> On 2011/04/21 05:22:48, eroman wrote:
> > These go at the top.
> 
> I've moved them to the top.
> 
> I checked and this appears to be the most common way of doing it in the Chrome
> codebase.  Just to rant though, I disagree with the approach as it makes
> "include what you use" tougher (if you include after other headers, it makes
it
> harder for those headers to forget to include what they use).  I'm sure it's
> been argued to death already though.

Just FYI, I was also looking at the style guide for this again (because of the
iwyu thing).  As long as each .h file has a "primary" .cc file that includes it
first, then my argument is wrong - it's already included at the very top in some
.cc file so it must have all of its necessary headers.  So, please ignore my
rant :D

[Jói, 2011-05-11 18:36:08]

Gentle ping - I know you were away for a few days.

Cheers,
Jói


On Thu, May 5, 2011 at 10:58 AM,  <joi@chromium.org> wrote:
> FYI - nothing important, just a couple of things I noticed today.
>
>
>
http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
> File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
> (right):
>
>
http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
> net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:8: #include
> "net/proxy/dhcp_proxy_script_adapter_fetcher_win.h"
> On 2011/04/21 05:22:48, eroman wrote:
>>
>> nit: it isn't explicit in the style guide, but I assume it is
>
> consistent to put
>>
>> the header of the file under test at the top (similar to what we do
>
> for .cc
>>
>> files).
>
> I was just looking at include order in the style guide again.  The
> Google style actually does make this explicit, and the Chromium style
> guide refers to it.  So thanks for pointing this out to me, I had been
> using the wrong approach for quite a while.
>
>
http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
> File net/proxy/dhcp_proxy_script_fetcher_win.h (right):
>
>
http://codereview.chromium.org/6831025/diff/14001/net/proxy/dhcp_proxy_script...
> net/proxy/dhcp_proxy_script_fetcher_win.h:14: #include <list>
> On 2011/05/03 21:20:59, Jói wrote:
>>
>> On 2011/04/21 05:22:48, eroman wrote:
>> > These go at the top.
>
>> I've moved them to the top.
>
>> I checked and this appears to be the most common way of doing it in
>
> the Chrome
>>
>> codebase.  Just to rant though, I disagree with the approach as it
>
> makes
>>
>> "include what you use" tougher (if you include after other headers, it
>
> makes it
>>
>> harder for those headers to forget to include what they use).  I'm
>
> sure it's
>>
>> been argued to death already though.
>
> Just FYI, I was also looking at the style guide for this again (because
> of the iwyu thing).  As long as each .h file has a "primary" .cc file
> that includes it first, then my argument is wrong - it's already
> included at the very top in some .cc file so it must have all of its
> necessary headers.  So, please ignore my rant :D
>
> http://codereview.chromium.org/6831025/
>

[eroman, 2011-05-13 05:03:32]

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:55:
wait_timer_.Start(base::TimeDelta::FromMilliseconds(ImplGetTimeoutMs()),
(optional) how about having ImplGetTimeout return a TimeDelta?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:68:
DCHECK(CalledOnValidThread());
There is a problem here: Cancel() is being called from destructor.

Since DhcpProxyScriptAdapterFetcher is refcounted threadsafe, it may actually
end up releasing the final reference (and hence reaching the destrcutor) from a
worker thread.

This could happen for instance if we are shutting down, and the final reference
that remains is that held by the NewRunnableMethod task which we posted to
worker thread.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:125: std::string url) {
Please use a string reference here instead. (Note that things will still work
properly from NewRunnableMethod, it is smart enough to copy the string rather
than the reference).

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:241: return
std::string(reinterpret_cast<const char *>(wpad_params.Data));
I recommend using the constructor that takes a pointer + length rather than a
c-string (since it is more resilient to things like embedded NULLs).

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:39: // Starts a fetch. 
DhcpProxyScriptAdapterFetcher takes ownership of
Same comment as before: please update this text, taking "ownership" of a const
string& doesn't make sense.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:57: virtual int result()
const;
style nit: please don't use hacker_style() for virtual methods, only for vanilla
accessors.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:60: // |IsComplete()| is true.
 Returns the empty string if |GetResult()|
What is GetResult()?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:84: void
OnQueryDhcpDone(std::string url);
can this use a const string&?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:153: URLRequestContext*
url_request_context_;
Can you contain this in a scoped_refptr<> ?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:52:
base::PlatformThread::Sleep(5);
I don't much like this sleep loop, or how it tests a (non-volatile) bool. Can't
the compiler decide to not re-read that variable?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:63:
base::TimeDelta::FromMilliseconds(fetcher_delay_ms_),
So the assumption being made here is that the caller will call Start() before we
re-run the message loop?

Since if they don't we may reach NotifyFetchCompletion() before even having
received a request in it!

I think it will be simpler to simply define your own ProxyScriptFetcher that
does this delay internally.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:115: while
(!finished_) {
This is the pattern we use for net unit tests:

TestCompletionCallback callback;
fetcher_->Fetch("adapter name", &callback);
EXPECT_EQ(ERR_OK, callback.WaitUntilDone());

the TestCompletionCallback::WaitUntilDone() will automatically spin the message
loop until the callback is called.

Can you use that model here?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:277: }  // namespace
net
nit: add a newline between these two braces to match the opening style.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher.h:59: virtual int Fetch(string16*
utf16_text,
Can this take a GURL* parameter rather than exposing it as a separate getter?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher.h:67: virtual URLRequestContext*
GetRequestContext() const = 0;
Is this necessary?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_factory.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_factory.h:18: // Factory object
(non-singleton) for creating the appropriate concrete base
What do you mean by non-singleton? This is in fact a singleton.

I would prefer it as a non-singleton, since we are trying to move away from
globals. Especially in the multi-profile world, it is increasingly important to
be able to configure things separately for different profiles, singletons make
that goal much harder.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc:13: // Test harness that
does nothing but restore previous value to the
I appreciate the thoroughness of testing. But in truth I would have have
suggested omitting this.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:147: DCHECK(state_ ==
STATE_SOME_RESULTS);
DCHECK_EQ

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:241: NOTREACHED();
Rather than a NOTREACHED(), how about logging this information? (since it is
possible to reach).

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:98: typedef
std::list<scoped_refptr<DhcpProxyScriptAdapterFetcher>> FetcherList;
sorta surprised this works, i remember having issues with >> being tokenized as
a stream operator in the past.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:119: URLRequestContext*
url_request_context_;
No hold reference?

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:121: friend class
WindowsDhcpProxyScriptFetcherTestFriend;
Please use FRIEND_TEST

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:68:
fetcher_->Fetch(&pac_text_, &completion_callback_);
Same comment as earlier, please use a TestCompletionCallback, and then do:

callback.WaitUntilDone()

to block until it gets called.

[Jói, 2011-05-13 20:19:09]

I've uploaded a new version.  There is one change still pending to handle the
last reference being released from the worker thread issue more gracefully, but
if you have time to review the rest please do so.

Cheers,
Jói

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:55:
wait_timer_.Start(base::TimeDelta::FromMilliseconds(ImplGetTimeoutMs()),
On 2011/05/13 05:03:32, eroman wrote:
> (optional) how about having ImplGetTimeout return a TimeDelta?

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:68:
DCHECK(CalledOnValidThread());
On 2011/05/13 05:03:32, eroman wrote:
> There is a problem here: Cancel() is being called from destructor.
> 
> Since DhcpProxyScriptAdapterFetcher is refcounted threadsafe, it may actually
> end up releasing the final reference (and hence reaching the destrcutor) from
a
> worker thread.
> 
> This could happen for instance if we are shutting down, and the final
reference
> that remains is that held by the NewRunnableMethod task which we posted to
> worker thread.

Good catch.  Added a DetachFromThread to the destructor for this reason.  If I
do something fancier about counting our reference to URLRequestContext object,
this will change again.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:125: std::string url) {
On 2011/05/13 05:03:32, eroman wrote:
> Please use a string reference here instead. (Note that things will still work
> properly from NewRunnableMethod, it is smart enough to copy the string rather
> than the reference).

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:241: return
std::string(reinterpret_cast<const char *>(wpad_params.Data));
On 2011/05/13 05:03:32, eroman wrote:
> I recommend using the constructor that takes a pointer + length rather than a
> c-string (since it is more resilient to things like embedded NULLs).

An embedded NULL would be an error (remember, the string is a URL), and the
DCHECK above should validate that there are no embedded nulls.  I'd rather
return a "normal" string with no embedded nulls than return a string with
embedded nulls since if there are embedded nulls in what we get back from the
DHCP server, something is broken anyway.

Added a comment:

    // Return only up to the first null in case of embedded NULLs; if the
    // server is giving us back a buffer with embedded NULLs, something is
    // broken anyway.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:39: // Starts a fetch. 
DhcpProxyScriptAdapterFetcher takes ownership of
On 2011/05/13 05:03:32, eroman wrote:
> Same comment as before: please update this text, taking "ownership" of a const
> string& doesn't make sense.

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:57: virtual int result()
const;
On 2011/05/13 05:03:32, eroman wrote:
> style nit: please don't use hacker_style() for virtual methods, only for
vanilla
> accessors.

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:60: // |IsComplete()| is true.
 Returns the empty string if |GetResult()|
On 2011/05/13 05:03:32, eroman wrote:
> What is GetResult()?

Hmm, I guess it was result() above before I changed it to hacker-style :)  Now
changed back.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:84: void
OnQueryDhcpDone(std::string url);
On 2011/05/13 05:03:32, eroman wrote:
> can this use a const string&?

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:153: URLRequestContext*
url_request_context_;
On 2011/05/13 05:03:32, eroman wrote:
> Can you contain this in a scoped_refptr<> ?

I attempted to do so but it would result in a problem because the last reference
to DhcpProxyScriptAdapterFetcher might be released by the worker thread (whereas
the url_request_context_ member would never be accessed if the only remaining
reference is from the worker thread task posting thing).

It would be OK to leave as is since the comment on the constructor already
mentions that the client has to maintain lifetime of the URLRequestContext, but
it's not exactly clean.  I'm working on a better fix, but wanted to respond to
the rest of your review feedback first.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:52:
base::PlatformThread::Sleep(5);
On 2011/05/13 05:03:32, eroman wrote:
> I don't much like this sleep loop, or how it tests a (non-volatile) bool.
Can't
> the compiler decide to not re-read that variable?

Marked it volatile just in case.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:63:
base::TimeDelta::FromMilliseconds(fetcher_delay_ms_),
On 2011/05/13 05:03:32, eroman wrote:
> So the assumption being made here is that the caller will call Start() before
we
> re-run the message loop?
> 
> Since if they don't we may reach NotifyFetchCompletion() before even having
> received a request in it!
> 
> I think it will be simpler to simply define your own ProxyScriptFetcher that
> does this delay internally.

Added this comment to OnFetcherTimer, and extended the DCHECK so that it would
hit in the case you described (it does not occur right now):

    // Note that there is an assumption by this mock implementation that
    // DhcpProxyScriptAdapterFetcher::Fetch will call ImplCreateScriptFetcher
    // and call Fetch on the fetcher before the message loop is re-entered.
    // This holds true today, but if you hit this DCHECK the problem can
    // possibly be resolved by having a separate subclass of
    // MockProxyScriptFetcher that adds the delay internally (instead of
    // the simple approach currently used in ImplCreateScriptFetcher above).
    DCHECK(fetcher_ && fetcher_->has_pending_request());

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:115: while
(!finished_) {
On 2011/05/13 05:03:32, eroman wrote:
> This is the pattern we use for net unit tests:
> 
> TestCompletionCallback callback;
> fetcher_->Fetch("adapter name", &callback);
> EXPECT_EQ(ERR_OK, callback.WaitUntilDone());
> 
> the TestCompletionCallback::WaitUntilDone() will automatically spin the
message
> loop until the callback is called.
> 
> Can you use that model here?

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:277: }  // namespace
net
On 2011/05/13 05:03:32, eroman wrote:
> nit: add a newline between these two braces to match the opening style.

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher.h:59: virtual int Fetch(string16*
utf16_text,
On 2011/05/13 05:03:32, eroman wrote:
> Can this take a GURL* parameter rather than exposing it as a separate getter?

It could, but in my opinion the current approach isolates responsibilities
better.

OTOH, there is no need to force a copy to get the URL; I have changed the method
to return a const reference to the stored GURL.

Clients may not need the PAC URL, in which case it just adds complexity to the
client.  For example, in the current use case, it is only needed in
InitProxyResolver if detection of a PAC URL is successful, at which point it
seems easier for that client to call GetPacURL() rather than having to add a
member variable to store something it may or may not use.

Conversely, the DHCP fetcher needs to store the PAC URL somewhere.  It can
either provide the storage itself, or put the burden on its clients to provide
the storage.  The only time I would put the burden on the clients is when the
clients will always need this piece of information (which is true for the PAC
script text), or when there is at least one client that will store the
information past the lifetime of the DHCP fetcher and letting it provide storage
would allow it to avoid ever making a copy.  Neither is true in current usage.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher.h:67: virtual URLRequestContext*
GetRequestContext() const = 0;
On 2011/05/13 05:03:32, eroman wrote:
> Is this necessary?

Nope, it was a hold-over from when I merged the class hierarchies.  Removed.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_factory.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_factory.h:18: // Factory object
(non-singleton) for creating the appropriate concrete base
On 2011/05/13 05:03:32, eroman wrote:
> What do you mean by non-singleton? This is in fact a singleton.
> 
> I would prefer it as a non-singleton, since we are trying to move away from
> globals. Especially in the multi-profile world, it is increasingly important
to
> be able to configure things separately for different profiles, singletons make
> that goal much harder.

I commented that way because you use it via static methods rather than
::GetInstance()->FooBar().  Guess it should have been just "Static factory for
..."

The option that the factory stores here is a command-line option, so it is
inherently global, which is why I preferred to keep it initialized via
BrowserMain.  Since you still disagree with initializing it there, I have made
the factory object non-static and moved its initialization to the two places
where it is used (connection_tester.cc and proxy_service_factory.cc).

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_factory_unittest.cc:13: // Test harness that
does nothing but restore previous value to the
On 2011/05/13 05:03:32, eroman wrote:
> I appreciate the thoroughness of testing. But in truth I would have have
> suggested omitting this.

This is just so that these unit tests can't interfere with other tests;
modifying global state in a unit test and not restoring it does not seem kosher
to me.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:147: DCHECK(state_ ==
STATE_SOME_RESULTS);
On 2011/05/13 05:03:32, eroman wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:241: NOTREACHED();
On 2011/05/13 05:03:32, eroman wrote:
> Rather than a NOTREACHED(), how about logging this information? (since it is
> possible to reach).

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:98: typedef
std::list<scoped_refptr<DhcpProxyScriptAdapterFetcher>> FetcherList;
On 2011/05/13 05:03:32, eroman wrote:
> sorta surprised this works, i remember having issues with >> being tokenized
as
> a stream operator in the past.

I think it used to be an issue on VS 2005, and I guess we may still want to be
compatible with that so I've added a space.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:119: URLRequestContext*
url_request_context_;
On 2011/05/13 05:03:32, eroman wrote:
> No hold reference?

Done.

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:121: friend class
WindowsDhcpProxyScriptFetcherTestFriend;
On 2011/05/13 05:03:32, eroman wrote:
> Please use FRIEND_TEST

This wasn't even being used any more so I've removed it.  Thanks for taking
issue with it :)

I would posit though, that it should be fine to use this instead of FRIEND_TEST
in certain cases, perhaps with improved naming (start with TestFriend instead of
end with TestFriend)?  For classes that have a lot of tests that need to be
friends with it, it can be nice not to have twenty FRIEND_TEST lines in the
class, and there is nothing magical about that macro that should make it more
desirable (apart from the obvious naming, which should be achievable with the
TestFriend prefix or some other prefix).

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/23001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:68:
fetcher_->Fetch(&pac_text_, &completion_callback_);
On 2011/05/13 05:03:32, eroman wrote:
> Same comment as earlier, please use a TestCompletionCallback, and then do:
> 
> callback.WaitUntilDone()
> 
> to block until it gets called.

This would make it difficult to implement the cancel timer, and I'm not sure the
requirement to support such cancellation is generic enough for me to add support
for it to TestCompletionCallback.

To make this a bit more similar to the pattern you mentioned, I have moved the
two different places where I was spinning the message loop into a common
function RealFetchTester::WaitUntilDone().

[Jói, 2011-05-16 17:22:49]

Eric, I've uploaded a new version that I think should be good to go.  In case
you've started looking at patch set 8, this new version (patch set 9) has the
following differences:
- Isolate the worker thread to a separate refcounted object with weak reference
back to the now not refcounted adapter fetcher.
- Use a scoped_refptr to hold URLRequestContext.
- Fix a couple of unit tests that caught some breakage in how I had merged the
mandatory PAC changes (and act on a TODO I had written for myself when merging).

Cheers,
Jói

[eroman, 2011-05-17 04:39:39]

LGTM after considering below comments.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:61: void
DhcpProxyScriptAdapterFetcher::Cancel() {
This is a one-shot class right? (I am guessing so, since Fetch() asserts that
state_ == STATE_START).

Just checking, since we don't explicitly cancel worker_thread_, so if we were to
call Fetch() after Cancel() might not do the right thing.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:45: //
DhcpProxyScriptAdapterFetcher.
could also mention that on completion result is obtained from GetPacScript() /
GetPacURL()

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:57: virtual int GetResult()
const;
Is this not the same as the integer passed to the completion callback?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:83: class WorkerThread :
public base::RefCountedThreadSafe<WorkerThread> {
(optional) WorkerThread sounds like it should be a base::Thread object. Another
naming idea would be "Job".

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:86: explicit WorkerThread(
Please also add a virtual destructor to prevent accidental leaks in derived
classes.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:94: void ThreadFunc(const
std::string& adapter_name);
can these two methods be private?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:101: // Unit testing seam.
what is a "seam"?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:105: // All work except
QueryDHcpOnWorkerThread and destruction should
ThreadFunc?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:110:
base::WeakPtr<DhcpProxyScriptAdapterFetcher> owner_;
I like your general approach of keeping the parent class non-threadsafe.

However I am not sure if it is safe to use WeakPtr<> across threads. I *think*
it is OK since it looks like the only operation which might ever run off-thread
in this case is ~WeakPtr. However since the header has a caveat saying it is not
threadsafe, I would like to ask for confirmation.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:114: // thread has gone away,
that would mean this object is in state
nit: not sure I understand this comment. CANCEL isn't a concept in WorkerThread,
all it knows if whether owner_ has already been destroyed.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:77: while
(test_owner_ &&
Per my earlier comment, I do not like spin loops like this in tests, they can be
problematic.

As this is written there are some races. Since test_owner_ is being nulled off
thread, it is possible for test_owner_ to be non-null at the start of this
while, but then in the second clause when we do test_owner_->dhcp_delay_ms_ it
is already NULLed and/or deleted.

Note that you could also use a WaitableEvent for signalling completion. In here
you could do a WaitableEvent::TimedWait() with the desired delay.If the test
finishes before that, you could signal the event.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:152:
base::PlatformThread::Sleep(15);
why does there need to be a sleep here?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:84: DCHECK(state_ == STATE_DONE);
DCHECK_EQ ?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:149: CancelImpl(false);
I don't think we need CancelImpl(false). Can't you just call Cancel() at the
very end?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:184: fetchers_.reset();
Can you reset all the state *before* calling the callback. In case the caller
decides to start a new fetch from within the callback, we will run into problems
here.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:24: class
WindowsDhcpProxyScriptFetcher
nit: for consistency with the file name, I suggest naming this
DhcpProxyScriptFetcherWin

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:97: typedef
ScopedVector<DhcpProxyScriptAdapterFetcher> FetcherVector;
cool, i didn't know about ScopedVector until reading this.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:32: for
(std::set<std::string>::iterator it = adapter_names.begin();
nit: const_iterator ?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:480: void
TestShortCircuitLessPreferredAdapters(FetcherClient* client) {
I did not have time to review this file thoroughly, hopefully I can give it a
more thorough pass later.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:192: CreateNetLogParameterForSource(pac_source,
&effective_pac_url);
Sort of strange for CreateNetLogParameterForSource() to be mutating the url.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:205: } else {
nit: omit the "else" when the clause above returns.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.h (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.h:46: // |resolver|, |proxy_script_fetcher| and
|net_log| must remain valid for
can you also add dhcp_proxy_script_fetcher here?

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.h:54: virtual ~InitProxyResolver();
virtual is no longer needed here.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver_unittest.cc:556:
SynchronousSuccessDhcpFetcher(const string16& expected_text)
nit: explicit

http://codereview.chromium.org/6831025/diff/35001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:797: DhcpProxyScriptFetcher*
ProxyService::GetDhcpProxyScriptFetcher() const {
Is this used somewhere? (I wander if we can get rid of it).

[Jói, 2011-05-17 15:38:35]

Thanks a lot Eric. I've uploaded a new version that addresses your final round
of comments.

I plan to submit later today based on your LGTM, once this last version has
passed through the trybots.

Cheers,
Jói

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:61: void
DhcpProxyScriptAdapterFetcher::Cancel() {
On 2011/05/17 04:39:39, eroman wrote:
> This is a one-shot class right? (I am guessing so, since Fetch() asserts that
> state_ == STATE_START).
> 
> Just checking, since we don't explicitly cancel worker_thread_, so if we were
to
> call Fetch() after Cancel() might not do the right thing.

Correct, this is documented on Fetch():

  // You may only call Fetch() once on a given instance of
  // DhcpProxyScriptAdapterFetcher.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:45: //
DhcpProxyScriptAdapterFetcher.
On 2011/05/17 04:39:39, eroman wrote:
> could also mention that on completion result is obtained from GetPacScript() /
> GetPacURL()

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:57: virtual int GetResult()
const;
On 2011/05/17 04:39:39, eroman wrote:
> Is this not the same as the integer passed to the completion callback?

It is. The implementation of going through the fetchers to find which ones are
done, have an error etc. is a lot simpler by leaving the state accessible there.
I've updated the comment as follows:

  // Returns the network error indicating the result of the fetch. Will
  // return IO_PENDING until the fetch is complete or cancelled. This is
  // the same network error passed to the |callback| provided to |Fetch()|.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:83: class WorkerThread :
public base::RefCountedThreadSafe<WorkerThread> {
On 2011/05/17 04:39:39, eroman wrote:
> (optional) WorkerThread sounds like it should be a base::Thread object.
Another
> naming idea would be "Job".

I'll leave it as is since it encapsulates our use of the worker thread pool. 
Agreed the naming is not totally ideal but I'm not sure "Job" captures it better
(that sounds like something you might post to a thread e.g. like a Task).

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:86: explicit WorkerThread(
On 2011/05/17 04:39:39, eroman wrote:
> Please also add a virtual destructor to prevent accidental leaks in derived
> classes.

Doh, I have to stop forgetting this.  Thanks for catching it.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:94: void ThreadFunc(const
std::string& adapter_name);
On 2011/05/17 04:39:39, eroman wrote:
> can these two methods be private?

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:101: // Unit testing seam.
On 2011/05/17 04:39:39, eroman wrote:
> what is a "seam"?

Sorry, I thought it was common terminology; it's used extensively in a book I
like, "Working Effectively with Legacy Code" that is all about adding tests to
existing code.  I've changed the comment to say "Virtual method introduced to
allow unit testing."

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:105: // All work except
QueryDHcpOnWorkerThread and destruction should
On 2011/05/17 04:39:39, eroman wrote:
> ThreadFunc?

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:110:
base::WeakPtr<DhcpProxyScriptAdapterFetcher> owner_;
On 2011/05/17 04:39:39, eroman wrote:
> I like your general approach of keeping the parent class non-threadsafe.
> 
> However I am not sure if it is safe to use WeakPtr<> across threads. I *think*
> it is OK since it looks like the only operation which might ever run
off-thread
> in this case is ~WeakPtr. However since the header has a caveat saying it is
not
> threadsafe, I would like to ask for confirmation.

Thanks.  The weak_ptr.h header file has a comment on the WeakReference class
that says:

  // While Flag is bound to a specific thread, it may be deleted from another
  // via base::WeakPtr::~WeakPtr().

However, looking at the implementation, it looks like you need to call
SupportsWeakPtr<>::DetachFromThread() for this to be true. I've added this to
DhcpProxyScriptAdapterFetcher's destructor.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.h:114: // thread has gone away,
that would mean this object is in state
On 2011/05/17 04:39:39, eroman wrote:
> nit: not sure I understand this comment. CANCEL isn't a concept in
WorkerThread,
> all it knows if whether owner_ has already been destroyed.

Copy/paste error.  Updated the comment to:

    // Used by worker thread to post a message back to the original
    // thread.  Fine to use a proxy since in the case where the original
    // thread has gone away, that would mean the |owner_| object is gone
    // anyway, so there is nobody to receive the result.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:77: while
(test_owner_ &&
On 2011/05/17 04:39:39, eroman wrote:
> Per my earlier comment, I do not like spin loops like this in tests, they can
be
> problematic.
> 
> As this is written there are some races. Since test_owner_ is being nulled off
> thread, it is possible for test_owner_ to be non-null at the start of this
> while, but then in the second clause when we do test_owner_->dhcp_delay_ms_ it
> is already NULLed and/or deleted.
> 
> Note that you could also use a WaitableEvent for signalling completion. In
here
> you could do a WaitableEvent::TimedWait() with the desired delay.If the test
> finishes before that, you could signal the event.

Thanks for pressing on this. Switched to using a WaitableEvent, it's much nicer
that way.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc:152:
base::PlatformThread::Sleep(15);
On 2011/05/17 04:39:39, eroman wrote:
> why does there need to be a sleep here?

The reason is lost in the mists of time :)  Removed.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:84: DCHECK(state_ == STATE_DONE);
On 2011/05/17 04:39:39, eroman wrote:
> DCHECK_EQ ?

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:149: CancelImpl(false);
On 2011/05/17 04:39:39, eroman wrote:
> I don't think we need CancelImpl(false). Can't you just call Cancel() at the
> very end?

By golly, you're right :)  Removed the CancelImpl function and rolled it into
Cancel, calling that at the end of TransitionToDone.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.cc:184: fetchers_.reset();
On 2011/05/17 04:39:39, eroman wrote:
> Can you reset all the state *before* calling the callback. In case the caller
> decides to start a new fetch from within the callback, we will run into
problems
> here.

Done, thanks for catching that.  I had a test for reuse but it didn't reuse
within the callback.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win.h (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:24: class
WindowsDhcpProxyScriptFetcher
On 2011/05/17 04:39:39, eroman wrote:
> nit: for consistency with the file name, I suggest naming this
> DhcpProxyScriptFetcherWin

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win.h:97: typedef
ScopedVector<DhcpProxyScriptAdapterFetcher> FetcherVector;
On 2011/05/17 04:39:39, eroman wrote:
> cool, i didn't know about ScopedVector until reading this.

Yep, it's nice :)

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:32: for
(std::set<std::string>::iterator it = adapter_names.begin();
On 2011/05/17 04:39:39, eroman wrote:
> nit: const_iterator ?

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:480: void
TestShortCircuitLessPreferredAdapters(FetcherClient* client) {
On 2011/05/17 04:39:39, eroman wrote:
> I did not have time to review this file thoroughly, hopefully I can give it a
> more thorough pass later.

OK, thanks Eric.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:192: CreateNetLogParameterForSource(pac_source,
&effective_pac_url);
On 2011/05/17 04:39:39, eroman wrote:
> Sort of strange for CreateNetLogParameterForSource() to be mutating the url.

Renamed to CreateNetLogParameterAndDetermineURL()

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.cc:205: } else {
On 2011/05/17 04:39:39, eroman wrote:
> nit: omit the "else" when the clause above returns.

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver.h (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.h:46: // |resolver|, |proxy_script_fetcher| and
|net_log| must remain valid for
On 2011/05/17 04:39:39, eroman wrote:
> can you also add dhcp_proxy_script_fetcher here?

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver.h:54: virtual ~InitProxyResolver();
On 2011/05/17 04:39:39, eroman wrote:
> virtual is no longer needed here.

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
File net/proxy/init_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/init_proxy_resolv...
net/proxy/init_proxy_resolver_unittest.cc:556:
SynchronousSuccessDhcpFetcher(const string16& expected_text)
On 2011/05/17 04:39:39, eroman wrote:
> nit: explicit

Done.

http://codereview.chromium.org/6831025/diff/35001/net/proxy/proxy_service.cc
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/6831025/diff/35001/net/proxy/proxy_service.cc#...
net/proxy/proxy_service.cc:797: DhcpProxyScriptFetcher*
ProxyService::GetDhcpProxyScriptFetcher() const {
On 2011/05/17 04:39:39, eroman wrote:
> Is this used somewhere? (I wander if we can get rid of it).

Removed.

[Timur Iskhodzhanov, 2011-06-27 14:44:01]

http://codereview.chromium.org/6831025/diff/44002/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/44002/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:226: void Cancel() OVERRIDE
{
FYI,
You're overriding the Cancel() method which is called in the base class
destructor as well.
Are you aware that the destructor of the base class would end up calling the not
overridden version of the method?

[Jói, 2011-06-27 15:19:06]

http://codereview.chromium.org/6831025/diff/44002/net/proxy/dhcp_proxy_script...
File net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc (right):

http://codereview.chromium.org/6831025/diff/44002/net/proxy/dhcp_proxy_script...
net/proxy/dhcp_proxy_script_fetcher_win_unittest.cc:226: void Cancel() OVERRIDE
{
On 2011/06/27 14:44:01, Timur Iskhodzhanov wrote:
> FYI,
> You're overriding the Cancel() method which is called in the base class
> destructor as well.
> Are you aware that the destructor of the base class would end up calling the
not
> overridden version of the method?

Thanks, yes I'm aware.  I only need to stop the timer here when Cancel is called
outside of destruction so this is fine in this case.