Refactor proxy handling for ChromeOS to not go through the CrosSettings interface.

Refactor proxy handling for ChromeOS to not go through the CrosSettings interface.

The ProxyCrosSettingsProvider was not a real SignedSettings handler since the
network manager was introduced and was only used to parse the data for the UI.
However this does not go well with the unification of the SignedSettings
handling and belongs to the presentation layer. Therefore we extract the
parsing functionality in a helper class and move some of the logic up the
UI handling layer.

BUG=chromium-os:14054
TEST=Proxy UI should still be working (automation tests).

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

[pastarmovj, 2011-11-07 12:12:07]

Please take a look at this refactoring of proxy_cros_settings_provider.*. I also
used the occasion to put some proper namespaces before Value.

[Mattias Nissler (ping if slow), 2011-11-08 09:25:21]

http://codereview.chromium.org/8467012/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8467012/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:79:
browser->profile(), setting_path, &value)) {
+4 indentation

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

http://codereview.chromium.org/8467012/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:65: if
(pref_name.find(kProxyPrefsPrefix) == 0) {
Should be StartsWith()

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:79:
browser->profile(), setting_path, &value)) {
+4 indentation

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:756: // Notify whoever is
interested in this change.
I still think an actual observer or callback-based solution would be more
appropriate in this case.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:72: }
// namespace

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:74: void
ProxyCrosSettingsParser::SetProxyPrefValue(Profile* profile,
These large functions suck. I wonder whether we can just switch
ProxyConfigServiceImpl to hand out and accept a DictionaryValue representation
of proxy config? Kuan, WDYT? Probably not in this CL though.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_parser.h (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:7: #pragma once
#include <string>

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:9: #include "base/values.h"
forward declaration would suffice.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:10: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
don't need this?

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:30: 
Class description.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:36: const base::Value*
in_value);
newline

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:44: // helper functions.
Make it a namespace then?

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:45: explicit
ProxyCrosSettingsParser();
newline

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:19: 
merge the two include blocks.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:98: if
(proxy_registrar_.IsEmpty()) {
why not just listen in any case?

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:117:
ProcessUserMetric(value, metric);
I don't like the duplication here...

[kuan, 2011-11-08 14:45:24]

i wld like to clarify that ProxyCrosSettingsProvider was NEVER meant to be a
SignedSettings provider; the original author (who has left the company) created
it  as a CrosSettings provider.  It just so happens that back then, proxy
configuration was a signed setting, something that is completely invisible and
separate from ProxyCrosSettingsProvider; the latter is only responsible for
handling proxy settings between UI and backend ProxyConfigServiceImpl , no
matter where the settings come from.

i'm not sure if someone else who created the mechanism of fetching/setting cros
settings via CrosSettings providers should approve this cl, 'cos this cl
completely goes away from the CrosSettingsProvider design.

http://codereview.chromium.org/8467012/diff/1/chrome/browser/chromeos/proxy_c...
File chrome/browser/chromeos/proxy_cros_settings_parser.h (right):

http://codereview.chromium.org/8467012/diff/1/chrome/browser/chromeos/proxy_c...
chrome/browser/chromeos/proxy_cros_settings_parser.h:43: // We should never need
objects of this class it is meant only to provide its
s/class it/class; it/

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_config_service_impl.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_config_service_impl.cc:760:
content::NotificationService::NoDetails());
this is the wrong place to fire this notification; it shld be done in
OnUISetCurrentNetwork (i.e. the fn right below).
DetermineEffectiveConfig (where u're doing it now) is called not just for ui
changes, whereas OnUISetCurrentNetwork is only called for ui changes.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:74: void
ProxyCrosSettingsParser::SetProxyPrefValue(Profile* profile,
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> These large functions suck. I wonder whether we can just switch
> ProxyConfigServiceImpl to hand out and accept a DictionaryValue representation
> of proxy config? Kuan, WDYT? Probably not in this CL though.

somewhere some code has to do the parsing with multiple if's.  u're, in essence,
suggesting moving these large functions from here to ProxyConfigServiceImpl. 
personally, i prefer to separate ui, data and logic. these dictionaries are
really used only for UI to pass info from javascript to native code, whereas
ProxyConfigServiceImpl should just handle the so-called "backend" like using
structures.
however, if ur suggestion is the preferred approach by concensus, u're right
that it probably shldn't be in this cl.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:108:
proxy_handler_->SetNetworkName(network_name);
maybe use a local var for GetProfile()->GetProxyConfigTracker?

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:48: 
u may want to comment here and proxy_cros_settings_parser.h/cc that
kProxySettings must include all settings declared in
proxy_cros_settings_parser.h, so that future changes to settings will be
consistent throughout.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:65: if
(pref_name.find(kProxyPrefsPrefix) == 0) {
is StartWith enough?
in any case, maybe place this fn in a separate fn like IsProxyPref() and call it
everywhere else?

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:97: //
We already listen for all events that concern proxies.
i was confused by ur comment and the code below.  u only want to execute the
code below if u haven't registered for notification right?  if yes, maybe
rephrase the comment to something like "if we're not yet listening for events
...., do so now."
shouldn't u be incrementing proxy_registrar_clients_ for ObserverPref of every
proxy setting even if proxy_registrar_ is not empty?  'cos u decrement
proxy_registrar_clients for StopObservingPref of every proxy setting.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:99:
proxy_registrar_clients_++;
should pre-increment?

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:117:
ProcessUserMetric(value, metric);
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> I don't like the duplication here...

well, it's not exact duplication 'cos u miss out the resetting handling_change_
to false like for other settings.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:135: }
else if (path.find(kProxyPrefsPrefix) == 0) {
wouldn't kProxyPRefsPrefix return true for the first "if
CrosSettings::IsCrosSettings"?  then ur path.find will never execute.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:160:
NotifyNetworkChanged();
like i mentioned in chrome_notification_types.h, NotifyNetworkChanged is
probably not the correct function name.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:209:
DCHECK(value);
DCHECK(value) is useless for chrome on chromeos, 'cos it's always run in
non-debug mode, so u're gonna crash below if value is null.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:211:
pref_callback_map_.find(kProxySettings[i]);
indentation

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:212:
iter != pref_callback_map_.end(); ++iter) {
indentation

http://codereview.chromium.org/8467012/diff/3001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:831:
NOTIFICATION_CURRENT_NETWORK_CHANGED,
based on ur notification name and comments, i think u misunderstood what it
actually does.  it's not when user changes the current network.  it's when user
clicks on the "Options" button for a specific network, which may be the
currently active one or any other inactive remembered network.  this has nothing
to do with changing the currently active network - user has to click "Connect"
to a new network to change the currently active network.

[pastarmovj, 2011-11-09 17:51:53]

Thank you both for the review. I tried to address all your comments.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:79:
browser->profile(), setting_path, &value)) {
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> +4 indentation

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_parser.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:72: }
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> // namespace

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.cc:74: void
ProxyCrosSettingsParser::SetProxyPrefValue(Profile* profile,
On 2011/11/08 14:45:25, kuan wrote:
> On 2011/11/08 09:25:22, Mattias Nissler wrote:
> > These large functions suck. I wonder whether we can just switch
> > ProxyConfigServiceImpl to hand out and accept a DictionaryValue
representation
> > of proxy config? Kuan, WDYT? Probably not in this CL though.
> 
> somewhere some code has to do the parsing with multiple if's.  u're, in
essence,
> suggesting moving these large functions from here to ProxyConfigServiceImpl. 
> personally, i prefer to separate ui, data and logic. these dictionaries are
> really used only for UI to pass info from javascript to native code, whereas
> ProxyConfigServiceImpl should just handle the so-called "backend" like using
> structures.
> however, if ur suggestion is the preferred approach by concensus, u're right
> that it probably shldn't be in this cl.

Ok keeping as is for now. Those functions are anyhow now two standalone
functions and pretty straightforward in their intent so we can keep them there
for a while.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
File chrome/browser/chromeos/proxy_cros_settings_parser.h (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:7: #pragma once
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> #include <string>

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:9: #include "base/values.h"
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> forward declaration would suffice.

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:10: #include
"chrome/browser/chromeos/proxy_config_service_impl.h"
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> don't need this?

Done. I need it in the cc only.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:30: 
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> Class description.

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:36: const base::Value*
in_value);
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/chromeos/prox...
chrome/browser/chromeos/proxy_cros_settings_parser.h:44: // helper functions.
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> Make it a namespace then?

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:108:
proxy_handler_->SetNetworkName(network_name);
On 2011/11/08 14:45:25, kuan wrote:
> maybe use a local var for GetProfile()->GetProxyConfigTracker?

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
(right):

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:19: 
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> merge the two include blocks.

Done. Facepalm for me :)

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:48: 
On 2011/11/08 14:45:25, kuan wrote:
> u may want to comment here and proxy_cros_settings_parser.h/cc that
> kProxySettings must include all settings declared in
> proxy_cros_settings_parser.h, so that future changes to settings will be
> consistent throughout. 

I moved that to proxy_cros_settings_parser.h.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:65: if
(pref_name.find(kProxyPrefsPrefix) == 0) {
On 2011/11/08 14:45:25, kuan wrote:
> is StartWith enough?
> in any case, maybe place this fn in a separate fn like IsProxyPref() and call
it
> everywhere else?

It is enough and I actually made a nice helper function for that.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:97: //
We already listen for all events that concern proxies.
On 2011/11/08 14:45:25, kuan wrote:
> i was confused by ur comment and the code below.  u only want to execute the
> code below if u haven't registered for notification right?  if yes, maybe
> rephrase the comment to something like "if we're not yet listening for events
> ...., do so now."
> shouldn't u be incrementing proxy_registrar_clients_ for ObserverPref of every
> proxy setting even if proxy_registrar_ is not empty?  'cos u decrement
> proxy_registrar_clients for StopObservingPref of every proxy setting.

You were right the comment was outdated and wrong. Fixed.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:98: if
(proxy_registrar_.IsEmpty()) {
On 2011/11/08 09:25:22, Mattias Nissler wrote:
> why not just listen in any case?

Done so.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:99:
proxy_registrar_clients_++;
On 2011/11/08 14:45:25, kuan wrote:
> should pre-increment?

Removed altogether.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:117:
ProcessUserMetric(value, metric);
On 2011/11/08 14:45:25, kuan wrote:
> On 2011/11/08 09:25:22, Mattias Nissler wrote:
> > I don't like the duplication here...
> 
> well, it's not exact duplication 'cos u miss out the resetting
handling_change_
> to false like for other settings.

I agree the one line repetition is really not such a big deal I could reverse
the order and possibly get rid of it but why convoluting the logic just for
that?

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:135: }
else if (path.find(kProxyPrefsPrefix) == 0) {
On 2011/11/08 14:45:25, kuan wrote:
> wouldn't kProxyPRefsPrefix return true for the first "if
> CrosSettings::IsCrosSettings"?  then ur path.find will never execute.

True good catch!

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:160:
NotifyNetworkChanged();
On 2011/11/08 14:45:25, kuan wrote:
> like i mentioned in chrome_notification_types.h, NotifyNetworkChanged is
> probably not the correct function name.

Is removed now in favor of a callback.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:209:
DCHECK(value);
On 2011/11/08 14:45:25, kuan wrote:
> DCHECK(value) is useless for chrome on chromeos, 'cos it's always run in
> non-debug mode, so u're gonna crash below if value is null.

This is not really true. If you build chrome from external sources and with the
proper flags you get debug builds of chrome on ChromeOS. Crashing will happen
anyway but in debug we'll get the nice stack trace too.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:211:
pref_callback_map_.find(kProxySettings[i]);
On 2011/11/08 14:45:25, kuan wrote:
> indentation

I stole that from the function above but now it's changed to my preferred
syntax. Initialize the iterator outside of the for decl.

http://codereview.chromium.org/8467012/diff/3001/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:212:
iter != pref_callback_map_.end(); ++iter) {
On 2011/11/08 14:45:25, kuan wrote:
> indentation

Done.

http://codereview.chromium.org/8467012/diff/3001/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/8467012/diff/3001/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:831:
NOTIFICATION_CURRENT_NETWORK_CHANGED,
On 2011/11/08 14:45:25, kuan wrote:
> based on ur notification name and comments, i think u misunderstood what it
> actually does.  it's not when user changes the current network.  it's when
user
> clicks on the "Options" button for a specific network, which may be the
> currently active one or any other inactive remembered network.  this has
nothing
> to do with changing the currently active network - user has to click "Connect"
> to a new network to change the currently active network.

No such notification anymore. But thanks for the clarification anyway it was
really helpful!

[kuan, 2011-11-10 14:27:25]

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

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:476: for (;iter !=
callbacks_.end(); ++iter) {
s/;iter/; iter/

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:479: }
don't need curly braces

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:487: for (;iter !=
callbacks_.end(); ++iter) {
s/;iter/; iter/

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:793: iter++;
pre-increment

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

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:185: // Add/Remove callback
functions for network changed by the UI.
s/network/notification when network to be viewed is/

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:300: // Callbacks for
notification when netowrk has been changed from the UI.
s/netowrk/network to be viewed/

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_parser.h (right):

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_parser.h:41: bool IsProxyPref(const
std::string& path);
describe IsProxyPref, insert a line after.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:104:
PrefProxyConfigTracker* proxy_config = GetProfile()->GetProxyConfigTracker();
hm... maybe use proxy_tracker?

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

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:22:
#include "content/public/browser/notification_source.h"
now tt u've added notification_service.h, u can remove notification_details.h
and notification_source.h 'cos they're included in the former.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:34:
PrefProxyConfigTracker* proxy_config =
maybe use proxy_tracker?

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:46: //
Observe the PrefProxyConfigTracker for changes from the UI.
s/PrefProxyConfigTracker/chromeos::ProxyConfigServiceImpl/

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:47:
PrefProxyConfigTracker* proxy_config =
s/proxy_config/proxy_tracker/

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:178:
void CoreChromeOSOptionsHandler::NotifyNetworkChanged() {
i'm still uncomfortable with the fn name, 'cos it sounds like network has
changed, which hasn't.  maybe change to something to the effect of
NotifyViewingProxyChanged or NotifyChangeInProxyToBeViewed?

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:36:
#include "chrome/browser/net/pref_proxy_config_tracker.h"
u don't need this, 'cos it's included by proxy_config_service_impl.h

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:695:
Profile::FromWebUI(web_ui_)->GetProxyConfigTracker()->UISetCurrentNetwork(
i think u need to check "if (web_ui_)"; i vaguely remember that
PopulateDictionaryDetails can sometimes be called b4 web_ui_ is initialized.

[pastarmovj, 2011-11-10 16:08:25]

Thanks for looking into it again! I have addressed all the nits and try jobs are
already running on it.

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

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:476: for (;iter !=
callbacks_.end(); ++iter) {
On 2011/11/10 14:27:25, kuan wrote:
> s/;iter/; iter/

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:479: }
On 2011/11/10 14:27:25, kuan wrote:
> don't need curly braces

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:487: for (;iter !=
callbacks_.end(); ++iter) {
On 2011/11/10 14:27:25, kuan wrote:
> s/;iter/; iter/

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:793: iter++;
On 2011/11/10 14:27:25, kuan wrote:
> pre-increment

Done.

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

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:185: // Add/Remove callback
functions for network changed by the UI.
On 2011/11/10 14:27:25, kuan wrote:
> s/network/notification when network to be viewed is/

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.h:300: // Callbacks for
notification when netowrk has been changed from the UI.
On 2011/11/10 14:27:25, kuan wrote:
> s/netowrk/network to be viewed/

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
File chrome/browser/chromeos/proxy_cros_settings_parser.h (right):

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_cros_settings_parser.h:41: bool IsProxyPref(const
std::string& path);
On 2011/11/10 14:27:25, kuan wrote:
> describe IsProxyPref, insert a line after.

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc (right):

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/proxy_settings_ui.cc:104:
PrefProxyConfigTracker* proxy_config = GetProfile()->GetProxyConfigTracker();
On 2011/11/10 14:27:25, kuan wrote:
> hm... maybe use proxy_tracker?

Done.

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

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:22:
#include "content/public/browser/notification_source.h"
On 2011/11/10 14:27:25, kuan wrote:
> now tt u've added notification_service.h, u can remove notification_details.h
> and notification_source.h 'cos they're included in the former.

Isn't the policy to include everything that we use even if it is indirectly
included? The IWYU model?

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:34:
PrefProxyConfigTracker* proxy_config =
On 2011/11/10 14:27:25, kuan wrote:
> maybe use proxy_tracker?

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:46: //
Observe the PrefProxyConfigTracker for changes from the UI.
On 2011/11/10 14:27:25, kuan wrote:
> s/PrefProxyConfigTracker/chromeos::ProxyConfigServiceImpl/

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:47:
PrefProxyConfigTracker* proxy_config =
On 2011/11/10 14:27:25, kuan wrote:
> s/proxy_config/proxy_tracker/

Done.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:178:
void CoreChromeOSOptionsHandler::NotifyNetworkChanged() {
On 2011/11/10 14:27:25, kuan wrote:
> i'm still uncomfortable with the fn name, 'cos it sounds like network has
> changed, which hasn't.  maybe change to something to the effect of
> NotifyViewingProxyChanged or NotifyChangeInProxyToBeViewed?

True the meaning has changed and given that it actually also notifies changes to
prefs from the ui it can be called even more generic NotifyProxyPrefsChanged.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:36:
#include "chrome/browser/net/pref_proxy_config_tracker.h"
On 2011/11/10 14:27:25, kuan wrote:
> u don't need this, 'cos it's included by proxy_config_service_impl.h

Again I think that we need to include whatever we use.

http://codereview.chromium.org/8467012/diff/11001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:695:
Profile::FromWebUI(web_ui_)->GetProxyConfigTracker()->UISetCurrentNetwork(
On 2011/11/10 14:27:25, kuan wrote:
> i think u need to check "if (web_ui_)"; i vaguely remember that
> PopulateDictionaryDetails can sometimes be called b4 web_ui_ is initialized.

Done.

[kuan, 2011-11-11 12:08:15]

lgtm

[Mattias Nissler (ping if slow), 2011-11-11 12:10:45]

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

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:479: }
std::find

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:491: }
std::find

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:789: iter =
callbacks_.erase(iter);
You can't do that while still iterating.

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

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:33: if
(initialized_) {
the way this is handled in ProxyConfigServiceImpl, you can even Remove the
callback without checking whether you added it before. Also, have you verified
that callback comparisons actually do the right thing?

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:43:
initialized_ = true;
maybe move this to the end of the function?

[pastarmovj, 2011-11-11 15:17:31]

Addressed your comments

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

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:479: }
On 2011/11/11 12:10:46, Mattias Nissler wrote:
> std::find

After spending an hour playing with templates and traits I came up with the
solution you see in the new version. WDYT?

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:491: }
On 2011/11/11 12:10:46, Mattias Nissler wrote:
> std::find

Ditto.

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:789: iter =
callbacks_.erase(iter);
On 2011/11/11 12:10:46, Mattias Nissler wrote:
> You can't do that while still iterating.

All other iterators are invalidated true but the iterator returned by erase is
the one pointing to the next element after the erased one in the updated vector
(possibly one pointing to end()) so we can safely resume from it.

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

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:33: if
(initialized_) {
On 2011/11/11 12:10:46, Mattias Nissler wrote:
> the way this is handled in ProxyConfigServiceImpl, you can even Remove the
> callback without checking whether you added it before. Also, have you verified
> that callback comparisons actually do the right thing?

You are right I cleaned up a bit.

The Equals operator in the Callback class checks if the object pointer and the
function pointer coincide and the WeakPointerFactory is still only wrapper for
this so as long as this is the same this check should return true.

http://codereview.chromium.org/8467012/diff/22001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc:43:
initialized_ = true;
On 2011/11/11 12:10:46, Mattias Nissler wrote:
> maybe move this to the end of the function?

Given no returns here the only change we won't do the register is if we crash
which will sort of invalidate the destructor too ;) but sure semantically it is
not initialized at this point yet.

[Mattias Nissler (ping if slow), 2011-11-14 08:35:44]

lgtm

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

http://codereview.chromium.org/8467012/diff/34001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:45: // Tiny stl helper
function to allow using the find_if syntax on objects that
s/stl/STL/

http://codereview.chromium.org/8467012/diff/34001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:46: // doesn't use the ==
operator but implement the Equals function which is the
s/the == operator/operator==/

http://codereview.chromium.org/8467012/diff/34001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:50: const T& key) {
indentation

[pastarmovj, 2011-11-14 14:20:34]

Fixed the nits. Trybots seem fine and I did the manual test that Kuan proposed -
setting different settings for different networks and check that they are
correctly distinguished.

I am hitting the CQ.

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

http://codereview.chromium.org/8467012/diff/34001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:45: // Tiny stl helper
function to allow using the find_if syntax on objects that
On 2011/11/14 08:35:45, Mattias Nissler wrote:
> s/stl/STL/

Done.

http://codereview.chromium.org/8467012/diff/34001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:46: // doesn't use the ==
operator but implement the Equals function which is the
On 2011/11/14 08:35:45, Mattias Nissler wrote:
> s/the == operator/operator==/

Done.

http://codereview.chromium.org/8467012/diff/34001/chrome/browser/chromeos/pro...
chrome/browser/chromeos/proxy_config_service_impl.cc:50: const T& key) {
On 2011/11/14 08:35:45, Mattias Nissler wrote:
> indentation

Done.

[commit-bot: I haz the power, 2011-11-14 14:20:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/8467012/26006

[commit-bot: I haz the power, 2011-11-14 14:21:05]

Presubmit check for 8467012-26006 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/chromeos/core_chromeos_options_handler.cc:184

Presubmit checks took 2.4s to calculate.