Add NetworkNotificationView to tray_network.

Add NetworkNotificationView to tray_network. 
Changes NetworkMessageObserver for new notifications.


BUG=124727
TEST=Using stub and enabling ash-notify in chrome:flags: connect to WEP network with 'bad' and 'error' for password to generate error notifications. Connect to 'low data' cellular network for low data. Also test errors on device.

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

[stevenjb, 2012-05-22 17:14:47]

This CL is ready for review.

[sadrul, 2012-05-22 19:05:17]

(Reviewed only ash/system changes. I don't know much about this chromeos/ code)

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:632: void LinkClicked(views::Link* source,
int event_flags) {
This is overridden from views::LinkListener, right? +comment, +virtual,
+OVERRIDE

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:677: bool OnMousePressed(const
views::MouseEvent& event) {
+virtual, +OVERRIDE

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:762: ShowNotificationView();
Why do we hide first then show?

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:768: if (!errors_->messages().empty())
Same here. In both places, TrayNetwork should just update |notification_| and
the bubble should update itself. (and do ShowNotificationView only if
|notification_| is NULL)

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
File ash/system/network/tray_network.h (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.h:9: #include <map>
This isn't necessary here?

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:260: int default_bubble_height_;
I don't believe this is related to notification views? Please remove from this
CL if that is the case. In case it is related, what does this do?

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:53: const int kDetailedBubbleMaxHeight =
kTrayPopupItemHeight * 5;
Let's call this kNotificationBubbleMaxHeight. I don't think we need to worry
about this for detailed views.

[stevenjb, 2012-05-22 22:39:00]

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:632: void LinkClicked(views::Link* source,
int event_flags) {
On 2012/05/22 19:05:17, sadrul wrote:
> This is overridden from views::LinkListener, right? +comment, +virtual,
> +OVERRIDE

Done.

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:677: bool OnMousePressed(const
views::MouseEvent& event) {
On 2012/05/22 19:05:17, sadrul wrote:
> +virtual, +OVERRIDE

Done.

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:762: ShowNotificationView();
On 2012/05/22 19:05:17, sadrul wrote:
> Why do we hide first then show?
To re-create the view. I added an Update() instead to make it more clear.

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:768: if (!errors_->messages().empty())
On 2012/05/22 19:05:17, sadrul wrote:
> Same here. In both places, TrayNetwork should just update |notification_| and
> the bubble should update itself. (and do ShowNotificationView only if
> |notification_| is NULL)
ditto.

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
File ash/system/network/tray_network.h (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/network/tray_net...
ash/system/network/tray_network.h:9: #include <map>
On 2012/05/22 19:05:17, sadrul wrote:
> This isn't necessary here?

Done.

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray.h
File ash/system/tray/system_tray.h (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray...
ash/system/tray/system_tray.h:260: int default_bubble_height_;
On 2012/05/22 19:05:17, sadrul wrote:
> I don't believe this is related to notification views? Please remove from this
> CL if that is the case. In case it is related, what does this do?
Yes we need it, otherwise the detailed view gets sized to the size of the
network list. 
Clarified the comment.

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray...
File ash/system/tray/system_tray_bubble.cc (right):

http://codereview.chromium.org/10407080/diff/4001/ash/system/tray/system_tray...
ash/system/tray/system_tray_bubble.cc:53: const int kDetailedBubbleMaxHeight =
kTrayPopupItemHeight * 5;
On 2012/05/22 19:05:17, sadrul wrote:
> Let's call this kNotificationBubbleMaxHeight. I don't think we need to worry
> about this for detailed views.
This has nothing to do with notifications, it only applies to sizing the
detailed view when opened directly.

Currently, when we open the detailed view, it is always opened from the default
view, and we re-use the bubble so the height is set to the height of the default
view, which is what we want.

When we click on a notification, we want to open the detailed view directly. If
we have already opened the default view and set default_bubble_height_ in
SystemTray, we use that height, but if we have not (i.e. we receive a
notification before opening the system tray), we need a default height to use.
Also, if we ever have a configuration with fewer than 4 tray items (+ the
header), the view would be too small, so we also use this as a minimum height
for the detailed view.

Attempted to clarify the comment.

[stevenjb, 2012-05-23 16:50:05]

PTAL

[sadrul, 2012-05-23 17:09:09]

ash/ LGTM

[stevenjb, 2012-05-23 17:48:51]

+gspencer, can you take a look at the observer changes in browser/chromeos?
Thanks!

[Greg Spencer (Chromium), 2012-05-23 18:37:39]

http://codereview.chromium.org/10407080/diff/12002/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_message_observer.cc (right):

http://codereview.chromium.org/10407080/diff/12002/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_message_observer.cc:50: const std::string& id,
Would it be better supply an ash::NetworkObserver::ErrorType here and not bother
with a string?  I guess you'd still have to translate when you create the
SystemNotification, but it seems less error prone that way (i.e. you would only
get values that existed in the enum).

http://codereview.chromium.org/10407080/diff/12002/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_message_observer.cc:120: std::string id_;
This isn't used anywhere, and can be removed.

[stevenjb, 2012-05-23 18:54:37]

https://chromiumcodereview.appspot.com/10407080/diff/12002/chrome/browser/chr...
File chrome/browser/chromeos/network_message_observer.cc (right):

https://chromiumcodereview.appspot.com/10407080/diff/12002/chrome/browser/chr...
chrome/browser/chromeos/network_message_observer.cc:50: const std::string& id,
On 2012/05/23 18:37:39, Greg Spencer (Chromium) wrote:
> Would it be better supply an ash::NetworkObserver::ErrorType here and not
bother
> with a string?  I guess you'd still have to translate when you create the
> SystemNotification, but it seems less error prone that way (i.e. you would
only
> get values that existed in the enum).
Good suggestion, changed to pass error_type_ and set everything else here based
on that.

https://chromiumcodereview.appspot.com/10407080/diff/12002/chrome/browser/chr...
chrome/browser/chromeos/network_message_observer.cc:120: std::string id_;
On 2012/05/23 18:37:39, Greg Spencer (Chromium) wrote:
> This isn't used anywhere, and can be removed.

Done.

[Greg Spencer (Chromium), 2012-05-23 19:00:27]

http://codereview.chromium.org/10407080/diff/12004/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_message_observer.cc (right):

http://codereview.chromium.org/10407080/diff/12004/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_message_observer.cc:71: NOTREACHED();
Wouldn't you want to move this NOTREACHED() outside the switch so that Clang
will tell us if not all values are handled?

You could say:
if (id.empty())
  NOTREACHED() << "Invalid error type received";

[stevenjb, 2012-05-23 19:21:52]

https://chromiumcodereview.appspot.com/10407080/diff/12004/chrome/browser/chr...
File chrome/browser/chromeos/network_message_observer.cc (right):

https://chromiumcodereview.appspot.com/10407080/diff/12004/chrome/browser/chr...
chrome/browser/chromeos/network_message_observer.cc:71: NOTREACHED();
On 2012/05/23 19:00:28, Greg Spencer (Chromium) wrote:
> Wouldn't you want to move this NOTREACHED() outside the switch so that Clang
> will tell us if not all values are handled?
> 
> You could say:
> if (id.empty())
>   NOTREACHED() << "Invalid error type received";
Yes, good point. Fixed.

[Greg Spencer (Chromium), 2012-05-23 22:12:49]

lgtm