Create windows for new app window bounds API

Create windows for new app window bounds API

Added create options "innerBounds" and "outerBounds" for
the new app window bounds API.

BUG=315471, 349397
TEST=browser_tests (AppWindowAPITest.*)

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

[tmdiep, 2014-03-04 03:13:13]

This patch handles window creation for the new bounds API. It turned out to be a
little complicated, but hopefully logical.

benwells: For general review.
tapted: For cocoa and general review if you're interested.
Thanks in advance.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc
File apps/app_window.cc (left):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#oldco...
apps/app_window.cc:861: bounds.Inset(native_app_window_->GetFrameInsets());
If we store the content bounds in the window geometry cache, during the
restoration of the bounds, AdjustBoundsToBeVisibleOnScreen() will position the
content wrt the screen. You can potentially end up with the title bar off the
screen, so it's better to work with frame bounds, not content bounds when
positioning windows.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#oldco...
apps/app_window.cc:936: SizeConstraints size_constraints(params.minimum_size,
params.maximum_size);
Because the API lets you specify content or window constraints, we may not be
able to resolve the constraints until we have the frame insets, so this is now
done later.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:791: bounds.Inset(-native_app_window_->GetFrameInsets());
Fixed bug - NativeAppWindow::SetBounds() sets frame bounds, so the frame insets
should have been added.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:1085:
SetContentSizeConstraints(size, size_constraints_.GetMaximumSize());
Fixed bug - when the constraints were changed after window creation, the resize
and fullscreen controls were not updated accordingly. So
SetContentSizeConstraints() now makes everything consistent.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/gtk/ap...
File chrome/browser/ui/gtk/apps/native_app_window_gtk.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/gtk/ap...
chrome/browser/ui/gtk/apps/native_app_window_gtk.cc:64: gfx::Insets frame_insets
= GetFrameInsets();
The initial window bounds is broken on GTK because the window is initialized
asynchronously and the frame insets will be empty here. Since GTK will be
obsolete soon, I will leave it broken.

[benwells, 2014-03-05 02:33:29]

Looking real nice ...

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newcode7
apps/app_window.cc:7: #include <algorithm>
Is this needed?

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:102: gfx::Size GetCombinedWindowConstraints(const gfx::Size&
window_size,
Nit: I think window_size and content_size should be window_constraints and
content_constraints.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:116: const gfx::Size& content_size,
Same here

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:1023: params.window_spec.bounds.set_width(kDefaultWidth);
I think we should set the default on the content_spec so the content size is the
same on all platforms. I think this also matches what we did before.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h
File apps/app_window.h (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h#newcod...
apps/app_window.h:126: // INT_MIN designates 'unspecified' for the position
components, and 0 for
Does INT_MIN mean 0 for size, or does 0 mean unspecified? This comment confuses
me.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h#newcod...
apps/app_window.h:180: // until the frame insets are known, the returned value
is just an estimate.
I don't understand the comment about frame insets. The are passed in, so why
would they be unknown?

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/apps/app_...
File chrome/browser/apps/app_window_browsertest.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/apps/app_...
chrome/browser/apps/app_window_browsertest.cc:133:
IN_PROC_BROWSER_TEST_F(AppWindowAPITest, TestInitialBounds) {
It would be good to also have a test for the deprecated bounds stuff, to make
sure we don't break existing apps. Maybe we can look at re-enabling the
TestBounds and renaming to TestDeprecatedBounds?

TestBounds uses onresize so maybe it can be fixed with the same thing you used
on the constraints tests.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:109: "The $1 property
cannot be specified for both inner and outer bounds.",
Nit: put this in kConflictingBlahBlah up with the other errors.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:221: if
(!ParseBoundsSpec(*options, &create_params, &error_))
Yay for making this monster function shorter. Maybe call it GetBoundsSpec to be
consistent with GetFrameOptions (or rename GetFrameOptions to ParseFrameOptions,
I don't mind).

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:330: if (inner_bounds
&& outer_bounds &&
You can put all of these conflict checks into one if (innerBounds &&
outer_bounds) check to make the coder a little saner.

Maybe you could also put this into a kind of:
all_good = true;
all_good = all_good || CheckBoundsConflict(...);
all_good = all_good || CheckBoundsConflict(...);
if (all_good)
  return false;

chunk, but I'll leave that up to you.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:384: //
TODO(jeremya): remove these, since they do the same thing as
I don't think we have a plan or bug to remove these, can you remove the TODO?

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:391:
params->content_spec.bounds.set_x(*options.default_left.get());
My understanding is that previously left / top in this fiedl and in bounds
referred to the window left / top.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:91: bool
ensure_position_initialized) {
Nit: indenting.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/gtk/ap...
File chrome/browser/ui/gtk/apps/native_app_window_gtk.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/gtk/ap...
chrome/browser/ui/gtk/apps/native_app_window_gtk.cc:64: gfx::Insets frame_insets
= GetFrameInsets();
On 2014/03/04 03:13:14, tmdiep wrote:
> The initial window bounds is broken on GTK because the window is initialized
> asynchronously and the frame insets will be empty here. Since GTK will be
> obsolete soon, I will leave it broken.

Is it more broken with your change? I'm wondering if windows which use IDs will
get progressively larger or smaller now as we're switched to remembering /
restoring window size.

If it is more broken we might want to log a bug / todo that we can jump on if
aura gets switched off for m35.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/gtk/ap...
chrome/browser/ui/gtk/apps/native_app_window_gtk.cc:67: if (initial_bounds.x()
!= INT_MIN && initial_bounds.y() != INT_MIN)
Isn't there a constant you should be using instead of INT_MIN?

https://codereview.chromium.org/186343002/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/window_api/test.js (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/window_api/test.js:537: 
These tests are awesome, hopefully they won't suffer from the flakiness plaguing
the other chrome.app.window tests...

[tmdiep, 2014-03-05 03:06:46]

Just answering the questions. Will address the review comments soon.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newcode7
apps/app_window.cc:7: #include <algorithm>
On 2014/03/05 02:33:30, benwells wrote:
> Is this needed?

Apparently it is if you use std::min/max on VS2013. Just being paranoid because
someone had to add it for one of my other patches.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h
File apps/app_window.h (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h#newcod...
apps/app_window.h:126: // INT_MIN designates 'unspecified' for the position
components, and 0 for
On 2014/03/05 02:33:30, benwells wrote:
> Does INT_MIN mean 0 for size, or does 0 mean unspecified? This comment
confuses
> me.

I'll reword this comment.
INT_MIN means unspecified for position components.
0 means unspecified for size components.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h#newcod...
apps/app_window.h:180: // until the frame insets are known, the returned value
is just an estimate.
On 2014/03/05 02:33:30, benwells wrote:
> I don't understand the comment about frame insets. The are passed in, so why
> would they be unknown?

We need the window created first to get the real frame insets. But some native
app window initialization passes in an initial size during window creation (e.g.
mac). So I just pass in empty frame insets to get an estimated initial size,
then set the bounds precisely later. All this happens when the window is still
hidden, so there is no jank.

I'll reword this comment.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:391:
params->content_spec.bounds.set_x(*options.default_left.get());
On 2014/03/05 02:33:30, benwells wrote:
> My understanding is that previously left / top in this fiedl and in bounds
> referred to the window left / top.

Ugh. bounds.left and bounds.top were actually intended to be content, but due to
a bug in NativeAppWindowViews::GetFrameInsets(), ended up being window left/top.
Mac and Linux GTK got it right. But it really doesn't make sense to position
content wrt to the screen anyway, so maybe I'll make the deprecated left/top all
set the window position.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/gtk/ap...
File chrome/browser/ui/gtk/apps/native_app_window_gtk.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/gtk/ap...
chrome/browser/ui/gtk/apps/native_app_window_gtk.cc:64: gfx::Insets frame_insets
= GetFrameInsets();
On 2014/03/05 02:33:30, benwells wrote:
> On 2014/03/04 03:13:14, tmdiep wrote:
> > The initial window bounds is broken on GTK because the window is initialized
> > asynchronously and the frame insets will be empty here. Since GTK will be
> > obsolete soon, I will leave it broken.
> 
> Is it more broken with your change? I'm wondering if windows which use IDs
will
> get progressively larger or smaller now as we're switched to remembering /
> restoring window size.
> 
> If it is more broken we might want to log a bug / todo that we can jump on if
> aura gets switched off for m35.

It's not significantly broken. Existing apps will behave the same as before, but
apps that use the new outer bounds will have initial size and constraints a bit
off (in the order of frame insets). I expect linux aura will get to stable
before this API gets to stable.

[tapted, 2014-03-05 03:14:03]

oh! ben beat me :p

I had a few questions too, but wasn't super thorough in the non-cocoa stuff

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:134: // static
nit: remove - Don't usually see `// static` for data members (I guess it's
redundant)

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:141: bounds.set_x(kUnspecifiedPosition);
nit: bounds.SetRect(kUnspecifiedPosition, kUnspecifiedPosition, 0, 0);

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:147: AppWindow::BoundsSpecification::~BoundsSpecification()
{}
nit: move up below constructor

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:791: bounds.Inset(-native_app_window_->GetFrameInsets());
On 2014/03/04 03:13:14, tmdiep wrote:
> Fixed bug - NativeAppWindow::SetBounds() sets frame bounds, so the frame
insets
> should have been added.

Will this shift the origin too? (do we want that?)

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:1023: params.window_spec.bounds.set_width(kDefaultWidth);
should this be setting the content_spec instead of the window_spec? (otherwise I
think we would get different default sizes on different platforms)

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h
File apps/app_window.h (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.h#newcod...
apps/app_window.h:134: // Reset the bounds fields to their 'unspecified' values.
nit: mention that minimum/maximum size are unchanged.

https://codereview.chromium.org/186343002/diff/20001/apps/size_constraints.cc
File apps/size_constraints.cc (right):

https://codereview.chromium.org/186343002/diff/20001/apps/size_constraints.cc...
apps/size_constraints.cc:26: : size_constraints.width() - frame_insets.width(),
Do we need to worry about this going negative? Or (a nonzero) size_constraints
== frame_insets resulting in the difference becoming 0 ==> kUnboundedSize?

https://codereview.chromium.org/186343002/diff/20001/apps/size_constraints.h
File apps/size_constraints.h (right):

https://codereview.chromium.org/186343002/diff/20001/apps/size_constraints.h#...
apps/size_constraints.h:9: #include "ui/gfx/insets.h"
nit: since it's not a member, this can be forward-declared

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:123: if (input_spec
&& input_spec->left.get())
nit: 

if (!input_spec)
  return;

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:319: // be mixed.
nit: I read this thinking it would be an error if a developer specified both
(but that wouldn't really work). Maybe

"..will be ignored - do not allow them to be mixed." -> "..will be ignored -
there is no attempt to merge them."

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:91: bool
ensure_position_initialized) {
nit: indenting. But also this is always called with
|ensure_position_initialized| = true - drop the argument?

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:98:
NSHeight(main_screen_rect) - bounds.y() - bounds.height(),
nit (a bit nicer):
`NSHeight(main_screen_rect) - bounds.y() - bounds.height()` ->
`NSHeight(main_screen_rect) - bounds.bottom()`

or, even better,

NSRect cocoa_bounds = NSRectFromCGRect(bounds.ToCGRect());
cocoa_bounds.origin.y = NSHeight(main_screen_rect) - NSMaxY(cocoa_bounds);

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:102: // If coordinates
are < 0, center window on primary screen.
This comment doesn't really match (-ve coordinates come up whenever the primary
screen is not the bottom left one).

But it feels better to do this first. That is, take `bounds` by value instead of
reference, and flip once. I.e. do

if (bounds.x() == BoundsSpecification::kUnspecifiedPosition)
  bounds.set_x(floor((NSWidth(main_screen_rect) - bounds.width()) / 2));
/* same for y */
NSRect cocoa_bounds = NSRectFromCGRect(bounds.ToCGRect());
/* etc */

[tmdiep, 2014-03-05 04:29:05]

Just responses to tapted's questions. Will address all the review comments now.

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc
File apps/app_window.cc (right):

https://codereview.chromium.org/186343002/diff/20001/apps/app_window.cc#newco...
apps/app_window.cc:791: bounds.Inset(-native_app_window_->GetFrameInsets());
On 2014/03/05 03:14:03, tapted wrote:
> On 2014/03/04 03:13:14, tmdiep wrote:
> > Fixed bug - NativeAppWindow::SetBounds() sets frame bounds, so the frame
> insets
> > should have been added.
> 
> Will this shift the origin too? (do we want that?)

The existing code shifts the origin. With this change, the origin stays the
same, which is what I would expect.

https://codereview.chromium.org/186343002/diff/20001/apps/size_constraints.cc
File apps/size_constraints.cc (right):

https://codereview.chromium.org/186343002/diff/20001/apps/size_constraints.cc...
apps/size_constraints.cc:26: : size_constraints.width() - frame_insets.width(),
On 2014/03/05 03:14:03, tapted wrote:
> Do we need to worry about this going negative? Or (a nonzero) size_constraints
> == frame_insets resulting in the difference becoming 0 ==> kUnboundedSize?

Hmm good point. This is really just used to add insets to the bounds
constraints, so I might invert it.

[tmdiep, 2014-03-06 01:06:49]

I've addressed all review comments in patch set 3. I have only replied to the
comments, if required.

In patch set 4, I've maintained "backcompatibility" with the buggy bounds
behavior on Windows and ChromeOS. See comment below.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:391:
params->content_spec.bounds.set_x(*options.default_left.get());
On 2014/03/05 03:06:47, tmdiep wrote:
> On 2014/03/05 02:33:30, benwells wrote:
> > My understanding is that previously left / top in this fiedl and in bounds
> > referred to the window left / top.
> 
> Ugh. bounds.left and bounds.top were actually intended to be content, but due
to
> a bug in NativeAppWindowViews::GetFrameInsets(), ended up being window
left/top.
> Mac and Linux GTK got it right. But it really doesn't make sense to position
> content wrt to the screen anyway, so maybe I'll make the deprecated left/top
all
> set the window position.

I will make top/left be interpreted as top/left of the frame during window
creation, set bounds, get bounds, etc as it seems apps really have started to
rely on the bug: crbug.com/349397.

It won't be consistent with the documentation, but we will deprecate bounds and
change the doco soon anyway.

[tapted, 2014-03-06 03:31:53]

nice! lgtm

[benwells, 2014-03-06 22:40:16]

lgtm with a baby nit.

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/186343002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/app_window/app_window_api.cc:391:
params->content_spec.bounds.set_x(*options.default_left.get());
On 2014/03/06 01:06:49, tmdiep wrote:
> On 2014/03/05 03:06:47, tmdiep wrote:
> > On 2014/03/05 02:33:30, benwells wrote:
> > > My understanding is that previously left / top in this fiedl and in bounds
> > > referred to the window left / top.
> > 
> > Ugh. bounds.left and bounds.top were actually intended to be content, but
due
> to
> > a bug in NativeAppWindowViews::GetFrameInsets(), ended up being window
> left/top.
> > Mac and Linux GTK got it right. But it really doesn't make sense to position
> > content wrt to the screen anyway, so maybe I'll make the deprecated left/top
> all
> > set the window position.
> 
> I will make top/left be interpreted as top/left of the frame during window
> creation, set bounds, get bounds, etc as it seems apps really have started to
> rely on the bug: crbug.com/349397.
> 
> It won't be consistent with the documentation, but we will deprecate bounds
and
> change the doco soon anyway.

Awesome, love it. You should add a comment briefly explaining why, or people
might think this is a bug and 'fix' it.

[tmdiep, 2014-03-10 22:40:07]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-03-10 23:05:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/186343002/100001

[commit-bot: I haz the power, 2014-03-10 23:58:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/186343002/100001

[commit-bot: I haz the power, 2014-03-11 04:12:49]

Change committed as 256125