Don't poll the PAC script during periods of network inactivity.

Don't poll the PAC script during periods of network inactivity.

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

[wtc, 2012-01-13 01:08:45]

Patch Set 2 LGTM.

I reviewed the CL superficially, so I'm counting on you to
check it carefully.

Please fix the comment problem (noted below) before you check
this in.

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

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:345: ProxyService::PacPollPolicy::~PacPollPolicy() {}

Nit: why can't this destructor be defined in the header?

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:65: // period of inactivity will likely see

Nit: this line is too short.

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:78: int64* next_delay_ms) const = 0;

You didn't document these two methods.

[eroman, 2012-01-13 01:29:10]

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

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:345: ProxyService::PacPollPolicy::~PacPollPolicy() {}
On 2012/01/13 01:08:45, wtc wrote:
> 
> Nit: why can't this destructor be defined in the header?

@erg has gone to a lot of effort to remove code from headers in order to speed
up compile speeds.

I believe it is desirable to keep the implementation for destructors (even
trivial ones) in the .cc file rather than the header for this reason.

I have cc-ed Eliot to verify that my interpretation of the rules is correct.

[Elliot Glaysher, 2012-01-13 18:19:18]

On 2012/01/13 01:29:10, eroman wrote:
> http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.cc
> File net/proxy/proxy_service.cc (right):
> 
>
http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.cc#n...
> net/proxy/proxy_service.cc:345: ProxyService::PacPollPolicy::~PacPollPolicy()
{}
> On 2012/01/13 01:08:45, wtc wrote:
> > 
> > Nit: why can't this destructor be defined in the header?
> 
> @erg has gone to a lot of effort to remove code from headers in order to speed
> up compile speeds.
> 
> I believe it is desirable to keep the implementation for destructors (even
> trivial ones) in the .cc file rather than the header for this reason.
> 
> I have cc-ed Eliot to verify that my interpretation of the rules is correct.

In this case, the dtor is the only concrete method in an entirely abstract
interface. There's no member variables so the dtor won't be very heavy and will
probably be inlined. It's a judgement call on your part, but I'd leave it in the
header.

(I would just like to point out that I hate that C++ makes us have to worry
about this.)

[eroman, 2012-01-13 18:54:31]

Thanks Eliot. In that case I will leave it in the header file.

[eroman, 2012-01-13 20:05:27]

All comments address. Will commit after some more manual testing.

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

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.cc#n...
net/proxy/proxy_service.cc:345: ProxyService::PacPollPolicy::~PacPollPolicy() {}
On 2012/01/13 01:29:10, eroman wrote:
> On 2012/01/13 01:08:45, wtc wrote:
> > 
> > Nit: why can't this destructor be defined in the header?
> 
> @erg has gone to a lot of effort to remove code from headers in order to speed
> up compile speeds.
> 
> I believe it is desirable to keep the implementation for destructors (even
> trivial ones) in the .cc file rather than the header for this reason.
> 
> I have cc-ed Eliot to verify that my interpretation of the rules is correct.

Done.

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:65: // period of inactivity will likely see
On 2012/01/13 01:08:45, wtc wrote:
> 
> Nit: this line is too short.

Done.

http://codereview.chromium.org/9139070/diff/2005/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:78: int64* next_delay_ms) const = 0;
On 2012/01/13 01:08:45, wtc wrote:
> 
> You didn't document these two methods.

Done.

[commit-bot: I haz the power, 2012-01-13 20:31:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/9139070/8001

[commit-bot: I haz the power, 2012-01-13 22:05:47]

Change committed as 117709

[wtc, 2012-01-17 22:42:53]

Patch set 3 LGTM.  Feel free to ignore the nits below.

http://codereview.chromium.org/9139070/diff/8001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/9139070/diff/8001/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:77: // code that the most last PAC fetch failed with
(or OK if it was a

Nit: remove "most"?

http://codereview.chromium.org/9139070/diff/8001/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:82: // by the preceding poll. Implementations must set
|next_delay_ms|.

preceding => current ?  The parameter is named current_delay_ms.

[eroman, 2012-01-17 22:47:21]

http://codereview.chromium.org/9139070/diff/8001/net/proxy/proxy_service.h
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/9139070/diff/8001/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:77: // code that the most last PAC fetch failed with
(or OK if it was a
On 2012/01/17 22:42:53, wtc wrote:
> 
> Nit: remove "most"?

Will do.

http://codereview.chromium.org/9139070/diff/8001/net/proxy/proxy_service.h#ne...
net/proxy/proxy_service.h:82: // by the preceding poll. Implementations must set
|next_delay_ms|.
On 2012/01/17 22:42:53, wtc wrote:
> 
> preceding => current ?  The parameter is named current_delay_ms.

There are two parameters, one called current_delay_ms, and one called
next_delay_ms. I believe the comment is correct.