Add proxy settings to network control at upper row of controls at login screen.

Add proxy settings to network control at upper row of controls at login screen.

BUG=http://crosbug.com/8458
TEST=Manual

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

[Denis Lagno, 2010-11-02 19:21:57]

please take a look

[whywhat, 2010-11-02 19:46:45]

LGTM overall with several nits.

http://codereview.chromium.org/4324001/diff/2001/3001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4324001/diff/2001/3001#newcode10050
chrome/app/generated_resources.grd:10050: <message
name="IDS_LOGIN_STATUSBAR_NETWORK_OPEN_OPTIONS_DIALOG" desc="The menu item in
the network menu button at login screen for opening the options dialog">
Guess it should be smth like IDS_STATUSBAR_NETWORK_OPEN_PROXY_SETTINGS_DIALOG.
The description should tell that it's for opening proxy settings, not just
options dialog.

http://codereview.chromium.org/4324001/diff/2001/3002
File chrome/browser/chromeos/login/background_view.cc (right):

http://codereview.chromium.org/4324001/diff/2001/3002#newcode266
chrome/browser/chromeos/login/background_view.cc:266: if (button_view ==
status_area_->network_view()) {
I think you don't need this check, since true will be returned below.

http://codereview.chromium.org/4324001/diff/2001/3002#newcode277
chrome/browser/chromeos/login/background_view.cc:277: void
BackgroundView::OpenButtonOptions(const views::View* button_view) const {
Perhaps this method should not be const in the first place. You could avoid
mutable dialog and const_cast then.
This would require more files to be changed, but the change is trivial.

http://codereview.chromium.org/4324001/diff/2001/3002#newcode300
chrome/browser/chromeos/login/background_view.cc:300:
proxy_settings_dialog_.reset(NULL);
I think NULL can be omitted.

http://codereview.chromium.org/4324001/diff/2001/3005
File chrome/browser/chromeos/login/proxy_settings_dialog.cc (right):

http://codereview.chromium.org/4324001/diff/2001/3005#newcode14
chrome/browser/chromeos/login/proxy_settings_dialog.cc:14: static const int
kProxySettingsDialogReasonableWidth = 750;
You don't need static for constants in unnamed namespace.

http://codereview.chromium.org/4324001/diff/2001/3005#newcode16
chrome/browser/chromeos/login/proxy_settings_dialog.cc:16: static const int
kProxySettingsDialogReasonableWidthRatio = 0.4;
I think these two constants should be of type double.

http://codereview.chromium.org/4324001/diff/2001/3005#newcode31
chrome/browser/chromeos/login/proxy_settings_dialog.cc:31:
GURL("chrome://settings/proxy?menu=off"),
This should be kProxySettingsURL, right?

http://codereview.chromium.org/4324001/diff/2001/3005#newcode35
chrome/browser/chromeos/login/proxy_settings_dialog.cc:35: std::min(
Could you add a bunch of variables here with self-descriptive names like:
int max_width = screen_bounds.width();
// rename to kProxySettingsDialogMinWidth?
int min_width = kProxySettingsDialogReasonableWidth;
int desired_width = screen_bounds.width() * k...Ratio
int width = std::min(max_width, std::max(min_width, desired_width));
perhaps make a helper function to use for both width and height

http://codereview.chromium.org/4324001/diff/2001/3008
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4324001/diff/2001/3008#newcode605
chrome/browser/chromeos/status/network_menu.cc:605: IsBrowserMode() ?
I think that using 'if' is more readable when ? : takes more than one short
line. Could you use 'if' here?

[Denis Lagno, 2010-11-03 11:16:14]

http://codereview.chromium.org/4324001/diff/2001/3001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4324001/diff/2001/3001#newcode10050
chrome/app/generated_resources.grd:10050: <message
name="IDS_LOGIN_STATUSBAR_NETWORK_OPEN_OPTIONS_DIALOG" desc="The menu item in
the network menu button at login screen for opening the options dialog">
On 2010/11/02 19:46:46, whywhat wrote:
> Guess it should be smth like IDS_STATUSBAR_NETWORK_OPEN_PROXY_SETTINGS_DIALOG.
> The description should tell that it's for opening proxy settings, not just
> options dialog.

Done.

http://codereview.chromium.org/4324001/diff/2001/3002
File chrome/browser/chromeos/login/background_view.cc (right):

http://codereview.chromium.org/4324001/diff/2001/3002#newcode266
chrome/browser/chromeos/login/background_view.cc:266: if (button_view ==
status_area_->network_view()) {
On 2010/11/02 19:46:46, whywhat wrote:
> I think you don't need this check, since true will be returned below.

I think eliminating this check is compiler's job.
From readability POV this check gives explicit hint that given button_view is
handled.

http://codereview.chromium.org/4324001/diff/2001/3002#newcode277
chrome/browser/chromeos/login/background_view.cc:277: void
BackgroundView::OpenButtonOptions(const views::View* button_view) const {
On 2010/11/02 19:46:46, whywhat wrote:
> Perhaps this method should not be const in the first place. You could avoid
> mutable dialog and const_cast then.
> This would require more files to be changed, but the change is trivial.

Done.

http://codereview.chromium.org/4324001/diff/2001/3002#newcode300
chrome/browser/chromeos/login/background_view.cc:300:
proxy_settings_dialog_.reset(NULL);
On 2010/11/02 19:46:46, whywhat wrote:
> I think NULL can be omitted.

Done.

http://codereview.chromium.org/4324001/diff/2001/3005
File chrome/browser/chromeos/login/proxy_settings_dialog.cc (right):

http://codereview.chromium.org/4324001/diff/2001/3005#newcode14
chrome/browser/chromeos/login/proxy_settings_dialog.cc:14: static const int
kProxySettingsDialogReasonableWidth = 750;
On 2010/11/02 19:46:46, whywhat wrote:
> You don't need static for constants in unnamed namespace.

Done.

http://codereview.chromium.org/4324001/diff/2001/3005#newcode16
chrome/browser/chromeos/login/proxy_settings_dialog.cc:16: static const int
kProxySettingsDialogReasonableWidthRatio = 0.4;
On 2010/11/02 19:46:46, whywhat wrote:
> I think these two constants should be of type double.

Done.

http://codereview.chromium.org/4324001/diff/2001/3005#newcode31
chrome/browser/chromeos/login/proxy_settings_dialog.cc:31:
GURL("chrome://settings/proxy?menu=off"),
On 2010/11/02 19:46:46, whywhat wrote:
> This should be kProxySettingsURL, right?

Done.

http://codereview.chromium.org/4324001/diff/2001/3005#newcode35
chrome/browser/chromeos/login/proxy_settings_dialog.cc:35: std::min(
On 2010/11/02 19:46:46, whywhat wrote:
> Could you add a bunch of variables here with self-descriptive names like:
> int max_width = screen_bounds.width();
> // rename to kProxySettingsDialogMinWidth?
> int min_width = kProxySettingsDialogReasonableWidth;
> int desired_width = screen_bounds.width() * k...Ratio
> int width = std::min(max_width, std::max(min_width, desired_width));
> perhaps make a helper function to use for both width and height

Done.

http://codereview.chromium.org/4324001/diff/2001/3008
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4324001/diff/2001/3008#newcode605
chrome/browser/chromeos/status/network_menu.cc:605: IsBrowserMode() ?
On 2010/11/02 19:46:46, whywhat wrote:
> I think that using 'if' is more readable when ? : takes more than one short
> line. Could you use 'if' here?

Done.