Set minimize property and re-enable tests on Aura

Set minimize property and re-enable tests on Aura

Currently we disable several important tests on Aura because Minimize() is unimplemented.
Most significant is ExtensionApiTest.UpdateWindowResize and ExtensionApiTest.UpdateWindowState, which test a lot of the chrome.windows api.

Setting the minimized property but not doing anything satisfies the tests with minimal change, until we determine what the expected minimize behavior is on Aura, and how this affects the extensions API.

BUG=104571
TEST=Tests pass (esp. browser_tests)


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

[stevenjb, 2012-02-23 18:22:47]

Ping?

[Ben Goodger (Google), 2012-02-23 18:27:37]

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
Why is this conditional on restore bounds already having been set?

[stevenjb, 2012-02-23 18:41:02]

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
On 2012/02/23 18:27:37, Ben Goodger (Google) wrote:
> Why is this conditional on restore bounds already having been set?
I didn't want to change the behavior if kRestoreBoundsKey has never been set. 
On the other hand, currently if we call GetRestoreBounds() without minimizing or
maximizing first it will return NULL, so maybe we do want to set it always?

[oshima, 2012-02-23 18:46:20]

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
On 2012/02/23 18:41:02, stevenjb (chromium) wrote:
> On 2012/02/23 18:27:37, Ben Goodger (Google) wrote:
> > Why is this conditional on restore bounds already having been set?
> I didn't want to change the behavior if kRestoreBoundsKey has never been set. 
> On the other hand, currently if we call GetRestoreBounds() without minimizing
or
> maximizing first it will return NULL, so maybe we do want to set it always?

If window is in normal state, there is nothing to restore to. I believe this
should be done in ash if not already done so.

[stevenjb, 2012-02-23 18:54:23]

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
On 2012/02/23 18:46:20, oshima wrote:
> On 2012/02/23 18:41:02, stevenjb (chromium) wrote:
> > On 2012/02/23 18:27:37, Ben Goodger (Google) wrote:
> > > Why is this conditional on restore bounds already having been set?
> > I didn't want to change the behavior if kRestoreBoundsKey has never been
set. 
> > On the other hand, currently if we call GetRestoreBounds() without
minimizing
> or
> > maximizing first it will return NULL, so maybe we do want to set it always?
> 
> If window is in normal state, there is nothing to restore to. I believe this
> should be done in ash if not already done so.

Actually, I was wrong, I was looking at the wrong function.
GetRestoredBounds() does check to see if the property exists, and if it does not
then it returns window_->bounds().

So, this is just a question of whether or not we want to always set the
property.

[oshima, 2012-02-24 00:29:38]

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
On 2012/02/23 18:54:23, stevenjb (chromium) wrote:
> On 2012/02/23 18:46:20, oshima wrote:
> > On 2012/02/23 18:41:02, stevenjb (chromium) wrote:
> > > On 2012/02/23 18:27:37, Ben Goodger (Google) wrote:
> > > > Why is this conditional on restore bounds already having been set?
> > > I didn't want to change the behavior if kRestoreBoundsKey has never been
> set. 
> > > On the other hand, currently if we call GetRestoreBounds() without
> minimizing
> > or
> > > maximizing first it will return NULL, so maybe we do want to set it
always?
> > 
> > If window is in normal state, there is nothing to restore to. I believe this
> > should be done in ash if not already done so.
> 
> Actually, I was wrong, I was looking at the wrong function.
> GetRestoredBounds() does check to see if the property exists, and if it does
not
> then it returns window_->bounds().
> 
> So, this is just a question of whether or not we want to always set the
> property.

Restore bounds make sense only when the window is not in normal state,
so I don't think we want to set it always. If we want/need to update the
restore bounds to new bounds while the window is in max/min/full state,
it should be done in ash rather than in views.
(NWA has "delete GetRestoreBounds(window)" which is a workaround to fix memory
leak.
 This will be fixed by benrg (not beng:))

[stevenjb, 2012-02-24 02:27:06]

On 2012/02/24 00:29:38, oshima wrote:
>
http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
> ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
> On 2012/02/23 18:54:23, stevenjb (chromium) wrote:
> > On 2012/02/23 18:46:20, oshima wrote:
> > > On 2012/02/23 18:41:02, stevenjb (chromium) wrote:
> > > > On 2012/02/23 18:27:37, Ben Goodger (Google) wrote:
> > > > > Why is this conditional on restore bounds already having been set?
> > > > I didn't want to change the behavior if kRestoreBoundsKey has never been
> > set. 
> > > > On the other hand, currently if we call GetRestoreBounds() without
> > minimizing
> > > or
> > > > maximizing first it will return NULL, so maybe we do want to set it
> always?
> > > 
> > > If window is in normal state, there is nothing to restore to. I believe
this
> > > should be done in ash if not already done so.
> > 
> > Actually, I was wrong, I was looking at the wrong function.
> > GetRestoredBounds() does check to see if the property exists, and if it does
> not
> > then it returns window_->bounds().
> > 
> > So, this is just a question of whether or not we want to always set the
> > property.
> 
> Restore bounds make sense only when the window is not in normal state,
> so I don't think we want to set it always. If we want/need to update the
> restore bounds to new bounds while the window is in max/min/full state,
> it should be done in ash rather than in views.
> (NWA has "delete GetRestoreBounds(window)" which is a workaround to fix memory
> leak.
>  This will be fixed by benrg (not beng:))

Ugh. I just lost a detailed explanation and I don't want to rewrite it.

Short version:

This logic in ExtensionTabUtil::CreateWindowValue is causing the test failure:

  if (browser->window()->IsMaximized() || browser->window()->IsFullscreen())
    bounds = browser->window()->GetBounds();
  else
    bounds = browser->window()->GetRestoredBounds();

I believe this is incorrect logic, I think it should be if (IsMinimized).
Changing that does fix the test that was failing.

However, searching through the code, I see other places where it looks like we
may be assuming that GetRestoredBounds() always returns the same result as
GetBounds() for normal windows. This is not true in Aura, but seems to be true
on other platforms. 

So, we can:
a) Change the logic in ExtensionTabUtil::CreateWindowValue() and (assuming that
doesn't break any tests) deal with any other places we misuse
GetRestoredBounds() if/when they come up.
b) Make this change to be consistent with other platforms
c) Change GetRestoredBounds() to return GetBounds() if not minimized or
maximized

I have a slight preference for b), but am fine with any solutions that allows us
to re-enable these tests.

[stevenjb, 2012-02-24 18:29:17]

+sky. This to resolve this before we can test
http://codereview.chromium.org/9428018.

[sky, 2012-02-24 18:52:40]

On Thu, Feb 23, 2012 at 6:27 PM,  <stevenjb@chromium.org> wrote:
> On 2012/02/24 00:29:38, oshima wrote:
>
>
http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
>>
>> File ui/views/widget/native_widget_aura.cc (right):
>
>
>
>
http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
>>
>> ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
>> On 2012/02/23 18:54:23, stevenjb (chromium) wrote:
>> > On 2012/02/23 18:46:20, oshima wrote:
>> > > On 2012/02/23 18:41:02, stevenjb (chromium) wrote:
>> > > > On 2012/02/23 18:27:37, Ben Goodger (Google) wrote:
>> > > > > Why is this conditional on restore bounds already having been set?
>> > > > I didn't want to change the behavior if kRestoreBoundsKey has never
>> > > > been
>> > set.
>> > > > On the other hand, currently if we call GetRestoreBounds() without
>> > minimizing
>> > > or
>> > > > maximizing first it will return NULL, so maybe we do want to set it
>> always?
>> > >
>> > > If window is in normal state, there is nothing to restore to. I
>> > > believe
>
> this
>>
>> > > should be done in ash if not already done so.
>> >
>> > Actually, I was wrong, I was looking at the wrong function.
>> > GetRestoredBounds() does check to see if the property exists, and if it
>> > does
>> not
>> > then it returns window_->bounds().
>> >
>> > So, this is just a question of whether or not we want to always set the
>> > property.
>
>
>> Restore bounds make sense only when the window is not in normal state,
>> so I don't think we want to set it always. If we want/need to update the
>> restore bounds to new bounds while the window is in max/min/full state,
>> it should be done in ash rather than in views.
>> (NWA has "delete GetRestoreBounds(window)" which is a workaround to fix
>> memory
>> leak.
>>  This will be fixed by benrg (not beng:))
>
>
> Ugh. I just lost a detailed explanation and I don't want to rewrite it.
>
> Short version:
>
> This logic in ExtensionTabUtil::CreateWindowValue is causing the test
> failure:
>
>  if (browser->window()->IsMaximized() || browser->window()->IsFullscreen())
>    bounds = browser->window()->GetBounds();
>  else
>    bounds = browser->window()->GetRestoredBounds();
>
> I believe this is incorrect logic, I think it should be if (IsMinimized).
> Changing that does fix the test that was failing.

I agree, this seems wrong.

> However, searching through the code, I see other places where it looks like
> we
> may be assuming that GetRestoredBounds() always returns the same result as
> GetBounds() for normal windows. This is not true in Aura, but seems to be
> true
> on other platforms.

I am fine with changing NWA so that GetRestoredBounds returns
GetBounds if normal.

  -Scott

> So, we can:
> a) Change the logic in ExtensionTabUtil::CreateWindowValue() and (assuming
> that
> doesn't break any tests) deal with any other places we misuse
> GetRestoredBounds() if/when they come up.
> b) Make this change to be consistent with other platforms
> c) Change GetRestoredBounds() to return GetBounds() if not minimized or
> maximized
>
> I have a slight preference for b), but am fine with any solutions that
> allows us
> to re-enable these tests.
>
>
>
> http://codereview.chromium.org/9372126/

[oshima, 2012-02-24 19:26:15]

On 2012/02/24 18:52:40, sky wrote:
> On Thu, Feb 23, 2012 at 6:27 PM,  <mailto:stevenjb@chromium.org> wrote:
> > On 2012/02/24 00:29:38, oshima wrote:
> >
> >
>
http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
> >>
> >> File ui/views/widget/native_widget_aura.cc (right):
> >
> >
> >
> >
>
http://codereview.chromium.org/9372126/diff/1/ui/views/widget/native_widget_a...
> >>
> >> ui/views/widget/native_widget_aura.cc:418: if (GetRestoreBounds(window_))
> >> On 2012/02/23 18:54:23, stevenjb (chromium) wrote:
> >> > On 2012/02/23 18:46:20, oshima wrote:
> >> > > On 2012/02/23 18:41:02, stevenjb (chromium) wrote:
> >> > > > On 2012/02/23 18:27:37, Ben Goodger (Google) wrote:
> >> > > > > Why is this conditional on restore bounds already having been set?
> >> > > > I didn't want to change the behavior if kRestoreBoundsKey has never
> >> > > > been
> >> > set.
> >> > > > On the other hand, currently if we call GetRestoreBounds() without
> >> > minimizing
> >> > > or
> >> > > > maximizing first it will return NULL, so maybe we do want to set it
> >> always?
> >> > >
> >> > > If window is in normal state, there is nothing to restore to. I
> >> > > believe
> >
> > this
> >>
> >> > > should be done in ash if not already done so.
> >> >
> >> > Actually, I was wrong, I was looking at the wrong function.
> >> > GetRestoredBounds() does check to see if the property exists, and if it
> >> > does
> >> not
> >> > then it returns window_->bounds().
> >> >
> >> > So, this is just a question of whether or not we want to always set the
> >> > property.
> >
> >
> >> Restore bounds make sense only when the window is not in normal state,
> >> so I don't think we want to set it always. If we want/need to update the
> >> restore bounds to new bounds while the window is in max/min/full state,
> >> it should be done in ash rather than in views.
> >> (NWA has "delete GetRestoreBounds(window)" which is a workaround to fix
> >> memory
> >> leak.
> >> &nbsp;This will be fixed by benrg (not beng:))
> >
> >
> > Ugh. I just lost a detailed explanation and I don't want to rewrite it.
> >
> > Short version:
> >
> > This logic in ExtensionTabUtil::CreateWindowValue is causing the test
> > failure:
> >
> > &nbsp;if (browser->window()->IsMaximized() ||
browser->window()->IsFullscreen())
> > &nbsp; &nbsp;bounds = browser->window()->GetBounds();
> > &nbsp;else
> > &nbsp; &nbsp;bounds = browser->window()->GetRestoredBounds();
> >
> > I believe this is incorrect logic, I think it should be if (IsMinimized).
> > Changing that does fix the test that was failing.
> 
> I agree, this seems wrong.
> 
> > However, searching through the code, I see other places where it looks like
> > we
> > may be assuming that GetRestoredBounds() always returns the same result as
> > GetBounds() for normal windows. This is not true in Aura, but seems to be
> > true
> > on other platforms.
> 
> I am fine with changing NWA so that GetRestoredBounds returns
> GetBounds if normal.

I'm ok to return bounds in NWA if normal, instead of always setting aura's
restore bounds.

- oshima
> 
>   -Scott
> 
> > So, we can:
> > a) Change the logic in ExtensionTabUtil::CreateWindowValue() and (assuming
> > that
> > doesn't break any tests) deal with any other places we misuse
> > GetRestoredBounds() if/when they come up.
> > b) Make this change to be consistent with other platforms
> > c) Change GetRestoredBounds() to return GetBounds() if not minimized or
> > maximized
> >
> > I have a slight preference for b), but am fine with any solutions that
> > allows us
> > to re-enable these tests.
> >
> >
> >
> > http://codereview.chromium.org/9372126/

[stevenjb, 2012-02-24 20:28:23]

OK, moved the logic to GetRestoredBounds().

I will fix the extension behavior in a followup/cleanup CL (I found an existing
bug/todo item).

[stevenjb, 2012-02-24 20:29:13]

(Note: patchest #2 includes some Rebase changes)

[sky, 2012-02-24 21:09:28]

LGTM

[oshima, 2012-02-24 21:24:59]

http://codereview.chromium.org/9372126/diff/9002/ui/views/widget/native_widge...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9372126/diff/9002/ui/views/widget/native_widge...
ui/views/widget/native_widget_aura.cc:415: if (!IsMinimized() && !IsMaximized())
I believe you need to check IsFullScreen() as well.

[stevenjb, 2012-02-24 21:50:45]

http://codereview.chromium.org/9372126/diff/9002/ui/views/widget/native_widge...
File ui/views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/9372126/diff/9002/ui/views/widget/native_widge...
ui/views/widget/native_widget_aura.cc:415: if (!IsMinimized() && !IsMaximized())
On 2012/02/24 21:24:59, oshima wrote:
> I believe you need to check IsFullScreen() as well.
Ugh, you're right, didn't realize that was a widget property. Done.

[oshima, 2012-02-24 22:32:09]

lgtm

[commit-bot: I haz the power, 2012-02-24 22:57:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9372126/6009

[commit-bot: I haz the power, 2012-02-25 01:42:31]

Change committed as 123622