Separate ProxyResolverMac and ProxyConfigServiceMac into their own files

Separate ProxyResolverMac and ProxyConfigServiceMac into their own files and extract common utility functions into other files.

TEST=trybot and MacUtilTest in base_unittests
BUG=27310
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=34243

[hayato, 2009-12-08 10:28:27]

Hi eroman,
Could you review it?

I am not sure GetValueFromDictionary is worth while to be in mac_util.h. Because
these Proxy{Resolver,ConfigService}Mac are now only callers to that.

[eroman, 2009-12-09 01:10:13]

Thanks for doing this!

LGTM.

I recommend having avi give ithis a look-over, as he is more familiar with the
mac side of things.

http://codereview.chromium.org/463028/diff/3001/4003
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/463028/diff/3001/4003#newcode139
base/mac_util_unittest.mm:139: CFSTR("value"));
reverse the parameter order: EXPECT_EQ(expected, actual)

http://codereview.chromium.org/463028/diff/3001/4005
File net/proxy/proxy_config_service_mac.cc (right):

http://codereview.chromium.org/463028/diff/3001/4005#newcode23
net/proxy/proxy_config_service_mac.cc:23: bool
GetBoolFromDictionary(CFDictionaryRef dict,
would this be of general usefulness to put into mac_util.h?

http://codereview.chromium.org/463028/diff/3001/4010
File net/proxy/proxy_server_mac.cc (right):

http://codereview.chromium.org/463028/diff/3001/4010#newcode23
net/proxy/proxy_server_mac.cc:23: scheme == SCHEME_DIRECT) {
Does this fit only 1 line?

[Avi (use Gerrit), 2009-12-09 03:12:34]

On Tue, Dec 8, 2009 at 8:10 PM, <eroman@chromium.org> wrote:

>
> I recommend having avi give ithis a look-over, as he is more familiar with
> the
> mac side of things.
>
> I will tomorrow...

[hayato, 2009-12-09 04:32:48]

Thank you for the review.

http://codereview.chromium.org/463028/diff/3001/4003
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/463028/diff/3001/4003#newcode139
base/mac_util_unittest.mm:139: CFSTR("value"));
On 2009/12/09 01:10:13, eroman wrote:
> reverse the parameter order: EXPECT_EQ(expected, actual)

Done.

http://codereview.chromium.org/463028/diff/3001/4005
File net/proxy/proxy_config_service_mac.cc (right):

http://codereview.chromium.org/463028/diff/3001/4005#newcode23
net/proxy/proxy_config_service_mac.cc:23: bool
GetBoolFromDictionary(CFDictionaryRef dict,
On 2009/12/09 01:10:13, eroman wrote:
> would this be of general usefulness to put into mac_util.h?

I am not sure, but I'd like to leave it as is until at least more than one file
need to call this function. ProxyConfigServiceMac is now a only caller.

http://codereview.chromium.org/463028/diff/3001/4010
File net/proxy/proxy_server_mac.cc (right):

http://codereview.chromium.org/463028/diff/3001/4010#newcode23
net/proxy/proxy_server_mac.cc:23: scheme == SCHEME_DIRECT) {
On 2009/12/09 01:10:13, eroman wrote:
> Does this fit only 1 line?

Done.

[Avi (use Gerrit), 2009-12-09 04:41:39]

Other than the EXPECT noted below, LG.

http://codereview.chromium.org/463028/diff/3002/4015
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/463028/diff/3002/4015#newcode138
base/mac_util_unittest.mm:138: dict, CFSTR("key"), CFStringGetTypeID()));
Does this pass? I'm not sure it's guaranteed to. Do EXPECT(CFEqual(..., ...))
instead.

[hayato, 2009-12-09 05:04:50]

Thank you for the review.

http://codereview.chromium.org/463028/diff/3002/4015
File base/mac_util_unittest.mm (right):

http://codereview.chromium.org/463028/diff/3002/4015#newcode138
base/mac_util_unittest.mm:138: dict, CFSTR("key"), CFStringGetTypeID()));
Done.
Nice catch. It happened to pass, but it's not guaranteed.

It might be better that we have EXPECT_CFEQUAL(expected, actual) in our testing
framework. I'll file the feature request in other place.

On 2009/12/09 04:41:39, Avi wrote:
> Does this pass? I'm not sure it's guaranteed to. Do EXPECT(CFEqual(..., ...))
> instead.

[eroman, 2009-12-09 05:14:50]

LGTM

http://codereview.chromium.org/463028/diff/1014/2007
File net/proxy/proxy_config_service_mac.cc (right):

http://codereview.chromium.org/463028/diff/1014/2007#newcode19
net/proxy/proxy_config_service_mac.cc:19: namespace {
If you can copy this file using:

svn cp proxy_resolver_mac.cc proxy_config_service_mac.c

Then the file history will be preserved (and the diffs will show up cleaner).

http://codereview.chromium.org/463028/diff/1014/2012
File net/proxy/proxy_server_mac.cc (right):

http://codereview.chromium.org/463028/diff/1014/2012#newcode15
net/proxy/proxy_server_mac.cc:15: namespace net {
This too might benefit from svn cp.

[hayato, 2009-12-09 05:53:55]

Thank you for the review.

http://codereview.chromium.org/463028/diff/1014/2007
File net/proxy/proxy_config_service_mac.cc (right):

http://codereview.chromium.org/463028/diff/1014/2007#newcode19
net/proxy/proxy_config_service_mac.cc:19: namespace {
Hmm. I am using git and git should detect these 'copy' or 'rename' automatically
and should produce correct patch for 'subversion'.
Unfortunately, this is not the case. I am sorry.

Although I am now finding a work around, I feel I have to use 'svn' instead of
'git' this time. I'll update this issue when done.
 
On 2009/12/09 05:14:50, eroman wrote:
> If you can copy this file using:
> 
> svn cp proxy_resolver_mac.cc proxy_config_service_mac.c
> 
> Then the file history will be preserved (and the diffs will show up cleaner).

[hayato, 2009-12-10 05:48:17]

I uploaded a new patch, which was made by svn using 'svn cp'.
Except for tracking history of 'copy', nothing has changed since previous patch.

I'll land this after confirming that all tests passed on trybot.