Fixed extension/app window maximum size calculation on Windows.

Fixed extension/app window maximum size calculation on Windows.

Previously the height of window title bars was added to the maximum
content size of windows only if there was a maximum width as well as a
maximum height. This fixes that so it's possible to specify maxHeight
without specifying maxWidth to chrome.app.window.create().

BUG=246289
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203962

[dharcourt, 2013-06-03 22:35:25]

Vincent, could you review this fix when you get a chance?

Peter, could you do an OWNERS review of this 6 line change to
chrome/browser/ui/views/extensions/shell_window_frame_view.cc?

Thanks!

    - Charles

https://codereview.chromium.org/15969028/diff/2001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_frame_view.cc (left):

https://codereview.chromium.org/15969028/diff/2001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_frame_view.cc:349: return
max_size;
This logic already exists in GetBoundsForClientView(), removing to avoid
duplication and decrease the risk of inconsistencies in the future.

https://codereview.chromium.org/15969028/diff/2001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_frame_view.cc:351: if
(!max_size.IsEmpty()) {
When max_size.height() is non-zero it should be adjusted even if
max_size.width() is zero, but max_size.IsEmpty() will return true whenever
max_size.width() is zero.

[jeremya, 2013-06-03 22:43:15]

lgtm, though won't this code be removed when native frames get turned on?

[Peter Kasting, 2013-06-03 22:48:43]

This change seems fine as far as it goes, but what about

* Enlarging the width for the client x, parallel to what you're doing for y
* Enlarging for any width/height of the frame _after_ the content area (i.e.
below/right), not just before

Are these somehow happening elsewhere?

[jeremya, 2013-06-03 23:01:30]

On 2013/06/03 22:48:43, Peter Kasting wrote:
> This change seems fine as far as it goes, but what about
> 
> * Enlarging the width for the client x, parallel to what you're doing for y
> * Enlarging for any width/height of the frame _after_ the content area (i.e.
> below/right), not just before
> 
> Are these somehow happening elsewhere?

The frame doesn't have any border on the left, right or bottom.

[Peter Kasting, 2013-06-03 23:05:25]

On 2013/06/03 23:01:30, jeremya wrote:
> On 2013/06/03 22:48:43, Peter Kasting wrote:
> > This change seems fine as far as it goes, but what about
> > 
> > * Enlarging the width for the client x, parallel to what you're doing for y
> > * Enlarging for any width/height of the frame _after_ the content area (i.e.
> > below/right), not just before
> > 
> > Are these somehow happening elsewhere?
> 
> The frame doesn't have any border on the left, right or bottom.

OK.  Can we DCHECK that or something to make it clear to readers why we don't
need to adjust for it?

LGTM

[scheib, 2013-06-03 23:35:52]

Great to have the fix. LGTM with comment:

https://codereview.chromium.org/15969028/diff/2001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_frame_view.cc (right):

https://codereview.chromium.org/15969028/diff/2001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_frame_view.cc:350:
max_size.Enlarge(0, GetBoundsForClientView().y());
Please comment here to address Peter's Q and the answer, e.g.:

// Add to the client size the height of the frame caption by measuring the
offset to the client view.

[dharcourt, 2013-06-03 23:38:13]

On 2013/06/03 23:05:25, Peter Kasting wrote:
> On 2013/06/03 23:01:30, jeremya wrote:
> > On 2013/06/03 22:48:43, Peter Kasting wrote:
> > > This change seems fine as far as it goes, but what about
> > > 
> > > * Enlarging the width for the client x, parallel to what you're doing for
y
> > > * Enlarging for any width/height of the frame _after_ the content area
(i.e.
> > > below/right), not just before
> > > 
> > > Are these somehow happening elsewhere?
> > 
> > The frame doesn't have any border on the left, right or bottom.
> 
> OK.  Can we DCHECK that or something to make it clear to readers why we don't
> need to adjust for it?
> 
> LGTM

It was simpler to support a possible bottom border than to DCHECK that there was
no such border, so I did that. I did DCHECK that there's no left and right
borders, but it would add only one line to support left and right borders and
avoid the vertical-border-only dependency of this code. Let me know if you'd
rather I do that.

[dharcourt, 2013-06-03 23:39:09]

On 2013/06/03 22:43:15, jeremya wrote:
> lgtm, though won't this code be removed when native frames get turned on?

I'm still coming up to speed with your native frames work, but it seems that
this would still be used for frame: "chrome".

[commit-bot: I haz the power, 2013-06-03 23:39:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/15969028/4002

[Peter Kasting, 2013-06-03 23:51:03]

https://codereview.chromium.org/15969028/diff/4002/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_frame_view.cc (right):

https://codereview.chromium.org/15969028/diff/4002/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_frame_view.cc:350:
DCHECK(client_size.width() == width());
Yeah, let's just go ahead and support non-zero border widths too, it's easy and
clear and future-proof.

[dharcourt, 2013-06-04 00:17:39]

https://codereview.chromium.org/15969028/diff/2001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_frame_view.cc (right):

https://codereview.chromium.org/15969028/diff/2001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_frame_view.cc:350:
max_size.Enlarge(0, GetBoundsForClientView().y());
On 2013/06/03 23:35:53, scheib wrote:
> Please comment here to address Peter's Q and the answer, e.g.:
> 
> // Add to the client size the height of the frame caption by measuring the
> offset to the client view.

Done.

https://codereview.chromium.org/15969028/diff/4002/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_frame_view.cc (right):

https://codereview.chromium.org/15969028/diff/4002/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_frame_view.cc:350:
DCHECK(client_size.width() == width());
On 2013/06/03 23:51:03, Peter Kasting wrote:
> Yeah, let's just go ahead and support non-zero border widths too, it's easy
and
> clear and future-proof.

Done.

[commit-bot: I haz the power, 2013-06-04 00:18:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/15969028/9002

[jeremya, 2013-06-04 02:00:18]

On 2013/06/03 23:39:09, dharcourt wrote:
> On 2013/06/03 22:43:15, jeremya wrote:
> > lgtm, though won't this code be removed when native frames get turned on?
> 
> I'm still coming up to speed with your native frames work, but it seems that
> this would still be used for frame: "chrome".

iirc the plan was to have frame: 'chrome' just turn into native windows, but
that might have changed :) in any case it would still be called for frameless
windows.

If native frames don't end up going ahead, there's a problem where in
non-composited contexts (e.g. winxp, "Basic" mode in win7/vista, and
remote-desktop) app windows don't have a distinguishable border
(http://crbug.com/158266). So in those cases it would be good to add a 1px
border to all sides of the window, which would make this code useful :)

[commit-bot: I haz the power, 2013-06-04 07:45:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dharcourt@chromium.org/15969028/9002

[commit-bot: I haz the power, 2013-06-04 14:06:12]

Change committed as 203962