Fixed problem with disappearing window controls for applications when maximizing / minimizing / max…

Fixed problem with disappearing window controls for applications when maximizing / minimizing / maximizing


BUG=151632
TEST=unit test to see that the locations of the controls are always at the right side of the screen


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

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

[Mr4D (OOO till 08-26), 2012-09-24 21:01:45]

As discussed. Please have a look!

[sky, 2012-09-24 21:24:16]

http://codereview.chromium.org/10986003/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc (right):

http://codereview.chromium.org/10986003/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:257: gfx::Point
location(bounds.width() - preferred.width(), 0);
control_widget_ is a child widget of the browser. By that I mean
control_widget_->GetNativeView()->parent() == browser_frame->GetNativeWindow().
All callers of this take the return value and do
control_widget_->GetNativeView()->SetBounds(). This means the expected
coordinates of this method are in the coordinate system of the browser frame
passed to the constructor. That isn't quite what you have here. I believe you
want:

return gfx::Rect(width() - preferred.width(), 0,
                 preferred.width(), preferred.height());

If I'm right, be sure to update the header describing what coordinates this
method returns.

[Mr4D (OOO till 08-26), 2012-09-25 14:23:50]

No change done - see comment.

http://codereview.chromium.org/10986003/diff/1/chrome/browser/ui/views/frame/...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc (right):

http://codereview.chromium.org/10986003/diff/1/chrome/browser/ui/views/frame/...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:257: gfx::Point
location(bounds.width() - preferred.width(), 0);
Well - in the case I have seen (and I can reproduce) width() returns 0. You get
to the state when you first maximize, then minimize and then restore. The reason
is (as far as I can see): The window at the time of this call is minimized and
when it gets restored it gets restored to maximized. The parent should be the
browser's window.

The way I see it is that the frame is it's own view and has therefore it's own
size. Debugging through the code confirms that. At the point of creation the
rectangle of the frame view is (0,0 - 0x0). Usually a resize is happening
shortly after which is then caught by the OnSizeChange observer. However - in
the case of the the maximize operation it is a restore to the same size as the
window was before - and thus this call is not made and the observer does not get
called. As such the view size of the frame never changes from (0,0 - 0x0). That
is the reason why I need to get the parent window's size of this frame to which
this view is attached to.

So in other words: The suggested changed does not work (and yes, I have tried it
to make sure).

[sky, 2012-09-25 16:58:34]

My suggested change didn't work because of another bug. 
BrowserFrameAura::WindowPropertyWatcher::OnWindowPropertyChanged needs to pass
true to UpdateFrame if the old state is MINIMIZED.

[Mr4D (OOO till 08-26), 2012-09-25 18:04:02]

Hmm... Well - that is the other route: Force a OnBoundsChanged event to trigger.
Works as well!

Uploaded and tested. Have another look!

[sky, 2012-09-25 18:39:49]

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc
(right):

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:148:
EXPECT_TRUE(window);
ASSERT_TRUE. Use expect if a failure won't be fatal. Here a failure is fatal
since 149 would crash.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:150:
EXPECT_EQ(1280, rect.right());
Why does this need to be 1280 here? The important thing is the window is
maximized and at the edge of the monitor. Which is not necessarily 1280. Same
comment on 166 below.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:154:
content::RunAllPendingInMessageLoop();
Why do you need to runall pending here and below?

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:155:
window = GetChildWindowNamed(
Your test would be more readable if you don't reuse the window variable.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:164:
EXPECT_TRUE(window);
ASSERT_TRUE

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_frame_aura.cc:66: old_state ==
ui::SHOW_STATE_MINIMIZED);
Update comment.

[Mr4D (OOO till 08-26), 2012-09-25 20:30:34]

Please have another look!

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc
(right):

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:148:
EXPECT_TRUE(window);
Ah! Didn't know and will keep it in mind. Done.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:150:
EXPECT_EQ(1280, rect.right());
I thought that this would be fixed for the unit tests and could make it
therefore better readable. Done.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:154:
content::RunAllPendingInMessageLoop();
I did try them before without and it failed. It appears that the creation of the
frame controls (checked with the existence of window) is somehow happening
delayed.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:155:
window = GetChildWindowNamed(
I disagree here - but you get it. Done.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura_browsertest.cc:164:
EXPECT_TRUE(window);
On 2012/09/25 18:39:49, sky wrote:
> ASSERT_TRUE

Done.

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_frame_aura.cc (right):

http://codereview.chromium.org/10986003/diff/1003/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_frame_aura.cc:66: old_state ==
ui::SHOW_STATE_MINIMIZED);
On 2012/09/25 18:39:49, sky wrote:
> Update comment.

Done.

[sky, 2012-09-25 21:42:54]

LGTM

[commit-bot: I haz the power, 2012-09-25 21:43:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10986003/7002

[commit-bot: I haz the power, 2012-09-25 23:51:32]

Change committed as 158692

[commit-bot: I haz the power, 2012-09-26 01:50:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/10986003/12004

[commit-bot: I haz the power, 2012-09-26 05:17:12]

Change committed as 158751