Use signal strength of detected networks when displaying remembered networks.

Use signal strength of detected networks when displaying remembered networks.


BUG=chromium-os:9355
TEST=Viewed changes on netbook.

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

[falken1, 2011-01-21 09:48:07]

Proposed fix for chromium-os:9355.

[stevenjb, 2011-01-21 17:11:18]

http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1008:
cros->wifi_networks();
Since we don't use "live_wifi_networks" elsewhere I would suggest renaming
wifi_networks -> remembered_wifi_networks and live_wifi_networks ->
wifi_networks.

http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1019: break;
Need to compare both name() and encryption(). We should probably have a
WifiNetwork method to do the comparison.

http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1031:
*rb.GetBitmapNamed(IDR_STATUSBAR_NETWORK_SECURE));
Does the secure icon trump the signal strength icon? Either way, the if
(encrypted) clause should be part of the if statement above. i.e. if (encrypted)
... else if (!found) ... else ...

[Charlie Lee, 2011-01-21 19:21:09]

http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1011:
wifi_networks.begin(); it != wifi_networks.end(); ++it) {
I would recommend instead of doing this in a double for loop O(nm), do it using
a hashmap O(n+m). So go through the "live" wifi networks and put each network in
the hashmap keyed by ssid+encryption. Then go through the remembered wifi
networks, and look for it in the hashmap. This will make it much more efficient
when there's a lot of wifi and remembered networks.

http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1031:
*rb.GetBitmapNamed(IDR_STATUSBAR_NETWORK_SECURE));
Isn't the secure icon just a badge? If so, then we should set the icon to that.

On 2011/01/21 17:11:18, Steven Bennetts wrote:
> Does the secure icon trump the signal strength icon? Either way, the if
> (encrypted) clause should be part of the if statement above. i.e. if
(encrypted)
> ... else if (!found) ... else ...

[falken, 2011-01-25 05:36:17]

Thank you for the comments, Steven and Charlie.

I have revised the patch based on your comments.  Regarding the secure icon, as
Charlie mentioned, it is just a badge.  Since the "live" network has the same
encrypted type as the remembered network, I just set the badge according to the
remembered network.

On 2011/01/21 19:21:09, Charlie Lee wrote:
>
http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
> File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):
> 
>
http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1011:
> wifi_networks.begin(); it != wifi_networks.end(); ++it) {
> I would recommend instead of doing this in a double for loop O(nm), do it
using
> a hashmap O(n+m). So go through the "live" wifi networks and put each network
in
> the hashmap keyed by ssid+encryption. Then go through the remembered wifi
> networks, and look for it in the hashmap. This will make it much more
efficient
> when there's a lot of wifi and remembered networks.
> 
>
http://codereview.chromium.org/6343004/diff/1/chrome/browser/chromeos/dom_ui/...
> chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1031:
> *rb.GetBitmapNamed(IDR_STATUSBAR_NETWORK_SECURE));
> Isn't the secure icon just a badge? If so, then we should set the icon to
that.
> 
> On 2011/01/21 17:11:18, Steven Bennetts wrote:
> > Does the secure icon trump the signal strength icon? Either way, the if
> > (encrypted) clause should be part of the if statement above. i.e. if
> (encrypted)
> > ... else if (!found) ... else ...

[Charlie Lee, 2011-01-25 17:56:07]

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1001: std::string
GetWifiNetworkKey(const chromeos::WifiNetwork* wifi)
parenthesis should be on the same line. also, I suggest renaming this to
GetWifiUniqueIdentifier or GetWifiUniqueName... something like that. network key
is interchangeable with password in some places, so avoid that.

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1025: ; it !=
wifi_networks.end(); ++it) {
I believe the ; should be at the end of the previous line.

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1049: icon =
chromeos::NetworkMenu::IconForDisplay(icon,
spacing

[stevenjb, 2011-01-25 18:36:55]

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1004: ss <<
wifi->encryption() << "|" << EscapePath(wifi->name());
stringsteam is unnecessary, and I'm not clear why we need EscapePath() here. I
think this can just be:
return wifi->encryption() + "|" + wifi->name();
(If there is a reason for EscapePath() that I missed, we should document it).

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1031: ++rit) {
Either put the second clause on its own line and fix the indentation of the
first clause:
  for (chromeos::WifiNetworkVector::const_iterator rit =
           remembered_wifi_networks.begin();
       rit != remembered_wifi_networks.end(); ++rit()) {
or declare rit above like so:
chromeos::WifiNetworkVector::const_iterator rit;
for (rit = remembered_wifi_networks.begin();
     rit != remembered_wifi_networks.end(); ++rit) {

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1034: =
wifi_map.find(GetWifiNetworkKey(*rit));
= should be on first line.

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1048: if
((*rit)->encrypted()) {
nit: since (*rit) is used a bunch here, might be more readable to assign it to a
local variable, i.e.
    chromeos::WifiNetwork* wifi = *rit;

[falken, 2011-01-26 05:49:20]

Thank you again for the comments, Charlie and Steven.  I've uploaded a new patch
incorporating your feedback.  (And I've given the coding style guidelines a
closer look!)

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1001: std::string
GetWifiNetworkKey(const chromeos::WifiNetwork* wifi)
I see what you mean about network key.  I don't understand which parenthesis
you're referring to, though..
On 2011/01/25 17:56:07, Charlie Lee wrote:
> parenthesis should be on the same line. also, I suggest renaming this to
> GetWifiUniqueIdentifier or GetWifiUniqueName... something like that. network
key
> is interchangeable with password in some places, so avoid that.

http://codereview.chromium.org/6343004/diff/6001/chrome/browser/chromeos/dom_...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1004: ss <<
wifi->encryption() << "|" << EscapePath(wifi->name());
Oops, for some reason I was thinking we had to escape in case name has a '|' but
now I see it wouldn't matter.
On 2011/01/25 18:36:55, Steven Bennetts wrote:
> stringsteam is unnecessary, and I'm not clear why we need EscapePath() here. I
> think this can just be:
> return wifi->encryption() + "|" + wifi->name();
> (If there is a reason for EscapePath() that I missed, we should document it).

[Charlie Lee, 2011-01-26 17:57:36]

LGTM with a couple of nits.

http://codereview.chromium.org/6343004/diff/12001/chrome/browser/chromeos/dom...
File chrome/browser/chromeos/dom_ui/internet_options_handler.cc (right):

http://codereview.chromium.org/6343004/diff/12001/chrome/browser/chromeos/dom...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1004: {
Sorry, I was talking about this open brace. It should go on the previous line.

http://codereview.chromium.org/6343004/diff/12001/chrome/browser/chromeos/dom...
chrome/browser/chromeos/dom_ui/internet_options_handler.cc:1051: icon,
nit: either move "icon," to the previous line, or combine it with the line below
to save space.

[stevenjb, 2011-01-26 18:02:24]

lgtm

[falken1, 2011-01-27 01:55:55]

Thanks again for the feedback.  I fixed the minor formatting issues in a new
patch.

[satorux1, 2011-01-27 02:03:48]

I'll be submitting this on behalf of falken.

On 2011/01/27 01:55:55, falken1 wrote:
> Thanks again for the feedback.  I fixed the minor formatting issues in a new
> patch.