ash uber tray: Add the detailed network popup

ash uber tray: Add the detailed network popup

BUG=110130, 109480
TEST=none

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

[sadrul, 2012-03-13 06:41:36]

This doesn't look quite like the mocks, yet, but this is a first step towards
that.

[Nikita (slow), 2012-03-13 08:41:26]

Network menu should honor login state and do not open network settings when not
logged in.

See all uses of 
StatusAreaViewChromeos::IsBrowserMode() in NetworkMenu class
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/sta...

[sadrul, 2012-03-13 13:09:17]

On 2012/03/13 08:41:26, Nikita Kostylev wrote:
> Network menu should honor login state and do not open network settings when
not
> logged in.

Good catch! Thanks! I have updated the CL to not show the settings option, and
the VPNs when not logged in.

> 
> See all uses of 
> StatusAreaViewChromeos::IsBrowserMode() in NetworkMenu class
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/sta...

[Nikita (slow), 2012-03-13 14:22:53]

On 2012/03/13 13:09:17, sadrul wrote:
> On 2012/03/13 08:41:26, Nikita Kostylev wrote:
> > Network menu should honor login state and do not open network settings when
> not
> > logged in.
> 
> Good catch! Thanks! I have updated the CL to not show the settings option, and
> the VPNs when not logged in.

At login screen we also have a special network menu item - "Change proxy
settings". Handled by
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...

[sadrul, 2012-03-13 14:45:25]

On 2012/03/13 14:22:53, Nikita Kostylev wrote:
> On 2012/03/13 13:09:17, sadrul wrote:
> > On 2012/03/13 08:41:26, Nikita Kostylev wrote:
> > > Network menu should honor login state and do not open network settings
when
> > not
> > > logged in.
> > 
> > Good catch! Thanks! I have updated the CL to not show the settings option,
and
> > the VPNs when not logged in.
> 
> At login screen we also have a special network menu item - "Change proxy
> settings". Handled by
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...

Added support for that.

Thanks for the quick feedbacks!

[Ben Goodger (Google), 2012-03-13 14:55:50]

http://codereview.chromium.org/9664056/diff/5005/ash/system/network/tray_netw...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/9664056/diff/5005/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:160: void Update() {
huge functions like this are easier to read/modify if you split all the various
updating chunks out into separate helper functions.

[sadrul, 2012-03-13 15:19:37]

http://codereview.chromium.org/9664056/diff/5005/ash/system/network/tray_netw...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/9664056/diff/5005/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:160: void Update() {
On 2012/03/13 14:55:50, Ben Goodger (Google) wrote:
> huge functions like this are easier to read/modify if you split all the
various
> updating chunks out into separate helper functions.

Did some refactoring. PTAL

[Ben Goodger (Google), 2012-03-13 15:31:18]

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:187: HoverHighlightView* container = new
HoverHighlightView(this);
AppendNetworkHeaderEntry();

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:202: std::vector<NetworkIconInfo> list;
can't these lines go into AppendNetworkEntries() too?

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:207: container = new
HoverHighlightView(this);
AppendAirplaneModeEntry();

[Nikita (slow), 2012-03-13 15:36:33]

http://codereview.chromium.org/9664056/diff/11002/ash/ash_strings.grd
File ash/ash_strings.grd (right):

http://codereview.chromium.org/9664056/diff/11002/ash/ash_strings.grd#newcode148
ash/ash_strings.grd:148: <message
name="IDS_ASH_STATUS_TRAY_NETWORK_PROXY_SETTINGS" desc="The label used in the
proxy settings entry in the network dialog in the login screen.">
Could we use existing IDS_STATUSBAR_NETWORK_OPEN_PROXY_SETTINGS_DIALOG?

http://codereview.chromium.org/9664056/diff/11002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/11002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:234: if
(!proxy_settings_dialog_.get()) {
#include "chrome/browser/chromeos/login/base_login_display_host.h"
#include "chrome/browser/chromeos/login/login_display_host.h"

BaseLoginDisplayHost::default_host()->OpenProxySettings();

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:46: views::BoxLayout::kHorizontal, 0, 3, 5));
nit: align

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:57: 5, 0));
nit: align

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:200: 
Where will be enable/disable interface (WiFi/Cellular) buttons implemented?

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:314: return NULL;
This method is not called when NetworkMenu opens items like Other... (connect to
unlisted WiFi).

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:366:
scoped_ptr<LoginHtmlDialog> proxy_settings_dialog_;
Should be removed.

[Nikita (slow), 2012-03-13 15:37:04]

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:314: return NULL;
On 2012/03/13 15:36:33, Nikita Kostylev wrote:
> This method is not called when NetworkMenu opens items like Other... (connect
to
> unlisted WiFi).

This was a question.

[sadrul, 2012-03-13 15:48:38]

Addressed all the comments. PTAL

http://codereview.chromium.org/9664056/diff/11002/ash/ash_strings.grd
File ash/ash_strings.grd (right):

http://codereview.chromium.org/9664056/diff/11002/ash/ash_strings.grd#newcode148
ash/ash_strings.grd:148: <message
name="IDS_ASH_STATUS_TRAY_NETWORK_PROXY_SETTINGS" desc="The label used in the
proxy settings entry in the network dialog in the login screen.">
On 2012/03/13 15:36:33, Nikita Kostylev wrote:
> Could we use existing IDS_STATUSBAR_NETWORK_OPEN_PROXY_SETTINGS_DIALOG?

Unfortunately, we are unable to use strings inside chrome/ from ash/

http://codereview.chromium.org/9664056/diff/11002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/11002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:234: if
(!proxy_settings_dialog_.get()) {
On 2012/03/13 15:36:33, Nikita Kostylev wrote:
> #include "chrome/browser/chromeos/login/base_login_display_host.h"
> #include "chrome/browser/chromeos/login/login_display_host.h"
> 
> BaseLoginDisplayHost::default_host()->OpenProxySettings();

Nice! Done. Thanks!

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:187: HoverHighlightView* container = new
HoverHighlightView(this);
On 2012/03/13 15:31:18, Ben Goodger (Google) wrote:
> AppendNetworkHeaderEntry();

Done.

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:200: 
On 2012/03/13 15:36:33, Nikita Kostylev wrote:
> Where will be enable/disable interface (WiFi/Cellular) buttons implemented?

I am not sure. They are not in the mocks [yet]. I will ask UX.

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:202: std::vector<NetworkIconInfo> list;
On 2012/03/13 15:31:18, Ben Goodger (Google) wrote:
> can't these lines go into AppendNetworkEntries() too?

Done.

http://codereview.chromium.org/9664056/diff/8032/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:207: container = new
HoverHighlightView(this);
On 2012/03/13 15:31:18, Ben Goodger (Google) wrote:
> AppendAirplaneModeEntry();

Done.

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:314: return NULL;
On 2012/03/13 15:37:04, Nikita Kostylev wrote:
> On 2012/03/13 15:36:33, Nikita Kostylev wrote:
> > This method is not called when NetworkMenu opens items like Other...
(connect
> to
> > unlisted WiFi).
> 
> This was a question.

I have changed this to return the ash container instead so that the dialogs are
parented correctly. Thanks.

http://codereview.chromium.org/9664056/diff/8032/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:366:
scoped_ptr<LoginHtmlDialog> proxy_settings_dialog_;
On 2012/03/13 15:36:33, Nikita Kostylev wrote:
> Should be removed.

Done.

[sadrul, 2012-03-13 17:49:30]

Included a change to make sure 'settings' etc. open a browser window if
necessary (e.g. there was no existing window). PTAL.

[Nikita (slow), 2012-03-13 18:01:15]

http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:311: return
ash::Shell::GetInstance()->GetContainer(
Shouldn't it be GetWidget()->GetNativeWindow()?

At the same time SettingBubbleContainer seems to be not correct container for
SystemTray which is located at kShellWindowId_StatusContainer.

[Nikita (slow), 2012-03-13 18:02:35]

http://codereview.chromium.org/9664056/diff/2024/ash/system/network/tray_netw...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/9664056/diff/2024/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:185: 
TODO somewhere for "Other" entry i.e. connecting to unlisted Wifi.

[sadrul, 2012-03-13 18:06:22]

http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:311: return
ash::Shell::GetInstance()->GetContainer(
On 2012/03/13 18:01:15, Nikita Kostylev wrote:
> Shouldn't it be GetWidget()->GetNativeWindow()?
> 
> At the same time SettingBubbleContainer seems to be not correct container for
> SystemTray which is located at kShellWindowId_StatusContainer.

There is no GetWidget() here.

The tray is in StatusContainer, but the popups that show up when you click on it
are in SettingBubbleContainer.

It is possible that NetworkMenu will try to show one of the network-config
dialogs in the login/lock screen, and they need to show up above them.
SettingBubbleContainer is a relevant layer (it could also be
kShellWindowId_LockSystemModalContainer, I suppose).

[sadrul, 2012-03-13 18:06:59]

http://codereview.chromium.org/9664056/diff/2024/ash/system/network/tray_netw...
File ash/system/network/tray_network.cc (right):

http://codereview.chromium.org/9664056/diff/2024/ash/system/network/tray_netw...
ash/system/network/tray_network.cc:185: 
On 2012/03/13 18:02:35, Nikita Kostylev wrote:
> TODO somewhere for "Other" entry i.e. connecting to unlisted Wifi.

I am not sure how that will be handled here. Discussing with UX

[sadrul, 2012-03-13 18:41:02]

http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:311: return
ash::Shell::GetInstance()->GetContainer(
On 2012/03/13 18:06:22, sadrul wrote:
> On 2012/03/13 18:01:15, Nikita Kostylev wrote:
> > Shouldn't it be GetWidget()->GetNativeWindow()?
> > 
> > At the same time SettingBubbleContainer seems to be not correct container
for
> > SystemTray which is located at kShellWindowId_StatusContainer.
> 
> There is no GetWidget() here.
> 
> The tray is in StatusContainer, but the popups that show up when you click on
it
> are in SettingBubbleContainer.
> 
> It is possible that NetworkMenu will try to show one of the network-config
> dialogs in the login/lock screen, and they need to show up above them.
> SettingBubbleContainer is a relevant layer (it could also be
> kShellWindowId_LockSystemModalContainer, I suppose).

It does feel like LockSystemModalContainer is a better place for this. Made this
change.

[Nikita (slow), 2012-03-14 11:28:51]

On 2012/03/13 18:41:02, sadrul wrote:
>
http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
> File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):
> 
>
http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
> chrome/browser/chromeos/system/ash_system_tray_delegate.cc:311: return
> ash::Shell::GetInstance()->GetContainer(
> On 2012/03/13 18:06:22, sadrul wrote:
> > On 2012/03/13 18:01:15, Nikita Kostylev wrote:
> > > Shouldn't it be GetWidget()->GetNativeWindow()?
> > > 
> > > At the same time SettingBubbleContainer seems to be not correct container
> for
> > > SystemTray which is located at kShellWindowId_StatusContainer.
> > 
> > There is no GetWidget() here.
> > 
> > The tray is in StatusContainer, but the popups that show up when you click
on
> it
> > are in SettingBubbleContainer.
> > 
> > It is possible that NetworkMenu will try to show one of the network-config
> > dialogs in the login/lock screen, and they need to show up above them.
> > SettingBubbleContainer is a relevant layer (it could also be
> > kShellWindowId_LockSystemModalContainer, I suppose).
> 
> It does feel like LockSystemModalContainer is a better place for this. Made
this
> change.

LockSystemModalContainer makes sense when at login screen and when not should it
be on SystemModalContainer?

[sadrul, 2012-03-14 12:10:01]

On 2012/03/14 11:28:51, Nikita Kostylev wrote:
> On 2012/03/13 18:41:02, sadrul wrote:
> >
>
http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
> > File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):
> > 
> >
>
http://codereview.chromium.org/9664056/diff/2024/chrome/browser/chromeos/syst...
> > chrome/browser/chromeos/system/ash_system_tray_delegate.cc:311: return
> > ash::Shell::GetInstance()->GetContainer(
> > On 2012/03/13 18:06:22, sadrul wrote:
> > > On 2012/03/13 18:01:15, Nikita Kostylev wrote:
> > > > Shouldn't it be GetWidget()->GetNativeWindow()?
> > > > 
> > > > At the same time SettingBubbleContainer seems to be not correct
container
> > for
> > > > SystemTray which is located at kShellWindowId_StatusContainer.
> > > 
> > > There is no GetWidget() here.
> > > 
> > > The tray is in StatusContainer, but the popups that show up when you click
> on
> > it
> > > are in SettingBubbleContainer.
> > > 
> > > It is possible that NetworkMenu will try to show one of the network-config
> > > dialogs in the login/lock screen, and they need to show up above them.
> > > SettingBubbleContainer is a relevant layer (it could also be
> > > kShellWindowId_LockSystemModalContainer, I suppose).
> > 
> > It does feel like LockSystemModalContainer is a better place for this. Made
> this
> > change.
> 
> LockSystemModalContainer makes sense when at login screen and when not should
it
> be on SystemModalContainer?

Probably. Made the change.

[sadrul, 2012-03-14 15:59:52]

screenie: http://www.corp.google.com/~sadrul/ubertray/network-scroll.png

[Ben Goodger (Google), 2012-03-14 16:09:40]

schweet.

-Ben

On Wed, Mar 14, 2012 at 8:59 AM, <sadrul@chromium.org> wrote:

> screenie: http://www.corp.google.com/~**sadrul/ubertray/network-**
> scroll.png<http://www.corp.google.com/~sadrul/ubertray/network-scroll.png>
>
>
http://codereview.chromium.**org/9664056/<http://codereview.chromium.org/9664...
>

[Nikita (slow), 2012-03-14 16:48:45]

lgtm on parts that I've commented on + special case handling that are missing
from this menu at this point + stuff that we've discussed on chat (promo bubble
etc.)

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

http://codereview.chromium.org/9664056/diff/13017/ash/system/network/tray_net...
ash/system/network/tray_network.cc:46: void AddIconAndLabel(const SkBitmap&
image, string16 label) {
const string16&

http://codereview.chromium.org/9664056/diff/13017/ash/system/network/tray_net...
ash/system/network/tray_network.cc:55: void AddLabel(string16 text) {
const string16&

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:187: if
(crosnet->ethernet_enabled()) {
Please add crosnet->ethernet_available().

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:214:
list->push_back(CreateNetworkIconInfo(cell[i], network_icon_.get()));
We have special cases for cellular:
1. If cellular is not activated we add this in cellular menu item.
2. If cellular is active supports data plan info we add separate menu item
  a. Plan info i.e. data left
  b. Text that it needs new plan
(network_menu.cc, MainMenuModel::InitMenuItems)
3. If we have cellular and have a carrier top-up URL defined we add a special
"View Account" menu item even if not registered on cellular / connected.
4. If cellular supports network scan (GSM) we add menu item for searching other
network.
5. If cellular is SIM locked then "Enable Mobile Data" would be shown with badge
+ open SIM unlock dialog.

Please add TODO somewhere.

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:327:
ash::internal::kShellWindowId_LockSystemModalContainer :
nit: 4 spaces alignment.

[Ben Goodger (Google), 2012-03-14 16:57:42]

General LGTM.

[sadrul, 2012-03-14 18:37:46]

Thanks!

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

http://codereview.chromium.org/9664056/diff/13017/ash/system/network/tray_net...
ash/system/network/tray_network.cc:46: void AddIconAndLabel(const SkBitmap&
image, string16 label) {
On 2012/03/14 16:48:45, Nikita Kostylev wrote:
> const string16&

Done.

http://codereview.chromium.org/9664056/diff/13017/ash/system/network/tray_net...
ash/system/network/tray_network.cc:55: void AddLabel(string16 text) {
On 2012/03/14 16:48:45, Nikita Kostylev wrote:
> const string16&

Done.

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:187: if
(crosnet->ethernet_enabled()) {
On 2012/03/14 16:48:45, Nikita Kostylev wrote:
> Please add crosnet->ethernet_available().

Done.

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:214:
list->push_back(CreateNetworkIconInfo(cell[i], network_icon_.get()));
On 2012/03/14 16:48:45, Nikita Kostylev wrote:
> We have special cases for cellular:
> 1. If cellular is not activated we add this in cellular menu item.
> 2. If cellular is active supports data plan info we add separate menu item
>   a. Plan info i.e. data left
>   b. Text that it needs new plan
> (network_menu.cc, MainMenuModel::InitMenuItems)
> 3. If we have cellular and have a carrier top-up URL defined we add a special
> "View Account" menu item even if not registered on cellular / connected.
> 4. If cellular supports network scan (GSM) we add menu item for searching
other
> network.
> 5. If cellular is SIM locked then "Enable Mobile Data" would be shown with
badge
> + open SIM unlock dialog.
> 
> Please add TODO somewhere.

Done.

http://codereview.chromium.org/9664056/diff/13017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:327:
ash::internal::kShellWindowId_LockSystemModalContainer :
On 2012/03/14 16:48:45, Nikita Kostylev wrote:
> nit: 4 spaces alignment.

Done.