Add NetworkStateListDetailedView

Add NetworkStateListDetailedView
Implemented behind --ash-network-state-handler
Copies the bulk of the NetworkMenuIcon logic to ash; NetworkMenuIcon will eventually be deprecated.

BUG=161869
For .grd changes:
TBR=sky@chromium.org

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

[stevenjb, 2012-11-15 01:37:54]

Bit of an early preview, I need to do a self-review if you want to wait (and I
need to break up the assets into a separate CL).

Feel free to wait to review until I send an update.

[pneubeck (no reviews), 2012-11-21 13:34:44]

Just a few nits during reading the code.

https://codereview.chromium.org/11361274/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/11361274/diff/1/ash/ash.gyp#newcode172
ash/ash.gyp:172: 'system/chromeos/network/network_state_list_detailed_view.h',
.cc before .h (also wrong two lines before)

https://codereview.chromium.org/11361274/diff/1/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/11361274/diff/1/ash/ash_strings.grd#newcode458
ash/ash_strings.grd:458: <message name="IDS_ASH_STATUS_TRAY_NETWORK_STATUS"
desc="Shows the network and its status in the network lsit.">
nit: lsit -> list

https://codereview.chromium.org/11361274/diff/1/ash/ash_strings.grd#newcode458
ash/ash_strings.grd:458: <message name="IDS_ASH_STATUS_TRAY_NETWORK_STATUS"
desc="Shows the network and its status in the network lsit.">
which network? the current network?

https://codereview.chromium.org/11361274/diff/1/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/11361274/diff/1/ash/ash_switches.cc#newcode54
ash/ash_switches.cc:54: // Enables the Oak tree viewer.
adapt the comment

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.h:18: class NetworkIcon {
It seems that only the GetImageForNetwork function should be called and not the
constructor?
Could the constructor be private?

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.h:48: // Updates strength_index_ for
wireless networks. Returns true if changed.
put strength_index_ between || for consistency

newlines between commented methods/members?

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_list_detailed_view_base.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_list_detailed_view_base.h:31: // Overridden
from NetworkDetailedView:
Nit: point instead of colon

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_state_list_detailed_view.cc:177: const
ui::Event& event) OVERRIDE {
only in headers?

[Greg Spencer (Chromium), 2012-11-21 18:01:50]

https://codereview.chromium.org/11361274/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/shell.h#newcode411
ash/shell.h:411: // This is gauranteed to outlive any UI elements so can be
safely accessed.
Maybe mention what manages the lifetime.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:31:
//==============================================================================
nit: Can you use "-" for all of the separators if you're going to have them?  Or
was there a reason you use "=" for some of these separators and not others?

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:74: static SizeBitmapMap*
empty_bitmaps_ = new SizeBitmapMap;
I think this should be named with g_empty_bitmaps or something distinguishing. 
I think trailing underscore is limited to member variables.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:232: if (network->technology() ==
flimflam::kNetworkTechnologyEvdo) {
Shouldn't you just leave this entire if/else out?  We don't tend to check in
commented out code (I think there's a specific prohibition in the style guide).

A TODO is sufficient, I think, and if you need to refer to it when you
implement, then just look back at this CL...

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:340: // For networks that always in
roaming don't show roaming badge.
nit: always in => are always

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:440: static NetworkIconMap*
icon_map_dark = NULL;
g_icon_map_dark?  I'm not sure what the naming convention should be (the style
guide doesn't say anything), but seems like statics should be distinguished
somehow.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:445: static NetworkIconMap*
icon_map_light = NULL;
g_icon_map_light?

[stevenjb, 2012-11-27 00:58:18]

OK, this should be ready for final review. Thanks for the early feedback.

https://codereview.chromium.org/11361274/diff/1/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/11361274/diff/1/ash/ash.gyp#newcode172
ash/ash.gyp:172: 'system/chromeos/network/network_state_list_detailed_view.h',
On 2012/11/21 13:34:45, pneubeck wrote:
> .cc before .h (also wrong two lines before)

Done.

https://codereview.chromium.org/11361274/diff/1/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://codereview.chromium.org/11361274/diff/1/ash/ash_strings.grd#newcode458
ash/ash_strings.grd:458: <message name="IDS_ASH_STATUS_TRAY_NETWORK_STATUS"
desc="Shows the network and its status in the network lsit.">
On 2012/11/21 13:34:45, pneubeck wrote:
> nit: lsit -> list

Done.

https://codereview.chromium.org/11361274/diff/1/ash/ash_strings.grd#newcode458
ash/ash_strings.grd:458: <message name="IDS_ASH_STATUS_TRAY_NETWORK_STATUS"
desc="Shows the network and its status in the network lsit.">
On 2012/11/21 13:34:45, pneubeck wrote:
> which network? the current network?
s/the/a/

https://codereview.chromium.org/11361274/diff/1/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/11361274/diff/1/ash/ash_switches.cc#newcode54
ash/ash_switches.cc:54: // Enables the Oak tree viewer.
On 2012/11/21 13:34:45, pneubeck wrote:
> adapt the comment
Moved to chromeos_switches in a separate CL (with correct comment)

https://codereview.chromium.org/11361274/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/shell.h#newcode411
ash/shell.h:411: // This is gauranteed to outlive any UI elements so can be
safely accessed.
On 2012/11/21 18:01:51, Greg Spencer (Chromium) wrote:
> Maybe mention what manages the lifetime.
Replaced with chromeos::NetworkStateHandler::Get()

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:31:
//==============================================================================
On 2012/11/21 18:01:51, Greg Spencer (Chromium) wrote:
> nit: Can you use "-" for all of the separators if you're going to have them? 
Or
> was there a reason you use "=" for some of these separators and not others?

Done.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:74: static SizeBitmapMap*
empty_bitmaps_ = new SizeBitmapMap;
On 2012/11/21 18:01:51, Greg Spencer (Chromium) wrote:
> I think this should be named with g_empty_bitmaps or something distinguishing.

> I think trailing underscore is limited to member variables.

Done.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:232: if (network->technology() ==
flimflam::kNetworkTechnologyEvdo) {
On 2012/11/21 18:01:51, Greg Spencer (Chromium) wrote:
> Shouldn't you just leave this entire if/else out?  We don't tend to check in
> commented out code (I think there's a specific prohibition in the style
guide).
> 
> A TODO is sufficient, I think, and if you need to refer to it when you
> implement, then just look back at this CL...

Actually, I didn't want to leave this as a TODO that never got done :) (This the
preliminary caveat for the CL).

As it turns out, data_left is deprecated entirely, so eliminated it.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:340: // For networks that always in
roaming don't show roaming badge.
On 2012/11/21 18:01:51, Greg Spencer (Chromium) wrote:
> nit: always in => are always

Done.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:440: static NetworkIconMap*
icon_map_dark = NULL;
On 2012/11/21 18:01:51, Greg Spencer (Chromium) wrote:
> g_icon_map_dark?  I'm not sure what the naming convention should be (the style
> guide doesn't say anything), but seems like statics should be distinguished
> somehow.
I don't like g_ in function scope, but I've seen s_ before, so will use that.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.cc:445: static NetworkIconMap*
icon_map_light = NULL;
On 2012/11/21 18:01:51, Greg Spencer (Chromium) wrote:
> g_icon_map_light?
Ditto

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_icon.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.h:18: class NetworkIcon {
On 2012/11/21 13:34:45, pneubeck wrote:
> It seems that only the GetImageForNetwork function should be called and not
the
> constructor?
> Could the constructor be private?

Moved NetworkIcon into the .cc file, put GetImageForNetwork into a namespace.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_icon.h:48: // Updates strength_index_ for
wireless networks. Returns true if changed.
On 2012/11/21 13:34:45, pneubeck wrote:
> put strength_index_ between || for consistency
> 
> newlines between commented methods/members?

Done.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_list_detailed_view_base.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_list_detailed_view_base.h:31: // Overridden
from NetworkDetailedView:
On 2012/11/21 13:34:45, pneubeck wrote:
> Nit: point instead of colon

Done.

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_state_list_detailed_view.cc:177: const
ui::Event& event) OVERRIDE {
On 2012/11/21 13:34:45, pneubeck wrote:
> only in headers?

Done.

[jennyz, 2012-11-27 01:43:52]

By the way, the cl title and description "Add NetworkListDetailedView" should be
changed to "Add NetworkStateListDetailedView".

Are you going to enabled the network state handler and deprecated the
NetworkListDetailedView in the near future? Is there any reason to do this
migration progressively? 

By the way, the latest patch reduced a lot of the files, are you putting them
into a separate cl?

https://codereview.chromium.org/11361274/diff/1/ash/resources/ash_resources.grd
File ash/resources/ash_resources.grd (right):

https://codereview.chromium.org/11361274/diff/1/ash/resources/ash_resources.g...
ash/resources/ash_resources.grd:150: <!-- ChromeOS specific icons -->
For chromeos specific resources, should do <if expr="pp_ifdef('chromeos')"> to
enclose them, right?

https://codereview.chromium.org/11361274/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/shell.h#newcode535
ash/shell.h:535: // Maintains a list of visible networks and their state
nit: "state" -> "states."

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
File ash/system/chromeos/network/network_state_list_detailed_view.h (right):

https://codereview.chromium.org/11361274/diff/1/ash/system/chromeos/network/n...
ash/system/chromeos/network/network_state_list_detailed_view.h:41: // Overridden
from NetworkDetailedView:
nit: ":" -> "." for consistency.

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.cc:359: (login_ ==
user::LOGGED_IN_NONE || !active_network)) {
This won't separate VPNs out from rest of network when user logged in and has
active network?

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.cc:369:
IDS_ASH_STATUS_TRAY_NETWORK_CONNECTING));
Why do you need to put the string "connecting..." in a separate string resource
IDS_ASH_STATUS_TRAY_NETWORK_CONNECTING, instead of making it part of
IDS_ASH_STATUS_TRAY_NETWORK_STATUS?

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
File ash/system/chromeos/network/tray_network_state_observer.cc (right):

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
ash/system/chromeos/network/tray_network_state_observer.cc:36:
tray_->PopupDetailedView(kTrayPopupAutoCloseDelayForTextInSeconds, false);
Is this a new UI feature for popping up detailed network view when wifi is
changed to enabled state, or it just matches an exisiting UI feature?

[stevenjb, 2012-11-27 17:20:32]

PTAL

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.cc:359: (login_ ==
user::LOGGED_IN_NONE || !active_network)) {
On 2012/11/27 01:43:52, jennyz wrote:
> This won't separate VPNs out from rest of network when user logged in and has
> active network? 
Oops, this was started before VPNs were separated out. Changed to always skip
VONs here.

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.cc:369:
IDS_ASH_STATUS_TRAY_NETWORK_CONNECTING));
On 2012/11/27 01:43:52, jennyz wrote:
> Why do you need to put the string "connecting..." in a separate string
resource
> IDS_ASH_STATUS_TRAY_NETWORK_CONNECTING, instead of making it part of
> IDS_ASH_STATUS_TRAY_NETWORK_STATUS?
Thanks, this was copied from some old code. Fixed.

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
File ash/system/chromeos/network/tray_network_state_observer.cc (right):

https://codereview.chromium.org/11361274/diff/6003/ash/system/chromeos/networ...
ash/system/chromeos/network/tray_network_state_observer.cc:36:
tray_->PopupDetailedView(kTrayPopupAutoCloseDelayForTextInSeconds, false);
On 2012/11/27 01:43:52, jennyz wrote:
> Is this a new UI feature for popping up detailed network view when wifi is
> changed to enabled state, or it just matches an exisiting UI feature?
This replaces TrayNetwork::OnWillToggleWifi() which will become deprecated. I
added a test in OnWillToggleWifi() to ensure that we don't call this twice.

[gauravsh, 2012-11-27 20:48:09]

https://codereview.chromium.org/11361274/diff/10028/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/11361274/diff/10028/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_icon.cc:33: class NetworkIcon {
Is there a reason why this is not declared in network_icon.h? Is it because this
is not meant to use elsewhere? It's just a bit odd that network_icon.h doesn't
actually declare the NetworkIcon class. Perhaps network_icon.h should be called
animation_observer.h then?

https://codereview.chromium.org/11361274/diff/10028/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/11361274/diff/10028/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:156: // TODO(stevenjb): Use
kServiceWatchListProperty and request properties
Any reason why you got rid of the service watch list handling? Can this be part
of a separate CL?

[stevenjb, 2012-11-27 22:19:19]

https://codereview.chromium.org/11361274/diff/10028/ash/system/chromeos/netwo...
File ash/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/11361274/diff/10028/ash/system/chromeos/netwo...
ash/system/chromeos/network/network_icon.cc:33: class NetworkIcon {
On 2012/11/27 20:48:09, gauravsh wrote:
> Is there a reason why this is not declared in network_icon.h? Is it because
this
> is not meant to use elsewhere? It's just a bit odd that network_icon.h doesn't
> actually declare the NetworkIcon class. Perhaps network_icon.h should be
called
> animation_observer.h then?

network_icon.h defines the network_icon namespace. We use a namespace here
because we only expose global functions to access this code; no other class
depends on the NetworkIcon implementation. 

I will rename this to NetworkIconImpl and put it in the anonymous namespace to
make that more clear.

AnimationObserver is in the network_icon namespace which is why it is there. It
could be extracted to a network_icon_animation_observer file/class if that would
be more clear.

https://codereview.chromium.org/11361274/diff/10028/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/11361274/diff/10028/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:156: // TODO(stevenjb): Use
kServiceWatchListProperty and request properties
On 2012/11/27 20:48:09, gauravsh wrote:
> Any reason why you got rid of the service watch list handling? Can this be
part
> of a separate CL?

I can put this in a separate CL and implement it correctly. This was an easier
fix to get the code working.

[jennyz, 2012-11-28 18:31:27]

lgtm

[stevenjb, 2012-11-29 03:51:04]

gspencer@ and/or gauravsh@, can you take one more look? Thanks.

[Greg Spencer (Chromium), 2012-11-29 04:20:45]

LGTM with nits.

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
ash/system/chromeos/network/network_icon.cc:34: // NetworkIcon class declaration
NetworkIcon -> NetworkIconImpl

And really... not the most useful comment: you could probably just remove it.

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.cc:563: std::string
ip_address("0.0.0,0");
Is that really supposed to be a comma? :-)

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_list_detailed_view.h (right):

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.h:85: std::string
carrier_id_;
nit: Maybe a comment on what each group of member vars is for?  Or remove the
blank lines if there's no reason for the grouping.

[stevenjb, 2012-11-29 18:18:18]

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
ash/system/chromeos/network/network_icon.cc:34: // NetworkIcon class declaration
On 2012/11/29 04:20:45, Greg Spencer (Chromium) wrote:
> NetworkIcon -> NetworkIconImpl
> 
> And really... not the most useful comment: you could probably just remove it.

Modified the comment to be a proper class comment.

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_list_detailed_view.cc (right):

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.cc:563: std::string
ip_address("0.0.0,0");
On 2012/11/29 04:20:46, Greg Spencer (Chromium) wrote:
> Is that really supposed to be a comma? :-)
No :P

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
File ash/system/chromeos/network/network_state_list_detailed_view.h (right):

https://codereview.chromium.org/11361274/diff/4025/ash/system/chromeos/networ...
ash/system/chromeos/network/network_state_list_detailed_view.h:85: std::string
carrier_id_;
On 2012/11/29 04:20:46, Greg Spencer (Chromium) wrote:
> nit: Maybe a comment on what each group of member vars is for?  Or remove the
> blank lines if there's no reason for the grouping.
Comments added.

[commit-bot: I haz the power, 2012-11-29 18:23:32]

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

[commit-bot: I haz the power, 2012-11-29 18:23:46]

Presubmit check for 11361274-7010 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 1.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2012-11-29 18:38:53]

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

[commit-bot: I haz the power, 2012-11-29 19:22:16]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 19:35:49]

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

[commit-bot: I haz the power, 2012-11-29 20:58:38]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests