Add NetworkHandler to own network handlers in src/chromeos/network

Add NetworkHandler to own network handlers in src/chromeos/network

This is just some code cleanup and should have no behavior changes.


BUG=239073
For webui/chromeos, browser/geolocation:
TBR=nkostylev@chromium.org,joth@chromium.org

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

[pneubeck (no reviews), 2013-05-13 09:17:06]

Most files miss the include of network_handler.h.

https://codereview.chromium.org/14729017/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater_impl.cc
(left):

https://codereview.chromium.org/14729017/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_impl.cc:24:
chromeos::ManagedNetworkConfigurationHandler* network_config_handler,
What is the improvement here? How will it make writing the missing test easier
(which will use a StrictMock of the config handler)?

https://codereview.chromium.org/14729017/diff/3001/chromeos/dbus/mock_dbus_th...
File chromeos/dbus/mock_dbus_thread_manager.cc (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/dbus/mock_dbus_th...
chromeos/dbus/mock_dbus_thread_manager.cc:201:
EXPECT_CALL(*mock_cryptohome_client_.get(), TpmIsEnabled(_))
unrelated change?

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:166:
NetworkHandler::Initialize();
This is one of the implications of your change that I dislike a lot.

This test is supposed to test the configuration handler in isolation. With this
initialization, anything can happen behind the scenes. Then additional unrelated
expectations have to be set (see  SetShillManagerClientExpectations) which
unnecessarily weaken the test.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:185:
AddProfileForTest(NetworkProfile(kUser1ProfilePath, kUser1));
I don't see the improvement here.
Ok, for now it doesn't make a huge difference if there is one test method in the
normal profile handler or not.
But if the new initialization code doesn't allow using this stub anymore, it
indicates that the move is not in the right direction. What if we really want to
use a stub here?


Another problem I see: How can we cover the initialization itself of any of the
handlers in tests and in isolation?

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:216:
EXPECT_CALL(mock_manager_client_, GetProperties(_)).Times(AnyNumber());
Are these necessary because of calls from the other handlers?
That's not good.

We should move towards better isolation in tests not less.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
File chromeos/network/network_handler.cc (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.cc:47: cert_loader_.reset(new CertLoader);
I'd suggest to use explicit passing of the mutual dependencies, like

... = new ProfileHandler(state_handler_.get())
... = new ManagedNetworkConfigurationHandler(profile_handler_.get())
...

That would improve the unit tests and avoid dependency/initialization errors.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.cc:136:
test_cert_loader_->AddObserver(observer);
I think, in this state, the Set*ForTest methods will behave in a rather
unexpected way.
There will be two handlers after a Set*ForTest call and both may have observers
and fire notifications.

The default handler should be destructed. But even then, there is no guarantee
that these calls will work reliably: The handlers might have done some work at
start up already. When these functions are called, the state of the handlers and
all related classes might be in an arbitrary state.

If possible, I strongly prefer using handler replacements from the very
beginning of a test instead of handing them in later.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:22: 
nit: remove empty line.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:38: NetworkStateHandler*
network_state_handler();
I thought you proposed state()?

I'd at least drop the prefix "network_".

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:44: void SetCertLoaderForTest(CertLoader*
cert_loader);
Since none of these are called yet, we could leave them out for now.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:65: // Initialize LoginState for tests so
they do not have to do so explicitly.
I'm not convinced that this belongs into this class.

[gauravsh, 2013-05-13 23:30:11]

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:166:
NetworkHandler::Initialize();
On 2013/05/13 09:17:06, pneubeck wrote:
> This is one of the implications of your change that I dislike a lot.
> 
> This test is supposed to test the configuration handler in isolation. With
this
> initialization, anything can happen behind the scenes. Then additional
unrelated
> expectations have to be set (see  SetShillManagerClientExpectations) which
> unnecessarily weaken the test.

I agree with this concern. Having recently dealt with this in the context of
mock network library expectations in unit tests, having to set canned mock
expectations unrelated to what's being tested makes the test much more fragile.

I think we can achieve the goal of keeping ownership of various Network*Handlers
and initialization order be clearly defined, while keeping tests well isolated.
Just let tests keep the ability to initialize these pieces individually without
needing to go through NetworkHandler. And have the production code always access
the handlers through NetworkHandler.

[stevenjb, 2013-05-13 23:49:34]

I specifically included network_handler.h in each *_handler.h to avoid requiring
two includes for every file that accesses the global handlers. It's not strict
adherence to include-what-you-use, but I think that it improves maintainability
since NetworkHandler only exists for maintaining and accessing the classes it
owns. I found it a nuisance to have to include cros_library in every file that
accessed NetworkLibrary.

Regarding the other related issues:

I fundamentally do not like passing around globals and storing them as members,
especially just for testing. I also feel that the handlers are better tested as
a group with "integration" tests so that if they have co-dependencies, those
dependencies are tested instead of mocked out. (I also however dislike Mock in
general and find it frequently leads to bad tests). Neither am I a fan of added
code complexity just for the sake of testing.

That said, I can't really argue against unit tests. However, I do think that
consistency is critical. If we are going to use members to initialize and access
handlers in some places, we should do so consistently.

So, I have uploaded a patch where I replace all access to handlers in
ManagedNetworkConfigurationHandler with explicit dependencies. 

Let me know what you think.

https://codereview.chromium.org/14729017/diff/3001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater_impl.cc
(left):

https://codereview.chromium.org/14729017/diff/3001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_impl.cc:24:
chromeos::ManagedNetworkConfigurationHandler* network_config_handler,
On 2013/05/13 09:17:06, pneubeck wrote:
> What is the improvement here? How will it make writing the missing test easier
> (which will use a StrictMock of the config handler)?
See meta comment

https://codereview.chromium.org/14729017/diff/3001/chromeos/dbus/mock_dbus_th...
File chromeos/dbus/mock_dbus_thread_manager.cc (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/dbus/mock_dbus_th...
chromeos/dbus/mock_dbus_thread_manager.cc:201:
EXPECT_CALL(*mock_cryptohome_client_.get(), TpmIsEnabled(_))
On 2013/05/13 09:17:06, pneubeck wrote:
> unrelated change?
Necessary to avoid Mock errors if we initialize NetworkHandler in combination
with mocks.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:166:
NetworkHandler::Initialize();
On 2013/05/13 09:17:06, pneubeck wrote:
> This is one of the implications of your change that I dislike a lot.
> 
> This test is supposed to test the configuration handler in isolation. With
this
> initialization, anything can happen behind the scenes. Then additional
unrelated
> expectations have to be set (see  SetShillManagerClientExpectations) which
> unnecessarily weaken the test.

See meta comment.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:185:
AddProfileForTest(NetworkProfile(kUser1ProfilePath, kUser1));
On 2013/05/13 09:17:06, pneubeck wrote:
> I don't see the improvement here.
> Ok, for now it doesn't make a huge difference if there is one test method in
the
> normal profile handler or not.
> But if the new initialization code doesn't allow using this stub anymore, it
> indicates that the move is not in the right direction. What if we really want
to
> use a stub here?
> 
> 
> Another problem I see: How can we cover the initialization itself of any of
the
> handlers in tests and in isolation?
See meta comment.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:216:
EXPECT_CALL(mock_manager_client_, GetProperties(_)).Times(AnyNumber());
On 2013/05/13 09:17:06, pneubeck wrote:
> Are these necessary because of calls from the other handlers?
> That's not good.
> 
> We should move towards better isolation in tests not less.
Yes. See meta comment.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
File chromeos/network/network_handler.cc (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.cc:47: cert_loader_.reset(new CertLoader);
On 2013/05/13 09:17:06, pneubeck wrote:
> I'd suggest to use explicit passing of the mutual dependencies, like
> 
> ... = new ProfileHandler(state_handler_.get())
> ... = new ManagedNetworkConfigurationHandler(profile_handler_.get())
> ...
> 
> That would improve the unit tests and avoid dependency/initialization errors.

See meta comment

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.cc:136:
test_cert_loader_->AddObserver(observer);
On 2013/05/13 09:17:06, pneubeck wrote:
> I think, in this state, the Set*ForTest methods will behave in a rather
> unexpected way.
> There will be two handlers after a Set*ForTest call and both may have
observers
> and fire notifications.
> 
> The default handler should be destructed. But even then, there is no guarantee
> that these calls will work reliably: The handlers might have done some work at
> start up already. When these functions are called, the state of the handlers
and
> all related classes might be in an arbitrary state.
> 
> If possible, I strongly prefer using handler replacements from the very
> beginning of a test instead of handing them in later.

If we exclusively use global accessors, then only one handler will ever be
active. This was just so that handlers that observe each other (and set that up
on init) work correctly with test implementations.

Alternately we can require that any test handlers be set up before
NetworkHandler::Init() gets called (we would need to call that explicitly
instead).

Since these are getting removed this is moot for now, we can decide this if/when
any of these get added.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:22: 
On 2013/05/13 09:17:06, pneubeck wrote:
> nit: remove empty line.

Done.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:38: NetworkStateHandler*
network_state_handler();
On 2013/05/13 09:17:06, pneubeck wrote:
> I thought you proposed state()?
> 
> I'd at least drop the prefix "network_".

I decided I prefer the name matching the class (clarity over brevity). I don't
like dropping 'network_' (geolocation at and cert would be inconsistent) but
could be persuaded to drop _handler if we really think brevity is important
here.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:44: void SetCertLoaderForTest(CertLoader*
cert_loader);
On 2013/05/13 09:17:06, pneubeck wrote:
> Since none of these are called yet, we could leave them out for now.
I added them specifically to make setting future mock implementations easier,
but yeah, should leave them out until we need them, especially since the test
strategy going forward is clearly up for discussion.
Done.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:65: // Initialize LoginState for tests so
they do not have to do so explicitly.
On 2013/05/13 09:17:06, pneubeck wrote:
> I'm not convinced that this belongs into this class.

Requiring any test that initializes NetworkHandler to also initialize LoginState
is rather tedious; this seemed like a reasonable compromise.

[pneubeck (no reviews), 2013-05-22 14:02:53]

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:38: NetworkStateHandler*
network_state_handler();
On 2013/05/13 23:49:35, stevenjb (chromium) wrote:
> On 2013/05/13 09:17:06, pneubeck wrote:
> > I thought you proposed state()?
> > 
> > I'd at least drop the prefix "network_".
> 
> I decided I prefer the name matching the class (clarity over brevity). I don't
> like dropping 'network_' (geolocation at and cert would be inconsistent) but
> could be persuaded to drop _handler if we really think brevity is important
> here.

I don't feel strong about that, so NIT:

That might indicate, that CertLoader/Geolocation do not belong here or have to
be renamed.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:65: // Initialize LoginState for tests so
they do not have to do so explicitly.
On 2013/05/13 23:49:35, stevenjb (chromium) wrote:
> On 2013/05/13 09:17:06, pneubeck wrote:
> > I'm not convinced that this belongs into this class.
> 
> Requiring any test that initializes NetworkHandler to also initialize
LoginState
> is rather tedious; this seemed like a reasonable compromise.

Tedious or not. It don't see an argument why it belongs here.

Which tests are we talking about? Browser tests shouldn't be affected. If a unit
tests uses this Handler instead of a Stub/Mock, then it's also correct to setup
the Handler's environment in the test (i.e. also the DBus clients).

https://codereview.chromium.org/14729017/diff/33001/chromeos/network/network_...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/14729017/diff/33001/chromeos/network/network_...
chromeos/network/network_handler.h:23: // This clas should NOT be used in unit
tests. Instead, construct individual
clas -> class

clarify "unit tests" -> "unit tests of the Network*Handlers" or "unit tests
within this module"

[pneubeck (no reviews), 2013-05-22 15:28:08]

On 2013/05/13 23:49:34, stevenjb (chromium) wrote:
> I specifically included network_handler.h in each *_handler.h to avoid
requiring
> two includes for every file that accesses the global handlers. It's not strict
> adherence to include-what-you-use, but I think that it improves
maintainability
> since NetworkHandler only exists for maintaining and accessing the classes it
> owns. I found it a nuisance to have to include cros_library in every file that
> accessed NetworkLibrary.

This is yet another side effect of using global access to singletons:
1) The caller has one unnecessary dependency on the owner of the singleton
(NetworkHandler::Get or NetworkStateHandler::Get)
-) and one dependency on the singleton itself (NetworkStateHandler)


The global access leads to a bunch of other problems:
2) implicit dependency:
  a) you can run into cyclic dependencies without immediately noticing
  b) code is harder to change as the side-effects of the changes are harder to
guess ("Oh, I didn't know that this code depends on that class...")
  c) startup and shutdown are difficult to get right, as the dependencies are
not clearly visible.
3) unit tests need a workaround to replace the globals that the class accesses.
4) more work if at sometime it's decided that the singleton isn't one anymore.

2c) is mitigated somewhat in this particular case, as we use explicit
initialize/shutdown calls instead of use lazy initialization, but it's still
difficult to see where to place these init and shutdown calls.

Your Init() workaround solves the immediate problem 3), however this problem is
only caused by the global access and you have to add more complexity to solve
it. The workaround doesn't address the other issues.


On the other hand, I guess that the issue, to which your concerns are related,
is:
5) Accessing a pointer to a singleton can lead to problems during destruction
(singleton destroyed before the pointer is invalidated)

However, this is in no way related to the _storage_ but to the access of the
pointer and has to be solved anyways, no matter if you're using global access or
a stored pointer.
This is a separate issue and is generally solved by using a WeakPtr or similar,
which I think would be totally fine.
Or said differently: The pattern "IsInitialized(), Get()" emulates a WeakPtr and
thereby violates separation of concerns.


The other argument against passing a pointer, one could think of, is:
6) Passing pointers doesn't scale: The number of arguments gets too huge.

This argument is doesn't help either: Many arguments indicate bad structuring of
the code. So actually, this is an argument pro "passing pointers", as such
indicators are helpful.
Such accumulations can be solved correctly by combining whole subsystems of
handlers to a owning class, this "container" can then be passed whenever a
dependency to the whole subsystem is required. (thus the NetworkHandler is an
example of preventing too many "handler"-arguments).


IMO, global access has clearly more disadvantages and few (or no?) advantages
compared to storing some kind of pointer (WeakPtr<Singleton>, Singleton*, ...
anything as long as there's some indirection) which doesn't really have any
disadvantages.

From my observation, there is also a trend to remove singletons from Chrome/OS.
Surely not without reason, so I'd guess they experienced enough pain with global
accesses to singletons.

> 
> Regarding the other related issues:
> 
> I fundamentally do not like passing around globals and storing them as
members,
> especially just for testing. I also feel that the handlers are better tested
as
> a group with "integration" tests so that if they have co-dependencies, those
> dependencies are tested instead of mocked out. (I also however dislike Mock in
> general and find it frequently leads to bad tests). Neither am I a fan of
added
> code complexity just for the sake of testing.

How does that conflict with passing handler-pointers?
If someone wants to write such a test with all handlers, he can still use the
NetworkHandler to create the Handlers.

Mocks are for testing interactions. If there is not interaction to test, then
the mock is not needed.
If there is interaction there is just no way around it, you will always end up
with something that is mock-like, no matter how much you try to hide it.

[stevenjb, 2013-05-22 16:35:10]

On 2013/05/22 15:28:08, pneubeck wrote:
> On 2013/05/13 23:49:34, stevenjb (chromium) wrote:
> > I specifically included network_handler.h in each *_handler.h to avoid
> requiring
> > two includes for every file that accesses the global handlers. It's not
strict
> > adherence to include-what-you-use, but I think that it improves
> maintainability
> > since NetworkHandler only exists for maintaining and accessing the classes
it
> > owns. I found it a nuisance to have to include cros_library in every file
that
> > accessed NetworkLibrary.
> 
> This is yet another side effect of using global access to singletons:
> 1) The caller has one unnecessary dependency on the owner of the singleton
> (NetworkHandler::Get or NetworkStateHandler::Get)
> -) and one dependency on the singleton itself (NetworkStateHandler)
> 
> 
> The global access leads to a bunch of other problems:
> 2) implicit dependency:
>   a) you can run into cyclic dependencies without immediately noticing
>   b) code is harder to change as the side-effects of the changes are harder to
> guess ("Oh, I didn't know that this code depends on that class...")
>   c) startup and shutdown are difficult to get right, as the dependencies are
> not clearly visible.
> 3) unit tests need a workaround to replace the globals that the class
accesses.
> 4) more work if at sometime it's decided that the singleton isn't one anymore.
> 
> 2c) is mitigated somewhat in this particular case, as we use explicit
> initialize/shutdown calls instead of use lazy initialization, but it's still
> difficult to see where to place these init and shutdown calls.
> 
> Your Init() workaround solves the immediate problem 3), however this problem
is
> only caused by the global access and you have to add more complexity to solve
> it. The workaround doesn't address the other issues.
> 
> 
> On the other hand, I guess that the issue, to which your concerns are related,
> is:
> 5) Accessing a pointer to a singleton can lead to problems during destruction
> (singleton destroyed before the pointer is invalidated)
> 
> However, this is in no way related to the _storage_ but to the access of the
> pointer and has to be solved anyways, no matter if you're using global access
or
> a stored pointer.
> This is a separate issue and is generally solved by using a WeakPtr or
similar,
> which I think would be totally fine.
> Or said differently: The pattern "IsInitialized(), Get()" emulates a WeakPtr
and
> thereby violates separation of concerns.
> 
> 
> The other argument against passing a pointer, one could think of, is:
> 6) Passing pointers doesn't scale: The number of arguments gets too huge.
> 
> This argument is doesn't help either: Many arguments indicate bad structuring
of
> the code. So actually, this is an argument pro "passing pointers", as such
> indicators are helpful.
> Such accumulations can be solved correctly by combining whole subsystems of
> handlers to a owning class, this "container" can then be passed whenever a
> dependency to the whole subsystem is required. (thus the NetworkHandler is an
> example of preventing too many "handler"-arguments).
> 
> 
> IMO, global access has clearly more disadvantages and few (or no?) advantages
> compared to storing some kind of pointer (WeakPtr<Singleton>, Singleton*, ...
> anything as long as there's some indirection) which doesn't really have any
> disadvantages.
> 
> From my observation, there is also a trend to remove singletons from
Chrome/OS.
> Surely not without reason, so I'd guess they experienced enough pain with
global
> accesses to singletons.
> 

So, that's a very lengthy comment. Rather than pick at symantic differences, do
you have an alternative proposal here, or sepcific changes you would like?

My objectives with this CL are: 
a) reduce the number of unowned singletons.
b) make the co-dependencies within the network handler classes explicit.

> > 
> > Regarding the other related issues:
> > 
> > I fundamentally do not like passing around globals and storing them as
> members,
> > especially just for testing. I also feel that the handlers are better tested
> as
> > a group with "integration" tests so that if they have co-dependencies, those
> > dependencies are tested instead of mocked out. (I also however dislike Mock
in
> > general and find it frequently leads to bad tests). Neither am I a fan of
> added
> > code complexity just for the sake of testing.
> 
> How does that conflict with passing handler-pointers?
> If someone wants to write such a test with all handlers, he can still use the
> NetworkHandler to create the Handlers.
> 
> Mocks are for testing interactions. If there is not interaction to test, then
> the mock is not needed.
> If there is interaction there is just no way around it, you will always end up
> with something that is mock-like, no matter how much you try to hide it.

So, despite my initial reservations, I believe that the latest patchset supports
both mocks and isolated tests. Again, is there a specific suggestion or
alternative proposal?

[stevenjb, 2013-05-22 17:09:27]

PTAL

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:38: NetworkStateHandler*
network_state_handler();
On 2013/05/22 14:02:53, pneubeck wrote:
> On 2013/05/13 23:49:35, stevenjb (chromium) wrote:
> > On 2013/05/13 09:17:06, pneubeck wrote:
> > > I thought you proposed state()?
> > > 
> > > I'd at least drop the prefix "network_".
> > 
> > I decided I prefer the name matching the class (clarity over brevity). I
don't
> > like dropping 'network_' (geolocation at and cert would be inconsistent) but
> > could be persuaded to drop _handler if we really think brevity is important
> > here.
> 
> I don't feel strong about that, so NIT:
> 
> That might indicate, that CertLoader/Geolocation do not belong here or have to
> be renamed.
I don't think we should need to rename the other classes.

https://codereview.chromium.org/14729017/diff/3001/chromeos/network/network_h...
chromeos/network/network_handler.h:65: // Initialize LoginState for tests so
they do not have to do so explicitly.
On 2013/05/22 14:02:53, pneubeck wrote:
> On 2013/05/13 23:49:35, stevenjb (chromium) wrote:
> > On 2013/05/13 09:17:06, pneubeck wrote:
> > > I'm not convinced that this belongs into this class.
> > 
> > Requiring any test that initializes NetworkHandler to also initialize
> LoginState
> > is rather tedious; this seemed like a reasonable compromise.
> 
> Tedious or not. It don't see an argument why it belongs here.
> 
> Which tests are we talking about? Browser tests shouldn't be affected. If a
unit
> tests uses this Handler instead of a Stub/Mock, then it's also correct to
setup
> the Handler's environment in the test (i.e. also the DBus clients).
I went ahead and eliminated this and made CertLoader check
LoginState::IsInitialized(). Hopefully this will not cause any future dev pain.

[Greg Spencer (Chromium), 2013-05-22 22:25:52]

LGTM

[pneubeck (no reviews), 2013-05-23 08:26:33]

On 2013/05/22 16:35:10, stevenjb (chromium) wrote:
> So, that's a very lengthy comment. 
I just wanted to prevent to appear arbitrary or cranky.
It also feels fundamental and important enough to me that it's worth spending a
few minutes on the topic.

> Rather than pick at symantic differences, do
> you have an alternative proposal here, or sepcific changes you would like?

- c'tors of the handlers could have the pointers to the other handlers that are
required, e.g.
   NetworkConnectionHandler(NetworkStateHandler* network_state_handler,
                            NetworkConfigurationHandler*
network_configuration_handler);


- users of particular handlers could obtain a pointer to that handler in their
constructor (or some init function),
e.g.
    NetworPortalDetectorImpl(NetworkStateHandler* network_state_handler)

    NetworkPortalDetector::CreateInstance() {
      ...
NetworPortalDetectorImpl(NetworkHandler::Get()->network_state_handler());    //
TODO: the StateHandler should be provided by ChromeBrowserMainPartsChromeos
    }

  or if lifetime should be checked better:
    NetworPortalDetectorImpl(WeakPtr<NetworkStateHandler> network_state_handler)

    with NetworkStateHandler : public SupportsWeakPtr<NetworkStateHandler>

  In any case, this class should be changed away from lazy initialization to a
singleton with separate Initialize and Get functions.
  Then the StateHandler could be provided by ChromeBrowserMainPartsChromeos.


Other example:
    BurnManager::Initialize(..., NetworkStateHandler* network_state_handler) {  
 // again, use WeakPtr if lifetime should be checked
       g_burn_manager = new BurnManager(... network_state_handler);
    }

    in ChromeBrowserMainPartsChromeos
      imageburner::BurnManager::Initialize(
        ... NetworkHandler::Get()->network_state_handler());

In the future, we could then even store the NetworkHandler in
ChromeBrowserMainPartsChromeos and access the NetworkHandler member directly
instead of using NetworkHandler::Get().

I would try to pull the Network*Handler::Get() out as far as currently possible
without making too huge changes.
If you feel that that is currently too much effort, I'd propose to put the
::Get() calls to the calling site of each c'tor. I can then move these calls
towards ChromeBrowserMainPartsChromeos in a separate CL.

Please let me know what you think about it.

[stevenjb, 2013-05-23 16:18:14]

On Thu, May 23, 2013 at 2:26 AM, <pneubeck@chromium.org> wrote:

> On 2013/05/22 16:35:10, stevenjb (chromium) wrote:
>
>> So, that's a very lengthy comment.
>>
> I just wanted to prevent to appear arbitrary or cranky.
> It also feels fundamental and important enough to me that it's worth
> spending a
> few minutes on the topic.
>
> I agree, but, I guess I am uneager to discuss the entire scope of
singletons vs. non-singletons in the context of a specific CL :) My
impression is that the jury is kind of still out, but fewer singletons is a
good thing...

>
>  Rather than pick at symantic differences, do
>> you have an alternative proposal here, or sepcific changes you would like?
>>
>
> - c'tors of the handlers could have the pointers to the other handlers
> that are
> required, e.g.
>    NetworkConnectionHandler(**NetworkStateHandler* network_state_handler,
>                             NetworkConfigurationHandler*
> network_configuration_handler)**;
>
> So, I did address that within the Network*Handler classes. The Handlers
should no longer be accessing each other using global getters. I just set
the pointers in Init() methonds instead of the constructor so that we can
have co-dependent classes.


> - users of particular handlers could obtain a pointer to that handler in
> their
> constructor (or some init function),
> e.g.
>     NetworPortalDetectorImpl(**NetworkStateHandler* network_state_handler)
>
>     NetworkPortalDetector::**CreateInstance() {
>       ...
> NetworPortalDetectorImpl(**NetworkHandler::Get()->**network_state_handler());
>    //
> TODO: the StateHandler should be provided by ChromeBrowserMainPartsChromeos
>     }
>
>   or if lifetime should be checked better:
>     NetworPortalDetectorImpl(**WeakPtr<NetworkStateHandler>
> network_state_handler)
>
>     with NetworkStateHandler : public SupportsWeakPtr<**
> NetworkStateHandler>
>
>   In any case, this class should be changed away from lazy initialization
> to a
> singleton with separate Initialize and Get functions.
>   Then the StateHandler could be provided by ChromeBrowserMainPartsChromeos
> **.
>
> On this point I'm going to disagree. We have *dozens* of classes that
depend on NetworkHandler. Passing global instances down to the constructor
of every such class (or even to something easily accessible to every such
class) would be tedious at best, and difficult to maintain imho. Some cases
are less complex, like NetworkPortalDetector, but classes like TrayNetwork
would be much more work to pass the dependent classes all the way down.

While I am familiar with the arguments generally in favor of that approach,
I am unconvinced the benefits outweigh the cost. That said, I think we can
agree that combining the Network*Handler classes into one singleton
instance instead of many is a step forward, and I would like to avoid
piling too much change into one CL.


> Other example:
>     BurnManager::Initialize(..., NetworkStateHandler*
> network_state_handler) {
>  // again, use WeakPtr if lifetime should be checked
>        g_burn_manager = new BurnManager(... network_state_handler);
>     }
>
>     in ChromeBrowserMainPartsChromeos
>       imageburner::BurnManager::**Initialize(
>         ... NetworkHandler::Get()->**network_state_handler());
>
> In the future, we could then even store the NetworkHandler in
> ChromeBrowserMainPartsChromeos and access the NetworkHandler member
> directly
> instead of using NetworkHandler::Get().
>
> I would try to pull the Network*Handler::Get() out as far as currently
> possible
> without making too huge changes.
> If you feel that that is currently too much effort, I'd propose to put the
> ::Get() calls to the calling site of each c'tor. I can then move these
> calls
> towards ChromeBrowserMainPartsChromeos in a separate CL.
>
> Sorry, I don't quite follow your suggestion. I believe that I think that
what you propose should be done separately, but I'm not sure. Could you
give a specific example of such a change, based on this CL?

[pneubeck (no reviews), 2013-05-24 12:08:35]

I think I brought up the most important arguments from my side.

From the functional side, I can give my lgtm. The CL should be safe to commit
and not do anything harmful.

From the structural side, the current patch is acceptable. But IMO there are
better solutions.

On 2013/05/23 16:18:14, stevenjb (chromium) wrote:
> I agree, but, I guess I am uneager to discuss the entire scope of
> singletons vs. non-singletons in the context of a specific CL :) My
> impression is that the jury is kind of still out, but fewer singletons is a
> good thing...

You might consider asking another dev who is more knowledgeable about large
scale structuring than I am. No offense intended and none taken by me either. I
also found an at least related discussion (about Dependency Injection) on the
forum:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/depend...

[stevenjb, 2013-05-24 16:19:27]

On Fri, May 24, 2013 at 6:08 AM, <pneubeck@chromium.org> wrote:

> I think I brought up the most important arguments from my side.
>
> From the functional side, I can give my lgtm. The CL should be safe to
> commit
> and not do anything harmful.
>
> OK, I will go ahead and move forward with this then, because I believe
this to be at least slightly better than not harmful :)


> From the structural side, the current patch is acceptable. But IMO there
> are
> better solutions.
>
> So, honestly, within the context of this CL, I am am curious what you
would prefer to see as an incremental step in the right direction. A
concrete example would be great, because I genuinely am uncertain what you
would propose.


> On 2013/05/23 16:18:14, stevenjb (chromium) wrote:
>
>> I agree, but, I guess I am uneager to discuss the entire scope of
>> singletons vs. non-singletons in the context of a specific CL :) My
>> impression is that the jury is kind of still out, but fewer singletons is
>> a
>> good thing...
>>
>
> You might consider asking another dev who is more knowledgeable about large
> scale structuring than I am. No offense intended and none taken by me
> either. I
> also found an at least related discussion (about Dependency Injection) on
> the
> forum:
> https://groups.google.com/a/**chromium.org/forum/#!searchin/**
> chromium-dev/dependency$**20injection/chromium-dev/**
>
5lq8X7p82Ew/x1NHd7X4QOwJ<https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/dependency$20injection/chromium-dev/5lq8X7p82Ew/x1NHd7X4QOwJ>
>
> That's a great discussion and I think it brings up two excellent points:
1) it's a complicated problem without a single obvious solution.
2) Startup and shutdown are the real tricky bits (and the Networking is
right in the middle of that problem).

If someone were to take the time to introduce a 'ServiceLocator' system as
Gregg proposed, which included a mechanism for describing startup/shutdown
dependencies, I would jump on it. We started down this path at a previous
company but it turns out to be a pretty difficult problem to solve
correctly.

Also, at some point, when the network refactoring has stabilized, I would
be much more interested in investigating whether we could find a single
owner for NetworkHandler. ash::Shell might be a good candidate, however
right now too much of the code that depends on NetworkHandler doesn't know
or care about ash::Shell, so introducing that dependency seems more of a
burden than the gain of eliminating a singleton. But at some point I hope
that will not be true.

[gauravsh, 2013-05-24 17:20:24]

lgtm

https://codereview.chromium.org/14729017/diff/58001/chromeos/network/network_...
File chromeos/network/network_handler.h (right):

https://codereview.chromium.org/14729017/diff/58001/chromeos/network/network_...
chromeos/network/network_handler.h:23: // This clas should NOT be used in unit
tests. Instead, construct individual
class

[commit-bot: I haz the power, 2013-05-24 17:31:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14729017/81001

[commit-bot: I haz the power, 2013-05-24 17:40:18]

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

[commit-bot: I haz the power, 2013-05-24 17:53:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14729017/81001

[commit-bot: I haz the power, 2013-05-24 18:13:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-28 16:19:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14729017/118001

[commit-bot: I haz the power, 2013-05-28 18:33:34]

Change committed as 202609