Adding a NetworkProfileHandler used by ManagedNetworkConfigurationHandler.

Adding a NetworkProfileHandler used by ManagedNetworkConfigurationHandler.

This handler tracks Shill's profiles.
Currently only the ManagedNetworkConfigurationHandler is making use of this
handler but it is also required for upcoming changes to proxy handling in ChromeOS.

In the ONC validator only an LOG(ERROR) was fixed to LOG(WARNING) if the flag error_on_missing_field=false.

BUG=157696
TEST=managed_network_configuration_handler_unittest.cc, networking_private_apitest.cc

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198798

[pneubeck (no reviews), 2013-04-26 17:43:18]

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:382: LOG(WARNING) << message;
This is the only change in behavior of the Validator.

[stevenjb, 2013-04-26 21:00:49]

https://codereview.chromium.org/13957012/diff/22001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:75: // committed.
crbug? Or fix before committing?

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:22: ShillManagerClient::TestInterface*
manager_test);
I don't know that I like this. Is there ever a situation where we would
initialize ShillProfileClientStub() without initializing
ShillManagerClientStub()? i.e. can't ShillProfileClientStub just use
DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface()?

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:60: };
This seems unnecessary and confusing. Why not just make GetByGUID a private
method and Map a typedef/member of of ManagedNetworkConfiguarionHandler ?

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:98: }
Is this only for logging? If so, name it appropriately, otherwise we need to
localize it.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:232: }
nit: I tend to prefer if() over case for complex code like this. It doesn't seem
like the kind of thing where a type might get added and a compiler warning would
prevent a bug; adding an 'else NOTREACHED()' would be sufficient.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:411: }
nit: Again, even though it's less complex, if/else is simpler.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:447: break;
nit: ditto

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:61: static void
Initialize(NetworkProfileHandler* profile_handler);
Again, I dislike passing singletons as construction arguments.

If we need to set a handler other than the singleton instance (and I don't think
we should ever need to), set profile_handler_ to the singleton in the
constructor, and add SetNetworkProfileHandlerForTest() to override it.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.cc:24: }
nit: Also better as if/else
Also, if only for logging name appropriately, otherwise we should localize.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.h:17: //                               //
unconfigured networks.
?

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.h:25: userhash(user_hash) {}
nit: } on separate line, blank line before members

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.h:34: CHROMEOS_EXPORT std::string
ProfileToString(const NetworkProfile* profile);
This should be namespaced (chromeos is a very large namespace) or, (preferably
imho) made a member of NetworkProfile (which should probably be a class).

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile_handler.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:29: NetworkProfileHandler*
g_profile_handler_instance = NULL;
nit: for debugging ease I prefer not to put the global instance in an anon
namespace.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:44: const std::string&
error_message) {
s/Get/Log

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:50: const
NetworkProfileHandler::ProfileList& profiles,
There's at least one case below where you really want a non-const iterator. If
you add a CompareByPath() method to NetworkProfile you could use std::find
instead of this and get either a const or non const iterator result.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:68:
g_profile_handler_instance->RequestInitialProfileList();
Why not do these in the constructor?

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:81:
RemovePropertyChangedObserver(g_profile_handler_instance);
In destructor?

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:117: DCHECK(profiles);
I'd rather not crash chrome if Shill fails to provide Profiles. We should
LOG(ERROR) and return instead.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:139: LOG(ERROR) << "  " << it->path;
remove

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:194:
profiles_.erase(profiles_.begin() + (found - profiles_.begin()));
Ew. See suggestion above for adding a comparator to NetworkProfile and using
std::find() instead of FindByPath.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:208: const std::string& userhash)
const {
This could also use std::find with a CompareByUserHash() operator.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_observer.h:20: virtual
~NetworkProfileObserver() {}
nit: these could be protected instead of using DISALLOW... (a bit cleaner for
pure virtual observer classes)

[pneubeck (no reviews), 2013-04-29 18:05:51]

Fixed most issues.
Please see also my comment
https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...

Thanks.

https://codereview.chromium.org/13957012/diff/22001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/networking_private_api.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/networking_private_api.cc:75: // committed.
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> crbug? Or fix before committing?
before committing

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:22: ShillManagerClient::TestInterface*
manager_test);
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> I don't know that I like this. Is there ever a situation where we would
> initialize ShillProfileClientStub() without initializing
> ShillManagerClientStub()? i.e. can't ShillProfileClientStub just use
> DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface()?

My thoughts were:
1.
managed_network_configuration_handler_unittest.cc is doing exactly that.
There, notifications from Shill and return values from calls to Shill have to be
consistent. With mocks that's rather cumbersome and repetitive.
Since this simple bookkeeping exists already in the Stubs, why not using it?
Of the ShillProfileClientStub it's using the storage of profile entries.
Of the NetworkProfileHandlerStub it's using the mapping between profiles and
users.

2.
Overall I find it rather unhealthy that the Stubs are mutually dependent on each
other. Because of that, we cannot use the Stubs one by one and cannot combine
Mocks with Stubs etc., but have to use all ClientStubs or none.
I see that that might be necessary for the ShillManagerClient but for the other
Clients?

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:60: };
Seems that I was overprotective against copies of the nested maps and ignorant
about the actual implementation of std::map.

Changing to map<string, map<string, dictionaryvalue*>>.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:98: }
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> Is this only for logging? If so, name it appropriately, otherwise we need to
> localize it.

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:232: }
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> nit: I tend to prefer if() over case for complex code like this. It doesn't
seem
> like the kind of thing where a type might get added and a compiler warning
would
> prevent a bug; adding an 'else NOTREACHED()' would be sufficient.

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:411: }
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> nit: Again, even though it's less complex, if/else is simpler.

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:447: break;
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> nit: ditto

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:61: static void
Initialize(NetworkProfileHandler* profile_handler);
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> Again, I dislike passing singletons as construction arguments.
> 
> If we need to set a handler other than the singleton instance (and I don't
think
> we should ever need to), set profile_handler_ to the singleton in the
> constructor, and add SetNetworkProfileHandlerForTest() to override it.

My reasons/ideas were:

The unit test of ManangedNetworkConfigurationHandler provides a Stub as an
argument.

This way the compiler forces the caller to provide a profile_handler and thus to
construct the Handlers in the correct order. It also doesn't depend on the
ProfileHandler to be a singleton and isn't using a global access.

In order to reduce the number of singletons (global ownership) and global
accesses, one solution could be to let DBusServices own the bunch of Handlers
which are explicitly dependent on each other. DBusServices itself could be
reachable globally if necessary.
Using the explicit argument here, was meant to be a step in that direction.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:93:
ManagedNetworkConfigurationHandler::Initialize(&stub_profile_handler_);
I think it would be better, to use the constructor directly instead of using
Initialize(..).

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.cc:24: }
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> nit: Also better as if/else
> Also, if only for logging name appropriately, otherwise we should localize.

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.h:17: //                               //
unconfigured networks.
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> ?

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.h:25: userhash(user_hash) {}
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> nit: } on separate line, blank line before members

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile.h:34: CHROMEOS_EXPORT std::string
ProfileToString(const NetworkProfile* profile);
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> This should be namespaced (chromeos is a very large namespace) or, (preferably
> imho) made a member of NetworkProfile (which should probably be a class).

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile_handler.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:44: const std::string&
error_message) {
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> s/Get/Log

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:50: const
NetworkProfileHandler::ProfileList& profiles,
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> There's at least one case below where you really want a non-const iterator. If
> you add a CompareByPath() method to NetworkProfile you could use std::find
> instead of this and get either a const or non const iterator result.

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:68:
g_profile_handler_instance->RequestInitialProfileList();
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> Why not do these in the constructor?

because of the derived Mock and Stub.
As many people would find it certainly confusing if the Stub would derive from
the actual implementation, the lengthy alternative seems to be:
- make ProfileHandler a pure interface
- call this implementation ..Base
- derive ..Impl from ..Base which un-/registers in the de-/constructor
- derive ..Stub from ..Base

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:117: DCHECK(profiles);
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> I'd rather not crash chrome if Shill fails to provide Profiles. We should
> LOG(ERROR) and return instead.

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:139: LOG(ERROR) << "  " << it->path;
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> remove

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:194:
profiles_.erase(profiles_.begin() + (found - profiles_.begin()));
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> Ew. See suggestion above for adding a comparator to NetworkProfile and using
> std::find() instead of FindByPath.

Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:208: const std::string& userhash)
const {
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> This could also use std::find with a CompareByUserHash() operator.

IMO, not worth the overhead of the comparator definition as above. Done.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_observer.h:20: virtual
~NetworkProfileObserver() {}
On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> nit: these could be protected instead of using DISALLOW... (a bit cleaner for
> pure virtual observer classes)
We should keep it consistent with other observers, e.g.
NetworkStateHandlerObserver.

I'd prefer using the macro, instead of forgetting to private-ing the copy or
assignment operator.

[stevenjb, 2013-04-30 17:42:46]

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:22: ShillManagerClient::TestInterface*
manager_test);
I believe strongly that we *shouldn't* combine mocks with stubs, not within a
subdirectory (i.e chromeos/dbus) anyway. We should either use all of the shill
stubs, or none.

The stubs are consciously designed to work together to emulate Shill behavior
and not work independently. While some may work independently now, maintaining
that is going to be difficult.

What problems arise if you use ShillManagerClientStub?

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:61: static void
Initialize(NetworkProfileHandler* profile_handler);
I agree that we might want to create a single class to maintain the various
network handlers at some point, once they start to stabalize.

I do not feel that passing them in constructors is necessarily a step in that
direction.

For now I would much rather treat singletons as singletons, until / unless that
changes.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:93:
ManagedNetworkConfigurationHandler::Initialize(&stub_profile_handler_);
On 2013/04/29 18:05:51, pneubeck wrote:
> I think it would be better, to use the constructor directly instead of using
> Initialize(..).

If we were using a mock or a locally defined stub, I would suggest adding a
setter to ManagedNetworkConfigurationHandler to allow overriding the profile
handler. But since we're using NetworkProfileHandlerStub anwyay, I'd just rather
use the instances created by DbusThreadManager.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:151:
ShillProfileClientStub stub_profile_client_;
Again, mixing mocks and stubs is going to be very difficult to maintain. Also,
if DbusThreadManager is initialized then there will be two stub instances, which
can lead to subtle errors (I've seen that happen).

Please either use a mock or local stub, or just use all of the stubs together as
they are intended.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile_handler.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:68:
g_profile_handler_instance->RequestInitialProfileList();
On 2013/04/29 18:05:51, pneubeck wrote:
> On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> > Why not do these in the constructor?
> 
> because of the derived Mock and Stub.
> As many people would find it certainly confusing if the Stub would derive from
> the actual implementation, the lengthy alternative seems to be:
> - make ProfileHandler a pure interface
> - call this implementation ..Base
> - derive ..Impl from ..Base which un-/registers in the de-/constructor
> - derive ..Stub from ..Base
I see. Typically we would use an Impl class and call:
g_profile_handler_instance = new NetworkProfileHandlerImpl()

I think you could easily do that without renaming the base class, and I think it
would be more clear.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_observer.h:20: virtual
~NetworkProfileObserver() {}
On 2013/04/29 18:05:51, pneubeck wrote:
> On 2013/04/26 21:00:49, stevenjb (chromium) wrote:
> > nit: these could be protected instead of using DISALLOW... (a bit cleaner
for
> > pure virtual observer classes)
> We should keep it consistent with other observers, e.g.
> NetworkStateHandlerObserver.
> 
> I'd prefer using the macro, instead of forgetting to private-ing the copy or
> assignment operator.

NetworkStateHandlerObserver is not a pure virtual class.

If you use DISALLOW_COPY_AND_ASSIGN, then the destructor should not be inlined.
(Virtual functions, including destructors, should never be inlined. The only
exception we make is for pure interface classes, which should have a protected
inlined empty destructor).

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:57: typedef
std::map<std::string, const base::DictionaryValue*> GuidToPolicy;
nit: Even though the Map is implicit, I think that GuidToPolicyMap would be more
clear.

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/network_...
File chromeos/network/network_profile_handler.cc (right):

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:195:
profiles_.erase(profiles_.begin() + (found - profiles_.begin()));
Can't this now just be profiles_.erase(found)?

[pneubeck (no reviews), 2013-05-03 17:32:55]

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:22: ShillManagerClient::TestInterface*
manager_test);
On 2013/04/30 17:42:47, stevenjb (chromium) wrote:
> I believe strongly that we *shouldn't* combine mocks with stubs, not within a
> subdirectory (i.e chromeos/dbus) anyway. We should either use all of the shill
> stubs, or none.
> 
> The stubs are consciously designed to work together to emulate Shill behavior
> and not work independently. While some may work independently now, maintaining
> that is going to be difficult.
> 
> What problems arise if you use ShillManagerClientStub?

I have to use the ClientMocks in the unit test to verify exactly which calls are
done by the ConfigHandler.

I don't use the ProfileStub anymore, which required to add a seperate
ProfilesStub in the test file.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:61: static void
Initialize(NetworkProfileHandler* profile_handler);
On 2013/04/30 17:42:47, stevenjb (chromium) wrote:
> I agree that we might want to create a single class to maintain the various
> network handlers at some point, once they start to stabalize.
> 
> I do not feel that passing them in constructors is necessarily a step in that
> direction.

It is a step to prevent cyclic dependencies and to prevent incorrect
initialization order.
How will we prevent that otherwise?

Chrome code ended up with cyclic dependencies in other cases already. One was
related to UserManager and DeviceSettingsService AFAIK.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:151:
ShillProfileClientStub stub_profile_client_;
On 2013/04/30 17:42:47, stevenjb (chromium) wrote:
> Again, mixing mocks and stubs is going to be very difficult to maintain. Also,
> if DbusThreadManager is initialized then there will be two stub instances,
which
> can lead to subtle errors (I've seen that happen).

Why should there be two stubs? The MockDBusThreadManager is used, not the normal
one with ClientStubs.

> 
> Please either use a mock or local stub, or just use all of the stubs together
as
> they are intended.

Anyways, using a local stub now.

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/22001/chromeos/network/network_...
chromeos/network/network_profile_observer.h:20: virtual
~NetworkProfileObserver() {}
Ok. I lost you.

There are three things you mentioned:

1) Protect ctor/dtor instead of DISALLOW...

Declaring a non-public default constructor doesn't influence the implicitly
defined copy ctor and assignment operator.
So I don't understand that point.

2) Declaring the ctor protected

Doesn't change anything, this class is abstract.

3) Declaring the dtor protected

Would prevent polymorphic destruction of NetworkProfileObserver, which I think
should be fine.
Done.

4) Not inlining the destructor

The reason for not inlining the destructor, according to the styleguide, is that
there could be a surprising amount of implicit work hidden or it might increase
the number of includes. Both is apparently not the case here. There was also
some discussion on some mailinglist:
https://groups.google.com/a/google.com/forum/#!search/destructor$20inline/c-s...

Since not-inlining the destructor here leads to overhead for the reader and even
more boiler-plate, I'd prefer keeping it inlined.

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:57: typedef
std::map<std::string, const base::DictionaryValue*> GuidToPolicy;
On 2013/04/30 17:42:47, stevenjb (chromium) wrote:
> nit: Even though the Map is implicit, I think that GuidToPolicyMap would be
more
> clear.
Done for both typenames.

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/network_...
File chromeos/network/network_profile_handler.cc (right):

https://codereview.chromium.org/13957012/diff/45001/chromeos/network/network_...
chromeos/network/network_profile_handler.cc:195:
profiles_.erase(profiles_.begin() + (found - profiles_.begin()));
On 2013/04/30 17:42:47, stevenjb (chromium) wrote:
> Can't this now just be profiles_.erase(found)?
Uhrg, of course.

[stevenjb, 2013-05-03 18:34:52]

LGTM, thanks!

https://codereview.chromium.org/13957012/diff/88001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/13957012/diff/88001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:21: explicit ShillProfileClientStub();
nit: no need for explicit now

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:59: class
ShillProfileStub {
nit: Let's name this something like ShillProfileTestClient. I know that "stub"
is a generic term, but in this context we kind of specifically refer to a
handful of related and co-dependent classes as "Stub" (part of my preference for
this implementation).

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/network_...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/network_...
chromeos/network/network_profile_observer.h:19: NetworkProfileObserver() {}
nit: as a pure virtual class, this doesn't really need either a constructor or
DISALLOW (with the destructor protected), but I really don't think it's
important enough to merit further discussion :)

[pneubeck (no reviews), 2013-05-06 07:24:43]

https://codereview.chromium.org/13957012/diff/88001/chromeos/dbus/shill_profi...
File chromeos/dbus/shill_profile_client_stub.h (right):

https://codereview.chromium.org/13957012/diff/88001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:21: explicit ShillProfileClientStub();
On 2013/05/03 18:34:53, stevenjb (chromium) wrote:
> nit: no need for explicit now

Done.

https://codereview.chromium.org/13957012/diff/88001/chromeos/dbus/shill_profi...
chromeos/dbus/shill_profile_client_stub.h:21: explicit ShillProfileClientStub();
On 2013/05/03 18:34:53, stevenjb (chromium) wrote:
> nit: no need for explicit now

Done.

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_unittest.cc:59: class
ShillProfileStub {
On 2013/05/03 18:34:53, stevenjb (chromium) wrote:
> nit: Let's name this something like ShillProfileTestClient. I know that "stub"
> is a generic term, but in this context we kind of specifically refer to a
> handful of related and co-dependent classes as "Stub" (part of my preference
for
> this implementation).

Done.

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/network_...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/88001/chromeos/network/network_...
chromeos/network/network_profile_observer.h:19: NetworkProfileObserver() {}
On 2013/05/03 18:34:53, stevenjb (chromium) wrote:
> nit: as a pure virtual class, this doesn't really need either a constructor or
> DISALLOW (with the destructor protected), but I really don't think it's
> important enough to merit further discussion :)

I think we found the sweet spot.
The disallowed assign prevents things like:

Observer *a, *b = ...
*a = *b;

Copy/Default constructor are useless (the latter was necessary, because of the
former).

[pneubeck (no reviews), 2013-05-07 13:48:58]

@Joao, could you please take a look at the diff of the last patch to patch 8.
Thanks!

[pneubeck (no reviews), 2013-05-07 13:50:09]

@Nikita, PTAL at the changes in the last patch. Thanks!

[Joao da Silva, 2013-05-07 14:31:35]

Changes from patch set 8 -> 11 (re username hash) lgtm. I don't fully understand
what's going on in the rest of the CL, left a couple of nitpicks.

https://codereview.chromium.org/13957012/diff/113001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:45: } //
namespace
nit: two spaces before //

https://codereview.chromium.org/13957012/diff/113001/chromeos/dbus/shill_prof...
File chromeos/dbus/shill_profile_client_stub.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/dbus/shill_prof...
chromeos/dbus/shill_profile_client_stub.cc:33: void PassEmptyDictionary(
This isn't used.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.cc:59: const char
kServicePath[] = "servicePath";
This is not used.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.cc:64: const char
kUIDataError[] = "Error.UIData";
These 2 are not used.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.cc:307: 
nit: remove newline

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.h:160: const
NetworkProfile& profile) const;
nit: indent

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
File chromeos/network/network_profile.h (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
chromeos/network/network_profile.h:27: std::string userhash;  // Only set for
user profiles.
Are these 2 const?

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
File chromeos/network/network_profile_handler_stub.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
chromeos/network/network_profile_handler_stub.cc:7: 
nit: remove newline

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
chromeos/network/network_profile_handler_stub.cc:11:
NetworkProfileHandler::AddProfile(profile);
You cloud just expose the protected method in the header:

class NetworkProfileHandlerStub : public NetworkProfileHandler {
 public:
  using NetworkProfileHandler::AddProfile;
  ...
};

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
chromeos/network/network_profile_observer.h:29: 
nit: remove newline

[pneubeck (no reviews), 2013-05-07 15:03:19]

https://codereview.chromium.org/13957012/diff/113001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/networking_private_apitest.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/networking_private_apitest.cc:45: } //
namespace
On 2013/05/07 14:31:36, Joao da Silva wrote:
> nit: two spaces before //

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/dbus/shill_prof...
File chromeos/dbus/shill_profile_client_stub.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/dbus/shill_prof...
chromeos/dbus/shill_profile_client_stub.cc:33: void PassEmptyDictionary(
On 2013/05/07 14:31:36, Joao da Silva wrote:
> This isn't used.

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.cc:59: const char
kServicePath[] = "servicePath";
On 2013/05/07 14:31:36, Joao da Silva wrote:
> This is not used.

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.cc:64: const char
kUIDataError[] = "Error.UIData";
On 2013/05/07 14:31:36, Joao da Silva wrote:
> These 2 are not used.

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.cc:307: 
On 2013/05/07 14:31:36, Joao da Silva wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler.h:160: const
NetworkProfile& profile) const;
On 2013/05/07 14:31:36, Joao da Silva wrote:
> nit: indent

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
File chromeos/network/network_profile.h (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
chromeos/network/network_profile.h:27: std::string userhash;  // Only set for
user profiles.
On 2013/05/07 14:31:36, Joao da Silva wrote:
> Are these 2 const?

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
File chromeos/network/network_profile_handler_stub.cc (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
chromeos/network/network_profile_handler_stub.cc:11:
NetworkProfileHandler::AddProfile(profile);
On 2013/05/07 14:31:36, Joao da Silva wrote:
> You cloud just expose the protected method in the header:
> 
> class NetworkProfileHandlerStub : public NetworkProfileHandler {
>  public:
>   using NetworkProfileHandler::AddProfile;
>   ...
> };

Done.

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
File chromeos/network/network_profile_observer.h (right):

https://codereview.chromium.org/13957012/diff/113001/chromeos/network/network...
chromeos/network/network_profile_observer.h:29: 
On 2013/05/07 14:31:36, Joao da Silva wrote:
> nit: remove newline

Done.

[commit-bot: I haz the power, 2013-05-07 15:20:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/13957012/119004

[Nikita (slow), 2013-05-07 15:40:39]

lgtm

[commit-bot: I haz the power, 2013-05-07 15:45:54]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-07 17:41:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/13957012/119004

[commit-bot: I haz the power, 2013-05-07 21:08:15]

Change committed as 198798