Add chrome.app.window.[get|set][Min|Max][Width|Height]

Add chrome.app.window.[get|set][Min|Max][Width|Height]

For API proposal: http://goo.gl/nd2Pc2

This implements the bindings for the API.

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

[tapted, 2013-10-16 23:31:51]

lgtm, I'm not too intimate with the API bindings logic though (but it seems
right).

https://codereview.chromium.org/25449002/diff/25001/chrome/browser/extensions...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/25449002/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:144:
min_size.set_width(params->min_width.get() ? *(params->min_width) : 0);
maybe-nit: 0 -> SizeConstraints::kUnboundedSize ?

I see an argument for 0 too though, since actually setting 0 is fine for setting
unbounded.

https://codereview.chromium.org/25449002/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/25449002/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:104: const
ShellWindow::SizeConstraints& size_constraints =
Hm. this is identical to the code in AppWindowContents::NativeWindowChanged,
same for bounds/fullscreen/minimized/maximized. I'd support them being merged
and made consistent, but that's probably orthogonal to this CL.

[jackhou1, 2013-10-17 03:56:49]

https://codereview.chromium.org/25449002/diff/25001/chrome/browser/extensions...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/25449002/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:144:
min_size.set_width(params->min_width.get() ? *(params->min_width) : 0);
On 2013/10/16 23:31:52, tapted wrote:
> maybe-nit: 0 -> SizeConstraints::kUnboundedSize ?
> 
> I see an argument for 0 too though, since actually setting 0 is fine for
setting
> unbounded.

Done.

[jackhou1, 2013-10-17 03:57:34]

scheib, please review all

[jackhou1, 2013-10-21 09:02:22]

scheib: ping.

[benwells, 2013-10-21 21:18:57]

I'll take a look ...

[benwells, 2013-10-21 21:30:05]

https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:177: // Get the current minimum
width of the window.
For all of these, document what is returned if there is no min or max.

https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:190: static void
setMinWidth(optional long minWidth);
For all of these, document how to clear the min or max.

[mlamouri (slow - plz ping), 2013-10-22 03:40:54]

On 2013/10/21 21:30:05, benwells wrote:
>
https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
> File chrome/common/extensions/api/app_window.idl (right):
> 
>
https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
> chrome/common/extensions/api/app_window.idl:177: // Get the current minimum
> width of the window.
> For all of these, document what is returned if there is no min or max.
> 
>
https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
> chrome/common/extensions/api/app_window.idl:190: static void
> setMinWidth(optional long minWidth);
> For all of these, document how to clear the min or max.

Wouldn't be better to have attributes instead of setter/getter here? Given that
everything seems to be sync there, using attributes might be more developer
friendly. We could then clear the value by setting null and/or undefined
(depends on WebIDL and EcmaScript rules I guess).

[jackhou1, 2013-10-22 05:20:29]

> Wouldn't be better to have attributes instead of setter/getter here? Given
that
> everything seems to be sync there, using attributes might be more developer
> friendly. We could then clear the value by setting null and/or undefined
> (depends on WebIDL and EcmaScript rules I guess).

This would probably work, but the existing API uses get/set (e.g. getBounds), so
I think we'd want to keep them consistent.

https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:177: // Get the current minimum
width of the window.
On 2013/10/21 21:30:06, benwells wrote:
> For all of these, document what is returned if there is no min or max.

Done.

https://codereview.chromium.org/25449002/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:190: static void
setMinWidth(optional long minWidth);
On 2013/10/21 21:30:06, benwells wrote:
> For all of these, document how to clear the min or max.

Done.

[benwells, 2013-10-22 21:03:02]

lgtm

https://codereview.chromium.org/25449002/diff/132001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/25449002/diff/132001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:193: // Set the current minimum
width of the window. Set to |undefined| to remove
Nit: document as set to null or call without a parameter. Setting something to
undefined is a bit weird.

[jackhou1, 2013-10-23 01:54:14]

https://codereview.chromium.org/25449002/diff/132001/chrome/common/extensions...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/25449002/diff/132001/chrome/common/extensions...
chrome/common/extensions/api/app_window.idl:193: // Set the current minimum
width of the window. Set to |undefined| to remove
On 2013/10/22 21:03:02, benwells wrote:
> Nit: document as set to null or call without a parameter. Setting something to
> undefined is a bit weird.

Done.

[commit-bot: I haz the power, 2013-10-30 04:12:15]

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

[commit-bot: I haz the power, 2013-10-30 09:29:26]

Change committed as 231780