Update Panel mininimize/restore button on strip and expansion state change.

Update Panel mininimize/restore button on strip and expansion state change.

Previously code for each platform was doing it at draw/bounds change/layout.

Also removed nearby code that was obsoleted by overflow strip removal.

BUG=125039, 125054
TEST=minimize/restore panels and drag them to/from dock. verify buttons update.


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

[jennb, 2012-05-02 02:38:29]

This accomplishes the goal of consolidating the button visibility changes. As
for delaying restore button appearing, we could change Panel::SetExpansionState
to use a PostTask to update the buttons in a separate message loop pass. Won't
know if that helps or not until I see the glitch on your machine. Could you
review this in the meanwhile and see if the rest of the changes look ok?

Jenn

[Dmitry Titov, 2012-05-02 03:23:08]

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:172: void Panel::UpdateTitlebarButtons() {
Why not just keep the names here in sync if this is a simple delegate to the
native impl? Also, this method is not about all titlebar buttons.

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:298: // Updates titlebar buttons.
It'd be useful to add more info to the comment so it doesn't just repeat the
name of the method. For example, what aspect is updated and when this is called.
See also comment in panel.cc about naming.

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_frame_view.cc:801: Layout();
Maybe we should layout both buttons in Layout() method, and here only switch the
visibility? Then this method would correspond to its name and purpose - simply
switch the visibility of the buttons.

[jennb, 2012-05-02 19:10:35]

Ready for another look.

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:172: void Panel::UpdateTitlebarButtons() {
On 2012/05/02 03:23:08, Dmitry Titov wrote:
> Why not just keep the names here in sync if this is a simple delegate to the
> native impl? Also, this method is not about all titlebar buttons.

Done.

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/10260028/diff/8001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_frame_view.cc:801: Layout();
On 2012/05/02 03:23:08, Dmitry Titov wrote:
> Maybe we should layout both buttons in Layout() method, and here only switch
the
> visibility? Then this method would correspond to its name and purpose - simply
> switch the visibility of the buttons.

Done.

[Dmitry Titov, 2012-05-02 20:29:15]

lgtm

[commit-bot: I haz the power, 2012-05-02 20:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/10260028/14001

[commit-bot: I haz the power, 2012-05-02 22:39:45]

Change committed as 135012