[Ash] Always show minimize button.

[Ash] Always show minimize button.

Previously, windows that are not maximizable would only have
a close button. The window is still minimizable by clicking
on the taskbar icon.
After this change, these windows will have a minimize and
close icon.

BUG=351179

[jackhou1, 2014-09-19 02:06:39]

jackhou@chromium.org changed reviewers:
+ pkotwicz@chromium.org

[jackhou1, 2014-09-19 02:06:39]

pkotwicz, could you take a look?

Do you know what users (if any) of CustomFrameViewAsh actually expect
!CanMaximize to imply !CanMinimize?

I noticed with app windows (native_app_window_views), the window can be
minimized by clicking the icon on the taskbar even if there's no minimize
button, so it doesn't seem to make sense to hide the button.

[pkotwicz, 2014-09-19 03:34:32]

This looks good.

We used to not have a minimize button and the maximize button minimized as well.
I believe that logic is from back then.

There is some discussion on crbug.com/328241 w.r.t whether any apps,
particularly the calculator app, should not have a minimize button. Double
checking with kuscher@ would be useful.

https://codereview.chromium.org/553333007/diff/20001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.cc (right):

https://codereview.chromium.org/553333007/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.cc:360: return
minimize_button_->visible();
Can you please remove this method and its callers?

https://codereview.chromium.org/553333007/diff/20001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view.h (right):

https://codereview.chromium.org/553333007/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view.h:37:
FrameCaptionButtonContainerView(views::Widget* frame);
explicit

https://codereview.chromium.org/553333007/diff/20001/ash/frame/caption_button...
File ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc
(right):

https://codereview.chromium.org/553333007/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:108:
// allowed.
Nit: Remove mention of minimizng being allowed

https://codereview.chromium.org/553333007/diff/20001/ash/frame/caption_button...
ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc:121:
// The minimize button should still be visible when maximizing is disallowed.
How about: "The maximize button should not be visible when maximizing is
disallowed."

[pkotwicz, 2014-09-19 03:37:02]

For the sake of clarity, the CL looks good from my end. However, can you please
double check with kuscher@ whether we want this change (I strongly suspect that
the answer is yes)

[jackhou1, 2014-09-19 03:58:44]

> There is some discussion on crbug.com/328241 w.r.t whether any apps,
> particularly the calculator app, should not have a minimize button. Double
> checking with kuscher@ would be useful.

Interesting. There's also this bug, http://crbug.com/341010, which recommends
adding CanMinimize to views::Widget. Maybe that would be better and we should
keep the logic in FrameCaptionButtonContainerView? Or should we do both?

[jackhou1, 2014-09-19 04:01:14]

jackhou@chromium.org changed reviewers:
+ kuscher@chromium.org

[jackhou1, 2014-09-19 04:01:15]

kuscher, could we get your input on this change? 

Apps which cannot maximize (e.g. Calculator) currently have just a close button.
They will now have a minimize button as well, but still no maximize button. Does
that SGTY? An alternative is to show a disabled maximize button.

[pkotwicz, 2014-09-19 05:38:19]

I think that we should do both. I think that it is ok to assume that the widget
is minimizable for all users of FrameCaptionButtonContainerView. I can't imagine
of a case where we would want to use FrameCaptionButtonContainerView and not
have a minimize button. Unused code paths tend to break over time.

PS: kuscher@ is a product manager. I have not seen him comment on code reviews
(but have also never asked for his feedback in a code review) I have had a lot
of success in asking for his feedback in bugs.

[Kuscher, 2014-09-19 05:44:43]

On 2014/09/19 05:38:19, pkotwicz wrote:
> I think that we should do both. I think that it is ok to assume that the
widget
> is minimizable for all users of FrameCaptionButtonContainerView. I can't
imagine
> of a case where we would want to use FrameCaptionButtonContainerView and not
> have a minimize button. Unused code paths tend to break over time.
> 
> PS: kuscher@ is a product manager. I have not seen him comment on code reviews
> (but have also never asked for his feedback in a code review) I have had a lot
> of success in asking for his feedback in bugs.

I do read them every so often ;) This change LGTM

[pkotwicz, 2014-09-19 06:19:25]

We might actually be depending on the old behavior. I will look in more depth
tomorrow morning. In particular, can you check windows which are opened from the
lock screen use FrameCaptionButtonContainerView? See crbug.com/167386
Sorry for not thinking about this earlier.

[pkotwicz, 2014-09-19 14:28:40]

I have checked and there are windows which use FrameCaptionButtonContainerView
which are accessible from the login screen.
Adding WidgetDelegate::CanMinimize() sounds like a good idea.

Sorry about the confusion

[jackhou1, 2014-09-21 05:52:15]

On 2014/09/19 14:28:40, pkotwicz wrote:
> I have checked and there are windows which use FrameCaptionButtonContainerView
> which are accessible from the login screen.
> Adding WidgetDelegate::CanMinimize() sounds like a good idea.
> 
> Sorry about the confusion

Cool, thanks for looking into this. CL to add CanMinimize is here:
https://codereview.chromium.org/583603004/.

[jackhou1, 2014-09-24 11:20:05]

This change is no longer needed. New CL with the same effect is here:
https://codereview.chromium.org/603523002/