chromeos: 1st draft of ProxyConfigService for chromeos...

chromeos: 1st draft of ProxyConfigService for chromeos

what this cl is:
- a REALLY bare skeleton implementation of ProxyConfigService for chromeos (lots of design details remain to be worked out on the chromeos side, but i needed to get something basic up)
- focused on getting chrome part correct, extracting design flow and implementation from linux variant that are relevant to chromeos, e.g.: 
  - provide access of ProxyConfigService interface on IO thread
  - provide methods on UI thread for UI to read/modify proxy config (like linux's GConf notifications on UI thread)
  - fetch initial config on UI thread (this is not absolutely necessary for chromeos, 'cos i don't use GConf, but i just follow linux for now)
  - however, the class is RefCountedThreadSafe (so that both net::ProxyService and DOMUI can access it), and the code resides in chrome/browser/chromeos dir instead of net/proxy.  
  - design details are in .h files
- initial config is hardcoded as pac script and loaded as owner (TODO: load this from cros settings persisted on chromeos device)
  - this should work like the current chromeos session manager which sets the auto-proxy environment variable in login script to the same pac script
- implement an augmented analogue to net::ProxyConfig to hold actual proxy config and UI settings (e.g. source and readonly attributes)
  - backend uses this as the main proxy config, and only converts it to net:::ProxyConfig for network stack on the IO thread in net::ProxyService::GetLatestProxyConfig.
  - UI methods also use this structure
- provide methods to get and set configs from UI thread 
  - UI can use dom_ui->GetProfile()->GetChromeOSProxyConfigServiceImpl() to access these methods on the UI thread
  - these methods are by no means final
    - i only added them here to verify the design flow that modifications can be made from UI thread and picked up by IO thread.
    - david and i will improve or modify them to whatever works best for frontend and backend.
- unittest that tests most functionalities: socks and bypass_rules will be enabled in a later cl.

TODOs after this cl:
- work with UI, load initial config as owner from cros settings
- implement policy mechanism, merge it with owner during initial load and modifications
- persist proxy settings
- etc etc etc

BUG=chromium-os:5127
TEST=nothing yet.


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

[Paweł Hajdan Jr., 2010-08-06 17:45:40]

Drive-by with a test comment. Please let me take another look before committing.

http://codereview.chromium.org/3047052/diff/1/5
File net/proxy/proxy_config_service_chromeos_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/1/5#newcode54
net/proxy/proxy_config_service_chromeos_unittest.cc:54: CHECK(it !=
settings.end()) << "key " << key << " not found";
Why CHECK? Can it be converted to something like this?

EXPECT_TRUE(it != settings.end());
if (it == settings.end())
  return dummy value;

This way we avoid crashing entire unit_tests binary when the test fails.

[kuan, 2010-08-12 21:21:22]

i've restructured and reimplemented the original cl per eric's suggestions on a
separate email thread.  pls take another look.  thanks.

http://codereview.chromium.org/3047052/diff/1/5
File net/proxy/proxy_config_service_chromeos_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/1/5#newcode54
net/proxy/proxy_config_service_chromeos_unittest.cc:54: CHECK(it !=
settings.end()) << "key " << key << " not found";
On 2010/08/06 17:45:40, Paweł Hajdan Jr. wrote:
> Why CHECK? Can it be converted to something like this?
> 
> EXPECT_TRUE(it != settings.end());
> if (it == settings.end())
>   return dummy value;
> 
> This way we avoid crashing entire unit_tests binary when the test fails.

Done.

[Paweł Hajdan Jr., 2010-08-12 21:30:21]

http://codereview.chromium.org/3047052/diff/8001/9004
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9004#newcode56
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:56: // We have to
return something, so simply use the first available entry.
Returning garbage has its own problems. It seems like this is only used by test
code. Can we convert the signature to bool Get(const std::string&, value_type*)?

[eroman, 2010-08-13 07:41:21]

This looks like a good start!

Some high level comments before I get caught up in nits:

(1) Please use ChromeThread::IO / ChromeThread::UI in place of raw MessageLoop*
throughout proxy_config_service_impl.cc.

The Linux implementation was forced to use MessageLoop pointers since net
doesn't have access to ChromeThread, but here you should be able to use
ChromeThread.

(2) I don't much like for the read-only bitfields, or policy map. I think it
would be clearer to represent all of this in a single data structure. Basically
the chromeos class can use an augmented analog to ProxyConfig internally, and
remap it to a ProxyConfig when it needs to pass it off to the network stack.

Here is a (simplified) proof of concept to illustrate:

struct MyServerForScheme {
  // Whether this came from the policy, or owner, or default config
  Source source;
  bool is_readonly;  // Whether
  ProxyServer server;
}

struct MyProxyConfig {
  // (would likely want a Mode enum to toggle between
  // proxy-per scheme, auto-detect, or PAC script specifier).

  MyServerForScheme http;
  MyServerForScheme https;
  MyServerForScheme ftp;
};

// Convert our simpler internal datastructure to a net::ProxyConfig
// (discarding all the read-only and source info).
void MyProxyConfigToProxyConfig(const MyProxyConfig&, net::ProxyConfig*);

// Converts a net::ProxyConfig to a MyProxyConfig. Would probably
// be needed since the shared code to get  proxy from environment
// variables will fill a net::ProxyConfig. This would default-intialize
// the read-only and source bits to something reasonable.
void ProxyConfigToMyProxyConfig(const net::ProxyConfig&, MyProxyConfig*);

This will be easier to maintain, since when you want to add more bits of
information it won't have to be offloaded into some other map.

(3) Please extract ProxyConfigServiceLinux::Delegate::GetConfigFromEnv rather
than duplicating it.

I understand from your comments that you intended to do this as a follow-up, but
I feel like it should be in place even for the initial commit as it involves a
lot of code and unittest code which is otherwise cluttering this CL.

If you want help extracting this let me know and we can figure something out. I
reckon we could do a separate commit beforehand that splits off the relevant
functions into its own file.

http://codereview.chromium.org/3047052/diff/8001/9001
File chrome/browser/chromeos/proxy_config_service.h (right):

http://codereview.chromium.org/3047052/diff/8001/9001#newcode19
chrome/browser/chromeos/proxy_config_service.h:19: explicit
ProxyConfigService(const scoped_refptr<ProxyConfigServiceImpl> impl)
nit. plz make a const-reference.

http://codereview.chromium.org/3047052/diff/8001/9001#newcode22
chrome/browser/chromeos/proxy_config_service.h:22: impl_ = NULL;
Can you remove this destructor? (will implicitly be derefed by the generated
destructor).

http://codereview.chromium.org/3047052/diff/8001/9002
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9002#newcode20
chrome/browser/chromeos/proxy_config_service_impl.cc:20: std::string
FixupProxyHostScheme(net::ProxyServer::Scheme scheme,
please remove.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode78
chrome/browser/chromeos/proxy_config_service_impl.cc:78: if (!io_loop_)
please don't handle this case. this was a hack needed for glib/kde dependency in
the linux one.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode90
chrome/browser/chromeos/proxy_config_service_impl.cc:90:
cached_config_.set_id(1);  // mark it as valid
please don't use set_id() anywhere in here  (conversely don't need to use
is_valid()).

Instead it will be simpler to just always keep it set to something valid.

I know ProxyConfigServiceLinux does this, but that class has a number of
problems.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode103
chrome/browser/chromeos/proxy_config_service_impl.cc:103: if
(!reference_config_.is_valid()) {
See comment above - it will be better to never do such is_valid() check, and
instead set reference_config_ to CreateDirect() above is something were to fail.

Hence this check should not be necessary.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode129
chrome/browser/chromeos/proxy_config_service_impl.cc:129: if
(!reference_config_.is_valid() || !new_config.Equals(reference_config_)) {
ditto: please remove the is_valid() check.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode163
chrome/browser/chromeos/proxy_config_service_impl.cc:163: *config =
cached_config_.is_valid() ?
Same comment as before, please remove the is_valid() check and just return
cached_config_.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode176
chrome/browser/chromeos/proxy_config_service_impl.cc:176: bool
ProxyConfigServiceImpl::GetProxyFromEnvVarForScheme(const char* variable,
Please remove this (and extract the functionality from linux version).

http://codereview.chromium.org/3047052/diff/8001/9002#newcode195
chrome/browser/chromeos/proxy_config_service_impl.cc:195: bool
ProxyConfigServiceImpl::GetProxyFromEnvVar(const char* variable,
remove

http://codereview.chromium.org/3047052/diff/8001/9002#newcode201
chrome/browser/chromeos/proxy_config_service_impl.cc:201: bool
ProxyConfigServiceImpl::GetConfigFromEnv(net::ProxyConfig* config) {
remove

http://codereview.chromium.org/3047052/diff/8001/9003
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/3047052/diff/8001/9003#newcode25
chrome/browser/chromeos/proxy_config_service_impl.h:25: // - is ref-counted and
thread-safe
nit: this wording is a bit misleading. it isn't actually threadsafe (some
methods expect to be called on UI thread others on IO thread), but it is
RefCountedThreadSafe.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode39
chrome/browser/chromeos/proxy_config_service_impl.h:39: //
ProxyConfigServiceImpl is created on the UI thread in
Nice comment blocks!

http://codereview.chromium.org/3047052/diff/8001/9003#newcode61
chrome/browser/chromeos/proxy_config_service_impl.h:61:
ProxyConfigServiceImpl();
Can you remove this default constructor, and just have the second constructor?
(passing NULL in the non-unittest case).

http://codereview.chromium.org/3047052/diff/8001/9003#newcode65
chrome/browser/chromeos/proxy_config_service_impl.h:65: virtual
~ProxyConfigServiceImpl() {}
Please move the definition to the .cc file.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode72
chrome/browser/chromeos/proxy_config_service_impl.h:72: void
SetupAndFetchInitialConfig(MessageLoop* ui_loop, MessageLoop* io_loop);
Can you just call the initialization directly from the constructor, hence not
need this function (at least not as a public method).

Also per earlier comment, can use ChromeThread so the |ui_loop| and |io_loop|
pointers should not be needed.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode92
chrome/browser/chromeos/proxy_config_service_impl.h:92: void
UISetProxyConfig(bool is_owner, const net::ProxyConfig& new_config);
what is |is_owner| needed for? it appears to be unused in that function.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode99
chrome/browser/chromeos/proxy_config_service_impl.h:99: enum ReadOnlyProperty {
nit: enum declaration should be at the top of this public section (before the
ctor).

http://codereview.chromium.org/3047052/diff/8001/9003#newcode131
chrome/browser/chromeos/proxy_config_service_impl.h:131: enum RulesSourceType {
enums should be at the start of this private section.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode133
chrome/browser/chromeos/proxy_config_service_impl.h:133: RULES_SOURCE_POLICY, 
// Configuration is from policy.
I don't know what these mean "from policy" versus "from owner". Are these
general chromeos things that a chromeos developer reading the code would
understand?

http://codereview.chromium.org/3047052/diff/8001/9003#newcode138
chrome/browser/chromeos/proxy_config_service_impl.h:138: enum RulesSourceIndex {
move this to start of private section.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode161
chrome/browser/chromeos/proxy_config_service_impl.h:161: bool
IsRuleReadOnly(RulesSourceIndex source_index, bool is_owner) const;
all methods should be listed before the data members.

http://codereview.chromium.org/3047052/diff/8001/9008
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9008#newcode333
chrome/browser/profile_impl.cc:333: chromeos_proxy_config_service_impl_ = NULL;
This should not be necessary, it should already be NULL here no?

http://codereview.chromium.org/3047052/diff/8001/9008#newcode507
chrome/browser/profile_impl.cc:507: chromeos_proxy_config_service_impl_ = NULL;
This shouldn't be necessary. (In fact it likely won't have any effect in
deleting the ProxyConfigServiceImpl earlier, since a reference will likely still
be held by the IO thread that won't be released until later).

http://codereview.chromium.org/3047052/diff/8001/9014
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9014#newcode618
net/proxy/proxy_service.cc:618: #elif defined(OS_LINUX) && !defined(OS_CHROMEOS)
How about adding the chomeos case above this line:

#elif defined(OS_CHROMEOS)
NOTREACHED() << ...;
return NULL;

Then the predicate here can remain as simply OS_LINUX.

http://codereview.chromium.org/3047052/diff/8001/9014#newcode638
net/proxy/proxy_service.cc:638: #elif defined(OS_CHROMEOS)
See comment above.

[kuan, 2010-08-18 16:33:20]

i addressed all ur comments in the latest uploaded snapshot, including the major
ones:
- changed to use ChromeThread
- created new augmented analog to net::ProxyConfig and removed readonly bits
- removed env vars implementation, 'cos chromeos really doesn't use them and
shld use cros settings instead, which i'll implement in upcoming cls.  for now,
i simply hardcode the proxy config to the same setting as that hardcoded in
chromeos login script.
pls take another look.  thanks.

http://codereview.chromium.org/3047052/diff/8001/9001
File chrome/browser/chromeos/proxy_config_service.h (right):

http://codereview.chromium.org/3047052/diff/8001/9001#newcode19
chrome/browser/chromeos/proxy_config_service.h:19: explicit
ProxyConfigService(const scoped_refptr<ProxyConfigServiceImpl> impl)
On 2010/08/13 07:41:21, eroman wrote:
> nit. plz make a const-reference.

Done.

http://codereview.chromium.org/3047052/diff/8001/9001#newcode22
chrome/browser/chromeos/proxy_config_service.h:22: impl_ = NULL;
On 2010/08/13 07:41:21, eroman wrote:
> Can you remove this destructor? (will implicitly be derefed by the generated
> destructor).

Done.

http://codereview.chromium.org/3047052/diff/8001/9002
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9002#newcode20
chrome/browser/chromeos/proxy_config_service_impl.cc:20: std::string
FixupProxyHostScheme(net::ProxyServer::Scheme scheme,
On 2010/08/13 07:41:21, eroman wrote:
> please remove.

Done.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode78
chrome/browser/chromeos/proxy_config_service_impl.cc:78: if (!io_loop_)
On 2010/08/13 07:41:21, eroman wrote:
> please don't handle this case. this was a hack needed for glib/kde dependency
in
> the linux one.

Done.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode90
chrome/browser/chromeos/proxy_config_service_impl.cc:90:
cached_config_.set_id(1);  // mark it as valid
On 2010/08/13 07:41:21, eroman wrote:
> please don't use set_id() anywhere in here  (conversely don't need to use
> is_valid()).
> 
> Instead it will be simpler to just always keep it set to something valid.
> 
> I know ProxyConfigServiceLinux does this, but that class has a number of
> problems.

Done.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode103
chrome/browser/chromeos/proxy_config_service_impl.cc:103: if
(!reference_config_.is_valid()) {
On 2010/08/13 07:41:21, eroman wrote:
> See comment above - it will be better to never do such is_valid() check, and
> instead set reference_config_ to CreateDirect() above is something were to
fail.
> 
> Hence this check should not be necessary.

Done.  Both cached_config_ and reference_config_ are init appropriately (either
hardcoded for now or as Direct) in constructor.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode129
chrome/browser/chromeos/proxy_config_service_impl.cc:129: if
(!reference_config_.is_valid() || !new_config.Equals(reference_config_)) {
On 2010/08/13 07:41:21, eroman wrote:
> ditto: please remove the is_valid() check.

Done.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode163
chrome/browser/chromeos/proxy_config_service_impl.cc:163: *config =
cached_config_.is_valid() ?
On 2010/08/13 07:41:21, eroman wrote:
> Same comment as before, please remove the is_valid() check and just return
> cached_config_.

Done.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode176
chrome/browser/chromeos/proxy_config_service_impl.cc:176: bool
ProxyConfigServiceImpl::GetProxyFromEnvVarForScheme(const char* variable,
On 2010/08/13 07:41:21, eroman wrote:
> Please remove this (and extract the functionality from linux version).

Done.  didn't extract from linux version either, 'cos chromeos really doesn't
use it.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode195
chrome/browser/chromeos/proxy_config_service_impl.cc:195: bool
ProxyConfigServiceImpl::GetProxyFromEnvVar(const char* variable,
On 2010/08/13 07:41:21, eroman wrote:
> remove

Done.

http://codereview.chromium.org/3047052/diff/8001/9002#newcode201
chrome/browser/chromeos/proxy_config_service_impl.cc:201: bool
ProxyConfigServiceImpl::GetConfigFromEnv(net::ProxyConfig* config) {
On 2010/08/13 07:41:21, eroman wrote:
> remove

Done.

http://codereview.chromium.org/3047052/diff/8001/9003
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/3047052/diff/8001/9003#newcode25
chrome/browser/chromeos/proxy_config_service_impl.h:25: // - is ref-counted and
thread-safe
On 2010/08/13 07:41:21, eroman wrote:
> nit: this wording is a bit misleading. it isn't actually threadsafe (some
> methods expect to be called on UI thread others on IO thread), but it is
> RefCountedThreadSafe.

Done.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode39
chrome/browser/chromeos/proxy_config_service_impl.h:39: //
ProxyConfigServiceImpl is created on the UI thread in
On 2010/08/13 07:41:21, eroman wrote:
> Nice comment blocks!

hehe.. u set a good example, and i tried to follow :)

http://codereview.chromium.org/3047052/diff/8001/9003#newcode61
chrome/browser/chromeos/proxy_config_service_impl.h:61:
ProxyConfigServiceImpl();
On 2010/08/13 07:41:21, eroman wrote:
> Can you remove this default constructor, and just have the second constructor?
> (passing NULL in the non-unittest case).

i initially did, but after changing everything to use ChromeThread, unit_test
won't work 'cos it has its own io_thread.  so, i had to keep this constructor,
and add the io_loop (and a chromeos's proxy config to init) to the other
constructor.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode65
chrome/browser/chromeos/proxy_config_service_impl.h:65: virtual
~ProxyConfigServiceImpl() {}
On 2010/08/13 07:41:21, eroman wrote:
> Please move the definition to the .cc file.

Done.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode72
chrome/browser/chromeos/proxy_config_service_impl.h:72: void
SetupAndFetchInitialConfig(MessageLoop* ui_loop, MessageLoop* io_loop);
On 2010/08/13 07:41:21, eroman wrote:
> Can you just call the initialization directly from the constructor, hence not
> need this function (at least not as a public method).
> 
> Also per earlier comment, can use ChromeThread so the |ui_loop| and |io_loop|
> pointers should not be needed.

Done.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode92
chrome/browser/chromeos/proxy_config_service_impl.h:92: void
UISetProxyConfig(bool is_owner, const net::ProxyConfig& new_config);
On 2010/08/13 07:41:21, eroman wrote:
> what is |is_owner| needed for? it appears to be unused in that function.

i'll probably need it later when i verify if config can be modified by current
user.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode99
chrome/browser/chromeos/proxy_config_service_impl.h:99: enum ReadOnlyProperty {
On 2010/08/13 07:41:21, eroman wrote:
> nit: enum declaration should be at the top of this public section (before the
> ctor).

this is removed.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode131
chrome/browser/chromeos/proxy_config_service_impl.h:131: enum RulesSourceType {
On 2010/08/13 07:41:21, eroman wrote:
> enums should be at the start of this private section.

this is removed.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode133
chrome/browser/chromeos/proxy_config_service_impl.h:133: RULES_SOURCE_POLICY, 
// Configuration is from policy.
On 2010/08/13 07:41:21, eroman wrote:
> I don't know what these mean "from policy" versus "from owner". Are these
> general chromeos things that a chromeos developer reading the code would
> understand?

yes, chromeos has concept of policy (which is enterprise) and owner (which is
the first user to use the device out-of-the-box.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode138
chrome/browser/chromeos/proxy_config_service_impl.h:138: enum RulesSourceIndex {
On 2010/08/13 07:41:21, eroman wrote:
> move this to start of private section.

removed.

http://codereview.chromium.org/3047052/diff/8001/9003#newcode161
chrome/browser/chromeos/proxy_config_service_impl.h:161: bool
IsRuleReadOnly(RulesSourceIndex source_index, bool is_owner) const;
On 2010/08/13 07:41:21, eroman wrote:
> all methods should be listed before the data members.

Done.

http://codereview.chromium.org/3047052/diff/8001/9004
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9004#newcode56
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:56: // We have to
return something, so simply use the first available entry.
On 2010/08/12 21:30:21, Paweł Hajdan Jr. wrote:
> Returning garbage has its own problems. It seems like this is only used by
test
> code. Can we convert the signature to bool Get(const std::string&,
value_type*)?

function is removed.

http://codereview.chromium.org/3047052/diff/8001/9008
File chrome/browser/profile_impl.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9008#newcode333
chrome/browser/profile_impl.cc:333: chromeos_proxy_config_service_impl_ = NULL;
On 2010/08/13 07:41:21, eroman wrote:
> This should not be necessary, it should already be NULL here no?

Done.

http://codereview.chromium.org/3047052/diff/8001/9008#newcode507
chrome/browser/profile_impl.cc:507: chromeos_proxy_config_service_impl_ = NULL;
On 2010/08/13 07:41:21, eroman wrote:
> This shouldn't be necessary. (In fact it likely won't have any effect in
> deleting the ProxyConfigServiceImpl earlier, since a reference will likely
still
> be held by the IO thread that won't be released until later).

Done.

http://codereview.chromium.org/3047052/diff/8001/9014
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/3047052/diff/8001/9014#newcode618
net/proxy/proxy_service.cc:618: #elif defined(OS_LINUX) && !defined(OS_CHROMEOS)
On 2010/08/13 07:41:21, eroman wrote:
> How about adding the chomeos case above this line:
> 
> #elif defined(OS_CHROMEOS)
> NOTREACHED() << ...;
> return NULL;
> 
> Then the predicate here can remain as simply OS_LINUX.

Done.

http://codereview.chromium.org/3047052/diff/8001/9014#newcode638
net/proxy/proxy_service.cc:638: #elif defined(OS_CHROMEOS)
On 2010/08/13 07:41:21, eroman wrote:
> See comment above.

Done.

[Paweł Hajdan Jr., 2010-08-18 18:07:52]

LGTM

[eroman, 2010-08-18 18:50:37]

http://codereview.chromium.org/3047052/diff/8001/9003
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/3047052/diff/8001/9003#newcode61
chrome/browser/chromeos/proxy_config_service_impl.h:61:
ProxyConfigServiceImpl();
On 2010/08/18 16:33:20, kuan wrote:
> On 2010/08/13 07:41:21, eroman wrote:
> > Can you remove this default constructor, and just have the second
constructor?
> > (passing NULL in the non-unittest case).
> 
> i initially did, but after changing everything to use ChromeThread, unit_test
> won't work 'cos it has its own io_thread.  so, i had to keep this constructor,
> and add the io_loop (and a chromeos's proxy config to init) to the other
> constructor.

You shouldn't need a raw MessageLoop* just for the unit_tests:
instead for unit-tests it is common to fake ChromeThread.

For example, you could map both ChromeThread::IO and ChromeThread::UI to the
same message loop, and avoid even needing to create other threads.

ChromeThread simply maintains a list based on the last instantiated object for
that type. So you can use the special constructor ChromeThread(ID, MessageLoop*)
and as long as this is in scope throughout your test the ID will get mapped to
the message loop of choice.

For an example that does this see chrome/browser/net/preditor_unittest.cc

[eroman, 2010-08-18 20:31:19]

http://codereview.chromium.org/3047052/diff/8001/9003
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/3047052/diff/8001/9003#newcode39
chrome/browser/chromeos/proxy_config_service_impl.h:39: //
ProxyConfigServiceImpl is created on the UI thread in
On 2010/08/18 16:33:20, kuan wrote:
> On 2010/08/13 07:41:21, eroman wrote:
> > Nice comment blocks!
> 
> hehe.. u set a good example, and i tried to follow :)
> 

To be clear, I did not write ProxyConfigServiceLinux... so the credit belongs to
sdoyon who did a great job with the documentation!

http://codereview.chromium.org/3047052/diff/14003/41001
File chrome/browser/chromeos/proxy_config_service.h (right):

http://codereview.chromium.org/3047052/diff/14003/41001#newcode14
chrome/browser/chromeos/proxy_config_service.h:14: // and fowards
net::ProxyConfigService interface to the actual implementation,
wording nit: "and forwards" --> "that forwards".

http://codereview.chromium.org/3047052/diff/14003/41002
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/14003/41002#newcode28
chrome/browser/chromeos/proxy_config_service_impl.cc:28: return
kSources[static_cast<int>(source)];
Please use a switch here instead.
Switch is more resistant to changes made to the enum, and easer to read IMO.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode35
chrome/browser/chromeos/proxy_config_service_impl.cc:35: std::ostream&
operator<<(std::ostream& out,
I wander if all of this "to string" code can be moved to the unit-test.

(As far as I can tell, it is only used for defining expectations in the
unit-test)

Although I suppose it could be useful for debugging to have a .ToString() method
you can call.

(There are historical reasons why ProxyConfig has a ToString() method, but it
will likely be deleted in the future).

http://codereview.chromium.org/3047052/diff/14003/41002#newcode39
chrome/browser/chromeos/proxy_config_service_impl.cc:39: if
(proxy.pac_url.is_valid())
nit: I think you can avoid this check, and just output:

proxy.possibly_invalid_spec()

(should probably only be outputting AutomaticProxy when it makes sense, notably
the mode was MODE_AUTO_DETECT).

http://codereview.chromium.org/3047052/diff/14003/41002#newcode61
chrome/browser/chromeos/proxy_config_service_impl.cc:61: out << "Auto
detection:\n" << config.automatic_proxy;
I don't understand why the to stringing of "automatic_proxy". 

I would expect MODE_AUTO_DETECT to correspond with "do web proxy autodetection",
which requires no additional parameters.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode79
chrome/browser/chromeos/proxy_config_service_impl.cc:79: out << "Bypass list: ";
note that the bypass list should only be output for

MODE_SINGLE_PROXY and for MODE_PROXY_PER_SCHEME.

The bypass list will not be applied under the other modes.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode108
chrome/browser/chromeos/proxy_config_service_impl.cc:108: Value*
ProxyConfigServiceImpl::ProxyConfig::ToValue() const {
Why is this needed?
Smells like copy-paste to me :)
Please delete.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode125
chrome/browser/chromeos/proxy_config_service_impl.cc:125:
GURL("http://proxyconfig.corp.google.com/wpad.dat");
I understand this is a temporary hard-coding, however I have some suggestions on
making it more generic:

(a) could change it to "http://wpad/wpad.dat".
for goog corp these are equivalent, however for other networks http://wpad/ is
more generic as it is the default host for auto-discovery.

(b) or simply set "auto_detect = true".

http://codereview.chromium.org/3047052/diff/14003/41002#newcode152
chrome/browser/chromeos/proxy_config_service_impl.cc:152:
config->automatic_proxy.is_readonly =
Populating all of these read-only members seems like overkill.

Wouldn't it be simpler to wrap the logic inside of a generic method, like:

ProxyConfig::ManualProxy::IsReadOnlyForUser(bool is_owner) {
  return source == ....;
}

That should get rid of this ugly switch statement, and make UIGetProxyConfig()
not require the "is_owner" parameter.


The other problem with having a "read-only" field is it makes the
UISetProxyConfig() method more confusing, since it isn't obvious that the
"read-only" fields actually are meaningless when setting.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode175
chrome/browser/chromeos/proxy_config_service_impl.cc:175: void
ProxyConfigServiceImpl::UISetProxyConfig(bool is_owner,
suggestion: I suggest keeping this one private, and instead having a simpler
public API that simplifies the modality:

UISetProxyConfigToSingleProxy(const ProxyServer&);
UISetProxyConfigToAutoDetect();
UISetProxyConfigToDirect();
UISetProxyConfigToProxyPerScheme({map from scheme to proxy server});

The advantage is (a) simpler to construct valid ProxyConfigs this way, and (b)
it removes confusion on setting things like "source". Conceptually I imagine the
"source" is an inferred property that applies only to where it was "read" from,
and has no impact for writing the value out.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode180
chrome/browser/chromeos/proxy_config_service_impl.cc:180: // See if it is
different from what we had before.
I presume in the future this code will also write the value out to a chromeos
backing store here?

http://codereview.chromium.org/3047052/diff/14003/41002#newcode181
chrome/browser/chromeos/proxy_config_service_impl.cc:181: if
(!new_config.Equals(reference_config_)) {
I don't think this is necessary.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode188
chrome/browser/chromeos/proxy_config_service_impl.cc:188:
ChromeThread::PostTask(ChromeThread::IO, FROM_HERE, task);
Strictly speaking you should check the return value here.

If PostTask returns false delete |task| to avoid it from leaking.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode289
chrome/browser/chromeos/proxy_config_service_impl.cc:289: LOG(INFO) << "Proxy
configuration changed";
Note that if you are debugging, you can observe the proxy changes using
about:net-internals -- it emits an event showing the new and old values each
time it changes.

http://codereview.chromium.org/3047052/diff/14003/41003
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/3047052/diff/14003/41003#newcode43
chrome/browser/chromeos/proxy_config_service_impl.h:43: //
SetupAndFetchInitialConfig() is immediately called to synchronously fetch
no longer a SetupAndFetch..

http://codereview.chromium.org/3047052/diff/14003/41003#newcode48
chrome/browser/chromeos/proxy_config_service_impl.h:48: // From the IO thread,
it is accessed periodically through the
"the its" --> "the"

http://codereview.chromium.org/3047052/diff/14003/41003#newcode65
chrome/browser/chromeos/proxy_config_service_impl.h:65: // - source of
configuration (i.e. where it was picked up from, e.g. policy
nit: "a lot of "i.e" and "e.g" in this paragraph, consider simplifying.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode104
chrome/browser/chromeos/proxy_config_service_impl.h:104: struct ManualProxy {
How about using inheritance here?
Perhaps something like this:

struct Setting {
  Setting() : source(NONE) {}
  Source source;
  bool CanBeWrittenByUser(bool user_is_owner);
}

struct AutomaticProxy : public Setting {
  GURL pac_url;
};

struct ManualProxy : public Setting {
  net::ProxyServer server;
};

http://codereview.chromium.org/3047052/diff/14003/41003#newcode118
chrome/browser/chromeos/proxy_config_service_impl.h:118: bool Equals(const
ProxyConfig& other) const {
Is this actually necessary?

(Equals() methods are difficult to maintain, don't add one unless you need it).

I commented in the location where you use Equals(), and I don't think it is
necessary.

Although if you really needed some form of equality could always do:

return
ChromeosProxyConfigToNetProxyConfig(*this).ChromeosProxyConfigToNetProxyConfig(ToNetProxyConfig(other));

http://codereview.chromium.org/3047052/diff/14003/41003#newcode134
chrome/browser/chromeos/proxy_config_service_impl.h:134: Value* ToValue() const;
Remove.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode186
chrome/browser/chromeos/proxy_config_service_impl.h:186: void
ChromeosProxyConfigToNetProxyConfig(const ProxyConfig &chromeos_config,
I suggest making this a method of ProxyConfigServiceImpl::ProxyConfig

http://codereview.chromium.org/3047052/diff/14003/41004
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/14003/41004#newcode25
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:25: class
SynchConfigGetter {
I don't think any of this is needed.

You can run the tests entirely on one thread (by having ChromeThread::UI and
ChromeThread::IO both point at the same message loop.

[kuan, 2010-08-20 16:56:12]

i've addressed all ur comments, except for the unittest, in the latest snapshot.
 cld u pls first take a look at the 3 files: proxy_config_service.h,
proxy_config_service_impl.h/cc?  i'll need more time to do the unittest, but
i'll like the config-service part finalized so that dhg can proceed with ui.
thanks.

http://codereview.chromium.org/3047052/diff/14003/41001
File chrome/browser/chromeos/proxy_config_service.h (right):

http://codereview.chromium.org/3047052/diff/14003/41001#newcode14
chrome/browser/chromeos/proxy_config_service.h:14: // and fowards
net::ProxyConfigService interface to the actual implementation,
On 2010/08/18 20:31:19, eroman wrote:
> wording nit: "and forwards" --> "that forwards".

Done.

http://codereview.chromium.org/3047052/diff/14003/41002
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/14003/41002#newcode28
chrome/browser/chromeos/proxy_config_service_impl.cc:28: return
kSources[static_cast<int>(source)];
On 2010/08/18 20:31:19, eroman wrote:
> Please use a switch here instead.
> Switch is more resistant to changes made to the enum, and easer to read IMO.

Done.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode35
chrome/browser/chromeos/proxy_config_service_impl.cc:35: std::ostream&
operator<<(std::ostream& out,
On 2010/08/18 20:31:19, eroman wrote:
> I wander if all of this "to string" code can be moved to the unit-test.
> 
> (As far as I can tell, it is only used for defining expectations in the
> unit-test)
> 
> Although I suppose it could be useful for debugging to have a .ToString()
method
> you can call.
> 
> (There are historical reasons why ProxyConfig has a ToString() method, but it
> will likely be deleted in the future).

i'm actually using it in the net_internals_ui.cc for my own debugging.  so yeah,
it helps, for now anyway.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode39
chrome/browser/chromeos/proxy_config_service_impl.cc:39: if
(proxy.pac_url.is_valid())
On 2010/08/18 20:31:19, eroman wrote:
> nit: I think you can avoid this check, and just output:
> 
> proxy.possibly_invalid_spec()
> 
> (should probably only be outputting AutomaticProxy when it makes sense,
notably
> the mode was MODE_AUTO_DETECT).

Done.  i removed this function and did the actual stringing in the ProxyConfig
function below.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode61
chrome/browser/chromeos/proxy_config_service_impl.cc:61: out << "Auto
detection:\n" << config.automatic_proxy;
On 2010/08/18 20:31:19, eroman wrote:
> I don't understand why the to stringing of "automatic_proxy". 
> 
> I would expect MODE_AUTO_DETECT to correspond with "do web proxy
autodetection",
> which requires no additional parameters.

Done. see comments above.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode79
chrome/browser/chromeos/proxy_config_service_impl.cc:79: out << "Bypass list: ";
On 2010/08/18 20:31:19, eroman wrote:
> note that the bypass list should only be output for
> 
> MODE_SINGLE_PROXY and for MODE_PROXY_PER_SCHEME.
> 
> The bypass list will not be applied under the other modes.

Done.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode108
chrome/browser/chromeos/proxy_config_service_impl.cc:108: Value*
ProxyConfigServiceImpl::ProxyConfig::ToValue() const {
On 2010/08/18 20:31:19, eroman wrote:
> Why is this needed?
> Smells like copy-paste to me :)
> Please delete.

Done.  hehe, guilty as charged, i did change the TODO to my name though :)

http://codereview.chromium.org/3047052/diff/14003/41002#newcode125
chrome/browser/chromeos/proxy_config_service_impl.cc:125:
GURL("http://proxyconfig.corp.google.com/wpad.dat");
On 2010/08/18 20:31:19, eroman wrote:
> I understand this is a temporary hard-coding, however I have some suggestions
on
> making it more generic:
> 
> (a) could change it to "http://wpad/wpad.dat".
> for goog corp these are equivalent, however for other networks http://wpad/ is
> more generic as it is the default host for auto-discovery.
> 
> (b) or simply set "auto_detect = true".

Done.  i set it to auto-detect.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode152
chrome/browser/chromeos/proxy_config_service_impl.cc:152:
config->automatic_proxy.is_readonly =
On 2010/08/18 20:31:19, eroman wrote:
> Populating all of these read-only members seems like overkill.
> 
> Wouldn't it be simpler to wrap the logic inside of a generic method, like:
> 
> ProxyConfig::ManualProxy::IsReadOnlyForUser(bool is_owner) {
>   return source == ....;
> }
> 
> That should get rid of this ugly switch statement, and make UIGetProxyConfig()
> not require the "is_owner" parameter.
> 
> 
> The other problem with having a "read-only" field is it makes the
> UISetProxyConfig() method more confusing, since it isn't obvious that the
> "read-only" fields actually are meaningless when setting.

Done.  i removed all these, since there's no readonly field in the structures
anymore.  instead, UI will call CanBeWrittenByUser to check.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode175
chrome/browser/chromeos/proxy_config_service_impl.cc:175: void
ProxyConfigServiceImpl::UISetProxyConfig(bool is_owner,
On 2010/08/18 20:31:19, eroman wrote:
> suggestion: I suggest keeping this one private, and instead having a simpler
> public API that simplifies the modality:
> 
> UISetProxyConfigToSingleProxy(const ProxyServer&);
> UISetProxyConfigToAutoDetect();
> UISetProxyConfigToDirect();
> UISetProxyConfigToProxyPerScheme({map from scheme to proxy server});
> 
> The advantage is (a) simpler to construct valid ProxyConfigs this way, and (b)
> it removes confusion on setting things like "source". Conceptually I imagine
the
> "source" is an inferred property that applies only to where it was "read"
from,
> and has no impact for writing the value out.
> 
> 

Done.  i added UISetProxyConfigToPACScript.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode180
chrome/browser/chromeos/proxy_config_service_impl.cc:180: // See if it is
different from what we had before.
On 2010/08/18 20:31:19, eroman wrote:
> I presume in the future this code will also write the value out to a chromeos
> backing store here?

Done.  i added TODO.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode181
chrome/browser/chromeos/proxy_config_service_impl.cc:181: if
(!new_config.Equals(reference_config_)) {
On 2010/08/18 20:31:19, eroman wrote:
> I don't think this is necessary.

Done.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode188
chrome/browser/chromeos/proxy_config_service_impl.cc:188:
ChromeThread::PostTask(ChromeThread::IO, FROM_HERE, task);
On 2010/08/18 20:31:19, eroman wrote:
> Strictly speaking you should check the return value here.
> 
> If PostTask returns false delete |task| to avoid it from leaking.

Done.

http://codereview.chromium.org/3047052/diff/14003/41002#newcode289
chrome/browser/chromeos/proxy_config_service_impl.cc:289: LOG(INFO) << "Proxy
configuration changed";
On 2010/08/18 20:31:19, eroman wrote:
> Note that if you are debugging, you can observe the proxy changes using
> about:net-internals -- it emits an event showing the new and old values each
> time it changes.

yes, i've actually been using that page to verify that get/set from ui are
picked up by io thread. :)

http://codereview.chromium.org/3047052/diff/14003/41003
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/3047052/diff/14003/41003#newcode43
chrome/browser/chromeos/proxy_config_service_impl.h:43: //
SetupAndFetchInitialConfig() is immediately called to synchronously fetch
On 2010/08/18 20:31:19, eroman wrote:
> no longer a SetupAndFetch..

Done.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode48
chrome/browser/chromeos/proxy_config_service_impl.h:48: // From the IO thread,
it is accessed periodically through the
On 2010/08/18 20:31:19, eroman wrote:
> "the its" --> "the"

Done.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode65
chrome/browser/chromeos/proxy_config_service_impl.h:65: // - source of
configuration (i.e. where it was picked up from, e.g. policy
On 2010/08/18 20:31:19, eroman wrote:
> nit: "a lot of "i.e" and "e.g" in this paragraph, consider simplifying.

Done.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode104
chrome/browser/chromeos/proxy_config_service_impl.h:104: struct ManualProxy {
On 2010/08/18 20:31:19, eroman wrote:
> How about using inheritance here?
> Perhaps something like this:
> 
> struct Setting {
>   Setting() : source(NONE) {}
>   Source source;
>   bool CanBeWrittenByUser(bool user_is_owner);
> }
> 
> struct AutomaticProxy : public Setting {
>   GURL pac_url;
> };
> 
> struct ManualProxy : public Setting {
>   net::ProxyServer server;
> };

Done.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode118
chrome/browser/chromeos/proxy_config_service_impl.h:118: bool Equals(const
ProxyConfig& other) const {
On 2010/08/18 20:31:19, eroman wrote:
> Is this actually necessary?
> 
> (Equals() methods are difficult to maintain, don't add one unless you need
it).
> 
> I commented in the location where you use Equals(), and I don't think it is
> necessary.
> 
> Although if you really needed some form of equality could always do:
> 
> return
>
ChromeosProxyConfigToNetProxyConfig(*this).ChromeosProxyConfigToNetProxyConfig(ToNetProxyConfig(other));

Done.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode134
chrome/browser/chromeos/proxy_config_service_impl.h:134: Value* ToValue() const;
On 2010/08/18 20:31:19, eroman wrote:
> Remove.

Done.

http://codereview.chromium.org/3047052/diff/14003/41003#newcode186
chrome/browser/chromeos/proxy_config_service_impl.h:186: void
ChromeosProxyConfigToNetProxyConfig(const ProxyConfig &chromeos_config,
On 2010/08/18 20:31:19, eroman wrote:
> I suggest making this a method of ProxyConfigServiceImpl::ProxyConfig

Done.

[eroman, 2010-08-20 18:48:27]

http://codereview.chromium.org/3047052/diff/19003/52002
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/19003/52002#newcode24
chrome/browser/chromeos/proxy_config_service_impl.cc:24: source_name =
"SOURCE_NONE";
This switch can be simplified (Please don't use the temporary variable for
returning):

switch (source) {
  case XXX:
    return "XXX";
  case YYY:
    return "YYY";
  ...
}

http://codereview.chromium.org/3047052/diff/19003/52002#newcode26
chrome/browser/chromeos/proxy_config_service_impl.cc:26: case
chromeos::ProxyConfigServiceImpl::ProxyConfig::SOURCE_POLICY:
the "chromeos::" prefix is not needed anywhere in this file.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode52
chrome/browser/chromeos/proxy_config_service_impl.cc:52: <<
SourceToString(config.automatic_proxy.source) << "\n";
Why output the automatic_proxy's source here? shouldn't automatic_proxy only be
relevant to MODE_PAC_SCRIPT ?

http://codereview.chromium.org/3047052/diff/19003/52002#newcode56
chrome/browser/chromeos/proxy_config_service_impl.cc:56: <<
SourceToString(config.automatic_proxy.source) << "\n";
ditto.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode108
chrome/browser/chromeos/proxy_config_service_impl.cc:108: // Setting can only be
written by user user is owner and setting is not from
something doesn't seem right with this phrase. "user user is owner"

http://codereview.chromium.org/3047052/diff/19003/52002#newcode127
chrome/browser/chromeos/proxy_config_service_impl.cc:127: *net_config =
net::ProxyConfig::CreateDirect();
In place of CreateDirection(), how about just net::ProxyConfig().

http://codereview.chromium.org/3047052/diff/19003/52002#newcode137
chrome/browser/chromeos/proxy_config_service_impl.cc:137:
net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
Why use SINGLE_PROXY for socks?

[kuan, 2010-08-20 21:26:08]

i've addressed ur comments in the latest snapshot (patch 6).  again, unittest is
not ready yet.  pls take another look, thanks.

http://codereview.chromium.org/3047052/diff/19003/52002
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/19003/52002#newcode24
chrome/browser/chromeos/proxy_config_service_impl.cc:24: source_name =
"SOURCE_NONE";
On 2010/08/20 18:48:28, eroman wrote:
> This switch can be simplified (Please don't use the temporary variable for
> returning):
> 
> switch (source) {
>   case XXX:
>     return "XXX";
>   case YYY:
>     return "YYY";
>   ...
> }

Done.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode26
chrome/browser/chromeos/proxy_config_service_impl.cc:26: case
chromeos::ProxyConfigServiceImpl::ProxyConfig::SOURCE_POLICY:
On 2010/08/20 18:48:28, eroman wrote:
> the "chromeos::" prefix is not needed anywhere in this file.

Done.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode52
chrome/browser/chromeos/proxy_config_service_impl.cc:52: <<
SourceToString(config.automatic_proxy.source) << "\n";
On 2010/08/20 18:48:28, eroman wrote:
> Why output the automatic_proxy's source here? shouldn't automatic_proxy only
be
> relevant to MODE_PAC_SCRIPT ?

it's for debugging to make sure the source is set correctly.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode56
chrome/browser/chromeos/proxy_config_service_impl.cc:56: <<
SourceToString(config.automatic_proxy.source) << "\n";
On 2010/08/20 18:48:28, eroman wrote:
> ditto.

see my reply abv.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode108
chrome/browser/chromeos/proxy_config_service_impl.cc:108: // Setting can only be
written by user user is owner and setting is not from
On 2010/08/20 18:48:28, eroman wrote:
> something doesn't seem right with this phrase. "user user is owner"

Done.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode127
chrome/browser/chromeos/proxy_config_service_impl.cc:127: *net_config =
net::ProxyConfig::CreateDirect();
On 2010/08/20 18:48:28, eroman wrote:
> In place of CreateDirection(), how about just net::ProxyConfig().

Done.  also changed the one above and the one below.

http://codereview.chromium.org/3047052/diff/19003/52002#newcode137
chrome/browser/chromeos/proxy_config_service_impl.cc:137:
net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
On 2010/08/20 18:48:28, eroman wrote:
> Why use SINGLE_PROXY for socks?

apparently, i misunderstood the linux code; i've changed to treat socks_proxy
like the others.

[eroman, 2010-08-20 22:37:53]

Other than the unittests, LGTM

http://codereview.chromium.org/3047052/diff/19003/52002
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/19003/52002#newcode137
chrome/browser/chromeos/proxy_config_service_impl.cc:137:
net::ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
On 2010/08/20 21:26:08, kuan wrote:
> On 2010/08/20 18:48:28, eroman wrote:
> > Why use SINGLE_PROXY for socks?
> 
> apparently, i misunderstood the linux code; i've changed to treat socks_proxy
> like the others.

I am going to rename socks_proxy to make this more clear
(http://codereview.chromium.org/3146029/show)

http://codereview.chromium.org/3047052/diff/48002/30003#newcode27
chrome/browser/chromeos/proxy_config_service_impl.cc:27: source_name =
"SOURCE_POLICY";
Delete.

http://codereview.chromium.org/3047052/diff/48002/30003#newcode110
chrome/browser/chromeos/proxy_config_service_impl.cc:110: return user_is_owner
&& source != ProxyConfig::SOURCE_POLICY;
This should remain as CreateDirect().

http://codereview.chromium.org/3047052/diff/48002/30005
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/48002/30005#newcode40
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:40: Wait();
There is no need to do this Wait() on init, please remove.

http://codereview.chromium.org/3047052/diff/48002/30005#newcode52
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:52: Wait();
There is no need to do this Wait() on exit. If the thread is owned then stopping
the thread will necessarily drain all tasks.

http://codereview.chromium.org/3047052/diff/48002/30005#newcode478
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:478:
sync_config_getter.SyncGetLatestProxyConfig(&io_config_2);
Should test that the observer got called too.

[kuan, 2010-08-23 13:39:18]

i've addressed ur comments for proxy_config_service_impl.cc in patch 7 and
rewrote unittest for proxy_config_service_impl_unittest.cc in patches 7 and 8.
the other files actually only contain differences on trunk, so u can ignore
them.
thanks.

http://codereview.chromium.org/3047052/diff/48002/30003
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/3047052/diff/48002/30003#newcode27
chrome/browser/chromeos/proxy_config_service_impl.cc:27: break;
On 2010/08/20 22:37:53, eroman wrote:
> Delete.

Done.

http://codereview.chromium.org/3047052/diff/48002/30003#newcode110
chrome/browser/chromeos/proxy_config_service_impl.cc:110: *net_config =
net::ProxyConfig();
On 2010/08/20 22:37:53, eroman wrote:
> This should remain as CreateDirect().

Done.

http://codereview.chromium.org/3047052/diff/48002/30005
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/48002/30005#newcode40
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:40: Wait();
On 2010/08/20 22:37:53, eroman wrote:
> There is no need to do this Wait() on init, please remove.

Done.

http://codereview.chromium.org/3047052/diff/48002/30005#newcode52
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:52: Wait();
On 2010/08/20 22:37:53, eroman wrote:
> There is no need to do this Wait() on exit. If the thread is owned then
stopping
> the thread will necessarily drain all tasks.

Done.

http://codereview.chromium.org/3047052/diff/48002/30005#newcode478
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:478:
sync_config_getter.SyncGetLatestProxyConfig(&io_config_2);
On 2010/08/20 22:37:53, eroman wrote:
> Should test that the observer got called too.

Done.

[eroman, 2010-08-23 20:18:12]

LGTM.

I ran out of steam looking through the unittest, but overall looked good.

http://codereview.chromium.org/3047052/diff/19004/67004
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/19004/67004#newcode264
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:264: // Takes
ownership of |config_service|.
What does "take ownership" of a refcounted value mean?

http://codereview.chromium.org/3047052/diff/19004/67004#newcode269
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:269: void
SetAutomaticProxy(ProxyConfigServiceImpl::ProxyConfig::Mode mode,
Please place each argument on its own line. See
http://dev.chromium.org/developers/coding-style for more details.

(it talks about it in the context of function calls, but applies the same for
function declarations).

http://codereview.chromium.org/3047052/diff/19004/67004#newcode279
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:279: void
SetManualProxy(ProxyConfigServiceImpl::ProxyConfig::Mode mode,
ditto.

http://codereview.chromium.org/3047052/diff/19004/67004#newcode290
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:290: void
InitConfigWithTestInput(const Input& input,
ditto.

http://codereview.chromium.org/3047052/diff/19004/67004#newcode307
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:307: case
MK_MODE(PROXY_PER_SCHEME):
There is a lot of boiler plate code in this file, i wander if it can be trimmed
down?

http://codereview.chromium.org/3047052/diff/19004/67004#newcode437
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:437:
config_service()->UISetProxyConfigToProxyPerScheme("http",
please use parens around multi line ifs. also see style guide for recommended
way to break long function call.

[kuan, 2010-08-24 16:31:39]

just fyi: i've addressed ur comments in the latest snapshot.  since u've already
lgtm'ed, i'll be submitting soon.
thanks!

http://codereview.chromium.org/3047052/diff/19004/67004
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/3047052/diff/19004/67004#newcode264
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:264: // Takes
ownership of |config_service|.
On 2010/08/23 20:18:13, eroman wrote:
> What does "take ownership" of a refcounted value mean?

Done. copied blindly, removed now.

http://codereview.chromium.org/3047052/diff/19004/67004#newcode269
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:269: void
SetAutomaticProxy(ProxyConfigServiceImpl::ProxyConfig::Mode mode,
On 2010/08/23 20:18:13, eroman wrote:
> Please place each argument on its own line. See
> http://dev.chromium.org/developers/coding-style for more details.
> 
> (it talks about it in the context of function calls, but applies the same for
> function declarations).

Done.

http://codereview.chromium.org/3047052/diff/19004/67004#newcode279
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:279: void
SetManualProxy(ProxyConfigServiceImpl::ProxyConfig::Mode mode,
On 2010/08/23 20:18:13, eroman wrote:
> ditto.

Done.

http://codereview.chromium.org/3047052/diff/19004/67004#newcode290
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:290: void
InitConfigWithTestInput(const Input& input,
On 2010/08/23 20:18:13, eroman wrote:
> ditto.

Done.

http://codereview.chromium.org/3047052/diff/19004/67004#newcode307
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:307: case
MK_MODE(PROXY_PER_SCHEME):
On 2010/08/23 20:18:13, eroman wrote:
> There is a lot of boiler plate code in this file, i wander if it can be
trimmed
> down?

i moved the if check to inside SetManualProxy. per our discussion, this is ok.

http://codereview.chromium.org/3047052/diff/19004/67004#newcode437
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:437:
config_service()->UISetProxyConfigToProxyPerScheme("http",
On 2010/08/23 20:18:13, eroman wrote:
> please use parens around multi line ifs. also see style guide for recommended
> way to break long function call.

Done.