[cros] Make disconnected icons used by network drop-down and network button consistent.

[cros] Make disconnected icons used by network drop-down and network button consistent.

BUG=chromium-os:20789
TEST=manual


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

[altimofeev, 2011-11-28 13:41:12]

PTAL
Steven: network icon part
Anton: login WebUI

[stevenjb, 2011-11-28 19:35:44]

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu_icon.h (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_icon.h:45: ConnectionType
last_network_type);
This is non-intuitive in the constructor; my first impression was that it was
limiting the icon somehow. I would rather see a setter added instead.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.h (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.h:26: bool oobe,
ConnectionType last_network_type);
Again, I would rather see a setter added here.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc:87:
static_cast<ConnectionType>(last_network_type)));
Since this is the only place we might set last_network_type to something other
than WIFI, we can just call a setter here if >= 0.

[whywhat, 2011-11-29 07:58:06]

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/network_dropdown.js (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:375: * @param
{string} element_id Id of the element which is network drop-down.
Should be named "elementId"
Would "Id of network drop-down element" be a better description?

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:377: * @param
{integer} last Last active network type.
Could you rename it to smth less general? i.e. lastNetworkType or
lastActiveNetwork?
Also add a comment about -1 as a special value that you use in the next file

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:388: * Deactivates
network drop-down. Deactivating not active drop-down does
not active -> inactive

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:390: * @param
{string} element_id Id of the element which is network drop-down.
Ditto about comment and name

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:395:
chrome.send('networkDropdownHide', []);
nit: I thought you don't need to specify [] explicitly, though I could be wrong
:)

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/screen_error_message.js (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_error_message.js:115: * Shows or
hides offline message based on network on/offline state.
Add comments about the parameters, please.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_error_message.js:117:
updateState_: function(state, network, reason, last) {
Ditto about last

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc:11: #include
"chrome/browser/chromeos/cros/network_library.h"
Should be one line above

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc:80: double
last_network_type = -1;  // Javasript passes integer as double.
typo: Javasript -> Javascript :)
Better to put comment on a separate line

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:138: :
last_network_type_(TYPE_WIFI), web_ui_(web_ui) {
Initialize each member on its own line, please.
I'd initialize state_ as well.

[altimofeev, 2011-11-29 17:10:45]

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu_icon.h (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu_icon.h:45: ConnectionType
last_network_type);
On 2011/11/28 19:35:45, Steven Bennetts wrote:
> This is non-intuitive in the constructor; my first impression was that it was
> limiting the icon somehow. I would rather see a setter added instead.

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/network_dropdown.js (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:375: * @param
{string} element_id Id of the element which is network drop-down.
On 2011/11/29 07:58:06, whywhat wrote:
> Should be named "elementId"
> Would "Id of network drop-down element" be a better description?

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:377: * @param
{integer} last Last active network type.
On 2011/11/29 07:58:06, whywhat wrote:
> Could you rename it to smth less general? i.e. lastNetworkType or
> lastActiveNetwork?
> Also add a comment about -1 as a special value that you use in the next file

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:388: * Deactivates
network drop-down. Deactivating not active drop-down does
On 2011/11/29 07:58:06, whywhat wrote:
> not active -> inactive

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:390: * @param
{string} element_id Id of the element which is network drop-down.
On 2011/11/29 07:58:06, whywhat wrote:
> Ditto about comment and name

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/network_dropdown.js:395:
chrome.send('networkDropdownHide', []);
On 2011/11/29 07:58:06, whywhat wrote:
> nit: I thought you don't need to specify [] explicitly, though I could be
wrong
> :)

You are right, they are not obligatory. But I think usage of '[]' is consistent
with the fact, that C++ handler anyway has ListValue argument.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/screen_error_message.js (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_error_message.js:115: * Shows or
hides offline message based on network on/offline state.
On 2011/11/29 07:58:06, whywhat wrote:
> Add comments about the parameters, please.

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/screen_error_message.js:117:
updateState_: function(state, network, reason, last) {
On 2011/11/29 07:58:06, whywhat wrote:
> Ditto about last

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown.h (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown.h:26: bool oobe,
ConnectionType last_network_type);
On 2011/11/28 19:35:45, Steven Bennetts wrote:
> Again, I would rather see a setter added here.

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc:11: #include
"chrome/browser/chromeos/cros/network_library.h"
On 2011/11/29 07:58:06, whywhat wrote:
> Should be one line above

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc:80: double
last_network_type = -1;  // Javasript passes integer as double.
On 2011/11/29 07:58:06, whywhat wrote:
> typo: Javasript -> Javascript :)
> Better to put comment on a separate line

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/network_dropdown_handler.cc:87:
static_cast<ConnectionType>(last_network_type)));
On 2011/11/28 19:35:45, Steven Bennetts wrote:
> Since this is the only place we might set last_network_type to something other
> than WIFI, we can just call a setter here if >= 0.

Done.

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

http://codereview.chromium.org/8681029/diff/2013/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:138: :
last_network_type_(TYPE_WIFI), web_ui_(web_ui) {
On 2011/11/29 07:58:06, whywhat wrote:
> Initialize each member on its own line, please.
> I'd initialize state_ as well.

Done.

[stevenjb, 2011-11-29 17:38:43]

lgtm

[whywhat, 2011-11-30 07:46:44]

lgtm

[commit-bot: I haz the power, 2011-11-30 09:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/8681029/18001

[commit-bot: I haz the power, 2011-11-30 11:14:27]

Change committed as 112195