[Win Aero] Correctly set the minimum window size.

[Win Aero] Correctly set the minimum window size.

This also fixes the value of [inner|outer]Bounds.[width|height]
to account for the border added by
AppWindowDesktopWindowTreeHostWin::GetClientAreaInsets

BUG=394567, 395929
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289257

[benwells1, 2014-07-22 05:37:52]

https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc (right):

https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc:119: height() -
client_bounds.height());
This looks right. Are there tests which should have caught this when I
introduced the glass app window frame?

[jackhou1, 2014-07-23 03:36:04]

https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
File chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc (right):

https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc:119: height() -
client_bounds.height());
On 2014/07/22 05:37:52, benwells1 wrote:
> This looks right. Are there tests which should have caught this when I
> introduced the glass app window frame?

The API is covered in platform_apps/window_api/test.js, but the bug is that the
size we pass to WM_GETMINMAXINFO is wrong. So we'd have to have a platform
specific test in order to cover this.

[benwells, 2014-07-23 08:08:39]

On 2014/07/23 03:36:04, jackhou1 wrote:
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> File chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc (right):
> 
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc:119: height()
-
> client_bounds.height());
> On 2014/07/22 05:37:52, benwells1 wrote:
> > This looks right. Are there tests which should have caught this when I
> > introduced the glass app window frame?
> 
> The API is covered in platform_apps/window_api/test.js, but the bug is that
the
> size we pass to WM_GETMINMAXINFO is wrong. So we'd have to have a platform
> specific test in order to cover this.

Ah right. I read the bug wrong, I assumed step 2 in the bug referred to the min
/ max returned by the API, but I guess it meant the *actual* min and max of the
window.

I wonder if we can test this better. E.g. have a platform specific function to
get the actual min / max, and then check that setting the outer min / max always
sets that correctly no matter the frame type / platform.

[jackhou1, 2014-07-24 01:25:14]

On 2014/07/23 08:08:39, benwells wrote:
> On 2014/07/23 03:36:04, jackhou1 wrote:
> >
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> > File chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc
(right):
> > 
> >
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> > chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc:119:
height()
> -
> > client_bounds.height());
> > On 2014/07/22 05:37:52, benwells1 wrote:
> > > This looks right. Are there tests which should have caught this when I
> > > introduced the glass app window frame?
> > 
> > The API is covered in platform_apps/window_api/test.js, but the bug is that
> the
> > size we pass to WM_GETMINMAXINFO is wrong. So we'd have to have a platform
> > specific test in order to cover this.
> 
> Ah right. I read the bug wrong, I assumed step 2 in the bug referred to the
min
> / max returned by the API, but I guess it meant the *actual* min and max of
the
> window.
> 
> I wonder if we can test this better. E.g. have a platform specific function to
> get the actual min / max, and then check that setting the outer min / max
always
> sets that correctly no matter the frame type / platform.

The way we tell the OS about minimum/maximum sizes varies quite a bit between
OSes. I think the right way to test it is probably a unittest on the
*TreeHost/View, otherwise we'd need boilerplate to unify it. Or possibly we
could just have QA tests, which is how NativeAppWindowCocoa is mostly tested.

I've added a test that catches the off-by-one bug in
GlassAppWindowFrameViewWin::GetWindowBoundsForClientBounds.

[benwells, 2014-08-05 22:23:42]

On 2014/07/24 01:25:14, jackhou1 wrote:
> On 2014/07/23 08:08:39, benwells wrote:
> > On 2014/07/23 03:36:04, jackhou1 wrote:
> > >
> >
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> > > File chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> > > chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc:119:
> height()
> > -
> > > client_bounds.height());
> > > On 2014/07/22 05:37:52, benwells1 wrote:
> > > > This looks right. Are there tests which should have caught this when I
> > > > introduced the glass app window frame?
> > > 
> > > The API is covered in platform_apps/window_api/test.js, but the bug is
that
> > the
> > > size we pass to WM_GETMINMAXINFO is wrong. So we'd have to have a platform
> > > specific test in order to cover this.
> > 
> > Ah right. I read the bug wrong, I assumed step 2 in the bug referred to the
> min
> > / max returned by the API, but I guess it meant the *actual* min and max of
> the
> > window.
> > 
> > I wonder if we can test this better. E.g. have a platform specific function
to
> > get the actual min / max, and then check that setting the outer min / max
> always
> > sets that correctly no matter the frame type / platform.
> 
> The way we tell the OS about minimum/maximum sizes varies quite a bit between
> OSes. I think the right way to test it is probably a unittest on the
> *TreeHost/View, otherwise we'd need boilerplate to unify it. Or possibly we
> could just have QA tests, which is how NativeAppWindowCocoa is mostly tested.
> 
> I've added a test that catches the off-by-one bug in
> GlassAppWindowFrameViewWin::GetWindowBoundsForClientBounds.

Sorry I can't remember if you had more things to do for this CL or not - last I
remember we talked about it and you were investigating something. Not sure if
that was before or after your last message.

Is this ready to review?

[jackhou1, 2014-08-06 01:24:13]

On 2014/08/05 22:23:42, benwells wrote:
> On 2014/07/24 01:25:14, jackhou1 wrote:
> > On 2014/07/23 08:08:39, benwells wrote:
> > > On 2014/07/23 03:36:04, jackhou1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> > > > File chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/405323002/diff/1/chrome/browser/ui/views/apps...
> > > > chrome/browser/ui/views/apps/glass_app_window_frame_view_win.cc:119:
> > height()
> > > -
> > > > client_bounds.height());
> > > > On 2014/07/22 05:37:52, benwells1 wrote:
> > > > > This looks right. Are there tests which should have caught this when I
> > > > > introduced the glass app window frame?
> > > > 
> > > > The API is covered in platform_apps/window_api/test.js, but the bug is
> that
> > > the
> > > > size we pass to WM_GETMINMAXINFO is wrong. So we'd have to have a
platform
> > > > specific test in order to cover this.
> > > 
> > > Ah right. I read the bug wrong, I assumed step 2 in the bug referred to
the
> > min
> > > / max returned by the API, but I guess it meant the *actual* min and max
of
> > the
> > > window.
> > > 
> > > I wonder if we can test this better. E.g. have a platform specific
function
> to
> > > get the actual min / max, and then check that setting the outer min / max
> > always
> > > sets that correctly no matter the frame type / platform.
> > 
> > The way we tell the OS about minimum/maximum sizes varies quite a bit
between
> > OSes. I think the right way to test it is probably a unittest on the
> > *TreeHost/View, otherwise we'd need boilerplate to unify it. Or possibly we
> > could just have QA tests, which is how NativeAppWindowCocoa is mostly
tested.
> > 
> > I've added a test that catches the off-by-one bug in
> > GlassAppWindowFrameViewWin::GetWindowBoundsForClientBounds.
> 
> Sorry I can't remember if you had more things to do for this CL or not - last
I
> remember we talked about it and you were investigating something. Not sure if
> that was before or after your last message.
> 
> Is this ready to review?

Nope, sorry for the lack of communication. I've written some tests, and they
indicate that the fix in this CL don't work. I'm still figuring out how to get
this to work.

[jackhou1, 2014-08-11 04:19:20]

This is ready for review now.

[benwells, 2014-08-12 03:51:41]

https://codereview.chromium.org/405323002/diff/80001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/405323002/diff/80001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:343:
ExtensionTestMessageListener launched_listener("Launched", true);
Nit: You don't need this listener. Just call LoadAndLaunchPlatformApp with
"Launched" instead of &launched_listener.

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/outer_bounds/test.js (right):

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/outer_bounds/test.js:7: //    frame:
{color: '#ff0000'},
Can we test for no frame, standard frame and colored frame? You should be able
to do this with the one test.

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/outer_bounds/test.js:18: }, function
() {});
Nit: You can send the Launched message in this callback instead of having a
main.js file.

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/window_api_interactive/test.js
(right):

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/window_api_interactive/test.js:59:
//frame: {color: '#ff0000'},
Ditto about the three tests.

[jackhou1, 2014-08-12 06:12:08]

https://codereview.chromium.org/405323002/diff/80001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_interactive_uitest.cc (right):

https://codereview.chromium.org/405323002/diff/80001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_interactive_uitest.cc:343:
ExtensionTestMessageListener launched_listener("Launched", true);
On 2014/08/12 03:51:40, benwells wrote:
> Nit: You don't need this listener. Just call LoadAndLaunchPlatformApp with
> "Launched" instead of &launched_listener.

Done.

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/outer_bounds/test.js (right):

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/outer_bounds/test.js:7: //    frame:
{color: '#ff0000'},
On 2014/08/12 03:51:40, benwells wrote:
> Can we test for no frame, standard frame and colored frame? You should be able
> to do this with the one test.

Done.

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/outer_bounds/test.js:18: }, function
() {});
On 2014/08/12 03:51:40, benwells wrote:
> Nit: You can send the Launched message in this callback instead of having a
> main.js file.

Done.

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/window_api_interactive/test.js
(right):

https://codereview.chromium.org/405323002/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/window_api_interactive/test.js:59:
//frame: {color: '#ff0000'},
On 2014/08/12 03:51:41, benwells wrote:
> Ditto about the three tests.

Done.

[benwells, 2014-08-13 05:54:50]

lgtm

[jackhou1, 2014-08-13 05:59:27]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-08-13 06:01:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/405323002/140001

[commit-bot: I haz the power, 2014-08-13 10:25:15]

Change committed as 289257