Update NetworkStateNotifier to use message_center.

Update NetworkStateNotifier to use message_center.

This is dependent on https://codereview.chromium.org/22796014/

Notes:
* This CL moves the "Cellular Activated" notification out 
  of DataPromoNotification into NetworkStateNotifier so
  that Ash now handles all network notifications except for
  "Google Chrome will use mobile data..." + any promo message 
  (which has Chrome dependencies).
* The "Cellular Activated" notification should now work 
  correctly for any number of activating networks.
* The logic for "Out of Credits" has been simplified, 
  which should also fix a case where it was not showing
  up when the flag was set but the network was never
  connected to (a useful case to be notified).

BUG=271546
For ash.gyp:
TBR=jamescook@chromium.org

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

[stevenjb, 2013-08-20 00:01:46]

The dependent CL needs to land before I can run the trybots, but this should be
ready for review.

[Jun Mukai, 2013-08-20 22:25:35]

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:335: service_path)));
As we discussed on another crbug, the clicked notification doesn't remove the
notification.
The specified callback will be called, but the notification remains in the
message center and visible to the user.  Is it expected?  I think it might be
better to remove the notification at the same time.
Otherwise, the stale notification may remain even after the user takes the
action.

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
chrome/browser/chromeos/status/data_promo_notification.cc:223:
message_center::MessageCenter::Get()->AddNotification(
let me note that usually chrome/browser code use NotificationUIManager (in
chrome/browser/notifications) instead of message_center directly.
That's just a simple bridge to MessageCenter in ChromeOS, but it has some
handling code, such like queuing when fullscreen / screen-locked.

I think this doesn't match to this case, since the notification should be shown
in such situation.

Therefore -- please write a comment here, like:
// Do not use NotificationUIManager since this notification should not be
blocked for fullscreen / screenlock.

[stevenjb, 2013-08-20 22:33:15]

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_connect.cc:335: service_path)));
On 2013/08/20 22:25:36, Jun Mukai wrote:
> As we discussed on another crbug, the clicked notification doesn't remove the
> notification.
> The specified callback will be called, but the notification remains in the
> message center and visible to the user.  Is it expected?  I think it might be
> better to remove the notification at the same time.
> Otherwise, the stale notification may remain even after the user takes the
> action.

Yes, it is expected that the notification will be dismissed on click.

Let's continue to discuss this in crbug.com/276633, but I can't think of an
example of a notification where we would -not- want to dismiss it after clicking
on it. If we don't make that the default we will need to provide a parameter to
dismiss on click, otherwise it is going to be tedious to do that for every
notification that wants that behavior.

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
chrome/browser/chromeos/status/data_promo_notification.cc:223:
message_center::MessageCenter::Get()->AddNotification(
On 2013/08/20 22:25:36, Jun Mukai wrote:
> let me note that usually chrome/browser code use NotificationUIManager (in
> chrome/browser/notifications) instead of message_center directly.
> That's just a simple bridge to MessageCenter in ChromeOS, but it has some
> handling code, such like queuing when fullscreen / screen-locked.
> 
> I think this doesn't match to this case, since the notification should be
shown
> in such situation.
> 
> Therefore -- please write a comment here, like:
> // Do not use NotificationUIManager since this notification should not be
> blocked for fullscreen / screenlock.

Chromeos System notifications should have a common interface, regardless of
whether the code happens to be in src/chrome or src/ash, without needing to
comment every instance. Maybe we should create a layer similar to
NotificationUIManager for system notifications?

[Jun Mukai, 2013-08-20 22:41:29]

lgtm

I don't know much about the network handler code, so I mainly looked at the
usage of message center. It looks good to me. You should get the review for the
network code change too.

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
chrome/browser/chromeos/status/data_promo_notification.cc:223:
message_center::MessageCenter::Get()->AddNotification(
On 2013/08/20 22:33:15, stevenjb (chromium) wrote:
> On 2013/08/20 22:25:36, Jun Mukai wrote:
> > let me note that usually chrome/browser code use NotificationUIManager (in
> > chrome/browser/notifications) instead of message_center directly.
> > That's just a simple bridge to MessageCenter in ChromeOS, but it has some
> > handling code, such like queuing when fullscreen / screen-locked.
> > 
> > I think this doesn't match to this case, since the notification should be
> shown
> > in such situation.
> > 
> > Therefore -- please write a comment here, like:
> > // Do not use NotificationUIManager since this notification should not be
> > blocked for fullscreen / screenlock.
> 
> Chromeos System notifications should have a common interface, regardless of
> whether the code happens to be in src/chrome or src/ash, without needing to
> comment every instance. Maybe we should create a layer similar to
> NotificationUIManager for system notifications?
> 

IIRC NotificationUIManager is a very tiny delegate to the message center, it was
just necessary for old balloon notifications.
Certainly that might be better to share the handling code of
fullscreen/lock-screens from ash, but that should be done in another CL.

[jennyz, 2013-08-21 00:02:42]

lgtm with minor nits.

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:123:
HasPendingConnectRequest())
nit: indent at the beginning of the continuing line.

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:235: // network_name
should never be empty if network_error_details is set.
Should we add a DCHECK(!network_name.empty())?

https://codereview.chromium.org/23075012/diff/8001/ui/message_center/notifica...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/23075012/diff/8001/ui/message_center/notifica...
ui/message_center/notification.h:170: // Helper method to create a simple System
notification. |click_callback|
|click_callback| is not consistent with the argument name |callback|.

[stevenjb, 2013-08-21 17:38:49]

pneubeck@, ptal at network_state_notifier and network_connect.

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:123:
HasPendingConnectRequest())
On 2013/08/21 00:02:42, jennyz wrote:
> nit: indent at the beginning of the continuing line.

I don't think there is a nice way to format this, so I went with what
google-clang-format suggests and moved the -> to the next line and indented
that.

https://codereview.chromium.org/23075012/diff/8001/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_notifier.cc:235: // network_name
should never be empty if network_error_details is set.
On 2013/08/21 00:02:42, jennyz wrote:
> Should we add a DCHECK(!network_name.empty())?

No, we shouldn't crash in that case, it's not an error, it just means that a
network somehow has an empty name. (i.e. it's logically possible, but shouldn't
happen in practice). I replaced 'never' with just 'shouldn't'.

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/23075012/diff/8001/chrome/browser/chromeos/st...
chrome/browser/chromeos/status/data_promo_notification.cc:223:
message_center::MessageCenter::Get()->AddNotification(
On 2013/08/20 22:41:29, Jun Mukai wrote:
> On 2013/08/20 22:33:15, stevenjb (chromium) wrote:
> > On 2013/08/20 22:25:36, Jun Mukai wrote:
> > > let me note that usually chrome/browser code use NotificationUIManager (in
> > > chrome/browser/notifications) instead of message_center directly.
> > > That's just a simple bridge to MessageCenter in ChromeOS, but it has some
> > > handling code, such like queuing when fullscreen / screen-locked.
> > > 
> > > I think this doesn't match to this case, since the notification should be
> > shown
> > > in such situation.
> > > 
> > > Therefore -- please write a comment here, like:
> > > // Do not use NotificationUIManager since this notification should not be
> > > blocked for fullscreen / screenlock.
> > 
> > Chromeos System notifications should have a common interface, regardless of
> > whether the code happens to be in src/chrome or src/ash, without needing to
> > comment every instance. Maybe we should create a layer similar to
> > NotificationUIManager for system notifications?
> > 
> 
> IIRC NotificationUIManager is a very tiny delegate to the message center, it
was
> just necessary for old balloon notifications.
> Certainly that might be better to share the handling code of
> fullscreen/lock-screens from ash, but that should be done in another CL.
Agreed. I think maybe we should have a chromeos::SystemNotificationManager that
both provides convenience methods, and handles any fullscreen/lock-screen
behavior. Alternately this could be built into message_center for type SYSTEM.

https://codereview.chromium.org/23075012/diff/8001/ui/message_center/notifica...
File ui/message_center/notification.h (right):

https://codereview.chromium.org/23075012/diff/8001/ui/message_center/notifica...
ui/message_center/notification.h:170: // Helper method to create a simple System
notification. |click_callback|
On 2013/08/21 00:02:42, jennyz wrote:
> |click_callback| is not consistent with the argument name |callback|.

Done.

[stevenjb, 2013-08-21 17:39:30]

armansito@ ptal at data_promo_notification

[armansito, 2013-08-21 19:51:30]

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:307:
cellular->activate_over_non_cellular_networks() &&
Why is this notification shown only when
Cellular.ActivateOverNonCellularNetworks is true? Do we have some other
notification logic for Verizon 3G?

https://codereview.chromium.org/23075012/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/23075012/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:97: std::string
carrier_id = device ? device->home_provider_id() : "";
Would notifications through this class ever be displayed for a roaming network?
It seems like using the home provider is correct for most cases but may need to
use "Cellular.ServingOperator" here if this applies to roaming networks in some
cases.

https://codereview.chromium.org/23075012/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:230:
default_network->path(), info_url)));
I realized that network_connect::ShowMobileSetup also uses this notification id
based on activation status. Is |kNetworkActivateNotificationId| reserved for
service activation only? If so, should this code here check whether or not the
service has been activated?

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

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:124:
network_connect::kNetworkConnectNotificationId, false /* by_user */);
nit: change comment to "not by user"

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:138:
network_connect::kNetworkConnectNotificationId, false /* by_user */);
nit: change comment to "not by user"

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:271: // directly to avoid a
spurrious notification.
spurrious -> spurious
nit: fit "directly" on first line

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:328:
ash::network_connect::kNetworkActivateNotificationId,
namespace qualifier redundant.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:52: const std::string&
service_path,
service_path unused

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:94: // if we connect to a
non cellular network.
nit (since it's a rare case): 
why restrict this to cellular? Someone might have more than one cellular device.
alternatively compare to the last default network's service_path

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:111: if
(!cellular->cellular_out_of_credits() || did_show_out_of_credits_)
the alternative did_show_out_of_credits_ prevents showing a notification after a
delay in line 130.
Note that " did_show_out_of_credits_ = true;" in line 128 is _not_ within the
if-block.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:114: // Only display a
notification if this is the default network, and we are
one condition is missing the comment: if there is not default network, display a
notification too.
(otherwise you could listen for DefaultNetworkChanged instead of
NetworkPropertiesUpdated)

[pneubeck (no reviews), 2013-08-22 16:51:45]

isn't the commit message partially out-dated?
I think you addressed
  * The flow for showing a connect error has changed to 
first request the latest properties of the service 
that failed. This ensures that the Error and ErrorDetail 
properties are up-to-date, and eliminates the need to 
cache ErrorDetail. 

in the other CL.

[stevenjb, 2013-08-22 18:01:01]

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (left):

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:307:
cellular->activate_over_non_cellular_networks() &&
On 2013/08/21 19:51:31, armansito wrote:
> Why is this notification shown only when
> Cellular.ActivateOverNonCellularNetworks is true? Do we have some other
> notification logic for Verizon 3G?

The notification says 'Activation of '<ph name="name">$1<ex>Generic
Wireless</ex></ph>' requires a network connection.'

My understanding (and the existing logic) is that this only applies when
activation is done over non cellular networks. Otherwise we will just show the
mobile setup dialog.

I believe that eventually we would like the mobile setup dialog to detect that
there is no network connection if it requires one and inform the user, but for
now we use a notification for that scenario.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_connect.cc (right):

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:124:
network_connect::kNetworkConnectNotificationId, false /* by_user */);
On 2013/08/22 16:28:22, pneubeck wrote:
> nit: change comment to "not by user"

Done.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:138:
network_connect::kNetworkConnectNotificationId, false /* by_user */);
On 2013/08/22 16:28:22, pneubeck wrote:
> nit: change comment to "not by user"

Done.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:271: // directly to avoid a
spurrious notification.
On 2013/08/22 16:28:22, pneubeck wrote:
> spurrious -> spurious
> nit: fit "directly" on first line

Done.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_connect.cc:328:
ash::network_connect::kNetworkActivateNotificationId,
On 2013/08/22 16:28:22, pneubeck wrote:
> namespace qualifier redundant.

Done.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:52: const std::string&
service_path,
On 2013/08/22 16:28:22, pneubeck wrote:
> service_path unused

Done.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:94: // if we connect to a
non cellular network.
On 2013/08/22 16:28:22, pneubeck wrote:
> nit (since it's a rare case): 
> why restrict this to cellular? Someone might have more than one cellular
device.
> alternatively compare to the last default network's service_path

For the purposes of showing an 'out of credits' notification, switching from one
cellular to another doesn't seem like a case where we would want to show the
notification again, it is more likely to be confusing than helpful. This is for
the case where we switched from cellular to wifi or ethernet. If we switch back
to cellular (after the delay) we'll want to inform the user.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:111: if
(!cellular->cellular_out_of_credits() || did_show_out_of_credits_)
On 2013/08/22 16:28:22, pneubeck wrote:
> the alternative did_show_out_of_credits_ prevents showing a notification after
a
> delay in line 130.
> Note that " did_show_out_of_credits_ = true;" in line 128 is _not_ within the
> if-block.
I'm not entirely sure I understand this, but I'll try to clarify the comments.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:114: // Only display a
notification if this is the default network, and we are
On 2013/08/22 16:28:22, pneubeck wrote:
> one condition is missing the comment: if there is not default network, display
a
> notification too.
> (otherwise you could listen for DefaultNetworkChanged instead of
> NetworkPropertiesUpdated)
Done.

https://codereview.chromium.org/23075012/diff/19001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/status/data_promo_notification.cc (right):

https://codereview.chromium.org/23075012/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:97: std::string
carrier_id = device ? device->home_provider_id() : "";
On 2013/08/21 19:51:31, armansito wrote:
> Would notifications through this class ever be displayed for a roaming
network?
> It seems like using the home provider is correct for most cases but may need
to
> use "Cellular.ServingOperator" here if this applies to roaming networks in
some
> cases.

I am not familiar with Cellular.ServingOperator. My intent is to preserve the
existing logic with this CL, we can (and probably should) refactor and update
this code at some point.

https://codereview.chromium.org/23075012/diff/19001/chrome/browser/chromeos/s...
chrome/browser/chromeos/status/data_promo_notification.cc:230:
default_network->path(), info_url)));
On 2013/08/21 19:51:31, armansito wrote:
> I realized that network_connect::ShowMobileSetup also uses this notification
id
> based on activation status. Is |kNetworkActivateNotificationId| reserved for
> service activation only? If so, should this code here check whether or not the
> service has been activated?

The notification id means that this notification would replace an Activation
notification or vice versa. Since these states should be mutually exclusive that
seemed to make sense to me, but we could change this to a unique id if you think
we may want to show both notifications?

[pneubeck (no reviews), 2013-08-22 20:10:03]

lgtm

[pneubeck (no reviews), 2013-08-22 20:10:32]

I mainly looked at ash/system/chromeos/network/*

[armansito, 2013-08-22 20:59:52]

Just one comment, but nothing you have to address in this CL. lgtm.

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_state_notifier.cc (right):

https://codereview.chromium.org/23075012/diff/19001/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_state_notifier.cc:111: if
(!cellular->cellular_out_of_credits() || did_show_out_of_credits_)
On 2013/08/22 18:01:02, stevenjb (chromium) wrote:
> On 2013/08/22 16:28:22, pneubeck wrote:
> > the alternative did_show_out_of_credits_ prevents showing a notification
after
> a
> > delay in line 130.
> > Note that " did_show_out_of_credits_ = true;" in line 128 is _not_ within
the
> > if-block.
> I'm not entirely sure I understand this, but I'll try to clarify the comments.

You may want to associate the |did_show_out_of_credits| with that particular
device. If the user, say, ran out of credit on their built-in modem. Then they
decided to plug-in a dongle which also happens to be out-of-credit and we
detected, without connecting its network that it is out of credit. In that case
we would want to show a notification.

I understand that such a revision is beyond the scope of this CL though. So I
can come back to it if it ever bothers us.

[stevenjb, 2013-08-22 23:43:27]

After talking to Arman, I went with his (and philip's earlier) suggestion of
tracking default network and using changes to it to reset
did_show_out_of_credits_ since it will be robust for the case of multiple
cellular networks, and is ultimately simpler.

[armansito, 2013-08-23 00:18:51]

On 2013/08/22 23:43:27, stevenjb (chromium) wrote:
> After talking to Arman, I went with his (and philip's earlier) suggestion of
> tracking default network and using changes to it to reset
> did_show_out_of_credits_ since it will be robust for the case of multiple
> cellular networks, and is ultimately simpler.

lgtm

[commit-bot: I haz the power, 2013-08-23 00:21:36]

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

[commit-bot: I haz the power, 2013-08-23 00:36:57]

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

[commit-bot: I haz the power, 2013-08-23 01:06:58]

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

[commit-bot: I haz the power, 2013-08-23 05:43:52]

Change committed as 219232