Massively simplify the NetworkChangeNotifier infrastructure:...

Massively simplify the NetworkChangeNotifier infrastructure:
  * Use a process-wide object (singleton pattern)
  * Create/destroy this object on the main thread, make it outlive all consumers
  * Make observer-related functions threadsafe

As a result, the notifier can now be used by any thread (eliminating things like NetworkChangeObserverProxy and NetworkChangeNotifierProxy, and expanding its usefulness); its creation and inner workings are much simplified (eliminating implementation-specific classes); and it is simpler to access (eliminating things like NetworkChangeNotifierThread and a LOT of passing pointers around).

BUG=none
TEST=Unittests; network changes still trigger notifications

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

[Peter Kasting, 2010-06-22 22:53:05]

willchan/eroman: Added both of you as reviewers since I want to make sure I
don't overlook something subtle.  Will is the obvious reviewer, Eric was Darin's
suggestion.

akalin/sanjeevr: CCed since, respectively, this removes the
NetworkChangeXXXProxy classes and touches the Service process.

The change description and my comments in network_change_notifier.h should be
fairly thorough.  The base for the new NetworkChangeNotifier class design was
the SystemMonitor class, which is another process-global object that monitors
for events and sends them to a threadsafe set of observers.  One could argue
that the NetworkChangeNotifier should be merged into the SystemMonitor; if you
feel strongly that way speak up, as I could take a look at it.

The two key changes here are the lifetime change of the NetworkChangeNotifier
(which is now manually managed in browser_main.cc; please feel free to comment
if there's a better way to do this) and the way that network changes now cause
async callbacks even on the IO thread (which I think should only affect unit
tests).  There are a lot of plumbing changes, but they're all pretty mechanical
-- once you can get the global object from anywhere on any thread, you don't
need to hand-plumb it anymore.

My main development system is a Windows desktop, so it's not trivial for me to
actually trigger real network changes inside a debugger.  If one of you can
sanity-check that that still works, I'd feel better.

[ncarter (slow), 2010-06-23 00:29:15]

Drive by: this is awesome.

http://codereview.chromium.org/2802015/diff/50001/51051
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51051#newcode39
chrome/service/service_process.cc:39: // The NetworkChanegNotifier must be
destroyed after the IO thread has been
"Chaneg"

http://codereview.chromium.org/2802015/diff/50001/51077
File net/net.gyp (right):

http://codereview.chromium.org/2802015/diff/50001/51077#newcode204
net/net.gyp:204: '-lIphlpapi.lib',
Do we prefer this over #pragma comment(lib, X) in the _win.cc file?

[Peter Kasting, 2010-06-23 00:33:13]

Edit: Added Darin as a reviewer to catch his attention.  Darin, could you
comment on the #pragma comment question below?

http://codereview.chromium.org/2802015/diff/50001/51051
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51051#newcode39
chrome/service/service_process.cc:39: // The NetworkChanegNotifier must be
destroyed after the IO thread has been
On 2010/06/23 00:29:16, ncarter wrote:
> "Chaneg"

Fixed, thanks.

http://codereview.chromium.org/2802015/diff/50001/51077
File net/net.gyp (right):

http://codereview.chromium.org/2802015/diff/50001/51077#newcode204
net/net.gyp:204: '-lIphlpapi.lib',
On 2010/06/23 00:29:16, ncarter wrote:
> Do we prefer this over #pragma comment(lib, X) in the _win.cc file?

Dunno, I was just trying to make things actually link (I'm not sure how it
worked before for targets like hresolv).  I think Darin had an opinion on this.

[darin (slow to review), 2010-06-23 00:55:46]

On Tue, Jun 22, 2010 at 5:29 PM, <nick@chromium.org> wrote:

> Drive by: this is awesome.
>
>
> http://codereview.chromium.org/2802015/diff/50001/51051
> File chrome/service/service_process.cc (right):
>
> http://codereview.chromium.org/2802015/diff/50001/51051#newcode39
> chrome/service/service_process.cc:39: // The NetworkChanegNotifier must
> be destroyed after the IO thread has been
> "Chaneg"
>
> http://codereview.chromium.org/2802015/diff/50001/51077
> File net/net.gyp (right):
>
> http://codereview.chromium.org/2802015/diff/50001/51077#newcode204
> net/net.gyp:204: '-lIphlpapi.lib',
> Do we prefer this over #pragma comment(lib, X) in the _win.cc file?


I have always preferred #pragma comment(lib, X) since it helps you
automatically
keep your library dependencies in sync with the code.  If the library
dependencies
are in the project files, then it is common for them to stick around long
after the
code that requires them has been deleted.  You end up loading libraries that
you
never really need.  This is more of a problem on Windows than on Linux.  I'm
not
sure about Mac.

Unless things have changed, I think we generally use #pragma comment(lib,
X).

-Darin



>
>
> http://codereview.chromium.org/2802015/show
>

[akalin, 2010-06-23 18:46:34]

http://codereview.chromium.org/2802015/diff/50001/51009
File chrome/browser/sync/engine/syncapi.cc (left):

http://codereview.chromium.org/2802015/diff/50001/51009#oldcode1290
chrome/browser/sync/engine/syncapi.cc:1290: talk_mediator_.reset(new
TalkMediatorImpl(
You'll need to sync and fix the argument passing here

http://codereview.chromium.org/2802015/diff/50001/51041
File chrome/common/net/notifier/listener/mediator_thread_impl.h (right):

http://codereview.chromium.org/2802015/diff/50001/51041#newcode62
chrome/common/net/notifier/listener/mediator_thread_impl.h:62: explicit
MediatorThreadImpl();
-explicit

http://codereview.chromium.org/2802015/diff/50001/51063
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51063#newcode52
net/base/network_change_notifier_linux.cc:52: DCHECK(MessageLoop::current() ==
io_loop_);
DCHECK_EQ (and everywhere else)

http://codereview.chromium.org/2802015/diff/50001/51064
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/50001/51064#newcode17
net/base/network_change_notifier_linux.h:17: public
base::RefCountedThreadSafe<NetworkChangeNotifierLinux> {
It doesn't make sense for this class to be ref-counted, IMO.  Is this only so
that you can use NewRunnableMethod?  Given that this class lives for the
lifetime of the program, I think it's safe to use
DISABLE_RUNNABLE_METHOD_REFCOUNT from base/task.h

[Peter Kasting, 2010-06-23 18:49:33]

On 2010/06/23 00:55:46, darin wrote:
> Unless things have changed, I think we generally use #pragma comment(lib,
> X).

OK, I've changed to that.  Thanks!

[Peter Kasting, 2010-06-23 20:57:19]

New snap up.

http://codereview.chromium.org/2802015/diff/50001/51009
File chrome/browser/sync/engine/syncapi.cc (left):

http://codereview.chromium.org/2802015/diff/50001/51009#oldcode1290
chrome/browser/sync/engine/syncapi.cc:1290: talk_mediator_.reset(new
TalkMediatorImpl(
On 2010/06/23 18:46:34, akalin wrote:
> You'll need to sync and fix the argument passing here

Done.

http://codereview.chromium.org/2802015/diff/50001/51041
File chrome/common/net/notifier/listener/mediator_thread_impl.h (right):

http://codereview.chromium.org/2802015/diff/50001/51041#newcode62
chrome/common/net/notifier/listener/mediator_thread_impl.h:62: explicit
MediatorThreadImpl();
On 2010/06/23 18:46:34, akalin wrote:
> -explicit

Thanks, fixed.

http://codereview.chromium.org/2802015/diff/50001/51063
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51063#newcode52
net/base/network_change_notifier_linux.cc:52: DCHECK(MessageLoop::current() ==
io_loop_);
On 2010/06/23 18:46:34, akalin wrote:
> DCHECK_EQ (and everywhere else)

Fixed.

http://codereview.chromium.org/2802015/diff/50001/51064
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/50001/51064#newcode17
net/base/network_change_notifier_linux.h:17: public
base::RefCountedThreadSafe<NetworkChangeNotifierLinux> {
On 2010/06/23 18:46:34, akalin wrote:
> It doesn't make sense for this class to be ref-counted, IMO.  Is this only so
> that you can use NewRunnableMethod?  Given that this class lives for the
> lifetime of the program, I think it's safe to use
> DISABLE_RUNNABLE_METHOD_REFCOUNT from base/task.h

Didn't know about that.  Fixed.

It turns out this also fixed a problem on Linux where this class would be
deleted erroneously very soon after creation because the creator didn't ref the
new object, and thus when any posted task released its ref, the class would be
destroyed.

[eroman, 2010-06-23 21:20:18]

This is a big change, I haven't reviewed all of it yet.

Yikes, there was a fantastic amount of code to support the sync case! Overall I
like this, and think it makes sense for the networkchangenotifier to be
threadsafe.

I do have some concerns on the initialization/destruction, see inline comments.

http://codereview.chromium.org/2802015/diff/50001/51001
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51001#newcode1443
chrome/browser/browser_main.cc:1443: network_change_notifier.reset();
This isn't right -- the network change notifier should outlive the IO thread.
(IO thread isn't stopped until browser_shutdown::Shutdown()).

http://codereview.chromium.org/2802015/diff/50001/51002
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51002#newcode130
chrome/browser/io_thread.cc:130: if (net::NetworkChangeNotifier::get()) {
Rather than having all these NULL checks, it would be better to make the
NetworkChangeNotifier a required dependency.
Unit-tests can either specify a pass-through implementation, or use a real one.

(The meaning of NULL in the argument was more useful than the meaning of NULL on
the singleton, I don't think there is any reason to support NULL on the
singleton now)

http://codereview.chromium.org/2802015/diff/50001/51002#newcode131
chrome/browser/io_thread.cc:131:
net::NetworkChangeNotifier::get()->InitIOThreadFunctionality(
There introduce a subtle race:

Although NetworkChangeNotifier is threadsafe, it would be an error to call it
from another thread before the IO thread has been started.

At the very least this should be documented. Better assert it. Even better
handle it. (a possible solution might involve using a MessageLoopProxy for
ChromeThread::IO earlier during initialization, or forcing the IO thread's
creation to happen early).

http://codereview.chromium.org/2802015/diff/50001/51051
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51051#newcode39
chrome/service/service_process.cc:39: // The NetworkChanegNotifier must be
destroyed after the IO thread has been
Well really, "it should be destroyed after any thread that might use it has
stopped". Since if there are consumers on other threads they might end up
accessing freed memory.

(Your code looks OK, I am just commenting on the comment).

http://codereview.chromium.org/2802015/diff/50001/51056
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/2802015/diff/50001/51056#newcode76
net/base/host_resolver_impl.h:76: // |notifier| must outlive HostResolverImpl. 
It can optionally be NULL, in
Delete this.

http://codereview.chromium.org/2802015/diff/50001/51057
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51057#newcode1061
net/base/host_resolver_impl_unittest.cc:1061:
NetworkChangeNotifier::get()->NotifyObserversOfIPAddressChange();
I am not a fan of having NotifyObserversOfIPAddressChange() be public. I will
have to think about this some more.

http://codereview.chromium.org/2802015/diff/50001/51061
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51061#newcode55
net/base/network_change_notifier.cc:55: // We cannot check that the current
message loop is a UI loop, because in some
I don't think this comment belongs here in net/.

http://codereview.chromium.org/2802015/diff/50001/51061#newcode59
net/base/network_change_notifier.cc:59: // See comments in Get() about not
locking.
Get() --> get()

http://codereview.chromium.org/2802015/diff/50001/51062
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/50001/51062#newcode25
net/base/network_change_notifier.h:25: virtual ~Observer() { }
No spaces here.

See:

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

http://codereview.chromium.org/2802015/diff/50001/51062#newcode32
net/base/network_change_notifier.h:32: Observer() { }
ditto.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode40
net/base/network_change_notifier.h:40: // Get(), and it must outlive any other
thread that might use it, particularly
Get() --> get()

http://codereview.chromium.org/2802015/diff/50001/51062#newcode41
net/base/network_change_notifier.h:41: // the IO thread.  After the IO thread is
created, you must call
It is a bit of a layering issue to be talking about "The IO thread" from the
chrome sense. The 'net/' module is supposed to be a separable library, which
doesn't have the concept of the main UI thread vs network thread.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
I don't much like how the base class sets g_network_change_notifier.

My preference is towards explicit assignments like:

NetworkChangeNotifier* notifier = NetworkChangeNotifier::Create();

NetworkChangeNotifier::set(notifier);


and similarly:

NetworkChangeNotifier* mock_notifier = new MockNetworkChangeNotifier();

NetworkChangeNotifier::set(mock_notifier);

http://codereview.chromium.org/2802015/diff/50001/51062#newcode48
net/base/network_change_notifier.h:48: static NetworkChangeNotifier*
CreateMock();
See comment above. I prefer use of dependency injection pattern. The
MockNetworkChangeNotifier implementation can be a test-only file, which
implements NetworkChangeNotifier.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode50
net/base/network_change_notifier.h:50: // Gets the process-wide
NetworkChangeNotifier (if not present, returns NULL).
I think the contract should be to always return a non-NULL object here, unless
initialization has not taken place yet. There isn't much advantage in having all
the consumers checking for NULL all the time.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode60
net/base/network_change_notifier.h:60: virtual ~NetworkChangeNotifier();
Move this declaration higher up.

See:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/2802015/diff/50001/51062#newcode63
net/base/network_change_notifier.h:63: // called after the IO thread is
initialized, on the same thread where the
Same comment regarding referring to "the io thread".

http://codereview.chromium.org/2802015/diff/50001/51062#newcode64
net/base/network_change_notifier.h:64: // object was created.  Don't call this
when using the mock object.
This comment isn't being followed right now. Is that intentional?

In Chrome, you create on the main thread, and then call
InitIOThreadFunctionality() from the IO thread.

Whereas in ServiceProcess you create on main thread, and call
InitIOThreadFunctionality from main thread.

The Chrome case differs from your comment. (It seems like both should work
though, short of maybe dropping some super-early notifications that happen
during IOThread::Init).

http://codereview.chromium.org/2802015/diff/50001/51062#newcode65
net/base/network_change_notifier.h:65: virtual void
InitIOThreadFunctionality(MessageLoop* io_loop) {}
Can you add a NOTREACHED() in the base implementation, or make it =0 ?

(I would rather see pass-through implementations in the non-linux notifiers).

http://codereview.chromium.org/2802015/diff/50001/51062#newcode70
net/base/network_change_notifier.h:70: virtual void AddObserver(Observer*
observer);
This doesn't need to be virtual.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode77
net/base/network_change_notifier.h:77: virtual void RemoveObserver(Observer*
observer);
Ditto.

http://codereview.chromium.org/2802015/diff/50001/51063
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51063#newcode58
net/base/network_change_notifier_linux.cc:58: DCHECK(MessageLoop::current() ==
io_loop_);
DCHECK_EQ (throughout this file)

http://codereview.chromium.org/2802015/diff/50001/51063#newcode63
net/base/network_change_notifier_linux.cc:63: DCHECK(MessageLoop::current() ==
io_loop_);
DCHECK_EQ

http://codereview.chromium.org/2802015/diff/50001/51063#newcode69
net/base/network_change_notifier_linux.cc:69: DCHECK(MessageLoop::current() ==
io_loop_);
DCHECK_EQ

http://codereview.chromium.org/2802015/diff/50001/51064
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/50001/51064#newcode19
net/base/network_change_notifier_linux.h:19: NetworkChangeNotifierLinux();
indent by only 2 spaces.

http://codereview.chromium.org/2802015/diff/3003/35006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35006#newcode906
chrome/browser/browser_main.cc:906: scoped_ptr<net::NetworkChangeNotifier>
network_change_notifier(
This is a problem in that you have defined the Linux implementation as
refcounted. Either all impls need to be refcounted, or all non-refcounted (my
preference is to have them all non-refcounted).

http://codereview.chromium.org/2802015/diff/3003/35007
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35007#newcode132
chrome/browser/io_thread.cc:132:
net::NetworkChangeNotifier::get()->InitIOThreadFunctionality(
See my comments in network_change_notifier_*.

[eroman, 2010-06-24 00:12:32]

http://codereview.chromium.org/2802015/diff/50001/51002
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51002#newcode131
chrome/browser/io_thread.cc:131:
net::NetworkChangeNotifier::get()->InitIOThreadFunctionality(
On 2010/06/23 21:20:18, eroman wrote:
> There introduce a subtle race:
> 
> Although NetworkChangeNotifier is threadsafe, it would be an error to call it
> from another thread before the IO thread has been started.
> 
> At the very least this should be documented. Better assert it. Even better
> handle it. (a possible solution might involve using a MessageLoopProxy for
> ChromeThread::IO earlier during initialization, or forcing the IO thread's
> creation to happen early).

My comment here is incorrect, please disregard.

(forgot to remove this after reading a later file that addressed my concern).

[willchan no longer on Chromium, 2010-06-24 00:48:40]

Some preliminary comments.

http://codereview.chromium.org/2802015/diff/3003/35006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35006#newcode906
chrome/browser/browser_main.cc:906: scoped_ptr<net::NetworkChangeNotifier>
network_change_notifier(
On 2010/06/23 21:20:19, eroman wrote:
> This is a problem in that you have defined the Linux implementation as
> refcounted. Either all impls need to be refcounted, or all non-refcounted (my
> preference is to have them all non-refcounted).

+1

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (left):

http://codereview.chromium.org/2802015/diff/3003/35069#oldcode8
net/base/network_change_notifier.h:8: #include "base/basictypes.h"
Why did you remove this?  It's needed for DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode8
net/base/network_change_notifier.h:8: #include "base/gtest_prod_util.h"
Where is this used?

http://codereview.chromium.org/2802015/diff/3003/35069#newcode16
net/base/network_change_notifier.h:16: extern NetworkChangeNotifier*
g_network_change_notifier;
I dislike exposing a global pointer.  I'd prefer all access be via functions. 
Perhaps if you made this a file static in network_change_notifier.cc.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode25
net/base/network_change_notifier.h:25: virtual ~Observer() { }
Unless there's a rule somewhere that we should be doing this, I'd prefer you not
enforce your personal style preference here, since it's more common in the net/
code to do {}, which is in accordance with examples from the style guide
(although not explicitly stated as a rule).  I'd rather try to keep code
consistent.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
I'm very much against globals.  I know what it's not an uncommon pattern in
chrome, but since I've had to work hard to try to fix some use of globals in
chrome, I'd very much like to push back on this unless you provide a good
justification for the use of globals here.  Dependency injection becomes ugly
when you are relying on globals (you seem to be doing some things to modify the
global to replace it with a mock, which becomes unclean since that global
modification can persist across tests), and it becomes non-obvious from an
interface that something has there is a dependency between two objects.  Can you
explain what this gains you here, given that the plumbing already existed to
pass NetworkChangeNotifier to most of the appropriate locations?  I guess it
would need to be plumbed through to the sync code.

If the only reason to use the global is to avoid some plumbing, then I'd prefer
you do the plumbing.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode65
net/base/network_change_notifier.h:65: virtual void
InitIOThreadFunctionality(MessageLoop* io_loop) {}
If you're going to keep this, this should be a MessageLoopForIO pointer.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode88
net/base/network_change_notifier.h:88:
scoped_refptr<ObserverListThreadSafe<Observer> > observer_list_;
As this class seems to not have any reason to modify the scoped_refptr, it seems
like we should make it const.

http://codereview.chromium.org/2802015/diff/3003/35070
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35070#newcode39
net/base/network_change_notifier_linux.cc:39: io_loop->PostTask(FROM_HERE,
NewRunnableMethod(this,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
"If the arguments do not all fit on one line, they should be broken up onto
multiple lines, with each subsequent line aligned with the first argument. Do
not add spaces after the open paren or before the close paren:"

Your subsequent lines do not line up with the first argument.

http://codereview.chromium.org/2802015/diff/3003/35070#newcode44
net/base/network_change_notifier_linux.cc:44: void
NetworkChangeNotifierLinux::InitOnIOThread() {
I wonder how bad it would be if we started up a separate thread for this.
PROs:
* Cleans up the interface, no need to have this Linux implementation specific
API.
* Further insulates net/ from chrome/ threading assumptions

CONs:
* Requires another thread.  Bleh.

http://codereview.chromium.org/2802015/diff/3003/35070#newcode97
net/base/network_change_notifier_linux.cc:97: rv =
io_loop_->WatchFileDescriptor(netlink_fd_, false,
ditto on the style comment wrt aligning with first argument

http://codereview.chromium.org/2802015/diff/3003/35071
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/3003/35071#newcode57
net/base/network_change_notifier_linux.h:57: // This is safe because all posted
tasks are on the IO loop, and part of the
I'm not sure how much I like this.  This imposes chrome threading assumptions on
net/.  I think I'd rather use a refcounted Core object.

[Peter Kasting, 2010-06-24 01:42:57]

New snap up.

http://codereview.chromium.org/2802015/diff/50001/51001
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51001#newcode1443
chrome/browser/browser_main.cc:1443: network_change_notifier.reset();
On 2010/06/23 21:20:18, eroman wrote:
> This isn't right -- the network change notifier should outlive the IO thread.
> (IO thread isn't stopped until browser_shutdown::Shutdown()).

That would explain why Linux was telling me the IO loop hadn't been destroyed. 
I had thought the IO thread was already gone and was trying to make sure I could
still shut down the notifier cleanly; I've removed the reset() call here
entirely, and hopefully I can still shut down successfully that way.

http://codereview.chromium.org/2802015/diff/50001/51002
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51002#newcode130
chrome/browser/io_thread.cc:130: if (net::NetworkChangeNotifier::get()) {
On 2010/06/23 21:20:18, eroman wrote:
> Rather than having all these NULL checks, it would be better to make the
> NetworkChangeNotifier a required dependency.

As noted in network_change_notifier.h, I've changed to this.

http://codereview.chromium.org/2802015/diff/50001/51002#newcode131
chrome/browser/io_thread.cc:131:
net::NetworkChangeNotifier::get()->InitIOThreadFunctionality(
On 2010/06/23 21:20:18, eroman wrote:
> Although NetworkChangeNotifier is threadsafe, it would be an error to call it
> from another thread before the IO thread has been started.

Confirmed with you that this comment isn't true :)

http://codereview.chromium.org/2802015/diff/50001/51051
File chrome/service/service_process.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51051#newcode39
chrome/service/service_process.cc:39: // The NetworkChanegNotifier must be
destroyed after the IO thread has been
On 2010/06/23 21:20:18, eroman wrote:
> Well really, "it should be destroyed after any thread that might use it has
> stopped". Since if there are consumers on other threads they might end up
> accessing freed memory.
> 
> (Your code looks OK, I am just commenting on the comment).

Clarified the comment.

http://codereview.chromium.org/2802015/diff/50001/51056
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/2802015/diff/50001/51056#newcode76
net/base/host_resolver_impl.h:76: // |notifier| must outlive HostResolverImpl. 
It can optionally be NULL, in
On 2010/06/23 21:20:18, eroman wrote:
> Delete this.

Thanks, fixed.

http://codereview.chromium.org/2802015/diff/50001/51057
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51057#newcode1061
net/base/host_resolver_impl_unittest.cc:1061:
NetworkChangeNotifier::get()->NotifyObserversOfIPAddressChange();
On 2010/06/23 21:20:18, eroman wrote:
> I am not a fan of having NotifyObserversOfIPAddressChange() be public. I will
> have to think about this some more.

I've now made it public only for UNIT_TEST mode; it's protected otherwise.

http://codereview.chromium.org/2802015/diff/50001/51061
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51061#newcode55
net/base/network_change_notifier.cc:55: // We cannot check that the current
message loop is a UI loop, because in some
On 2010/06/23 21:20:18, eroman wrote:
> I don't think this comment belongs here in net/.

I tried to rewrite it a bit to be more generic.  I still think the concept it
conveys -- that you can't assume TYPE_UI -- is worth noting.

http://codereview.chromium.org/2802015/diff/50001/51061#newcode59
net/base/network_change_notifier.cc:59: // See comments in Get() about not
locking.
On 2010/06/23 21:20:18, eroman wrote:
> Get() --> get()

Thanks, fixed.

http://codereview.chromium.org/2802015/diff/50001/51062
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/50001/51062#newcode25
net/base/network_change_notifier.h:25: virtual ~Observer() { }
On 2010/06/23 21:20:18, eroman wrote:
> No spaces here.

Oops.  I thought I'd taken those back out (after I read the style guide :) ). 
Fixed.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode40
net/base/network_change_notifier.h:40: // Get(), and it must outlive any other
thread that might use it, particularly
On 2010/06/23 21:20:18, eroman wrote:
> Get() --> get()

Thanks, fixed.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode41
net/base/network_change_notifier.h:41: // the IO thread.  After the IO thread is
created, you must call
On 2010/06/23 21:20:18, eroman wrote:
> It is a bit of a layering issue to be talking about "The IO thread" from the
> chrome sense. The 'net/' module is supposed to be a separable library, which
> doesn't have the concept of the main UI thread vs network thread.

I tried to rewrite this comment to assume less about its consumers while still
being accurate and complete.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/23 21:20:18, eroman wrote:
> I don't much like how the base class sets g_network_change_notifier.
> 
> My preference is towards explicit assignments like:
> 
> NetworkChangeNotifier* notifier = NetworkChangeNotifier::Create();
> 
> NetworkChangeNotifier::set(notifier);

After talking with you, I think I'm going to keep things this way because it
makes life a little simpler and safer on consumers -- there's no way to, say,
delete your object but forget to unset the global.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode48
net/base/network_change_notifier.h:48: static NetworkChangeNotifier*
CreateMock();
On 2010/06/23 21:20:18, eroman wrote:
> The
> MockNetworkChangeNotifier implementation can be a test-only file, which
> implements NetworkChangeNotifier.

Since I don't have an abstract class here and the concrete base is a perfect
mock as-is, I'm going to leave this.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode50
net/base/network_change_notifier.h:50: // Gets the process-wide
NetworkChangeNotifier (if not present, returns NULL).
On 2010/06/23 21:20:18, eroman wrote:
> I think the contract should be to always return a non-NULL object here, unless
> initialization has not taken place yet.

Changed.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode60
net/base/network_change_notifier.h:60: virtual ~NetworkChangeNotifier();
On 2010/06/23 21:20:18, eroman wrote:
> Move this declaration higher up.

Huh, static methods below constructor/destructor.  What do you know.  Fixed.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode63
net/base/network_change_notifier.h:63: // called after the IO thread is
initialized, on the same thread where the
On 2010/06/23 21:20:18, eroman wrote:
> Same comment regarding referring to "the io thread".

Tried to generalize this too.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode64
net/base/network_change_notifier.h:64: // object was created.  Don't call this
when using the mock object.
On 2010/06/23 21:20:18, eroman wrote:
> This comment isn't being followed right now. Is that intentional?

I noticed this separately, and fixed the comment to be accurate (it's safe to
call on any thread).

http://codereview.chromium.org/2802015/diff/50001/51062#newcode65
net/base/network_change_notifier.h:65: virtual void
InitIOThreadFunctionality(MessageLoop* io_loop) {}
On 2010/06/23 21:20:18, eroman wrote:
> Can you add a NOTREACHED() in the base implementation, or make it =0 ?
> 
> (I would rather see pass-through implementations in the non-linux notifiers).

I'm not sure this buys much other than more code.  It's not really an API error
to call this on the mock object (clarified comments to say this), and since only
Linux really needs it it seems OK to provide a default implementation.

I would agree with you if this base class wasn't a complete, standalone mock
as-is.

http://codereview.chromium.org/2802015/diff/50001/51062#newcode70
net/base/network_change_notifier.h:70: virtual void AddObserver(Observer*
observer);
On 2010/06/23 21:20:18, eroman wrote:
> This doesn't need to be virtual.

Forgotten holdover from the old code.  Fixed.

http://codereview.chromium.org/2802015/diff/50001/51063
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/50001/51063#newcode58
net/base/network_change_notifier_linux.cc:58: DCHECK(MessageLoop::current() ==
io_loop_);
On 2010/06/23 21:20:18, eroman wrote:
> DCHECK_EQ (throughout this file)

Yep, fixed after akalin caught this.

http://codereview.chromium.org/2802015/diff/50001/51064
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/50001/51064#newcode19
net/base/network_change_notifier_linux.h:19: NetworkChangeNotifierLinux();
On 2010/06/23 21:20:18, eroman wrote:
> indent by only 2 spaces.

Thanks, fixed.

http://codereview.chromium.org/2802015/diff/3003/35006
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35006#newcode906
chrome/browser/browser_main.cc:906: scoped_ptr<net::NetworkChangeNotifier>
network_change_notifier(
On 2010/06/23 21:20:19, eroman wrote:
> This is a problem in that you have defined the Linux implementation as
> refcounted. Either all impls need to be refcounted, or all non-refcounted (my
> preference is to have them all non-refcounted).

Yep, fixed as noted in my reply to akalin on his comment in the header.  I noted
the precise issue you describe in the comment I left on the
DISABLE_RUNNABLE_METHOD_REFCOUNT line.

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (left):

http://codereview.chromium.org/2802015/diff/3003/35069#oldcode8
net/base/network_change_notifier.h:8: #include "base/basictypes.h"
On 2010/06/24 00:48:41, willchan wrote:
> Why did you remove this?  It's needed for DISALLOW_COPY_AND_ASSIGN.

It's transitively included by observer_list_threadsafe.h.  I've restored it.

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode8
net/base/network_change_notifier.h:8: #include "base/gtest_prod_util.h"
On 2010/06/24 00:48:41, willchan wrote:
> Where is this used?

It's not; thanks for catching this.  I needed this for an experiment I later
deleted.  Fixed.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode16
net/base/network_change_notifier.h:16: extern NetworkChangeNotifier*
g_network_change_notifier;
On 2010/06/24 00:48:41, willchan wrote:
> I dislike exposing a global pointer.  I'd prefer all access be via functions. 
> Perhaps if you made this a file static in network_change_notifier.cc.

I only exposed this here so I could make get() be unix-hacker style.  No one
should actually be accessing this directly.  To enforce that I've made it a
private class-static.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode25
net/base/network_change_notifier.h:25: virtual ~Observer() { }
On 2010/06/24 00:48:41, willchan wrote:
> Unless there's a rule somewhere that we should be doing this, I'd prefer you
not
> enforce your personal style preference here

Sorry, it wasn't personal preference -- I had misremembered the style guide, and
I've fixed this.  In fact I thought I'd fixed the places I did this before I'd
uploaded the first patch, but he noted that I'd missed this one.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/24 00:48:41, willchan wrote:
> I'm very much against globals.  I know what it's not an uncommon pattern in
> chrome, but since I've had to work hard to try to fix some use of globals in
> chrome, I'd very much like to push back on this unless you provide a good
> justification for the use of globals here.  Dependency injection becomes ugly
> when you are relying on globals (you seem to be doing some things to modify
the
> global to replace it with a mock, which becomes unclean since that global
> modification can persist across tests), and it becomes non-obvious from an
> interface that something has there is a dependency between two objects.  Can
you
> explain what this gains you here, given that the plumbing already existed to
> pass NetworkChangeNotifier to most of the appropriate locations?  I guess it
> would need to be plumbed through to the sync code.
> 
> If the only reason to use the global is to avoid some plumbing, then I'd
prefer
> you do the plumbing.

Several comments.  First, as noted above, I've made this pointer private to the
class, so it's not directly accessible like a true "global".  I hope that
appeases much of your anxiety.  (The code didn't access or intend to access the
global directly anyway, but this will prevent anyone from writing new code that
does.)

Second, I think you misunderstand how the mocking setup I've written works. 
There is no modification of a global (or anything else) to swap a mock object in
for a real one.  Rather, during process startup, you must either ask for a real
object or a mock object to be initialized.  You cannot later change this object.
 You can't corrupt it inside a test without crashing.

Third, the primary reason to use a singleton pattern was because it makes the
most sense, semantically (there ought to be one object in a process watching the
system for network changes and flagging that to everyone else), but since the
object is now threadsafe and such a singleton is thus easy to use, saving on
plumbing isn't trivial.  I've removed hundreds of lines of plumbing code, and
that is significant in terms of usability, maintenance, and readability.  I hope
your aversion is to true globals, not to the entire concept of singletons.

http://codereview.chromium.org/2802015/diff/3003/35069#newcode65
net/base/network_change_notifier.h:65: virtual void
InitIOThreadFunctionality(MessageLoop* io_loop) {}
On 2010/06/24 00:48:41, willchan wrote:
> If you're going to keep this, this should be a MessageLoopForIO pointer.

I don't know how I could make service_process.cc work with that -- the only was
I know to get a MessageLoopForIO* is to get it from MessageLoopForIO::current(),
and that's not where the service process calls this from.  Is there another
accessor I've missed?

http://codereview.chromium.org/2802015/diff/3003/35069#newcode88
net/base/network_change_notifier.h:88:
scoped_refptr<ObserverListThreadSafe<Observer> > observer_list_;
On 2010/06/24 00:48:41, willchan wrote:
> As this class seems to not have any reason to modify the scoped_refptr, it
seems
> like we should make it const.

Sure.  Done.

http://codereview.chromium.org/2802015/diff/3003/35070
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35070#newcode39
net/base/network_change_notifier_linux.cc:39: io_loop->PostTask(FROM_HERE,
NewRunnableMethod(this,
On 2010/06/24 00:48:41, willchan wrote:
> Your subsequent lines do not line up with the first argument.

I know it doesn't seem to match anything in there, but brettw, who's a
readability reviewer, has confirmed in the past that this wrapping style (which
is quite common across the codebase) is OK.  It is also nice in that it avoids
exploding the call into about 6 lines.

http://codereview.chromium.org/2802015/diff/3003/35070#newcode44
net/base/network_change_notifier_linux.cc:44: void
NetworkChangeNotifierLinux::InitOnIOThread() {
On 2010/06/24 00:48:41, willchan wrote:
> I wonder how bad it would be if we started up a separate thread for this.
> PROs:
> * Cleans up the interface, no need to have this Linux implementation specific
> API.

True, but that turns out to be two total calls in the entire codebase.

> * Further insulates net/ from chrome/ threading assumptions

Aas noted in my replies to some of eroman's comments, I've tried to clean up the
comments (and the functionality) here to assume nothing that is not in base/. 
There shouldn't be any chrome/ threading assumptions.

> CONs:
> * Requires another thread.  Bleh.

It's also somewhat more complex to understand.  See for example the old Mac
implementation, which started its own thread, and was somewhat tricky (although
there were other problems there too).

I'm not violently opposed to changing, but I don't think it's terribly
beneficial.  This seems OK.  If this class were not a singleton, and many
callers were expected to instantiate it, I'd probably change my mind.

http://codereview.chromium.org/2802015/diff/3003/35071
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/3003/35071#newcode57
net/base/network_change_notifier_linux.h:57: // This is safe because all posted
tasks are on the IO loop, and part of the
On 2010/06/24 00:48:41, willchan wrote:
> I'm not sure how much I like this.  This imposes chrome threading assumptions
on
> net/.  I think I'd rather use a refcounted Core object.

I have clarified the comments somewhat to make it clear that this doesn't rely
on chrome/ threading assumptions.

A refcounted Core object would buy no safety and no API cleanliness, only extra
complexity.  I am opposed to using it.

[Peter Kasting, 2010-06-24 01:44:34]

On Wed, Jun 23, 2010 at 6:42 PM, <pkasting@chromium.org> wrote:

> A refcounted Core object would buy no safety and no API cleanliness,
> only extra complexity.  I am opposed to using it.


Sorry, that came out snarkier than I intended -- I meant to write that in a
less combative way before hitting send (/me hangs head).


> http://codereview.chromium.org/2802015/show
>

[eroman, 2010-06-24 05:57:54]

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/24 01:42:57, Peter Kasting wrote:
> On 2010/06/24 00:48:41, willchan wrote:
> > I'm very much against globals.  I know what it's not an uncommon pattern in
> > chrome, but since I've had to work hard to try to fix some use of globals in
> > chrome, I'd very much like to push back on this unless you provide a good
> > justification for the use of globals here.  Dependency injection becomes
ugly
> > when you are relying on globals (you seem to be doing some things to modify
> the
> > global to replace it with a mock, which becomes unclean since that global
> > modification can persist across tests), and it becomes non-obvious from an
> > interface that something has there is a dependency between two objects.  Can
> you
> > explain what this gains you here, given that the plumbing already existed to
> > pass NetworkChangeNotifier to most of the appropriate locations?  I guess it
> > would need to be plumbed through to the sync code.
> > 
> > If the only reason to use the global is to avoid some plumbing, then I'd
> prefer
> > you do the plumbing.
> 
> Several comments.  First, as noted above, I've made this pointer private to
the
> class, so it's not directly accessible like a true "global".  I hope that
> appeases much of your anxiety.  (The code didn't access or intend to access
the
> global directly anyway, but this will prevent anyone from writing new code
that
> does.)
> 
> Second, I think you misunderstand how the mocking setup I've written works. 
> There is no modification of a global (or anything else) to swap a mock object
in
> for a real one.  Rather, during process startup, you must either ask for a
real
> object or a mock object to be initialized.  You cannot later change this
object.
>  You can't corrupt it inside a test without crashing.
> 
> Third, the primary reason to use a singleton pattern was because it makes the
> most sense, semantically (there ought to be one object in a process watching
the
> system for network changes and flagging that to everyone else), but since the
> object is now threadsafe and such a singleton is thus easy to use, saving on
> plumbing isn't trivial.  I've removed hundreds of lines of plumbing code, and
> that is significant in terms of usability, maintenance, and readability.  I
hope
> your aversion is to true globals, not to the entire concept of singletons.

I agree with Will; I prefer passing it as an explicit dependency.

(it helps me in understanding the interaction of the code at a glance, and
avoids some of the sticky setup/teardown for test cases).

I think the sweetness of this CL was in making NetworkChangeNotifier threadsafe,
and the singleton pattern was a secondary issue, which could have been left to a
follow-up CL (reducing the mammoth size of this one :)

http://codereview.chromium.org/2802015/diff/23003/69018
File chrome/browser/sync/notifier/server_notifier_thread.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69018#newcode21
chrome/browser/sync/notifier/server_notifier_thread.cc:21:
ServerNotifierThread::ServerNotifierThread() : notifier::MediatorThreadImpl() {}
Is the initializer for MediatorThreadImpl() necessary?

http://codereview.chromium.org/2802015/diff/23003/69057
File chrome/test/unit/chrome_test_suite.h (right):

http://codereview.chromium.org/2802015/diff/23003/69057#newcode98
chrome/test/unit/chrome_test_suite.h:98:
network_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock());
IIRC, when you run in-process-browser tests then both
ChromeTestSuite::Initialize AND BrowserMain() will run in the same process.

I expect what is going to happen then is that once we reach BrowserMain(), it
will try to create a global networkchangenotifier and trip the DCHECK, because
this line here has already assigned the global pointer.

http://codereview.chromium.org/2802015/diff/23003/69068
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69068#newcode24
net/base/network_change_notifier_linux.cc:24: :
main_loop_(MessageLoop::current()),
Two minor comments about this:

(1) Can the |main_loop_| assertion be moved into the base class?
(2) Can you use PlatformThread::CurrentId() instead of MessageLoop::current()?
(Would be more robust to any ordering changes as to when the main message loop
is created during BrowserMain)

http://codereview.chromium.org/2802015/diff/23003/69068#newcode34
net/base/network_change_notifier_linux.cc:34: DCHECK((io_loop_ == NULL) ||
(io_loop_ == main_loop_));
Oh I see, this conflicts with my comments above. Nevermind.

http://codereview.chromium.org/2802015/diff/23003/69070
File net/base/network_change_notifier_mac.cc (left):

http://codereview.chromium.org/2802015/diff/23003/69070#oldcode43
net/base/network_change_notifier_mac.cc:43: class NetworkChangeNotifierImpl
I don't really understand this mac code. I wander if moving the registration
from a helper thread to the UI thread is going to have any performance impact?

http://codereview.chromium.org/2802015/diff/23003/69070
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69070#newcode15
net/base/network_change_notifier_mac.cc:15:
NetworkChangeNotifierMac::NetworkChangeNotifierMac()
I suggest adding:

DCHECK_EQ(MessageLoop::TYPE_UI, MessageLoop::current()->type());

Since this is now requiring construction to happen on a thread with a CFRunLoop.

http://codereview.chromium.org/2802015/diff/23003/69071
File net/base/network_change_notifier_mac.h (right):

http://codereview.chromium.org/2802015/diff/23003/69071#newcode22
net/base/network_change_notifier_mac.h:22: static void
DynamicStoreCallback(SCDynamicStoreRef /* store */,
nit: please move this below the dtor.

(Destructors and constructors should be first).

http://codereview.chromium.org/2802015/diff/23003/69102
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69102#newcode18
net/spdy/spdy_session_pool.cc:18:
net::NetworkChangeNotifier::get()->AddObserver(this);
please remove the "net::" prefix.

http://codereview.chromium.org/2802015/diff/23003/69102#newcode24
net/spdy/spdy_session_pool.cc:24:
net::NetworkChangeNotifier::get()->RemoveObserver(this);
ditto.

http://codereview.chromium.org/2802015/diff/23003/69108
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69108#newcode33
webkit/tools/test_shell/test_shell_request_context.cc:33: void
TestShellRequestContext::Init(
Note that test_shell needs to initialize the global NetworkChangeNotifier
somewhere, lest its consumer get NULL.

(I didn't see where that happens in this CL)

[Peter Kasting, 2010-06-24 07:06:58]

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/24 05:57:54, eroman wrote:
> I agree with Will; I prefer passing it as an explicit dependency.

This changelist demonstrates how many places needed to know about
notifier-related objects which didn't actually depend on the notifier at all. 
They merely needed to plumb it to another class, sometimes half a dozen hops
away from the ultimate destination.  This kind of dependence isn't just more
verbose, it's more brittle, because it creates artificial dependencies that are
hard to refactor.

When a half-dozen total consumers of an object require ~500 lines of plumbing
code across >100 files, it seems clear to me that the explicit dependency model
is causing problems.  If all those classes semantically needed to know about
network changes, then there would still be some justification -- but most
didn't.

Finally, the existing model made it extraordinarily hard to add new consumers. 
I want to add two users of the notifier on different classes in the UI thread. 
Trying to get the existing notifier objects (or proxies) from somewhere and
plumb them through at object creation time is quite hard given that these are
themselves singleton sorts of classes that are initialized early.

> (it helps me in understanding the interaction of the code at a glance, and
> avoids some of the sticky setup/teardown for test cases).

I'm close to convinced that my previous "getter can be NULL" model is the right
way, primarily because it eliminated basically all the hairiness in setup and
teardown for testcases.  See comments elsewhere.

> I think the sweetness of this CL was in making NetworkChangeNotifier
threadsafe,
> and the singleton pattern was a secondary issue, which could have been left to
a
> follow-up CL (reducing the mammoth size of this one :)

I don't agree.  For me the "threadsafe" and "singleton" aspects really fit
together and it makes sense to change them as one.  If I did agree, though, this
would be an argument for splitting one change into two, not for eliminating the
singleton pattern entirely.

http://codereview.chromium.org/2802015/diff/23003/69018
File chrome/browser/sync/notifier/server_notifier_thread.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69018#newcode21
chrome/browser/sync/notifier/server_notifier_thread.cc:21:
ServerNotifierThread::ServerNotifierThread() : notifier::MediatorThreadImpl() {}
On 2010/06/24 05:57:54, eroman wrote:
> Is the initializer for MediatorThreadImpl() necessary?

No, that can be removed too.  Will fix.

http://codereview.chromium.org/2802015/diff/23003/69057
File chrome/test/unit/chrome_test_suite.h (right):

http://codereview.chromium.org/2802015/diff/23003/69057#newcode98
chrome/test/unit/chrome_test_suite.h:98:
network_change_notifier_.reset(net::NetworkChangeNotifier::CreateMock());
On 2010/06/24 05:57:54, eroman wrote:
> IIRC, when you run in-process-browser tests then both
> ChromeTestSuite::Initialize AND BrowserMain() will run in the same process.
> 
> I expect what is going to happen then is that once we reach BrowserMain(), it
> will try to create a global networkchangenotifier and trip the DCHECK, because
> this line here has already assigned the global pointer.

Argh.

This is the sort of reason I originally used the pattern that the getter could
return NULL.  Do you have any ideas on how to solve this?  I am inclined to
return to my previous pattern.

http://codereview.chromium.org/2802015/diff/23003/69068
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69068#newcode24
net/base/network_change_notifier_linux.cc:24: :
main_loop_(MessageLoop::current()),
On 2010/06/24 05:57:54, eroman wrote:
> Two minor comments about this:
> 
> (1) Can the |main_loop_| assertion be moved into the base class?

Hmm.  It can if I want to enforce that even a mock object needs a message loop. 
I am more inclined to say that's only needed for real objects.  Not sure.

> (2) Can you use PlatformThread::CurrentId() instead of MessageLoop::current()?
> (Would be more robust to any ordering changes as to when the main message loop
> is created during BrowserMain)

See below.

http://codereview.chromium.org/2802015/diff/23003/69070
File net/base/network_change_notifier_mac.cc (left):

http://codereview.chromium.org/2802015/diff/23003/69070#oldcode43
net/base/network_change_notifier_mac.cc:43: class NetworkChangeNotifierImpl
On 2010/06/24 05:57:54, eroman wrote:
> I don't really understand this mac code. I wander if moving the registration
> from a helper thread to the UI thread is going to have any performance impact?

In terms of startup cost or ongoing perf?

I wouldn't expect either to be impacted, the former since we delay
initialization until later, the latter since UI-threaded observers' processing
functions are the main cost for these notifications, and those take the same
time whichever route you go.

http://codereview.chromium.org/2802015/diff/23003/69070
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69070#newcode15
net/base/network_change_notifier_mac.cc:15:
NetworkChangeNotifierMac::NetworkChangeNotifierMac()
On 2010/06/24 05:57:54, eroman wrote:
> I suggest adding:
> 
> DCHECK_EQ(MessageLoop::TYPE_UI, MessageLoop::current()->type());
> 
> Since this is now requiring construction to happen on a thread with a
CFRunLoop.

Crap, THAT was why I originally tried to DCHECK this in the base class
constructor.

This means I'm going to have to make the contract more specific, and say that
for a non-mock object, you must create it on a thread of TYPE_UI.

http://codereview.chromium.org/2802015/diff/23003/69071
File net/base/network_change_notifier_mac.h (right):

http://codereview.chromium.org/2802015/diff/23003/69071#newcode22
net/base/network_change_notifier_mac.h:22: static void
DynamicStoreCallback(SCDynamicStoreRef /* store */,
On 2010/06/24 05:57:54, eroman wrote:
> nit: please move this below the dtor.
> 
> (Destructors and constructors should be first).

Will fix.

http://codereview.chromium.org/2802015/diff/23003/69102
File net/spdy/spdy_session_pool.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69102#newcode18
net/spdy/spdy_session_pool.cc:18:
net::NetworkChangeNotifier::get()->AddObserver(this);
On 2010/06/24 05:57:54, eroman wrote:
> please remove the "net::" prefix.

Will fix both places in this file.

http://codereview.chromium.org/2802015/diff/23003/69108
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/2802015/diff/23003/69108#newcode33
webkit/tools/test_shell/test_shell_request_context.cc:33: void
TestShellRequestContext::Init(
On 2010/06/24 05:57:54, eroman wrote:
> Note that test_shell needs to initialize the global NetworkChangeNotifier
> somewhere, lest its consumer get NULL.
> 
> (I didn't see where that happens in this CL)

This is another case I can avoid by retuning to the "getter may return NULL"
model.

[Peter Kasting, 2010-06-24 07:27:32]

Give me a bit before taking another review pass.

In addition to addressing eroman's comments, and returning to a "getter can
return NULL" interface (which I think makes a lot of things more sane), I'd like
to reconsider some of willchan's suggestions about implementation threads.  I
think if I make Mac and Linux create their own threads, I can make the public
API noticeably simpler (especially in terms of explaining it), and remove the
differences between mock and real object creation.

I'm also debating whether to also make the notifier be able to safely die before
all its users die -- it makes usage completely bulletproof, but perhaps that
should be a non-goal, as it could hide real bugs (where we think we're still
catching changes).  Thoughts?

[Peter Kasting, 2010-06-24 21:24:13]

[Due to a 500 error, this got sent by email but not copied onto the review
thread here.  Pasting so this is visible to anyone reading.  Email recipients
please ignore.]

OK, new snap up.  I am much happier with this, and I think you guys will be too.

* It simplifies the main header noticeably.  No more "InitOnIOThread()" crap. 
No more discussion of thread types.

* You can create the object on whatever thread you want.  The platform
implementations handle their own needs internally.  Note that because the main
observer list is threadsafe I can get away with using private threads without
needing separate inner classes or refcounting.

* The main API functions are all static and handle NULL-checking internally, so
consumers can continue to use the API without explicit NULL-checks, like in
eroman's proposed model; but...

* ...The internal guts once again support not creating a notifier if you don't
need one, so I can avoid doing crazy hacks to ensure that all possible test
programs initialized the notifier early-on, and use a creation model like my
original design.

Really the only remaining constraint is that if you're going to use a notifier,
then you'd better make its lifetime broader than its users', or you can get race
conditions in the accessor functions.  I don't think it's easy to solve this, as
you'd need something like a file-static Lock in network_change_notifier.cc, and
such a thing is forbidden by the style guide.

[eroman, 2010-06-24 22:21:43]

lgtm

http://codereview.chromium.org/2802015/diff/110/67069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/110/67069#newcode41
net/base/network_change_notifier.h:41: // outlive all other threads which might
try to use it.
I suggest mentioning that the caller is responsible for deleting this pointer.

http://codereview.chromium.org/2802015/diff/110/67069#newcode63
net/base/network_change_notifier.h:63: // nothing.  It's also safe to call after
the notifier object has been
This comment is misleading, as it is racey to call either AddObserver or
RemoveObserver after the global resolver impl has been deleted.

(g_network_change_notifier is not accessed from within a lock, so consumers
might do method call on deleted pointer).

http://codereview.chromium.org/2802015/diff/110/67069#newcode72
net/base/network_change_notifier.h:72: // we can't make it conditionally public
vs. protected (or linking fails), and
Regarding the linking issue, inlining should fix it:

 public:
#ifdef UNIT_TESTS
  static void NotifyObserversOfIPAddressChangeForTests() {
    NotifyObserversOfIPAddressChange();
  }
#endif

 protected:
  static void NotifyObserversOfIPAddressChange();

http://codereview.chromium.org/2802015/diff/110/67072
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/2802015/diff/110/67072#newcode50
net/base/network_change_notifier_mac.cc:50: void
NetworkChangeNotifierLinux::WillDestroyCurrentMessageLoop() {
Typo: This is mac impl.

http://codereview.chromium.org/2802015/diff/110/67072#newcode51
net/base/network_change_notifier_mac.cc:51:
CFRunLoopRemoveSource(CFRunLoopGetCurrent(), run_loop_source_.get(),
Is it safe to call CFRunLoopRemoveSource() on an un-initialized run_loop_source_
?

(In the ctor there is a 1 second delayed task posted for Init(); since delayed
tasks are not drained when stopping the thread, it is possible that Init() has
not run yet).

http://codereview.chromium.org/2802015/diff/110/67099
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/2802015/diff/110/67099#newcode684
net/socket/tcp_client_socket_pool_unittest.cc:684:
NetworkChangeNotifier::NotifyObserversOfIPAddressChange();
Have you given any more thought to adding a MockNetworkChangeNotifier
implementation?

Rather than having the scoped global mock live in net_test_suite.h, you could do
something like this:


TEST_F(....) {
  // While in this scope, we will have a mock network change notifier as the
current global
  MockNetworkChangeNotifier mock_notifier;

  ...

  mock_notifier.SynchronouslyNotifyObserversOfIPAddressChange();
}

[Peter Kasting, 2010-06-24 23:59:22]

http://codereview.chromium.org/2802015/diff/110/67069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/110/67069#newcode41
net/base/network_change_notifier.h:41: // outlive all other threads which might
try to use it.
On 2010/06/24 22:21:44, eroman wrote:
> I suggest mentioning that the caller is responsible for deleting this pointer.

Good idea.  Done.

http://codereview.chromium.org/2802015/diff/110/67069#newcode63
net/base/network_change_notifier.h:63: // nothing.  It's also safe to call after
the notifier object has been
On 2010/06/24 22:21:44, eroman wrote:
> This comment is misleading, as it is racey to call either AddObserver or
> RemoveObserver after the global resolver impl has been deleted.

That's exactly what I am trying to say both here and in the Create() comment.  I
tried to make things a little more blunt.

http://codereview.chromium.org/2802015/diff/110/67069#newcode72
net/base/network_change_notifier.h:72: // we can't make it conditionally public
vs. protected (or linking fails), and
On 2010/06/24 22:21:44, eroman wrote:
> Regarding the linking issue, inlining should fix it:
> 
>  public:
> #ifdef UNIT_TESTS
>   static void NotifyObserversOfIPAddressChangeForTests() {
>     NotifyObserversOfIPAddressChange();
>   }
> #endif
> 
>  protected:
>   static void NotifyObserversOfIPAddressChange();

Good idea.  Done.

http://codereview.chromium.org/2802015/diff/110/67072
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/2802015/diff/110/67072#newcode50
net/base/network_change_notifier_mac.cc:50: void
NetworkChangeNotifierLinux::WillDestroyCurrentMessageLoop() {
On 2010/06/24 22:21:44, eroman wrote:
> Typo: This is mac impl.

Yep, fixed after the trybot flagged it.

http://codereview.chromium.org/2802015/diff/110/67072#newcode51
net/base/network_change_notifier_mac.cc:51:
CFRunLoopRemoveSource(CFRunLoopGetCurrent(), run_loop_source_.get(),
On 2010/06/24 22:21:44, eroman wrote:
> Is it safe to call CFRunLoopRemoveSource() on an un-initialized
run_loop_source_
> ?

Actually there was a different bug here -- I forgot to call
AddDestructionObserver() in Init(), so this code wasn't being reached.

Now that I've fixed that, we can only reach here when the run loop source is
non-NULL.  I added a DCHECK for that.

http://codereview.chromium.org/2802015/diff/110/67099
File net/socket/tcp_client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/2802015/diff/110/67099#newcode684
net/socket/tcp_client_socket_pool_unittest.cc:684:
NetworkChangeNotifier::NotifyObserversOfIPAddressChange();
On 2010/06/24 22:21:44, eroman wrote:
> Have you given any more thought to adding a MockNetworkChangeNotifier
> implementation?

There are ups and downs to this.

On the plus side, this eliminates #ifdef UNIT_TEST from the base class
definition.  It also allows for a simple object declaration, which is arguably a
plus.  One thing it doesn't allow you to do is synchronously notify observers,
unless you completely replace the observer implementation in the base class,
which would somewhat defeat the purpose (as you'd no longer be testing anything
about the real notifier).

On the minus side, declaring a notifier within a test scope can cause subtle,
hairy issues if the test suite or test base class has initialized an object that
accesses the notifier APIs.  And if we avoid this by leaving the scoping the way
things are right now, then there's no real benefit from having a separate mock
class, because the notify call once again has to be accessible globally.

In the end it seems like the safest usage is the current usage, which has the
nice property that it's almost identical to the "real" usage in the browser --
identical object scopes, names, and calls, except for "CreateMock" in one place
instead of "Create".

[eroman, 2010-06-25 00:29:58]

lgtm

http://codereview.chromium.org/2802015/diff/19018/66075
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/19018/66075#newcode11
net/base/network_change_notifier.h:11: class MessageLoop;
Delete.

[Peter Kasting, 2010-06-25 01:04:33]

http://codereview.chromium.org/2802015/diff/19018/66075
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/19018/66075#newcode11
net/base/network_change_notifier.h:11: class MessageLoop;
On 2010/06/25 00:29:58, eroman wrote:
> Delete.

Thanks, fixed.

[willchan no longer on Chromium, 2010-06-25 15:36:22]

I'm sorry I took so long to get to this.  I haven't reviewed all the files, I
just looked at the "core" files.

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/24 07:06:59, Peter Kasting wrote:
> On 2010/06/24 05:57:54, eroman wrote:
> > I agree with Will; I prefer passing it as an explicit dependency.
> 
> This changelist demonstrates how many places needed to know about
> notifier-related objects which didn't actually depend on the notifier at all. 
> They merely needed to plumb it to another class, sometimes half a dozen hops
> away from the ultimate destination.  This kind of dependence isn't just more
> verbose, it's more brittle, because it creates artificial dependencies that
are
> hard to refactor.
> 
> When a half-dozen total consumers of an object require ~500 lines of plumbing
> code across >100 files, it seems clear to me that the explicit dependency
model
> is causing problems.  If all those classes semantically needed to know about
> network changes, then there would still be some justification -- but most
> didn't.

Your point about plumbing is well-taken.  I have a few counterpoints.
* Plumbing is always a problem.  Typically, when you have a half a dozen hops
that you need to plumb through, that shows that it might be time to package some
parameters together into a struct or something, so that the individual parameter
doesn't necessarily have to be plumbed all the way.  If NCN weren't a singleton,
then this plumbing would probably be a good indicator that it's time for a
refactoring to reduce the plumbing.
* The use of globals doesn't have to mean you don't have to pass it in as an
explicit dependency.  Even when I do use globals, I try to show that an object
has a dependency on something, by making whoever constructs the object access
the global and passing it into the new object, rather than the new object itself
accessing the global.  This shows the dependency in the right location and
allows better easier dependency injection.

Even if you don't buy my first point, I hope you will at least find my second
point convincing.

> 
> Finally, the existing model made it extraordinarily hard to add new consumers.

> I want to add two users of the notifier on different classes in the UI thread.

> Trying to get the existing notifier objects (or proxies) from somewhere and
> plumb them through at object creation time is quite hard given that these are
> themselves singleton sorts of classes that are initialized early.
> 
> > (it helps me in understanding the interaction of the code at a glance, and
> > avoids some of the sticky setup/teardown for test cases).
> 
> I'm close to convinced that my previous "getter can be NULL" model is the
right
> way, primarily because it eliminated basically all the hairiness in setup and
> teardown for testcases.  See comments elsewhere.
> 
> > I think the sweetness of this CL was in making NetworkChangeNotifier
> threadsafe,
> > and the singleton pattern was a secondary issue, which could have been left
to
> a
> > follow-up CL (reducing the mammoth size of this one :)
> 
> I don't agree.  For me the "threadsafe" and "singleton" aspects really fit
> together and it makes sense to change them as one.  If I did agree, though,
this
> would be an argument for splitting one change into two, not for eliminating
the
> singleton pattern entirely.

http://codereview.chromium.org/2802015/diff/3003/35070
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35070#newcode39
net/base/network_change_notifier_linux.cc:39: io_loop->PostTask(FROM_HERE,
NewRunnableMethod(this,
On 2010/06/24 01:42:57, Peter Kasting wrote:
> On 2010/06/24 00:48:41, willchan wrote:
> > Your subsequent lines do not line up with the first argument.
> 
> I know it doesn't seem to match anything in there, but brettw, who's a
> readability reviewer, has confirmed in the past that this wrapping style
(which
> is quite common across the codebase) is OK.  It is also nice in that it avoids
> exploding the call into about 6 lines.

Yes, I know it's common in the chrome code base to do this, and your argument is
quite valid, but it looks to me that the style guide disagrees with this
behavior.  I'm curious where Brett confirmed this is ok.  I assume he consulted
the Google style mailing list about this.  I'm going to double check, since I'm
a readability reviewer too, and if I saw this in a readability review, I would
ask for it to be changed.  Maybe you can ask Brett to clarify this specific
instance.  That said, there's no reason to block your change on this nit.

http://codereview.chromium.org/2802015/diff/19018/66077
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/19018/66077#newcode26
net/base/network_change_notifier_linux.h:26: //
MessageLoop::DestructionObserver:
There's no clear style consensus on this anywhere, so I won't say you _have_ to
do it, but I think it's best to keep these virtuals public, since they were
public in the base class.  Making them private isn't super effective, since it
can be called via a pointer to the base class anyway.  That said, if you feel
strongly about this (yeah yeah, I know the counterarguments), then it's fine as
is.

http://codereview.chromium.org/2802015/diff/19018/66077#newcode49
net/base/network_change_notifier_linux.h:49: scoped_ptr<base::Thread>
notifier_thread_;
You are deleting includes and neglecting to include the headers you actually
use.  I'm afraid this is quite common behavior in chrome, but it's against the
style guide.  Please refer to
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...
which explicitly says:
"Note: If you use a symbol Foo in your source file, you should bring in a
definition for Foo yourself, either via an #include or via a forward
declaration. Do not depend on the symbol being brought in transitively via
headers not directly included. One exception is if Foo is used in myfile.cc,
it's ok to #include (or forward-declare) Foo in myfile.h, instead of myfile.cc."

[Peter Kasting, 2010-06-25 17:51:32]

Added Brett as a reviewer to get his attention.  Brett, can you comment on the
style point below where I mention you?  Thanks.

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/25 15:36:22, willchan wrote:
> * Plumbing is always a problem.  Typically, when you have a half a dozen hops
> that you need to plumb through, that shows that it might be time to package
some
> parameters together into a struct or something, so that the individual
parameter
> doesn't necessarily have to be plumbed all the way.  If NCN weren't a
singleton,
> then this plumbing would probably be a good indicator that it's time for a
> refactoring to reduce the plumbing.

I'm not sure I understand.  How does combining args into a struct reduce the
dependencies?  Doesn't that just reduce the number of arguments each function
takes?  It still seems like the objects have to get from the same start point to
the same finish point.

> I try to show that an object
> has a dependency on something, by making whoever constructs the object access
> the global and passing it into the new object, rather than the new object
itself
> accessing the global.  This shows the dependency in the right location and
> allows better easier dependency injection.

I agree with you about this pattern allowing easier dependency injection in
general.

There are a couple downsides.  This means that all objects which can construct
the target class need to know about and access the singleton in question, which
adds dependencies that to me seem artificial (and certainly increase maintenance
costs, especially if the singleton's access pattern changes radically).  In fact
I even view this as the opposite of "shows the dependency in the right location"
because at least to me, this code looks like "the constructing class depends on
this singleton, because the target class is unable to depend on it directly",
which isn't actually true.

It also restricts you to a style where singletons must be designed so that it's
safe and correct to pass handles to them around.  In the case of the NCN, my
change yesterday to eliminate the getter and make the APIs static gave me a
simplicity and safety win over my previous design that was more along the above
lines.

To me the takeaway is that a variety of design patterns can be useful for
different situations, and each individual case should be taken separately.  It
would be a mistake, for example, to try and turn the Profile object into
something like what I've done with the NCN, where there's a singleton that all
consumers access directly (or even where the functions on Profile become
static).  In that case, we use a model much like what you describe above, and it
makes a lot more sense.  For the NCN, I really believe that my current design is
the best one for consumers as they are today.

http://codereview.chromium.org/2802015/diff/3003/35070
File net/base/network_change_notifier_linux.cc (right):

http://codereview.chromium.org/2802015/diff/3003/35070#newcode39
net/base/network_change_notifier_linux.cc:39: io_loop->PostTask(FROM_HERE,
NewRunnableMethod(this,
On 2010/06/25 15:36:22, willchan wrote:
> On 2010/06/24 01:42:57, Peter Kasting wrote:
> > On 2010/06/24 00:48:41, willchan wrote:
> > > Your subsequent lines do not line up with the first argument.
> > 
> > I know it doesn't seem to match anything in there, but brettw, who's a
> > readability reviewer, has confirmed in the past that this wrapping style
> (which
> > is quite common across the codebase) is OK.  It is also nice in that it
avoids
> > exploding the call into about 6 lines.
> 
> Yes, I know it's common in the chrome code base to do this, and your argument
is
> quite valid, but it looks to me that the style guide disagrees with this
> behavior.  I'm curious where Brett confirmed this is ok.  I assume he
consulted
> the Google style mailing list about this.  I'm going to double check, since
I'm
> a readability reviewer too, and if I saw this in a readability review, I would
> ask for it to be changed.  Maybe you can ask Brett to clarify this specific
> instance.  That said, there's no reason to block your change on this nit.

Brett, can you take a glance at this line's style and verify whether it's OK or
not?  Maybe I'm misremembering.

http://codereview.chromium.org/2802015/diff/19018/66077
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/19018/66077#newcode26
net/base/network_change_notifier_linux.h:26: //
MessageLoop::DestructionObserver:
On 2010/06/25 15:36:22, willchan wrote:
> I think it's best to keep these virtuals public, since they were
> public in the base class.  Making them private isn't super effective, since it
> can be called via a pointer to the base class anyway.  That said, if you feel
> strongly about this (yeah yeah, I know the counterarguments), then it's fine
as
> is.

I don't feel violently.  It seems like a slight plus to enforce that people only
call interface methods through interface pointers, to prevent misuse of the
class.  Is there a benefit we get from the other way?  Less confusion when
reading the code, maybe?

http://codereview.chromium.org/2802015/diff/19018/66077#newcode49
net/base/network_change_notifier_linux.h:49: scoped_ptr<base::Thread>
notifier_thread_;
On 2010/06/25 15:36:22, willchan wrote:
> You are deleting includes and neglecting to include the headers you actually
> use.

Wait, what did I delete here?  base::Thread is declared above.  Do you mean
scoped_ptr.h?  I can certainly include that.  I didn't delete it, though.  It
was never in the include list.  I just didn't notice I needed it because
everything compiled.

I added scoped_ptr.h to both this and the Mac header.

[Peter Kasting, 2010-06-25 18:36:05]

On Fri, Jun 25, 2010 at 11:31 AM, <brettw@chromium.org> wrote:

>
> http://codereview.chromium.org/2802015/diff/3003/35070
> File net/base/network_change_notifier_linux.cc (right):
>
> http://codereview.chromium.org/2802015/diff/3003/35070#newcode39
> net/base/network_change_notifier_linux.cc:39:
> io_loop->PostTask(FROM_HERE, NewRunnableMethod(this,
> I don't recall any discussion on this particular style explicitly.
>
> Clearly if there was no nested function here you would align the wrapped
> arg to after the paren of the outer function.
>  void foo(bar, a,
>           b);
>
> It also seems pretty clear to me that if the wrapped line was very
> short, it should be aligned to the inner function:
>  void foo(bar, Call(a,
>                     b));
> In other words, my reading has been that "first argument" applies to the
> "first argument of the same function" and I've never heard anybody
> disagree.
>
> In this case, it doesn't fit with that rule either. The question is do
> you align inner function args with the first arg of the outer function?
> In some cases I think this may make sense. But I've generally read that
> style rule as applying to each function call in a nested call in
> isolation, so wrap to the first arg of that function, or 4 spaces if it
> doesn't fit. This makes sense to me because wrapping to the ( seems like
> you're saying something about the nesting level (which is incorrect),
> while wrapping to 4 spaces seems like the escape clause where you're not
> saying anything about the nesting level and the reader needs to figure
> it out.


If I understand you correctly, it sounds like you're saying "this style
seems OK", or perhaps even "I can't think of a better way to write this".

http://codereview.chromium.org/2802015/show
>

[willchan no longer on Chromium, 2010-06-25 19:09:28]

LGTM

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/25 17:51:33, Peter Kasting wrote:
> On 2010/06/25 15:36:22, willchan wrote:
> > * Plumbing is always a problem.  Typically, when you have a half a dozen
hops
> > that you need to plumb through, that shows that it might be time to package
> some
> > parameters together into a struct or something, so that the individual
> parameter
> > doesn't necessarily have to be plumbed all the way.  If NCN weren't a
> singleton,
> > then this plumbing would probably be a good indicator that it's time for a
> > refactoring to reduce the plumbing.
> 
> I'm not sure I understand.  How does combining args into a struct reduce the
> dependencies?  Doesn't that just reduce the number of arguments each function
> takes?  It still seems like the objects have to get from the same start point
to
> the same finish point.

Correct.  Combining args into a struct reduces the brittleness though of passing
dependencies and having to update them in a gazillion places, since once you're
passing through the struct, you can add a field and update in one place, rather
than all the different layers where you had to previously.  This addresses part
of the problem with plumbing data through layers.

> 
> > I try to show that an object
> > has a dependency on something, by making whoever constructs the object
access
> > the global and passing it into the new object, rather than the new object
> itself
> > accessing the global.  This shows the dependency in the right location and
> > allows better easier dependency injection.
> 
> I agree with you about this pattern allowing easier dependency injection in
> general.
> 
> There are a couple downsides.  This means that all objects which can construct
> the target class need to know about and access the singleton in question,
which
> adds dependencies that to me seem artificial (and certainly increase
maintenance
> costs, especially if the singleton's access pattern changes radically).  In
fact
> I even view this as the opposite of "shows the dependency in the right
location"
> because at least to me, this code looks like "the constructing class depends
on
> this singleton, because the target class is unable to depend on it directly",
> which isn't actually true.

This is a fair argument, but unless I'm misunderstanding, it fundamentally
applies to the dependency injection pattern in general.

> 
> It also restricts you to a style where singletons must be designed so that
it's
> safe and correct to pass handles to them around.  In the case of the NCN, my
> change yesterday to eliminate the getter and make the APIs static gave me a
> simplicity and safety win over my previous design that was more along the
above
> lines.
> 
> To me the takeaway is that a variety of design patterns can be useful for
> different situations, and each individual case should be taken separately.  It
> would be a mistake, for example, to try and turn the Profile object into
> something like what I've done with the NCN, where there's a singleton that all
> consumers access directly (or even where the functions on Profile become
> static).  In that case, we use a model much like what you describe above, and
it
> makes a lot more sense.  For the NCN, I really believe that my current design
is
> the best one for consumers as they are today.

It's not what I would have done, but I am fine with this.

http://codereview.chromium.org/2802015/diff/19018/66077
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/19018/66077#newcode26
net/base/network_change_notifier_linux.h:26: //
MessageLoop::DestructionObserver:
On 2010/06/25 17:51:33, Peter Kasting wrote:
> On 2010/06/25 15:36:22, willchan wrote:
> > I think it's best to keep these virtuals public, since they were
> > public in the base class.  Making them private isn't super effective, since
it
> > can be called via a pointer to the base class anyway.  That said, if you
feel
> > strongly about this (yeah yeah, I know the counterarguments), then it's fine
> as
> > is.
> 
> I don't feel violently.  It seems like a slight plus to enforce that people
only
> call interface methods through interface pointers, to prevent misuse of the
> class.  Is there a benefit we get from the other way?  Less confusion when
> reading the code, maybe?

Yeah, it's the confusion, because it can be misleading.  It's non-obvious when
reading this header file, and then reading some client code that holds a pointer
to the base class, that this could be called.  Changing the access control in
derived classes makes it non-obvious.

Again, this is personal style.  I'm just advocating my personal preference.  I
leave the final decision to you.

http://codereview.chromium.org/2802015/diff/19018/66077#newcode49
net/base/network_change_notifier_linux.h:49: scoped_ptr<base::Thread>
notifier_thread_;
On 2010/06/25 17:51:33, Peter Kasting wrote:
> On 2010/06/25 15:36:22, willchan wrote:
> > You are deleting includes and neglecting to include the headers you actually
> > use.
> 
> Wait, what did I delete here?  base::Thread is declared above.  Do you mean
> scoped_ptr.h?  I can certainly include that.  I didn't delete it, though.  It
> was never in the include list.  I just didn't notice I needed it because
> everything compiled.
> 
> I added scoped_ptr.h to both this and the Mac header.

You've deleted basictypes.h, which IIRC is required for DISALLOW_*

[willchan no longer on Chromium, 2010-06-25 19:21:50]

On 2010/06/25 18:36:05, Peter Kasting wrote:
> On Fri, Jun 25, 2010 at 11:31 AM, <mailto:brettw@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.org/2802015/diff/3003/35070
> > File net/base/network_change_notifier_linux.cc (right):
> >
> > http://codereview.chromium.org/2802015/diff/3003/35070#newcode39
> > net/base/network_change_notifier_linux.cc:39:
> > io_loop->PostTask(FROM_HERE, NewRunnableMethod(this,
> > I don't recall any discussion on this particular style explicitly.
> >
> > Clearly if there was no nested function here you would align the wrapped
> > arg to after the paren of the outer function.
> >  void foo(bar, a,
> >           b);
> >
> > It also seems pretty clear to me that if the wrapped line was very
> > short, it should be aligned to the inner function:
> >  void foo(bar, Call(a,
> >                     b));
> > In other words, my reading has been that "first argument" applies to the
> > "first argument of the same function" and I've never heard anybody
> > disagree.
> >
> > In this case, it doesn't fit with that rule either. The question is do
> > you align inner function args with the first arg of the outer function?
> > In some cases I think this may make sense. But I've generally read that
> > style rule as applying to each function call in a nested call in
> > isolation, so wrap to the first arg of that function, or 4 spaces if it
> > doesn't fit. This makes sense to me because wrapping to the ( seems like
> > you're saying something about the nesting level (which is incorrect),
> > while wrapping to 4 spaces seems like the escape clause where you're not
> > saying anything about the nesting level and the reader needs to figure
> > it out.
> 
> 
> If I understand you correctly, it sounds like you're saying "this style
> seems OK", or perhaps even "I can't think of a better way to write this".

FWIW, I would have written it as so:
    io_loop->PostTask(
        FROM_HERE,
	NewRunnableMethod(
            this, &NetworkChangeNotifierLinux::InitOnIOThread));

or

    io_loop->PostTask(
        FROM_HERE, NewRunnableMethod(
            this, &NetworkChangeNotifierLinux::InitOnIOThread));

> 
> http://codereview.chromium.org/2802015/show
> >
>

[Peter Kasting, 2010-06-25 20:01:37]

Thanks for the careful review.  I appreciate the thorough discussion of
dependency injection patterns, it helps me to understand what are the valid
alternative designs and their tradeoffs.

http://codereview.chromium.org/2802015/diff/3003/35069
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/2802015/diff/3003/35069#newcode43
net/base/network_change_notifier.h:43: static NetworkChangeNotifier* Create();
On 2010/06/25 19:09:28, willchan wrote:
> Combining args into a struct reduces the brittleness though of passing
> dependencies and having to update them in a gazillion places, since once
you're
> passing through the struct, you can add a field and update in one place,
rather
> than all the different layers where you had to previously.  This addresses
part
> of the problem with plumbing data through layers.

Ah, OK.  I had thought I misunderstood you.  You're right, this reduces
maintenance costs and reading burden (though it can cause its own problems later
if some of the involved classes change their required dependencies but others
don't).

I was more worried about layering, in the sense of whether it was appropriate
for class A to "know about" some type or object that class B needed.  There's a
bunch of code in the omnibox that ends up passing objects through several
classes, and it feels gross because it exposes the destination's dependency at
all points along the chain.  Sometimes that's hard to avoid, though.

> > There are a couple downsides.  This means that all objects which can
construct
> > the target class need to know about and access the singleton in question,
> which
> > adds dependencies that to me seem artificial (and certainly increase
> maintenance
> > costs, especially if the singleton's access pattern changes radically).
> 
> This is a fair argument, but unless I'm misunderstanding, it fundamentally
> applies to the dependency injection pattern in general.

Well... it applies when you inject the dependency via arguments at construction
time.  Technically, if the target class gets the service it needs via some
accessor, which test code can override to return a different service
implementation, you still have dependency injection, unless I'm mistaken.  The
injector has to override the accessor functionality as opposed to having to
construct its own instance of the target class.  I tend to prefer this sort of
pattern, but depending on how the target class is constructed, it might be much
harder to implement.

http://codereview.chromium.org/2802015/diff/19018/66077
File net/base/network_change_notifier_linux.h (right):

http://codereview.chromium.org/2802015/diff/19018/66077#newcode49
net/base/network_change_notifier_linux.h:49: scoped_ptr<base::Thread>
notifier_thread_;
On 2010/06/25 19:09:29, willchan wrote:
> You've deleted basictypes.h, which IIRC is required for DISALLOW_*

Oh, oops.  Fixed in all the network_change_notifier*.h files.