redesign and reimplement proxy config service and tracker, revise proxy ui on cros

redesign and reimplement proxy config service and tracker, revise proxy ui on cros
* original plan was to revise proxy ui on cros for all proxy sources and
  network types, refer to bug rpt 21219 for ui details
* redesign and reimplement PrefProxyConfigTracker and PrefProxyConfigService
  by switching the inter-dependencies between the two and renaming the latter
  - PrefProxyConfigService used to observe PrefProxyConfigTracker and retrieve
    new prefs proxy from the latter whenever the former gets notification of
    proxy change from prefs or delegate service to determine effective proxy
    config. Now, PrefProxyConfigTracker pushes new prefs proxy to the renamed
    ChromeProxyConfigService on the IO thread; the latter then uses it and proxy
    from delegate service to determine effective proxy config.
  - remove all thread-switching in PrefProxyConfigTracker which now lives and
    runs on UI thread, except for when it pushes proxy config to
    ChromeProxyConfigService on IO thread
  - enhances and moves ConfigState definition to namespace ProxyPrefs to
    indicate source of proxy config (policy, extension, other-precede, system,
    fallback) so that PrefProxyConfigTracker, ChromeProxyConfigService and
    chromeos::ProxyConfigServiceImpl can acccess it
  - extract code for deciding effective proxy config from PrefProxyConfigService
    into static PrefProxyConfigTracker method to be shared by all classes
* re-design and re-implement chromeos::ProxyConfigServiceImpl
  - this now extends PrefProxyConfigTracker to act as a special of "prefs",
    handles all proxy changes from network and prefs notifications and
    uses PrefProxyConfigTracker to push effective proxy to
    ChromeProxyConfigService and hence to network stack
  - remove wrapper chromeos::ProxyConfigService
  - this is the authority on cros, and ChromeProxyConfigService does not have
    a delegat service.
  - provide proxy config to ui for all proxy sources: policy, extension,
    network or recommended (in order of precedence), also indicates if proxy is
    user-modifiable
  - handle user profile changes to use correct PrefProxyConfigTracker
* modify CoreOptionsHandler and CoreChromeosOptionsHandler to implement dynamic
  monitoring of prefs::kProxy and on-the-fly ui changes of affected
  use-shared-proxies user pref
* modify ui native code, html, css and javascript for internet options and
  proxy pages to show yellow banner with different messages (same banner as
  options page when there's policy), en/dis-able ui fields, set current user
  profile in backend
* modify network dropdown menu to always show proxy settings menu item as long
  as there's a network
* modify automation tests, PrefProxyConfigTracker and ProxyConfigSerivceImpl
  unittests to handle redesign, fix unittests that fail due to redesign


BUG=chromium-os:20679, chromium-os:21219
TEST=verify per bugs rpts
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108616

[kuan, 2011-10-04 16:40:57]

it's difficult to split the files for review, 'cos the files are related and
provide context for each other.  but, if u absolutely have to, i suggest the
following:
- mnissler: generated_resources.grd, pref_proxy_config_service.*,
proxy_config_service_impl.*, proxy_cros_settings_provider.*
- xiyuan: proxy_cros_settings_provider.*, proxy_handler.*, *.css,
proxy*.js/html, options*.*
- stevenjb: generated_resources.grd, network_menu.*, internet_*.*,
proxy_settings_ui*

thanks.

[xiyuan, 2011-10-04 20:28:25]

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/proxy_options.js (right):

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:109:
localStrings.getString(controlledBy);
Let's put some comments here that controlledBy must match in
proxy_cros_settings_provider.cc and proxy_handler.cc

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:117:
bannerDiv.removeEventListener('click', this.handleSharedProxiesHint_);
Let's do this block before we add class/event listener.

addEventListener does not check if the handler is already in the listener list.
So if updateBannerVisibility_ is called multiple times, you may end up add
this.handleSharedProxiesHint_ multiple times and it will be called multiple
times on 'click' event.

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:154:
$('advancedConfig').style.display = 'none';
nit: $('advancedConfig').hidden = true;

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:177:
$('advancedConfig').style.display = '-webkit-box';
nit: $('advancedConfig').hidden = false;

[kuan, 2011-10-04 21:45:50]

xiyuan, i've addressed all ur comments in patch delta 6.  ptal.  thanks.

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
File chrome/browser/resources/options/chromeos/proxy_options.js (right):

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:109:
localStrings.getString(controlledBy);
On 2011/10/04 20:28:25, xiyuan wrote:
> Let's put some comments here that controlledBy must match in
> proxy_cros_settings_provider.cc and proxy_handler.cc

Done.

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:117:
bannerDiv.removeEventListener('click', this.handleSharedProxiesHint_);
On 2011/10/04 20:28:25, xiyuan wrote:
> Let's do this block before we add class/event listener.
> 
> addEventListener does not check if the handler is already in the listener
list.
> So if updateBannerVisibility_ is called multiple times, you may end up add
> this.handleSharedProxiesHint_ multiple times and it will be called multiple
> times on 'click' event.

Done.

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:154:
$('advancedConfig').style.display = 'none';
On 2011/10/04 20:28:25, xiyuan wrote:
> nit: $('advancedConfig').hidden = true;

Done.

http://codereview.chromium.org/8102019/diff/16001/chrome/browser/resources/op...
chrome/browser/resources/options/chromeos/proxy_options.js:177:
$('advancedConfig').style.display = '-webkit-box';
On 2011/10/04 20:28:25, xiyuan wrote:
> nit: $('advancedConfig').hidden = false;

Done.

[xiyuan, 2011-10-04 21:47:43]

LGTM on my part.

[stevenjb, 2011-10-05 00:22:57]

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:879:
ProxyConfig::SOURCE_EXTENSION : ProxyConfig::SOURCE_NONE_DISABLED);
nit: Maybe use an if / else if / else here instead of a double trinary?

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:883:
ProxyConfig::SOURCE_NETWORK_DISABLED : ProxyConfig::SOURCE_NETWORK;
I really think
  source = SOURCE_NETWORK
  is_disabled = IgnoreProxy(network)
 throughout this function would be more readable

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:111: };
Do we need to Source and SourceType in the same variable? It seems like the code
would be simpler if we stored |source| and |source_disabled| separately and
eliminated the Source enum.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:115: GURL    pac_url;  //
Set if proxy is using pac script.
nit: one space betwen GURL and pac

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:274: switch
(config.source) {
This logic would need to change if config.source were split up, but I still
think it will be more readable.

[Mattias Nissler (ping if slow), 2011-10-05 10:18:35]

Some comments and one more thing:

I'm not particularly happy with the design you have now, since the situation we
have now is essentially the following: ProxyConfigServiceImpl is aware of
PrefProxyConfigTracker and the pref values it manages. Thus,
ProxyConfigServiceImpl has the full information to determine what the active
proxy should be. However, we ProxyConfigServiceImpl still only returns the
network setting and we still rely on PrefProxyConfigService to make a final
decision. This not only makes the thing hard to understand, but the logic for
figuring out what the effective proxy is is essentially duplicated between
PrefProxyConfigService and ProxyConfigServiceImpl. I think the proper way to
resolve this is not use PrefProxyConfigService to wrap ProxyConfigServiceImpl on
ChromeOS and rather make  ProxyConfigServiceImpl authoritative. Bonus points if
ProxyConfigServiceImpl can share code with PrefProxyConfigService for the actual
decision-making (since that's similar in both situations).

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:56: const
ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {
indentation: parameters should line up (break after opening parentheses).

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:62: const
ProxyConfigServiceImpl::ProxyConfig& config) {
same here.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:111: std::string
ProxyConfigToString(
This and the functions above appear to be dead code. Remove? I guess you used it
for debug logging, but IMHO it's quite a lot to be maintained in tree. At the
very least, make #ifdef it out and put a comment why it's here.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:522: const std::string&
current_network, Profile* profile) {
nit: second parameter on next line.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:861: const Network*
network) {
second parameter fits on previous line.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:879:
ProxyConfig::SOURCE_EXTENSION : ProxyConfig::SOURCE_NONE_DISABLED);
You split the logic between PrefProxyConfigTracker and here, which is fragile.
Can we instead have PrefProxyConfigTracker return a more detailed state that
also indicates the configuration source?

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:883:
ProxyConfig::SOURCE_NETWORK_DISABLED : ProxyConfig::SOURCE_NETWORK;
On 2011/10/05 00:22:57, Steven Bennetts wrote:
> I really think
>   source = SOURCE_NETWORK
>   is_disabled = IgnoreProxy(network)
>  throughout this function would be more readable

+1

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:120: net::ProxyServer 
server;
same here: only one space.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:134: return
!(static_cast<int>(source) & kSourceDisabled);
Seems like Steven has a point in making an actual boolean flag instead of
packing the flag into source. If you want both bundled in one, you can always
make it a class or struct.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:276: case
ProxyConfigServiceImpl::ProxyConfig::SOURCE_RECOMMENDED:
I think this is misleading, since the banner is there to indicate why the user
cannot edit stuff. In the case of recommended proxies the settings can be
modified though. Can we just not show the banner in this case? This is also what
we do for regular prefs in Chrome.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:74: (ui_config_state_ !=
CONFIG_UNSET &&
nit: not breaking this line would make the thing more readable IMHO since it's
more obvious that there's only a config change if these two conditions are bot
true.

[kuan, 2011-10-07 00:30:41]

steven and mattias,
i've restructured the code based on all ur comments.  change is quite big, and i
uploaded a few snapshots as backup as i tried to restructure the code
repeatedly.  i believe u can just look at delta patch 7 which shld be the
difference between the new code and the code u last reviewed.

xiyuan, i didn't change the files u've already lgtm'ed, so u don't hv to review
again.
 
thanks.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:56: const
ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {
i can't line them up, 'cos they won't fit, i believe the style guideline is to
indent 4 spaces when it can't fit?

On 2011/10/05 10:18:36, Mattias Nissler wrote:
> indentation: parameters should line up (break after opening parentheses).

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:62: const
ProxyConfigServiceImpl::ProxyConfig& config) {
On 2011/10/05 10:18:36, Mattias Nissler wrote:
> same here.

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:111: std::string
ProxyConfigToString(
i need it for debugging, so i make #if defined.

On 2011/10/05 10:18:36, Mattias Nissler wrote:
> This and the functions above appear to be dead code. Remove? I guess you used
it
> for debug logging, but IMHO it's quite a lot to be maintained in tree. At the
> very least, make #ifdef it out and put a comment why it's here.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:522: const std::string&
current_network, Profile* profile) {
On 2011/10/05 10:18:36, Mattias Nissler wrote:
> nit: second parameter on next line.

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:861: const Network*
network) {
it doesn't fit, i align it with 1st param.

On 2011/10/05 10:18:36, Mattias Nissler wrote:
> second parameter fits on previous line.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:879:
ProxyConfig::SOURCE_EXTENSION : ProxyConfig::SOURCE_NONE_DISABLED);
On 2011/10/05 00:22:57, Steven Bennetts wrote:
> nit: Maybe use an if / else if / else here instead of a double trinary?

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:883:
ProxyConfig::SOURCE_NETWORK_DISABLED : ProxyConfig::SOURCE_NETWORK;
On 2011/10/05 00:22:57, Steven Bennetts wrote:
> I really think
>   source = SOURCE_NETWORK
>   is_disabled = IgnoreProxy(network)
>  throughout this function would be more readable

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:883:
ProxyConfig::SOURCE_NETWORK_DISABLED : ProxyConfig::SOURCE_NETWORK;
On 2011/10/05 00:22:57, Steven Bennetts wrote:
> I really think
>   source = SOURCE_NETWORK
>   is_disabled = IgnoreProxy(network)
>  throughout this function would be more readable

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:111: };
i use "source| and |user_modifiable|.

On 2011/10/05 00:22:57, Steven Bennetts wrote:
> Do we need to Source and SourceType in the same variable? It seems like the
code
> would be simpler if we stored |source| and |source_disabled| separately and
> eliminated the Source enum.

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:115: GURL    pac_url;  //
Set if proxy is using pac script.
On 2011/10/05 00:22:57, Steven Bennetts wrote:
> nit: one space betwen GURL and pac

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:120: net::ProxyServer 
server;
On 2011/10/05 10:18:36, Mattias Nissler wrote:
> same here: only one space.

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:134: return
!(static_cast<int>(source) & kSourceDisabled);
i removed it.

On 2011/10/05 10:18:36, Mattias Nissler wrote:
> Seems like Steven has a point in making an actual boolean flag instead of
> packing the flag into source. If you want both bundled in one, you can always
> make it a class or struct.

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:274: switch
(config.source) {
On 2011/10/05 00:22:57, Steven Bennetts wrote:
> This logic would need to change if config.source were split up, but I still
> think it will be more readable.

Done.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:276: case
ProxyConfigServiceImpl::ProxyConfig::SOURCE_RECOMMENDED:
this didn't from me; it comes from sumit and kenmoore. sumit wants user to know
proxy is recommended, not from network. this is why he asks glenn for permission
to change "Some settings have been disabled by your adminstrator." to "Some
settings are being managed by your adminstrator." so that both policy and
recommended proxies can use the same string (u were cc'ed that email from sumit)
.
On 2011/10/05 10:18:36, Mattias Nissler wrote:
> I think this is misleading, since the banner is there to indicate why the user
> cannot edit stuff. In the case of recommended proxies the settings can be
> modified though. Can we just not show the banner in this case? This is also
what
> we do for regular prefs in Chrome.

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:74: (ui_config_state_ !=
CONFIG_UNSET &&
i didn't want to, but it won't fit, so i don't hv a choice.

On 2011/10/05 10:18:36, Mattias Nissler wrote:
> nit: not breaking this line would make the thing more readable IMHO since it's
> more obvious that there's only a config change if these two conditions are bot
> true.

[Mattias Nissler (ping if slow), 2011-10-07 13:32:10]

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:276: case
ProxyConfigServiceImpl::ProxyConfig::SOURCE_RECOMMENDED:
On 2011/10/07 00:30:41, kuan wrote:
> this didn't from me; it comes from sumit and kenmoore. sumit wants user to
know
> proxy is recommended, not from network. this is why he asks glenn for
permission
> to change "Some settings have been disabled by your adminstrator." to "Some
> settings are being managed by your adminstrator." so that both policy and
> recommended proxies can use the same string (u were cc'ed that email from
sumit)

It seems I misunderstood this. The point here is that the banner should only be
shown if the user cannot change the settings. If that is the case (and looking
at the code it seems to to this), I'm fine.

> .
> On 2011/10/05 10:18:36, Mattias Nissler wrote:
> > I think this is misleading, since the banner is there to indicate why the
user
> > cannot edit stuff. In the case of recommended proxies the settings can be
> > modified though. Can we just not show the banner in this case? This is also
> what
> > we do for regular prefs in Chrome.
>

http://codereview.chromium.org/8102019/diff/17021/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:325: if (path ==
kProxyType) {
why only for the type?

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service.h:20: public
PrefProxyConfigTracker::Observer {
As I mentioned in the email: Why do we handle the pref observing here only to
pass changes on to ProxyConfigServiceImpl? That seems quite convoluted. Can't we
just extract the pref observing and reading out of PrefProxyConfigTracker into
some base class (say ProxyPrefs) and make ProxyConfigServiceImpl use ProxyPrefs
directly?

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:302: // Intentionally
ignore source which is only used for ui purposes and has no
s/ui/UI/

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:650: if
(!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
Yikes. Another reason to pull out pref observing and watching from
PrefProxyConfigTracker in order to be able to use it here without switching
threads back and forth.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:132: bool user_modifiable;
It would be good to have a comment explaining in which situations the config is
modifiable and in which it isn't.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:182: // it again with a
different network.
comment formatting.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:185: // Called from UI
thread to make the currently active |etwork the one to be
revert

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:227: // Methods called on io
thread from wrapper chromeos::ProxyConfigService as
s/io/IO/

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:231: // Called from
GetLatestProxyConfig.
maybe adjust to match the name then?

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:235: // Methods called on ui
thread from wrapper chromeos::ProxyConfigService to
s/ui/UI/

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:291: // Reset UI cache
variables that keep track of ui changes.
s/ui/UI/

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:331: if (path ==
kProxyType) {
why only kProxyType?

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.h:24: // be disapledy and
possibly edited on.
s/disapled/displayed/

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.h:25: virtual void
SetCurrentUserProfile(Profile* profile);
This is asking for trouble once multi-profile comes around and two different
profiles try and show proxy settings. Since CrosSettingsProvider is not
profile-specific, I think it's actually wrong to push proxy config through it.
It seems the way to go is convert ProxyCrosSettingsProvider into a
CoreOptionsHandler subclass that talks directly to chromeos::ProxyConfigService.
The options handler is bound to a webui, thus has a profile available.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:48: enum ConfigSource {
ConfigState and ConfigSource are basically doing the same job. Can we just merge
them?

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:34:
g_browser_process->chromeos_proxy_config_service_impl(),
Ah, so here is the other location where the per-profile-ness of proxy config
bites you, which you have tried to avoid by pushing the pref settings from the
tracker through chromeos::ProxyConfigService to chromes::ProxyConfigServiceImpl.
Actually, that approach is incorrect without multi-profile being enabled
already, since the different chromeos::ProxyConfigService instances (one for the
system request context, one for the main profile) may have conflicting proxy
configs they forward to chromeos::ProxyConfigServiceImpl.

I think the solution is to just create a chromeos::ProxyConfigServiceImpl per
profile (and another one for the system request context).

[kuan, 2011-10-18 16:25:34]

i've reimplemented the new design of proxy config service and proxy config
tracker on all platforms and chromeos::ProxyConfigServiceImpl.  i've also
addressed all comments that stil apply to the new code.

xiyuan, i didn't change the ui html/js files that u last lgtm-ed, but i added
new ui stuff, pls look at chrome/browser/ui/webui/*. 

steven, u can look at the files that u previously looked at.

dtu, pls look at chrome/browser/automation/* and chrome/test/*.

mattias, pls look at everything else.

thanks.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service.h:20: public
PrefProxyConfigTracker::Observer {
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> As I mentioned in the email: Why do we handle the pref observing here only to
> pass changes on to ProxyConfigServiceImpl? That seems quite convoluted. Can't
we
> just extract the pref observing and reading out of PrefProxyConfigTracker into
> some base class (say ProxyPrefs) and make ProxyConfigServiceImpl use
ProxyPrefs
> directly?

Done.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:302: // Intentionally
ignore source which is only used for ui purposes and has no
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> s/ui/UI/

Done.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:650: if
(!BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> Yikes. Another reason to pull out pref observing and watching from
> PrefProxyConfigTracker in order to be able to use it here without switching
> threads back and forth.

This entire class has been redesigned and re-implemented.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:132: bool user_modifiable;
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> It would be good to have a comment explaining in which situations the config
is
> modifiable and in which it isn't.

Done.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:182: // it again with a
different network.
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> comment formatting.

Done.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:185: // Called from UI
thread to make the currently active |etwork the one to be
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> revert

Done.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:227: // Methods called on io
thread from wrapper chromeos::ProxyConfigService as
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> s/io/IO/

function is removed.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:231: // Called from
GetLatestProxyConfig.
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> maybe adjust to match the name then?

function is removed.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:235: // Methods called on ui
thread from wrapper chromeos::ProxyConfigService to
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> s/ui/UI/

function is removed.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:291: // Reset UI cache
variables that keep track of ui changes.
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> s/ui/UI/

Done.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:331: if (path ==
kProxyType) {
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> why only kProxyType?

this is because all the other input fields are enabled or disabled in javascript
based on the value of this input field; u cannot disable a field in native code
and then call "field.disabled = true" in javascript, 'cos they have different
disabled reasons.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.h:24: // be disapledy and
possibly edited on.
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> s/disapled/displayed/

function is removed.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.h:25: virtual void
SetCurrentUserProfile(Profile* profile);
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> This is asking for trouble once multi-profile comes around and two different
> profiles try and show proxy settings. Since CrosSettingsProvider is not
> profile-specific, I think it's actually wrong to push proxy config through it.
> It seems the way to go is convert ProxyCrosSettingsProvider into a
> CoreOptionsHandler subclass that talks directly to
chromeos::ProxyConfigService.
> The options handler is bound to a webui, thus has a profile available.

i've modified constructor to take in Profile parameter.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.h (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.h:48: enum ConfigSource {
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> ConfigState and ConfigSource are basically doing the same job. Can we just
merge
> them?

Done.

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/8102019/diff/40003/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:34:
g_browser_process->chromeos_proxy_config_service_impl(),
On 2011/10/07 13:32:10, Mattias Nissler wrote:
> Ah, so here is the other location where the per-profile-ness of proxy config
> bites you, which you have tried to avoid by pushing the pref settings from the
> tracker through chromeos::ProxyConfigService to
chromes::ProxyConfigServiceImpl.
> Actually, that approach is incorrect without multi-profile being enabled
> already, since the different chromeos::ProxyConfigService instances (one for
the
> system request context, one for the main profile) may have conflicting proxy
> configs they forward to chromeos::ProxyConfigServiceImpl.
> 
> I think the solution is to just create a chromeos::ProxyConfigServiceImpl per
> profile (and another one for the system request context).

this and related classes have been re-designed and re-implemented.

[xiyuan, 2011-10-18 17:33:31]

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/advanced_options_handler.cc:556: // Disable the
button if proxy settings are managed by a sysadmin or
Can we use the following here?
#if defined(OS_CHROMEOS)
  return;
#else
....
#endif

This way, we can reduce the number of "#if !defined(OS_CHROMEOS)".

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:44:
return CreateValueForPref(pref, controlling_pref);
What if controlling_pref is NULL?

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:100:
(proxy_prefs_->IsObserved(*pref_name) ||
Should this be "&&" instead of "||"? What if the pref is neither
kUseSharedProxies nor kProxy?

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:21:
virtual void Initialize();
nit: Move this into "protected" or "private" section?

[dtu, 2011-10-18 18:07:00]

chrome/browser/automation/* and chrome/test/*
LGTM with comments.

http://codereview.chromium.org/8102019/diff/61001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:3635: def GetProxySettingsOnChromeOS(self):
It should take an optional windex=0 parameter here instead of always setting it
to 0.

http://codereview.chromium.org/8102019/diff/61001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:3664: def SetProxySettingsOnChromeOS(self, key,
value):
Same as above. It should take an optional windex=0 parameter here instead of
always setting it to 0.

[kuan, 2011-10-18 20:32:47]

xiyuan and dave,
i've addressed ur comments in delta set 14.  pls take another look. thanks.

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/advanced_options_handler.cc:556: // Disable the
button if proxy settings are managed by a sysadmin or
On 2011/10/18 17:33:31, xiyuan wrote:
> Can we use the following here?
> #if defined(OS_CHROMEOS)
>   return;
> #else
> ....
> #endif
> 
> This way, we can reduce the number of "#if !defined(OS_CHROMEOS)".

Done.

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:44:
return CreateValueForPref(pref, controlling_pref);
On 2011/10/18 17:33:31, xiyuan wrote:
> What if controlling_pref is NULL?

if controlling_pref is none, then kUseSharedProxies will just control itself,
which is ok;  this is what happens to other "normal" prefs.

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:100:
(proxy_prefs_->IsObserved(*pref_name) ||
On 2011/10/18 17:33:31, xiyuan wrote:
> Should this be "&&" instead of "||"? What if the pref is neither
> kUseSharedProxies nor kProxy?

i actually want ||; if pref is neither kUseSharedProxies nor kProxy, the base
implementation of  ::CoreOptionsHandler::Observe will be called below, like for
other "normal" prefs.

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h
(right):

http://codereview.chromium.org/8102019/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.h:21:
virtual void Initialize();
On 2011/10/18 17:33:31, xiyuan wrote:
> nit: Move this into "protected" or "private" section?

Done. i moved to protected section.

http://codereview.chromium.org/8102019/diff/61001/chrome/test/pyautolib/pyaut...
File chrome/test/pyautolib/pyauto.py (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:3635: def GetProxySettingsOnChromeOS(self):
On 2011/10/18 18:07:00, Dave Tu wrote:
> It should take an optional windex=0 parameter here instead of always setting
it
> to 0.

so i added windex=0 here and changed the windex=0 below to windex=windex, is
that correct?

http://codereview.chromium.org/8102019/diff/61001/chrome/test/pyautolib/pyaut...
chrome/test/pyautolib/pyauto.py:3664: def SetProxySettingsOnChromeOS(self, key,
value):
On 2011/10/18 18:07:00, Dave Tu wrote:
> Same as above. It should take an optional windex=0 parameter here instead of
> always setting it to 0.

same here: added windex=0, and changed below to windex=windex, correct?

[dtu, 2011-10-18 21:49:02]

chrome/browser/automation/* and chrome/test/* LGTM

[stevenjb, 2011-10-18 22:47:33]

Just a few comments on the chromeos/ code.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:4806: AddStubNetwork(wifi1,
PROFILE_SHARED);
This should be fine, but you will want to make sure it doesn't break the network
library unit tests.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:125: Mode mode;
I realize this was already like this, but we really shouldn't have public data
members.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:262: const
NotificationDetails& details);
OVERRIDE. Also this should be in a separate section from non overridden
functions.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:297: std::string
current_ui_network_;
Let's rename this current_ui_network_path_; I was confused with the difference
between this and current_ui_network_name_. (If we could get rid of name_ and
just look it up when we need it, that would be even better).

[kuan, 2011-10-19 00:12:29]

steven, i've addressed ur comments in delta set 15.  pls take a look.  thanks.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.cc:4806: AddStubNetwork(wifi1,
PROFILE_SHARED);
On 2011/10/18 22:47:33, Steven Bennetts wrote:
> This should be fine, but you will want to make sure it doesn't break the
network
> library unit tests.

Done. i ran unit_tests with --gtest_filter=*NetworkLibrary* and they pass.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:125: Mode mode;
On 2011/10/18 22:47:33, Steven Bennetts wrote:
> I realize this was already like this, but we really shouldn't have public data
> members.

this data member is part of structure ProxyConfig so it's public.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:262: const
NotificationDetails& details);
On 2011/10/18 22:47:33, Steven Bennetts wrote:
> OVERRIDE. Also this should be in a separate section from non overridden
> functions.

Done.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:297: std::string
current_ui_network_;
On 2011/10/18 22:47:33, Steven Bennetts wrote:
> Let's rename this current_ui_network_path_; I was confused with the difference
> between this and current_ui_network_name_. (If we could get rid of name_ and
> just look it up when we need it, that would be even better).

Done. i kept current_ui_network_, removed current_ui_network_name_ and change
current_network_name() to UIGetCurrentNetworkName to retrieve the name from
NetworkLibrary.  (i cached it b4 'cos i thot it might be wasting cycles, but i
guess not).

[xiyuan, 2011-10-19 00:47:59]

LGTM for chrome/browser/ui/webui/* and chrome/browser/resources/*

[Mattias Nissler (ping if slow), 2011-10-19 17:03:42]

Sorry for the delay, I'm busy with the Bavarian Enterprise Summit these days.

I haven't looked at everything, but what I've seen so far I like very much!
Thanks for doing this!

The only thing that bothers me a bit so far is all the OS_CHROMEOS include
guards related to storing PrefProxyConfigTracker vs ProxyConfigServiceImpl in
the various Profile subclasses. Maybe it's better in this case to just have a
platform-specific typedef in a central place that everybody can use, at least
for the member variable and accessor declarations?

I'm sending this now so you have feedback, but I'll look at the bits I haven't
checked out yet or only skimmed over later.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:57: // net::ProxyConfig does
not suffice.  So we create an augmented analog to
I'm wondering whether this is still true? net::ProxyConfig might actually fit
the bill now. Anyways, not in this CL :)

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:60: //   to
ProxyPrefs::ConfigSource)
ConfigSource is no longer present.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:240: // if the network is
currently active.  If  |only_set_if_empty| is true,
remove one space after If

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:189: scoped_ptr<PrefProxyConfigTracker>
pref_proxy_config_tracker_;
You might get away in this case by just using scoped_ptr<PrefProxyConfigTracker>
also for OS_CHROMEOS. Or is there anyone accessing this as a
chromeos::ProxyConfigServiceImpl?

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:106: // Registers the preferene
proxy.
Rather: Registers the proxy preference.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:53: // static
I'd rather put the // static next to the declarations inside the preprocessor
conditionals, otherwise people will probably not notice it anyway.

[stevenjb, 2011-10-19 17:29:39]

NetworkLibrary related changes lgtm (with nits + commentary)

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:125: Mode mode;
On 2011/10/19 00:12:30, kuan wrote:
> On 2011/10/18 22:47:33, Steven Bennetts wrote:
> > I realize this was already like this, but we really shouldn't have public
data
> > members.
> 
> this data member is part of structure ProxyConfig so it's public.

This is a struct? Ugh. It really seems more like a class given its complexity.
Not necessary to fix this here, but I think we should change it or break it up
in a follow up CL.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:550: std::string*
network_name) {
nit: DCHECK(netwowrk_name) here

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:210: const net::ProxyConfig&
config);
All of the above should also have OVERRIDE

[Mattias Nissler (ping if slow), 2011-10-19 19:27:36]

Finished my review pass, some more comments (mostly nits).

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2555:
&TestingAutomationProvider::SetProxySettings;
Why did you move this down? It seems that previous order reflects declaration
order of the handler function.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:163:
net_config->proxy_rules().bypass_rules.AddRuleToBypassLocal();
Why this?

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:249: spec,
bypass_rules.ToString());
indentation

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:258: spec,
bypass_rules.ToString());
indentation

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:333: if
(proxy_dict.GetBypassList(&proxy_bypass)) {
nit, optional: don't need curlies here.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:439: // Intentionally
ignore source which is only used for UI purposes and has no
source is no longer present.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:510: if (pref_service &&
pref_service->FindPreference(prefs::kUseSharedProxies)) {
Why can PrefService be NULL here?

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:550: std::string*
network_name) {
On 2011/10/19 17:29:39, Steven Bennetts wrote:
> nit: DCHECK(netwowrk_name) here

Disagree. We'll crash anyway on the next line if network_name is NULL, and that
produces a crash dump, so a DCHECK would be redundant.

This is actually not my personal opinion, but what I got told by senior Chrome
people.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:852:
active_config_.ToNetProxyConfig(&current_config);
I may be missing something, but it seems like that keeping |active_config_| in
net::ProxyConfig representation is actually easier, since here you convert on
read and in line 867 you convert on write, and nothing else seems to touch
|active_config_|

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:337:
scoped_ptr<TestingPrefService> pref_service_;
just declare a plain TestingPrefService member variable, no need to wrap it in
scoped_ptr.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:523: << "]" <<
std::endl;
debugging code? convert to proper vlog or remove?

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
This is for showing the banner, correct? Should we actually show the proxy
banner if it's just a recommended policy? We don't do so for any other
preference, so I don't think proxy should behave differently.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/advanced_options_handler.cc:553: #if
defined(OS_CHROMEOS)
#if !defined(OS_CHROMEOS) and no return and #else

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:30: 
remove extra blank line

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.cc:228: const std::string&
controlling_pref_name) {
parameters should be aligned

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/core_options_handler.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.h:77: // Creates dictioary
value for |pref|, |controlling_pref| controls if |pref|
s/dictioary/dictionary/

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.h:78: // is managed by
policy/managed; NULL indicates no ordher pref is controlling
s/ordher/other/

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.h:81: const
PrefService::Preference* controlling_pref);
parameters should be aligned.

[kuan, 2011-10-20 00:41:11]

mattias and steven,
i've addressed all ur comments in delta set 16.  pls take a look.
mattias, i also changed to use a typedef for PrefProxyConfigTracker and
ProxyConfigServiceImpl to reduce the #ifdef's in profile subclasses.
pls ignore the failed try jobs for now, i need to sync with tot to fix the merge
conflicts tomorrow.

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/61001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:125: Mode mode;
On 2011/10/19 17:29:39, Steven Bennetts wrote:
> On 2011/10/19 00:12:30, kuan wrote:
> > On 2011/10/18 22:47:33, Steven Bennetts wrote:
> > > I realize this was already like this, but we really shouldn't have public
> data
> > > members.
> > 
> > this data member is part of structure ProxyConfig so it's public.
> 
> This is a struct? Ugh. It really seems more like a class given its complexity.
> Not necessary to fix this here, but I think we should change it or break it up
> in a follow up CL.
> 

it has grown a lot more complicated than when it was first created.  if it's a
class, ProxyCrosSettingsProvider will have to use accessors to access the
private data members.  is that advisable?

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2555:
&TestingAutomationProvider::SetProxySettings;
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> Why did you move this down? It seems that previous order reflects declaration
> order of the handler function.

dave, who wrote this code and reviewed this changelist, advises me this is the
way to pass in a Profile (via Browser object's Profile()) as construction
parameter to ProxyCrosSettingsProvider.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:163:
net_config->proxy_rules().bypass_rules.AddRuleToBypassLocal();
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> Why this?

there was a bug filed for this for chromeos to bypass local host; someone from
moscow fixed it in the previous IOSetProxyConfig, but now that i've removed that
function, i think this is the right place to fix it.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:249: spec,
bypass_rules.ToString());
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:258: spec,
bypass_rules.ToString());
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:333: if
(proxy_dict.GetBypassList(&proxy_bypass)) {
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> nit, optional: don't need curlies here.

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:439: // Intentionally
ignore source which is only used for UI purposes and has no
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> source is no longer present.

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:510: if (pref_service &&
pref_service->FindPreference(prefs::kUseSharedProxies)) {
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> Why can PrefService be NULL here?

Done. i guess i was being overly cautious.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:550: std::string*
network_name) {
On 2011/10/19 17:29:39, Steven Bennetts wrote:
> nit: DCHECK(netwowrk_name) here

i did a "if (!network_name) return;"

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:550: std::string*
network_name) {
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> On 2011/10/19 17:29:39, Steven Bennetts wrote:
> > nit: DCHECK(netwowrk_name) here
> 
> Disagree. We'll crash anyway on the next line if network_name is NULL, and
that
> produces a crash dump, so a DCHECK would be redundant.
> 
> This is actually not my personal opinion, but what I got told by senior Chrome
> people.

i did a "if (!network_name) return;"

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:852:
active_config_.ToNetProxyConfig(&current_config);
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> I may be missing something, but it seems like that keeping |active_config_| in
> net::ProxyConfig representation is actually easier, since here you convert on
> read and in line 867 you convert on write, and nothing else seems to touch
> |active_config_|

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:57: // net::ProxyConfig does
not suffice.  So we create an augmented analog to
On 2011/10/19 17:03:42, Mattias Nissler wrote:
> I'm wondering whether this is still true? net::ProxyConfig might actually fit
> the bill now. Anyways, not in this CL :)

it is still true, 'cos this structure has ConfigState and user_modifiable and
its members align a lot more closely with the UI input fields :)

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:60: //   to
ProxyPrefs::ConfigSource)
On 2011/10/19 17:03:42, Mattias Nissler wrote:
> ConfigSource is no longer present.

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:210: const net::ProxyConfig&
config);
On 2011/10/19 17:29:39, Steven Bennetts wrote:
> All of the above should also have OVERRIDE

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:240: // if the network is
currently active.  If  |only_set_if_empty| is true,
On 2011/10/19 17:03:42, Mattias Nissler wrote:
> remove one space after If

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:337:
scoped_ptr<TestingPrefService> pref_service_;
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> just declare a plain TestingPrefService member variable, no need to wrap it in
> scoped_ptr.

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:523: << "]" <<
std::endl;
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> debugging code? convert to proper vlog or remove?

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> This is for showing the banner, correct? Should we actually show the proxy
> banner if it's just a recommended policy? We don't do so for any other
> preference, so I don't think proxy should behave differently.

this is from sumit.  we used to not allow proxy page to be opened when
use-shared-proxies is off, but now that we allow proxy page to be opened, we
need to tell user why the fields are disabled.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:189: scoped_ptr<PrefProxyConfigTracker>
pref_proxy_config_tracker_;
On 2011/10/19 17:03:42, Mattias Nissler wrote:
> You might get away in this case by just using
scoped_ptr<PrefProxyConfigTracker>
> also for OS_CHROMEOS. Or is there anyone accessing this as a
> chromeos::ProxyConfigServiceImpl?

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:106: // Registers the preferene
proxy.
On 2011/10/19 17:03:42, Mattias Nissler wrote:
> Rather: Registers the proxy preference.

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/proxy_se...
File chrome/browser/net/proxy_service_factory.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/net/proxy_se...
chrome/browser/net/proxy_service_factory.cc:53: // static
On 2011/10/19 17:03:42, Mattias Nissler wrote:
> I'd rather put the // static next to the declarations inside the preprocessor
> conditionals, otherwise people will probably not notice it anyway.

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/advanced_options_handler.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/advanced_options_handler.cc:553: #if
defined(OS_CHROMEOS)
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> #if !defined(OS_CHROMEOS) and no return and #else

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:30: 
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> remove extra blank line

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/core_options_handler.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.cc:228: const std::string&
controlling_pref_name) {
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> parameters should be aligned

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/core_options_handler.h (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.h:77: // Creates dictioary
value for |pref|, |controlling_pref| controls if |pref|
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> s/dictioary/dictionary/

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.h:78: // is managed by
policy/managed; NULL indicates no ordher pref is controlling
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> s/ordher/other/

Done.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/core_options_handler.h:81: const
PrefService::Preference* controlling_pref);
On 2011/10/19 19:27:36, Mattias Nissler wrote:
> parameters should be aligned.

Done.

[Mattias Nissler (ping if slow), 2011-10-24 10:18:27]

Sorry for the late reply, I was OOO late last week.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2555:
&TestingAutomationProvider::SetProxySettings;
On 2011/10/20 00:41:11, kuan wrote:
> On 2011/10/19 19:27:36, Mattias Nissler wrote:
> > Why did you move this down? It seems that previous order reflects
declaration
> > order of the handler function.
> 
> dave, who wrote this code and reviewed this changelist, advises me this is the
> way to pass in a Profile (via Browser object's Profile()) as construction
> parameter to ProxyCrosSettingsProvider.

Ah, I had missed the handler_map -> browser_handler_map change.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
On 2011/10/20 00:41:11, kuan wrote:
> On 2011/10/19 19:27:36, Mattias Nissler wrote:
> > This is for showing the banner, correct? Should we actually show the proxy
> > banner if it's just a recommended policy? We don't do so for any other
> > preference, so I don't think proxy should behave differently.
> 
> this is from sumit.  we used to not allow proxy page to be opened when
> use-shared-proxies is off, but now that we allow proxy page to be opened, we
> need to tell user why the fields are disabled.

That is true for managed policy. For recommended policy (which I assume we're
talking about here, since the case handler is for CONFIG_FALLBACK), the
textfields shouldn't be disabled, we should just show the default values. Since
they're still editable, this situation is not really different from the regular
behavior, so why should the user be bothered with where the defaults come from?
Also, all other settings UI only shows the banner for managed policy, so I'd
appreciate if we could stay consistent.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1272: void
GetProxySettings(Browser* browser, base::DictionaryValue* args,
should break after ,

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1275: void
SetProxySettings(Browser* browser, base::DictionaryValue* args,
same here.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:73: const
ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {
arguments on multiple lines should line up.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:371: <<
"\ntarget:\n" << tgt_output;
It seems like this should really be the message that gets logged if the
EXPECT_TRUE() below fails.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:512: i =
managed_idx + 1;
This whole index-searching seems pretty brittle to me. Can't you just put a
static table of the index triplets here? That way, it would also be more obvious
to the reader which TestParams go with each other.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:531: // hManaged
proxy pref should take effect over recommended proxy and
s/hManaged/Managed/

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:156: 
remove blank line.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:47: // Configuration is from sytem.
s/sytem/system/

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:108: #endif  // defined(OS_CHROMEOS)
I'm wondering whether we might even want to go a step further and put the
typedef and forward declaration into pref_proxy_config_service.h?

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:137: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
needed?

http://codereview.chromium.org/8102019/diff/71001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:67: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
needed?

[kuan, 2011-10-25 02:01:53]

mattias, i've addressed all ur comments in delta set 17.  unfortunately, that
delta also contains the changes that i synced to tot last thurs and the changes
i made to pass the try jobs last thurs.  pls take a look.  thanks.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> On 2011/10/20 00:41:11, kuan wrote:
> > On 2011/10/19 19:27:36, Mattias Nissler wrote:
> > > This is for showing the banner, correct? Should we actually show the proxy
> > > banner if it's just a recommended policy? We don't do so for any other
> > > preference, so I don't think proxy should behave differently.
> > 
> > this is from sumit.  we used to not allow proxy page to be opened when
> > use-shared-proxies is off, but now that we allow proxy page to be opened, we
> > need to tell user why the fields are disabled.
> 
> That is true for managed policy. For recommended policy (which I assume we're
> talking about here, since the case handler is for CONFIG_FALLBACK), the
> textfields shouldn't be disabled, we should just show the default values.
Since
> they're still editable, this situation is not really different from the
regular
> behavior, so why should the user be bothered with where the defaults come
from?
> Also, all other settings UI only shows the banner for managed policy, so I'd
> appreciate if we could stay consistent.

i'm simply implementing it according to requirements from sumit and kenmoore,
which were decided after long email thread discussions.  sumit wants users to
know the proxy is recommended, hence the banner.  if u still strongly disagree,
do you mind taking this up with sumit?  i will implement whatever is finally
decided.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1272: void
GetProxySettings(Browser* browser, base::DictionaryValue* args,
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> should break after ,

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.h:1275: void
SetProxySettings(Browser* browser, base::DictionaryValue* args,
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> same here.

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:73: const
ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> arguments on multiple lines should line up.

it won't fit if i align the arguments.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:371: <<
"\ntarget:\n" << tgt_output;
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> It seems like this should really be the message that gets logged if the
> EXPECT_TRUE() below fails.

test is no longer valid, so it's removed.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:512: i =
managed_idx + 1;
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> This whole index-searching seems pretty brittle to me. Can't you just put a
> static table of the index triplets here? That way, it would also be more
obvious
> to the reader which TestParams go with each other.

i intentionally don't want to hardcode the triple-pairing, 'cos then i change
the original table, i must remember to change the triple-pairings, which i think
it's more error-prone.  i already print out the triple-pairings if a test fails,
so i think that suffices.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:531: // hManaged
proxy pref should take effect over recommended proxy and
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> s/hManaged/Managed/

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:156: 
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> remove blank line.

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:47: // Configuration is from sytem.
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> s/sytem/system/

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:108: #endif  // defined(OS_CHROMEOS)
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> I'm wondering whether we might even want to go a step further and put the
> typedef and forward declaration into pref_proxy_config_service.h?

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:137: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> needed?

yes, else can't build.

http://codereview.chromium.org/8102019/diff/71001/chrome/test/base/testing_pr...
File chrome/test/base/testing_profile.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/test/base/testing_pr...
chrome/test/base/testing_profile.cc:67: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
On 2011/10/24 10:18:28, Mattias Nissler wrote:
> needed?

yes, else can't build.

[Mattias Nissler (ping if slow), 2011-10-25 12:43:10]

almost there.

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
On 2011/10/25 02:01:54, kuan wrote:
> On 2011/10/24 10:18:28, Mattias Nissler wrote:
> > On 2011/10/20 00:41:11, kuan wrote:
> > > On 2011/10/19 19:27:36, Mattias Nissler wrote:
> > > > This is for showing the banner, correct? Should we actually show the
proxy
> > > > banner if it's just a recommended policy? We don't do so for any other
> > > > preference, so I don't think proxy should behave differently.
> > > 
> > > this is from sumit.  we used to not allow proxy page to be opened when
> > > use-shared-proxies is off, but now that we allow proxy page to be opened,
we
> > > need to tell user why the fields are disabled.
> > 
> > That is true for managed policy. For recommended policy (which I assume
we're
> > talking about here, since the case handler is for CONFIG_FALLBACK), the
> > textfields shouldn't be disabled, we should just show the default values.
> Since
> > they're still editable, this situation is not really different from the
> regular
> > behavior, so why should the user be bothered with where the defaults come
> from?
> > Also, all other settings UI only shows the banner for managed policy, so I'd
> > appreciate if we could stay consistent.
> 
> i'm simply implementing it according to requirements from sumit and kenmoore,
> which were decided after long email thread discussions.  sumit wants users to
> know the proxy is recommended, hence the banner.  if u still strongly
disagree,
> do you mind taking this up with sumit?  i will implement whatever is finally
> decided.

I do feel strong about avoiding inconsistency to how the rest of the settings UI
behaves. Recommended settings are not flagged in any other parts of the UI, and
frankly, I don't think it makes sense to flag them specially. I'll respond on
the mail thread.

In the interest of getting this landed, can you just leave out this part and
make the change later if we actually decide for it?

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:73: const
ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {
On 2011/10/25 02:01:54, kuan wrote:
> On 2011/10/24 10:18:28, Mattias Nissler wrote:
> > arguments on multiple lines should line up.
> 
> it won't fit if i align the arguments.

Then break line 72 after (, the style guide says that parameters should be
aligned if possible and breaking the line makes it possible.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:47: static const
struct TestParams {
No need for static, we're in a anonymous namespace.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:512: i =
managed_idx + 1;
On 2011/10/25 02:01:54, kuan wrote:
> On 2011/10/24 10:18:28, Mattias Nissler wrote:
> > This whole index-searching seems pretty brittle to me. Can't you just put a
> > static table of the index triplets here? That way, it would also be more
> obvious
> > to the reader which TestParams go with each other.
> 
> i intentionally don't want to hardcode the triple-pairing, 'cos then i change
> the original table, i must remember to change the triple-pairings, which i
think
> it's more error-prone.  i already print out the triple-pairings if a test
fails,
> so i think that suffices.

I can see that it might be slightly more work for you in the future, but the way
it's currently done is basically unreadable for everyone else IMHO. I thought we
have the general goal to optimize code for being readable rather than for some
(IMHO minor) shortcuts when updating. I don't feel this discussion is worth more
of my time though, so your call :)

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:541:
IPC::Message* reply_message) {
indentation

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pre...
File chrome/browser/chromeos/preferences.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pre...
chrome/browser/chromeos/preferences.cc:203:
!(BrowserThread::CurrentlyOn(BrowserThread::UI) &&
Shouldn't this at least be a DCHECK? I guess you don't DCHECK since this is
probably about silencing a DCHECK for tests... In general I'd advice against
depending on some static data (which UserManager) is on initializing this and
rather handle the different default at the place where the pref is accessed.

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:278: bool
use_shared_proxies_;
You can just use |user_shared_proxies_pref_.GetValue()| instead,
BooleanPrefMember caches the pref value for you. Also, PrefService makes an
effort only to send notifications if the pref has actually changed.

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:48: static const
struct TestParams {
no need to make this static, since we're in a anonymous namespace.

[kuan, 2011-10-25 16:01:49]

mattias, i've addressed all ur comments in delta set 18 and 19; be forewarned
these delta also contain the differences that i synced to tot this morning.

william, i need approval from owner of chrome/browser/profiles and
chrome/browser/net dirs, pls take a look at the files in those subdirs.  since
it's the first time u're reviewing this, u can ignore the delta sets and just
look at main difference.

thanks!

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/68009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
On 2011/10/25 12:43:11, Mattias Nissler wrote:
> On 2011/10/25 02:01:54, kuan wrote:
> > On 2011/10/24 10:18:28, Mattias Nissler wrote:
> > > On 2011/10/20 00:41:11, kuan wrote:
> > > > On 2011/10/19 19:27:36, Mattias Nissler wrote:
> > > > > This is for showing the banner, correct? Should we actually show the
> proxy
> > > > > banner if it's just a recommended policy? We don't do so for any other
> > > > > preference, so I don't think proxy should behave differently.
> > > > 
> > > > this is from sumit.  we used to not allow proxy page to be opened when
> > > > use-shared-proxies is off, but now that we allow proxy page to be
opened,
> we
> > > > need to tell user why the fields are disabled.
> > > 
> > > That is true for managed policy. For recommended policy (which I assume
> we're
> > > talking about here, since the case handler is for CONFIG_FALLBACK), the
> > > textfields shouldn't be disabled, we should just show the default values.
> > Since
> > > they're still editable, this situation is not really different from the
> > regular
> > > behavior, so why should the user be bothered with where the defaults come
> > from?
> > > Also, all other settings UI only shows the banner for managed policy, so
I'd
> > > appreciate if we could stay consistent.
> > 
> > i'm simply implementing it according to requirements from sumit and
kenmoore,
> > which were decided after long email thread discussions.  sumit wants users
to
> > know the proxy is recommended, hence the banner.  if u still strongly
> disagree,
> > do you mind taking this up with sumit?  i will implement whatever is finally
> > decided.
> 
> I do feel strong about avoiding inconsistency to how the rest of the settings
UI
> behaves. Recommended settings are not flagged in any other parts of the UI,
and
> frankly, I don't think it makes sense to flag them specially. I'll respond on
> the mail thread.
> 
> In the interest of getting this landed, can you just leave out this part and
> make the change later if we actually decide for it?

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:73: const
ProxyConfigServiceImpl::ProxyConfig::ManualProxy& proxy) {
On 2011/10/25 12:43:11, Mattias Nissler wrote:
> On 2011/10/25 02:01:54, kuan wrote:
> > On 2011/10/24 10:18:28, Mattias Nissler wrote:
> > > arguments on multiple lines should line up.
> > 
> > it won't fit if i align the arguments.
> 
> Then break line 72 after (, the style guide says that parameters should be
> aligned if possible and breaking the line makes it possible.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:47: static const
struct TestParams {
On 2011/10/25 12:43:11, Mattias Nissler wrote:
> No need for static, we're in a anonymous namespace.

Done.

http://codereview.chromium.org/8102019/diff/71001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:512: i =
managed_idx + 1;
On 2011/10/25 12:43:11, Mattias Nissler wrote:
> On 2011/10/25 02:01:54, kuan wrote:
> > On 2011/10/24 10:18:28, Mattias Nissler wrote:
> > > This whole index-searching seems pretty brittle to me. Can't you just put
a
> > > static table of the index triplets here? That way, it would also be more
> > obvious
> > > to the reader which TestParams go with each other.
> > 
> > i intentionally don't want to hardcode the triple-pairing, 'cos then i
change
> > the original table, i must remember to change the triple-pairings, which i
> think
> > it's more error-prone.  i already print out the triple-pairings if a test
> fails,
> > so i think that suffices.
> 
> I can see that it might be slightly more work for you in the future, but the
way
> it's currently done is basically unreadable for everyone else IMHO. I thought
we
> have the general goal to optimize code for being readable rather than for some
> (IMHO minor) shortcuts when updating. I don't feel this discussion is worth
more
> of my time though, so your call :)

Done.

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider_chromeos.cc:541:
IPC::Message* reply_message) {
On 2011/10/25 12:43:11, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.h (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:278: bool
use_shared_proxies_;
On 2011/10/25 12:43:11, Mattias Nissler wrote:
> You can just use |user_shared_proxies_pref_.GetValue()| instead,
> BooleanPrefMember caches the pref value for you. Also, PrefService makes an
> effort only to send notifications if the pref has actually changed.

Done.

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:48: static const
struct TestParams {
On 2011/10/25 12:43:11, Mattias Nissler wrote:
> no need to make this static, since we're in a anonymous namespace.

Done.

[Mattias Nissler (ping if slow), 2011-10-25 16:17:52]

lgtm

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
I think we want to keep the banner in case shared proxies are not enabled, since
the input boxes will be disabled in that case, correct?

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:348:
default_use_shared_proxies);
I don't think we should actually write the default to user prefs. We can avoid
this SetBoolean() call if you just make a GetUseSharedProxies() that first
checks whether there's a non-default value, returns it if that's the case. If we
only have the default, ask UserManager and return the value.

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:550:
PrefService::SYNCABLE_PREF);
Do you actually want this to be synced?

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl_unittest.cc (right):

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl_unittest.cc:478: { 7, 6, 8, },
I like this much better now, and it even tests more situations :)

[kuan, 2011-10-25 17:17:33]

mattias, i've addressed ur comments in delta set 20, ptal.  thanks.

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
On 2011/10/25 16:17:52, Mattias Nissler wrote:
> I think we want to keep the banner in case shared proxies are not enabled,
since
> the input boxes will be disabled in that case, correct?

yes, that's handled in the default case, user_modifiable is already set to false
in ProxyConfigServiceImpl when use-shared-proxies is off.

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:348:
default_use_shared_proxies);
On 2011/10/25 16:17:52, Mattias Nissler wrote:
> I don't think we should actually write the default to user prefs. We can avoid
> this SetBoolean() call if you just make a GetUseSharedProxies() that first
> checks whether there's a non-default value, returns it if that's the case. If
we
> only have the default, ask UserManager and return the value.

Done.

http://codereview.chromium.org/8102019/diff/95009/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:550:
PrefService::SYNCABLE_PREF);
On 2011/10/25 16:17:52, Mattias Nissler wrote:
> Do you actually want this to be synced?

Done.

[Mattias Nissler (ping if slow), 2011-10-25 17:27:24]

LGTM, thanks for bearing with me!

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_provider.cc (right):

http://codereview.chromium.org/8102019/diff/84001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_provider.cc:283:
"policyManagedPrefsBannerText" : "enableSharedProxiesBannerText";
On 2011/10/25 17:17:33, kuan wrote:
> On 2011/10/25 16:17:52, Mattias Nissler wrote:
> > I think we want to keep the banner in case shared proxies are not enabled,
> since
> > the input boxes will be disabled in that case, correct?
> 
> yes, that's handled in the default case, user_modifiable is already set to
false
> in ProxyConfigServiceImpl when use-shared-proxies is off.

I see. Sorry for missing that.

http://codereview.chromium.org/8102019/diff/93087/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/93087/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:655: return
use_shared_proxies_.GetValue();
You can write this whole function more compact as follows:

const PrefService::Preference* user_shared_proxies_pref =
    prefs()->FindPreference(prefs::kUseSharedProxies);
if (!user_shared_proxies_pref ||
    user_shared_proxies_pref->IsDefaultValue()) {
  return !UserManager::Get()->user_is_logged_in();
}
return use_shared_proxies_.GetValue();

[willchan no longer on Chromium, 2011-10-25 22:28:37]

I skimmed it and love the change. I'm going to take a closer look at profiles
stuff. I'm going to entrust the chrome/browser/net proxy stuff to eroman. He's
better than me at all that proxy config stuff anyhow.

[willchan no longer on Chromium, 2011-11-02 00:26:41]

I tried to get to it today and failed. I will maybe get to it tonight, or I will
get to it tomorrow.

On 2011/10/25 22:28:37, willchan wrote:
> I skimmed it and love the change. I'm going to take a closer look at profiles
> stuff. I'm going to entrust the chrome/browser/net proxy stuff to eroman. He's
> better than me at all that proxy config stuff anyhow.

[willchan no longer on Chromium, 2011-11-02 23:08:17]

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.cc (right):

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.cc:70: VLOG(1) << "got
UpdateProxyConfig";
DVLOG instead? Do you really need this LOG message?

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.cc:136: VLOG(1) << "set chrome
proxy config service to "
Do you really need this? Won't all this be visible in net-internals anyway?

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.h (right):

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:124: virtual void
OnProxyConfigChanged(ProxyPrefs::ConfigState config_state,
Should this be private? Who calls this?

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:162: typedef
chromeos::ProxyConfigServiceImpl PrefProxyConfigTrackerType;
Is PrefProxyConfigTracker used in chromeos? If not, why is it getting built? It
sounds like PrefProxyConfigTracker should be called PrefProxyConfigTrackerImpl
and moved into pref_proxy_config_tracker_impl.h. Then there can be a
pref_proxy_config_tracker.h which only has the platform specific typedef code.

[kuan, 2011-11-03 20:20:14]

william, i've addressed all ur comments in the last delta set (24).  pls take a
look.  thanks.

http://codereview.chromium.org/8102019/diff/93087/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/93087/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:655: return
use_shared_proxies_.GetValue();
On 2011/10/25 17:27:24, Mattias Nissler wrote:
> You can write this whole function more compact as follows:
> 
> const PrefService::Preference* user_shared_proxies_pref =
>     prefs()->FindPreference(prefs::kUseSharedProxies);
> if (!user_shared_proxies_pref ||
>     user_shared_proxies_pref->IsDefaultValue()) {
>   return !UserManager::Get()->user_is_logged_in();
> }
> return use_shared_proxies_.GetValue();

Done.

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.cc (right):

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.cc:70: VLOG(1) << "got
UpdateProxyConfig";
On 2011/11/02 23:08:17, willchan wrote:
> DVLOG instead? Do you really need this LOG message?

Done.

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.cc:136: VLOG(1) << "set chrome
proxy config service to "
On 2011/11/02 23:08:17, willchan wrote:
> Do you really need this? Won't all this be visible in net-internals anyway?

this won't be visible in net-internals, it helps me figure out what has happened
on non-dev machines , like testers' chromeos devices where chrome is running in
non-debug mode and there're multiple config services running simultaneously.

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_tracker.h (right):

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:124: virtual void
OnProxyConfigChanged(ProxyPrefs::ConfigState config_state,
On 2011/11/02 23:08:17, willchan wrote:
> Should this be private? Who calls this?

chromeos variant extends this class and calls this method.

http://codereview.chromium.org/8102019/diff/89029/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_tracker.h:162: typedef
chromeos::ProxyConfigServiceImpl PrefProxyConfigTrackerType;
On 2011/11/02 23:08:17, willchan wrote:
> Is PrefProxyConfigTracker used in chromeos? If not, why is it getting built?
It
> sounds like PrefProxyConfigTracker should be called PrefProxyConfigTrackerImpl
> and moved into pref_proxy_config_tracker_impl.h. Then there can be a
> pref_proxy_config_tracker.h which only has the platform specific typedef code.

chromeos variant extends PrefProxyConfigTracker, so it has to be built.
the rest, i've done as u suggested.

[kuan, 2011-11-03 20:21:34]

fyi, i also rebased so delta set 24 includes those changes, had to sync in order
to submit tryjobs.

[willchan no longer on Chromium, 2011-11-04 01:13:15]

I didn't review chrome/browser/net as closely as I should have. Eric would have
been a better reviewer here as he remembers things more clearly than I. But it
overall LGTM, so I'm sending approval.

[commit-bot: I haz the power, 2011-11-04 01:21:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/8102019/112001

[commit-bot: I haz the power, 2011-11-04 01:21:28]

Can't apply patch for file chrome/browser/net/pref_proxy_config_tracker_impl.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/net/pref_proxy_config_tracker_impl.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 168.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/net/pref_proxy_config_tracker_impl.cc.rej

[commit-bot: I haz the power, 2011-11-04 01:29:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/8102019/121001

[commit-bot: I haz the power, 2011-11-04 01:29:48]

Can't apply patch for file chrome/browser/net/pref_proxy_config_tracker_impl.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/net/pref_proxy_config_tracker_impl.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 168.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/net/pref_proxy_config_tracker_impl.cc.rej

[commit-bot: I haz the power, 2011-11-04 01:45:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/8102019/113014

[commit-bot: I haz the power, 2011-11-04 01:45:50]

Can't apply patch for file chrome/browser/net/pref_proxy_config_tracker_impl.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/net/pref_proxy_config_tracker_impl.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 16.
Hunk #3 FAILED at 170.
3 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/net/pref_proxy_config_tracker_impl.cc.rej

[commit-bot: I haz the power, 2011-11-04 01:57:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/8102019/113014

[commit-bot: I haz the power, 2011-11-04 01:57:53]

Can't apply patch for file chrome/browser/net/pref_proxy_config_tracker_impl.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/net/pref_proxy_config_tracker_impl.cc
Hunk #1 FAILED at 2.
Hunk #2 FAILED at 16.
Hunk #3 FAILED at 170.
3 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/net/pref_proxy_config_tracker_impl.cc.rej

[commit-bot: I haz the power, 2011-11-04 02:15:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/8102019/119018

[commit-bot: I haz the power, 2011-11-04 02:15:54]

Can't process patch for file chrome/chrome_tests.gypi.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-11-04 02:19:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/8102019/122092

[commit-bot: I haz the power, 2011-11-04 02:20:35]

Presubmit check for 8102019-122092 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/browser/ui/webui/options/core_options_handler.cc:244

Presubmit checks took 8.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[achuithb, 2011-11-07 22:59:32]

http://codereview.chromium.org/8102019/diff/122092/chrome/browser/chromeos/pr...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8102019/diff/122092/chrome/browser/chromeos/pr...
chrome/browser/chromeos/proxy_config_service_impl.cc:9: #include "base/bind.h"
Kuan: I don't think you're using this?