Aligns the network menu width with combo box width.

Aligns the network menu width with combo box width.

BUG=chromium-os:7100
TEST=Boot device. On "Select network" screen click on the network
selection combo box. The networks menu that will appear has the same
width as combo box itself.

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

[Nikita (slow), 2010-10-01 08:19:11]

LGTM

http://codereview.chromium.org/3586005/diff/1/2
File chrome/browser/chromeos/login/network_selection_view.cc (right):

http://codereview.chromium.org/3586005/diff/1/2#newcode302
chrome/browser/chromeos/login/network_selection_view.cc:302:
((NetworkMenu*)network_dropdown_)->set_menu_offset(kMenuHorizontalOffset,
Why do you need casting here?
set_menu_offset has same public access as SetFirstLevelMenuWidth.

http://codereview.chromium.org/3586005/diff/1/3
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/3586005/diff/1/3#newcode248
chrome/browser/chromeos/status/network_menu.cc:248: if (min_width_ > 0)
Negative min_width_ 0 are treated the same?

[altimofeev, 2010-10-01 08:33:22]

http://codereview.chromium.org/3586005/diff/1/2
File chrome/browser/chromeos/login/network_selection_view.cc (right):

http://codereview.chromium.org/3586005/diff/1/2#newcode302
chrome/browser/chromeos/login/network_selection_view.cc:302:
((NetworkMenu*)network_dropdown_)->set_menu_offset(kMenuHorizontalOffset,
On 2010/10/01 08:19:11, Nikita Kostylev wrote:
> Why do you need casting here?
> set_menu_offset has same public access as SetFirstLevelMenuWidth.

Multiple inheritance for NetworkDropdownButton. NetworkMenu and
views::MenuButton both have this method.

http://codereview.chromium.org/3586005/diff/1/3
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/3586005/diff/1/3#newcode248
chrome/browser/chromeos/status/network_menu.cc:248: if (min_width_ > 0)
On 2010/10/01 08:19:11, Nikita Kostylev wrote:
> Negative min_width_ 0 are treated the same?

Didn't get the question, but made condition more strong.

[Nikita (slow), 2010-10-01 08:49:09]

http://codereview.chromium.org/3586005/diff/1/2
File chrome/browser/chromeos/login/network_selection_view.cc (right):

http://codereview.chromium.org/3586005/diff/1/2#newcode302
chrome/browser/chromeos/login/network_selection_view.cc:302:
((NetworkMenu*)network_dropdown_)->set_menu_offset(kMenuHorizontalOffset,
On 2010/10/01 08:33:22, altimofeev wrote:
> On 2010/10/01 08:19:11, Nikita Kostylev wrote:
> > Why do you need casting here?
> > set_menu_offset has same public access as SetFirstLevelMenuWidth.
> 
> Multiple inheritance for NetworkDropdownButton. NetworkMenu and
> views::MenuButton both have this method. 
> 
> 

If they essentially do the same, it should be single method.
If they do different things I'd rename one of the methods.
Having casting here is wrong.

http://codereview.chromium.org/3586005/diff/6001/7002
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/3586005/diff/6001/7002#newcode248
chrome/browser/chromeos/status/network_menu.cc:248: if (min_width_ != -1)
I don't think setting width to negative value makes sense.
I wasn't sure whether setting to 0 makes sense either (will it work?).

min_width_ > 0 or
mid_width_ >= 0

[altimofeev, 2010-10-01 10:05:15]

http://codereview.chromium.org/3586005/diff/1/2
File chrome/browser/chromeos/login/network_selection_view.cc (right):

http://codereview.chromium.org/3586005/diff/1/2#newcode302
chrome/browser/chromeos/login/network_selection_view.cc:302:
((NetworkMenu*)network_dropdown_)->set_menu_offset(kMenuHorizontalOffset,
On 2010/10/01 08:49:09, Nikita Kostylev wrote:
> On 2010/10/01 08:33:22, altimofeev wrote:
> > On 2010/10/01 08:19:11, Nikita Kostylev wrote:
> > > Why do you need casting here?
> > > set_menu_offset has same public access as SetFirstLevelMenuWidth.
> > 
> > Multiple inheritance for NetworkDropdownButton. NetworkMenu and
> > views::MenuButton both have this method. 
> > 
> > 
> 
> If they essentially do the same, it should be single method.
> If they do different things I'd rename one of the methods.
> Having casting here is wrong.

Probably, NetworkDropdownButton class should be rewriten to avoid multiple
inheritance from more than one non-interface classes, which is forbidden by the
Style Guide. So what about adding TODO here and changing the class
implementation in next CL?

http://codereview.chromium.org/3586005/diff/6001/7002
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/3586005/diff/6001/7002#newcode248
chrome/browser/chromeos/status/network_menu.cc:248: if (min_width_ != -1)
On 2010/10/01 08:49:09, Nikita Kostylev wrote:
> I don't think setting width to negative value makes sense.
> I wasn't sure whether setting to 0 makes sense either (will it work?).
> 
> min_width_ > 0 or
> mid_width_ >= 0

It depends on the Menu realization, since we simple pass the value. Current
situation is that calling SetMinimumWidth(0) brings to 0-width menu (simple
strip is shown) which is different from the expected behavior (set minimum !=
set width). So my intention is to delegate decision part to real menu as much as
possible. Checking only the value of '-1' looks like the least change to the
passing pipeline.

[altimofeev, 2010-10-01 12:23:16]

Deleted 'set_menu_offset' method for NetworkMenu.

[Nikita (slow), 2010-10-01 12:37:24]

LGTM

http://codereview.chromium.org/3586005/diff/6001/7002
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/3586005/diff/6001/7002#newcode248
chrome/browser/chromeos/status/network_menu.cc:248: if (min_width_ != -1)
On 2010/10/01 10:05:15, altimofeev wrote:
> On 2010/10/01 08:49:09, Nikita Kostylev wrote:
> > I don't think setting width to negative value makes sense.
> > I wasn't sure whether setting to 0 makes sense either (will it work?).
> > 
> > min_width_ > 0 or
> > mid_width_ >= 0
> 
> It depends on the Menu realization, since we simple pass the value. Current
> situation is that calling SetMinimumWidth(0) brings to 0-width menu (simple
> strip is shown) which is different from the expected behavior (set minimum !=
> set width). So my intention is to delegate decision part to real menu as much
as
> possible. Checking only the value of '-1' looks like the least change to the
> passing pipeline.

Short comment on that will be helpful here.

http://codereview.chromium.org/3586005/diff/12001/13001
File chrome/browser/chromeos/login/network_selection_view.cc (right):

http://codereview.chromium.org/3586005/diff/12001/13001#newcode305
chrome/browser/chromeos/login/network_selection_view.cc:305: const int
kDefaultMenuOffsetX = -2;
Please make those constants public in MenuButton then.