Add a unified observer to replace NetworkManagerObserver

Add a unified observer to replace NetworkManagerObserver

[reland of 187767 with fix for shutdown crash due to improper 
destruction order.]

The new observer implementation allows one to use either of NetworkLibrary
or NetworkStateHandler to be notified of manager events.

Also, switch over a few trivial NetworkManagerObserver consumers to
use this new observer.

TBR=sky@ for chrome/*.gypi changes

BUG=chromium:181250, chromium:167232
TEST=existing tests, tested on device


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

[gauravsh, 2013-03-11 22:51:15]

stevenjb: main review
xiyuan: chrome/browser/ui/webui/* OWNERS
tbarzic: chrome/browser/chromeos/extension/ OWNERS

[xiyuan, 2013-03-11 22:58:48]

chrome/browser/ui/webui/chromeos/* LGTM

[stevenjb, 2013-03-11 23:11:04]

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:176: NetworkLibrary* lib =
GetNetworkLibrary();
I'd feel more comfortable converting all of this at once; depending on both
NetworkStateHandler and NetowrkLibrary might have consistency issues so I would
like to limit the places we do this.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator_unittest.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator_unittest.cc:103:
MockNetworkLibrary network_library_;
Here too - I'd prefer to convert this all at once.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper.cc:118:
network_state_handler_->RemoveObserver(this);
nit: Use NetworkStateHandler::Get() here; if we accidentally destroy this after
NetworkStateHandler that will trigger a CHECK which will be easier to debug than
a random memory access.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper.h (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper.h:57:
ConnectivityStateHelperObserver* observer);
I wonder if this would be better as just "Add/RemoveObserver"? I think that
everywhere we use this the calling code is just observing "Connectivity" and
doesn't know or care about the concept of a "Manager"

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper_observer.h (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper_observer.h:16: virtual
void NetworkManagerChanged() = 0;
Call this ConnectivityChanged() instead? At some point we could also improve the
Helper to track the connected network and state, only triggering the callback
when connectivity actually changes.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_dropdown.cc:119:
CrosLibrary::Get()->GetNetworkLibrary()->RequestNetworkScan();
If we add RequestScan() to ConnectivityStateHelper (which seems reasonable) then
we can get rid of NetworkLibrary here entirely. Otherwise I would wait on this
also...

[tbarzic, 2013-03-11 23:12:31]

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:308: if
(cros_library) {
Do we still have to check that cros_library is present.. if so, please add a
comment.

(note that afaik there are some browser tests that do not initialize cros
library, but still reach this part of code).

[gauravsh, 2013-03-12 00:30:25]

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/e...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:308: if
(cros_library) {
On 2013/03/11 23:12:31, tbarzic wrote:
> Do we still have to check that cros_library is present.. if so, please add a
> comment.
> 
> (note that afaik there are some browser tests that do not initialize cros
> library, but still reach this part of code).

That's what I figured - this pattern is used in many browser tests to avoid
NetworkLibrary related operations in test code. However, I re-ran the
browser_tests after removing this check - and it doesn't look like there are any
tests relying on this behavior. So, removed.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:176: NetworkLibrary* lib =
GetNetworkLibrary();
On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> I'd feel more comfortable converting all of this at once; depending on both
> NetworkStateHandler and NetowrkLibrary might have consistency issues so I
would
> like to limit the places we do this.

This will still use the NetworkLibrary path in production.

The NetworkStateHandler path should only ever be used if the
--enable-new-network-change-notifier flag is set. We will not be turning on that
flag by default until all consumers have been converted to use the connectivity
state wrappers.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper.cc:118:
network_state_handler_->RemoveObserver(this);
On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> nit: Use NetworkStateHandler::Get() here; if we accidentally destroy this
after
> NetworkStateHandler that will trigger a CHECK which will be easier to debug
than
> a random memory access.

Good point. Done.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper.h (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper.h:57:
ConnectivityStateHelperObserver* observer);
On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> I wonder if this would be better as just "Add/RemoveObserver"? I think that
> everywhere we use this the calling code is just observing "Connectivity" and
> doesn't know or care about the concept of a "Manager"

I did this to keep things simple and match current behavior - NetworkLibrary
defines at least 3 different types of observers - NetworkManagerObserver,
NetworkObserver and DeviceObserver, and how they are used is fairly inconsistent
throughout the code base. Once I add support for these other *Observers in
NetworkLibrary, I will also their Add*Observer equivalents.

Again, all this distinction will go away once everything is using the
NetworkStateHandlerObserver by default.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper_observer.h (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper_observer.h:16: virtual
void NetworkManagerChanged() = 0;
On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> Call this ConnectivityChanged() instead? At some point we could also improve
the
> Helper to track the connected network and state, only triggering the callback
> when connectivity actually changes.

This name matches the equivalent observer method in the
NetworkStateHandlerObserver. Perhaps we can change this to ConnectivityChanged()
once we use the NetworkStateHandler path by default.

Note that my eventual plan is to get rid of ConnectivityStateHelper* - this
interface is just so that we can make the transition easily instead of with a
single large CL.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_dropdown.cc:119:
CrosLibrary::Get()->GetNetworkLibrary()->RequestNetworkScan();
On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> If we add RequestScan() to ConnectivityStateHelper (which seems reasonable)
then
> we can get rid of NetworkLibrary here entirely. Otherwise I would wait on this
> also...

See my comment in mobile_activator. This shouldn't change the default behavior
since the change notifier flag is disabled - so we'll use the NetworkLibrary
path.

[tbarzic, 2013-03-12 00:31:47]

c/b/c/extensions LGTM

[stevenjb, 2013-03-12 15:53:33]

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator.cc:176: NetworkLibrary* lib =
GetNetworkLibrary();
On 2013/03/12 00:30:25, gauravsh wrote:
> On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> > I'd feel more comfortable converting all of this at once; depending on both
> > NetworkStateHandler and NetowrkLibrary might have consistency issues so I
> would
> > like to limit the places we do this.
> 
> This will still use the NetworkLibrary path in production.
> 
> The NetworkStateHandler path should only ever be used if the
> --enable-new-network-change-notifier flag is set. We will not be turning on
that
> flag by default until all consumers have been converted to use the
connectivity
> state wrappers.

We will almost certainly want to turn on --enable-new-network-change-notifier
before we eliminate NetworkLibrary entirely, and I would like to minimize the
places where we use both in the same code. Given that this change is small and
not essential to this CL, I would really rather convert the entire file at once.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
File chrome/browser/chromeos/mobile/mobile_activator_unittest.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/m...
chrome/browser/chromeos/mobile/mobile_activator_unittest.cc:103:
MockNetworkLibrary network_library_;
On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> Here too - I'd prefer to convert this all at once.
And this.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper.h (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper.h:57:
ConnectivityStateHelperObserver* observer);
On 2013/03/12 00:30:25, gauravsh wrote:
> On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> > I wonder if this would be better as just "Add/RemoveObserver"? I think that
> > everywhere we use this the calling code is just observing "Connectivity" and
> > doesn't know or care about the concept of a "Manager"
> 
> I did this to keep things simple and match current behavior - NetworkLibrary
> defines at least 3 different types of observers - NetworkManagerObserver,
> NetworkObserver and DeviceObserver, and how they are used is fairly
inconsistent
> throughout the code base. Once I add support for these other *Observers in
> NetworkLibrary, I will also their Add*Observer equivalents.
> 
> Again, all this distinction will go away once everything is using the
> NetworkStateHandlerObserver by default.

I guess the question is whether or not it makes sense to preserve this class
after we eliminate NetworkLibrary? Having a "ConnectivityStateHelper" that just
tracks connectivity state makes a fair bit of sense to me and could simplify the
calling code. Let's discuss in person what we really want to do here.

[gauravsh, 2013-03-12 18:18:09]

Removed mobile_activator changes out of this CL, and get rid of NetworkLibrary
dependency from network_dropdown.*.

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.cc (right):

https://codereview.chromium.org/12729002/diff/12001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/login/network_dropdown.cc:119:
CrosLibrary::Get()->GetNetworkLibrary()->RequestNetworkScan();
On 2013/03/12 00:30:25, gauravsh wrote:
> On 2013/03/11 23:11:05, stevenjb (chromium) wrote:
> > If we add RequestScan() to ConnectivityStateHelper (which seems reasonable)
> then
> > we can get rid of NetworkLibrary here entirely. Otherwise I would wait on
this
> > also...
> 
> See my comment in mobile_activator. This shouldn't change the default behavior
> since the change notifier flag is disabled - so we'll use the NetworkLibrary
> path.

Actually, RequestScan() is simple to add, so did so.

[stevenjb, 2013-03-12 19:18:07]

OK, this looks fine, thanks. We can look into whether to remove / rename /
repurpose this once we've deprecated NetworkLibrary.
LGTM

[commit-bot: I haz the power, 2013-03-12 19:23:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/12729002/22018

[commit-bot: I haz the power, 2013-03-12 20:01:10]

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.

[gauravsh, 2013-03-12 22:44:15]

tbarzic: PTAL. I had to add a IsInitialized() check to
file_browser_event_router. Apparently a unit test triggers the
FileBrowserEventRouter::Shutdown code path

[tbarzic, 2013-03-12 22:46:39]

On 2013/03/12 22:44:15, gauravsh wrote:
> tbarzic: PTAL. I had to add a IsInitialized() check to
> file_browser_event_router. Apparently a unit test triggers the
> FileBrowserEventRouter::Shutdown code path

lgtm

[commit-bot: I haz the power, 2013-03-12 22:53:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/12729002/43001

[commit-bot: I haz the power, 2013-03-13 04:09:15]

Change committed as 187767

[falken, 2013-03-13 05:46:49]

On 2013/03/13 04:09:15, I haz the power (commit-bot) wrote:
> Change committed as 187767

Sorry to revert this change. It seems to cause VMTest crashes on Chrome OS bots.

Recent build:
http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%...

Excerpt of crash log:
 0 
chrome!chromeos::ConnectivityStateHelperNetworkLibrary::~ConnectivityStateHelperNetworkLibrary()
[connectivity_state_helper.cc : 193 + 0xc]

[falken, 2013-03-13 05:49:53]

http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%2520%2528...

I don't know why I can't copy/paste a link without it getting mangled. Go to the
ChromiumOS (x86) builder and see build 12414.

[gauravsh, 2013-03-13 23:52:21]

stevenjb: Could you take a look at the new patchset? I changed the shut down
order of ConnectivityStateHelper to happen before DBusServices is shut down.

Fixed the shut down order of ConSetHelper to shut down before

[stevenjb, 2013-03-14 00:10:47]

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:726:
chromeos::ConnectivityStateHelper::Shutdown();
It would be nice if this could be initialized at the end of DBusServices() and
shut down at the beginning, what are the barriers to that?

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper.cc (right):

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper.cc:70:
ConnectivityStateHelper::ConnectivityStateHelper() {}
nit: } on separate line when not in the class declaration.

[gauravsh, 2013-03-14 01:06:10]

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:726:
chromeos::ConnectivityStateHelper::Shutdown();
On 2013/03/14 00:10:47, stevenjb (chromium) wrote:
> It would be nice if this could be initialized at the end of DBusServices() and
> shut down at the beginning, what are the barriers to that?

We initialize ConnectivityStateHelper in PreMainMessageLoopRun, so it happens
after about:flags settings are applied. I added shutdown here in
PostMainMessageLoopRun to match that.

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/connectivity_state_helper.cc (right):

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/connectivity_state_helper.cc:70:
ConnectivityStateHelper::ConnectivityStateHelper() {}
On 2013/03/14 00:10:47, stevenjb (chromium) wrote:
> nit: } on separate line when not in the class declaration.

Done.

[stevenjb, 2013-03-14 01:40:56]

lgtm

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/12729002/diff/72001/chrome/browser/chromeos/c...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:726:
chromeos::ConnectivityStateHelper::Shutdown();
On 2013/03/14 01:06:11, gauravsh wrote:
> On 2013/03/14 00:10:47, stevenjb (chromium) wrote:
> > It would be nice if this could be initialized at the end of DBusServices()
and
> > shut down at the beginning, what are the barriers to that?
> 
> We initialize ConnectivityStateHelper in PreMainMessageLoopRun, so it happens
> after about:flags settings are applied. I added shutdown here in
> PostMainMessageLoopRun to match that. 

Ah, right, good old about_flags, forgot about that. OK.

[commit-bot: I haz the power, 2013-03-14 04:13:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/12729002/87001

[commit-bot: I haz the power, 2013-03-14 05:31:06]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
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-03-14 07:00:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/12729002/87001

[commit-bot: I haz the power, 2013-03-14 13:05:46]

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

[commit-bot: I haz the power, 2013-03-15 01:13:46]

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

[commit-bot: I haz the power, 2013-03-15 08:17:57]

Change committed as 188296