Add iOS support to ProxyService

Add iOS support to ProxyService

The iOS implementation is similar to the Mac implementation with the following
differences:
- the SCDynamicStore API does not exist on iOS, thus configuration changes are
  only detected by polling
- several features are not supported.

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

[eroman, 2012-08-23 17:38:01]

I don't see proxy_resolver_mac.cc used anywhere, is this needed? Even Mac Chrome
doesn't use this, except in single-process mode.

https://chromiumcodereview.appspot.com/10882010/diff/7006/net/proxy/proxy_con...
File net/proxy/proxy_config_service_ios.cc (right):

https://chromiumcodereview.appspot.com/10882010/diff/7006/net/proxy/proxy_con...
net/proxy/proxy_config_service_ios.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012

[droger, 2012-08-24 07:45:49]

On 2012/08/23 17:38:01, eroman wrote:
> I don't see proxy_resolver_mac.cc used anywhere, is this needed? Even Mac
Chrome
> doesn't use this, except in single-process mode.

I don't see anything different from the Mac code in our implementation, however
I may have missed something.
I put a breakpoint in the constructor, here is the stack trace, I hope this
helps. If this stack trace looks wrong to you, this means that we have some IOS
specific code somewhere that triggers the use of ProxyResolverMac, and I'll try
to find it.

ProxyResolverFactoryForSystem::CreateProxyResolver() + 252 at
proxy_service.cc:327

net::MultiThreadedProxyResolver::AddNewExecutor() + 615 at 
multi_threaded_proxy_resolver.cc:570

net::MultiThreadedProxyResolver::SetPacScript(scoped_refptr<net::ProxyResolverScriptData>
const&, base::Callback<void ()(int)> const&) + 592 at
multi_threaded_proxy_resolver.cc:519

net::ProxyService::InitProxyResolver::DoSetPacScript() + 446 at
proxy_service.cc:542

net::ProxyService::InitProxyResolver::DoLoop(int) + 823 at proxy_service.cc:503

net::ProxyService::InitProxyResolver::Start(net::ProxyResolver*,
net::ProxyScriptFetcher*, net::DhcpProxyScriptFetcher*, net::NetLog*,
net::ProxyConfig const&, base::TimeDelta, base::Callback<void ()(int)> const&) +
639 at proxy_service.cc:439

net::ProxyService::InitializeUsingLastFetchedConfig() + 991 at
proxy_service.cc:1539

net::ProxyService::OnProxyConfigChanged(net::ProxyConfig const&,
net::ProxyConfigService::ConfigAvailability) + 877 at proxy_service.cc:1506
 
net::ProxyService::OnProxyConfigChanged(net::ProxyConfig const&,
net::ProxyConfigService::ConfigAvailability) + 54 at proxy_service.cc:351

ChromeProxyConfigService::OnProxyConfigChanged(net::ProxyConfig const&,
net::ProxyConfigService::ConfigAvailability) + 545 at
pref_proxy_config_tracker_impl.cc:112

ChromeProxyConfigService::OnProxyConfigChanged(net::ProxyConfig const&,
net::ProxyConfigService::ConfigAvailability) + 54 at
pref_proxy_config_tracker_impl.cc:161

Chromium`net::PollingProxyConfigService::Core::GetConfigCompleted(net::ProxyConfig
const&) + 847 at polling_proxy_config_service.cc:129

[droger, 2012-08-24 08:10:04]

Please ignore my previous message.

I added added the related files from chrome/browser/net.
I hope this CL now makes sense.

Sorry about that.

[droger, 2012-08-28 07:49:56]

ping

[droger, 2012-08-30 13:32:24]

Adding wtc, because this is 6 days old already.

[cbentzel, 2012-08-30 17:08:47]

Quick comments:

- Eric was sheriff past two days, so hopefully should be more freed up to review
now.

- Could you add a BUG for this.

- I'm surprised that you are even creating a PrefProxyConfigTracker in the first
place, if iOS isn't supporting this. Should you just not instantiate it instead
of overriding it?

[wtc, 2012-08-30 18:52:01]

Patch set 6 LGTM.

I think I wrote some of the code in this CL originally, so
I may have reviewed my own code. It would be nice to wait for
eroman or cbentzel's review.

On the other hand, I believe someone else already reviewed
my code.

https://chromiumcodereview.appspot.com/10882010/diff/13001/chrome/browser/net...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

https://chromiumcodereview.appspot.com/10882010/diff/13001/chrome/browser/net...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:318: return
ProxyPrefs::CONFIG_UNSET;

I suggest putting the rest of this function inside a #else
block.

I am surprised that the iOS compiler does not warn about
unreachable code after the return statement.

https://chromiumcodereview.appspot.com/10882010/diff/13001/net/proxy/proxy_re...
File net/proxy/proxy_resolver_mac.cc (right):

https://chromiumcodereview.appspot.com/10882010/diff/13001/net/proxy/proxy_re...
net/proxy/proxy_resolver_mac.cc:22: 

The current Chromium Coding Style recommends putting the
platform-specific headers last. So please move lines 8-12
here.

http://www.chromium.org/developers/coding-style#TOC-Platform-defines

[droger, 2012-08-31 15:32:14]

On 2012/08/30 17:08:47, cbentzel wrote:
> - I'm surprised that you are even creating a PrefProxyConfigTracker in the
first
> place, if iOS isn't supporting this. Should you just not instantiate it
instead
> of overriding it?

I admit that don't really know if it would be possible to not instantiate it.
This would require some refactoring, because currently that crashes in several
places.

[eroman, 2012-08-31 20:02:43]

lgtm. apologies for the slow response

http://codereview.chromium.org/10882010/diff/13001/chrome/browser/net/pref_pr...
File chrome/browser/net/pref_proxy_config_tracker_impl.cc (right):

http://codereview.chromium.org/10882010/diff/13001/chrome/browser/net/pref_pr...
chrome/browser/net/pref_proxy_config_tracker_impl.cc:318: return
ProxyPrefs::CONFIG_UNSET;
Why is this necessary?

[commit-bot: I haz the power, 2012-09-04 11:30:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/10882010/18001

[commit-bot: I haz the power, 2012-09-04 15:51:24]

Change committed as 154758

[Ryan Sleevi, 2012-09-05 02:59:21]

https://chromiumcodereview.appspot.com/10882010/diff/18001/net/proxy/proxy_co...
File net/proxy/proxy_config_service_ios.cc (right):

https://chromiumcodereview.appspot.com/10882010/diff/18001/net/proxy/proxy_co...
net/proxy/proxy_config_service_ios.cc:106: DCHECK(io_thread_task_runner_);
You don't actually seem to use this, other than to DCHECK that it's destructed
on the right thread.

Was that intentional? It would seem like if so, it should be an parameter on the
PollingProxyConfigService, since that's the one that (presumably) has thread
affinity.

https://chromiumcodereview.appspot.com/10882010/diff/18001/net/proxy/proxy_co...
File net/proxy/proxy_config_service_ios.h (right):

https://chromiumcodereview.appspot.com/10882010/diff/18001/net/proxy/proxy_co...
net/proxy/proxy_config_service_ios.h:19: // This instance is expected to be
operated and deleted on the IO thread
drive by comment nit: willchan has pointed out in past reviews "the IO thread"
is an implementation-specific detail of "Chrome" (as in src/chrome). The
preference is to call this "the network thread" or even say "on the same thread"

[droger, 2012-09-05 14:25:24]

Thank you for the comments.

I'll submit a follow up CL to address the last comments.

http://codereview.chromium.org/10882010/diff/18001/net/proxy/proxy_config_ser...
File net/proxy/proxy_config_service_ios.cc (right):

http://codereview.chromium.org/10882010/diff/18001/net/proxy/proxy_config_ser...
net/proxy/proxy_config_service_ios.cc:106: DCHECK(io_thread_task_runner_);
On 2012/09/05 02:59:21, Ryan Sleevi wrote:
> You don't actually seem to use this, other than to DCHECK that it's destructed
> on the right thread.
> 
> Was that intentional? It would seem like if so, it should be an parameter on
the
> PollingProxyConfigService, since that's the one that (presumably) has thread
> affinity.

This is not intentional. It has been copy-pasted incorrectly from
ProxyConfigServiceMac.