Support toggling Wi-Fi with keyboard shortcut.

Support toggling Wi-Fi with keyboard shortcut.

- F11 key toggles Wi-Fi when Debugging keyboard shortcuts is enabled.
- A notification is shown when Wi-Fi enable/disable is toggled.

BUG=139039
TEST=F11 key toggles Wi-Fi with "Debug keyboard shortcuts" enabled in about:flags.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149887

[mazda, 2012-08-02 04:06:58]

Hi Daniel,
Could you review this change?

[Daniel Erat, 2012-08-02 04:33:14]

http://codereview.chromium.org/10824142/diff/1/ash/accelerators/accelerator_t...
File ash/accelerators/accelerator_table.cc (right):

http://codereview.chromium.org/10824142/diff/1/ash/accelerators/accelerator_t...
ash/accelerators/accelerator_table.cc:97: { true, ui::VKEY_F11, ui::EF_NONE,
TOGGLE_WIFI },
Is this in the wrong array?  Shouldn't it be in kDebugAcceleratorData instead?

Even if it's debug-only, I'm still a bit concerned that F11 is a bit too easy to
hit and may confuse people (since I think it usually toggles fullscreen on OSes
other than Chrome OS).

[mazda, 2012-08-02 05:44:13]

Thank you for your comment.
I addressed the comment and also changed the messages for the notification
following Kuscher's suggestion.
Please take another look.

http://codereview.chromium.org/10824142/diff/1/ash/accelerators/accelerator_t...
File ash/accelerators/accelerator_table.cc (right):

http://codereview.chromium.org/10824142/diff/1/ash/accelerators/accelerator_t...
ash/accelerators/accelerator_table.cc:97: { true, ui::VKEY_F11, ui::EF_NONE,
TOGGLE_WIFI },
On 2012/08/02 04:33:14, Daniel Erat wrote:
> Is this in the wrong array?  Shouldn't it be in kDebugAcceleratorData instead?
> 
> Even if it's debug-only, I'm still a bit concerned that F11 is a bit too easy
to
> hit and may confuse people (since I think it usually toggles fullscreen on
OSes
> other than Chrome OS).

Oops. Moved it to kDebugAcceleratorData.
I chose F11 because Wi-Fi key is at the position of F11 on Parrot. But that's
surely confusing on other keyboards.
What about Ctrl+Alt+W?

[Daniel Erat, 2012-08-02 14:26:45]

Ctrl-Alt-W sounds fine if it's not already used.

This mostly looks good to me, but I have one question about a tricky bit.

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

http://codereview.chromium.org/10824142/diff/9001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:696:
tray_->network_observer()->OnWifiToggled(!old_wifi_enabled);
Is there any danger of this getting out of sync?

Given NetworkLibrary* lib, GetWifiEnabled() looks like it calls
lib->wifi_enabled() and network_menu_->ToggleWifi() calls
lib->EnableWifiNetworkDevice(!lib->wifi_enabled()).

Are there any other callers of GetWifiEnabled()?  I'm concerned that given
lib->wifi_enabled() == true, someone could call ToggleWifi(), which would result
in network_observer()->OnWifiToggled(false) getting called, but then someone
else could call GetWifiEnabled() soon afterward and get true.

Would it be safer to cache the most-recently-requested state within
SystemTrayDelegate?  Is there any risk of the toggle requests failing?

[mazda, 2012-08-02 17:14:30]

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

http://codereview.chromium.org/10824142/diff/9001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:696:
tray_->network_observer()->OnWifiToggled(!old_wifi_enabled);
On 2012/08/02 14:26:45, Daniel Erat wrote:
> Is there any danger of this getting out of sync?
> 
> Given NetworkLibrary* lib, GetWifiEnabled() looks like it calls
> lib->wifi_enabled() and network_menu_->ToggleWifi() calls
> lib->EnableWifiNetworkDevice(!lib->wifi_enabled()).
> 
> Are there any other callers of GetWifiEnabled()?  I'm concerned that given
> lib->wifi_enabled() == true, someone could call ToggleWifi(), which would
result
> in network_observer()->OnWifiToggled(false) getting called, but then someone
> else could call GetWifiEnabled() soon afterward and get true.
> 
> Would it be safer to cache the most-recently-requested state within
> SystemTrayDelegate?  Is there any risk of the toggle requests failing?
> 

TrayNetwork calls GetWifiEnabled, but it's called in
TrayNetwork::OnNetworkRefresh so it gets the latest state.

I initially tried to cache the wifi state in TrayNetwork and monitor the change
of the state in TrayNetwork::OnNetworkRefresh. It's more reliable than the
current method, but it turned out that it's difficult to distinguish the state
change invoked by the user from the one by the system, which occurs immediately
after the boot. I'd like to identify the state change invoked only by the user.

How about renaming OnWifiToggled to OnWillToggleWifi? This implies more that the
user initiated toggling wifi.

[Daniel Erat, 2012-08-02 17:15:40]

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

http://codereview.chromium.org/10824142/diff/9001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:696:
tray_->network_observer()->OnWifiToggled(!old_wifi_enabled);
On 2012/08/02 17:14:30, mazda wrote:
> On 2012/08/02 14:26:45, Daniel Erat wrote:
> > Is there any danger of this getting out of sync?
> > 
> > Given NetworkLibrary* lib, GetWifiEnabled() looks like it calls
> > lib->wifi_enabled() and network_menu_->ToggleWifi() calls
> > lib->EnableWifiNetworkDevice(!lib->wifi_enabled()).
> > 
> > Are there any other callers of GetWifiEnabled()?  I'm concerned that given
> > lib->wifi_enabled() == true, someone could call ToggleWifi(), which would
> result
> > in network_observer()->OnWifiToggled(false) getting called, but then someone
> > else could call GetWifiEnabled() soon afterward and get true.
> > 
> > Would it be safer to cache the most-recently-requested state within
> > SystemTrayDelegate?  Is there any risk of the toggle requests failing?
> > 
> 
> TrayNetwork calls GetWifiEnabled, but it's called in
> TrayNetwork::OnNetworkRefresh so it gets the latest state.
> 
> I initially tried to cache the wifi state in TrayNetwork and monitor the
change
> of the state in TrayNetwork::OnNetworkRefresh. It's more reliable than the
> current method, but it turned out that it's difficult to distinguish the state
> change invoked by the user from the one by the system, which occurs
immediately
> after the boot. I'd like to identify the state change invoked only by the
user.
> 
> How about renaming OnWifiToggled to OnWillToggleWifi? This implies more that
the
> user initiated toggling wifi.

Sure, that sounds great to me.  Thanks!

[stevenjb, 2012-08-02 17:47:16]

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

http://codereview.chromium.org/10824142/diff/9001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:769: return returned_view;
This logic is pretty confusing. I think we would be better setting a bool
'request_wifi_view_' and use that to determine which type of view to create
here.

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

http://codereview.chromium.org/10824142/diff/9001/ash/system/network/tray_net...
ash/system/network/tray_network.h:67: TrayDetailsView* wifi_view_;
Having two different detailed view pointers is pretty confusing. At minimum we
should rename these something like detailed_list_ and detailed_wifi_, but I
think it would be better to create a common base class with a type enum +
accessor, and have a single pointer.

[mazda, 2012-08-02 19:49:16]

Thank you for the comments.
Please take another look.

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

http://codereview.chromium.org/10824142/diff/9001/ash/system/network/tray_net...
ash/system/network/tray_network.cc:769: return returned_view;
On 2012/08/02 17:47:16, stevenjb (chromium) wrote:
> This logic is pretty confusing. I think we would be better setting a bool
> 'request_wifi_view_' and use that to determine which type of view to create
> here.

Done.

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

http://codereview.chromium.org/10824142/diff/9001/ash/system/network/tray_net...
ash/system/network/tray_network.h:67: TrayDetailsView* wifi_view_;
On 2012/08/02 17:47:16, stevenjb (chromium) wrote:
> Having two different detailed view pointers is pretty confusing. At minimum we
> should rename these something like detailed_list_ and detailed_wifi_, but I
> think it would be better to create a common base class with a type enum +
> accessor, and have a single pointer.

Created a common base class and deleted wifi_view_. Thanks.

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

http://codereview.chromium.org/10824142/diff/9001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:696:
tray_->network_observer()->OnWifiToggled(!old_wifi_enabled);
On 2012/08/02 17:15:40, Daniel Erat wrote:
> On 2012/08/02 17:14:30, mazda wrote:
> > On 2012/08/02 14:26:45, Daniel Erat wrote:
> > > Is there any danger of this getting out of sync?
> > > 
> > > Given NetworkLibrary* lib, GetWifiEnabled() looks like it calls
> > > lib->wifi_enabled() and network_menu_->ToggleWifi() calls
> > > lib->EnableWifiNetworkDevice(!lib->wifi_enabled()).
> > > 
> > > Are there any other callers of GetWifiEnabled()?  I'm concerned that given
> > > lib->wifi_enabled() == true, someone could call ToggleWifi(), which would
> > result
> > > in network_observer()->OnWifiToggled(false) getting called, but then
someone
> > > else could call GetWifiEnabled() soon afterward and get true.
> > > 
> > > Would it be safer to cache the most-recently-requested state within
> > > SystemTrayDelegate?  Is there any risk of the toggle requests failing?
> > > 
> > 
> > TrayNetwork calls GetWifiEnabled, but it's called in
> > TrayNetwork::OnNetworkRefresh so it gets the latest state.
> > 
> > I initially tried to cache the wifi state in TrayNetwork and monitor the
> change
> > of the state in TrayNetwork::OnNetworkRefresh. It's more reliable than the
> > current method, but it turned out that it's difficult to distinguish the
state
> > change invoked by the user from the one by the system, which occurs
> immediately
> > after the boot. I'd like to identify the state change invoked only by the
> user.
> > 
> > How about renaming OnWifiToggled to OnWillToggleWifi? This implies more that
> the
> > user initiated toggling wifi.
> 
> Sure, that sounds great to me.  Thanks!

Renamed OnWifiToggled to OnWillToggleWifi.

[stevenjb, 2012-08-02 19:55:55]

Thanks for the changes, LGTM

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

http://codereview.chromium.org/10824142/diff/3003/ash/system/network/tray_net...
ash/system/network/tray_network.cc:206: virtual TrayNetwork::DetailedViewType
view_type() const = 0;
nit: I think this should be named GetViewType() since it is not just returning a
member.

[Daniel Erat, 2012-08-02 20:05:33]

lgtm

http://codereview.chromium.org/10824142/diff/3003/ash/system/network/network_...
File ash/system/network/network_observer.h (right):

http://codereview.chromium.org/10824142/diff/3003/ash/system/network/network_...
ash/system/network/network_observer.h:46: // Called when the user attempted to
toggle Wi-Fi enable/disable.
nit: Mind adding another sentence saying something like "Toggling is
asynchronous and subsequent calls to query the current state may return the old
value"?

[mazda, 2012-08-02 21:01:48]

Thank you for the comments. I fixed the nits.

Ben,
Could you do an OWNERS review for ash_system_tray_delegate.cc?
It looks there is no OWNERS file in the subdirectories containing
ash_system_tray_delegate.cc.

http://codereview.chromium.org/10824142/diff/3003/ash/system/network/network_...
File ash/system/network/network_observer.h (right):

http://codereview.chromium.org/10824142/diff/3003/ash/system/network/network_...
ash/system/network/network_observer.h:46: // Called when the user attempted to
toggle Wi-Fi enable/disable.
On 2012/08/02 20:05:34, Daniel Erat wrote:
> nit: Mind adding another sentence saying something like "Toggling is
> asynchronous and subsequent calls to query the current state may return the
old
> value"?

Done.

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

http://codereview.chromium.org/10824142/diff/3003/ash/system/network/tray_net...
ash/system/network/tray_network.cc:206: virtual TrayNetwork::DetailedViewType
view_type() const = 0;
On 2012/08/02 19:55:56, stevenjb (chromium) wrote:
> nit: I think this should be named GetViewType() since it is not just returning
a
> member.

Done.

[Ben Goodger (Google), 2012-08-03 17:05:57]

lgtm

[commit-bot: I haz the power, 2012-08-03 17:37:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10824142/6004

[commit-bot: I haz the power, 2012-08-03 19:06:05]

Change committed as 149887