Call OnNativeWindowChanged after maximizing and restoring.

Call OnNativeWindowChanged after maximizing and restoring.

OnNativeWindowChanged is normally called when a window's bounds change.
However, if an app maximizes via the AppWindow API, but its bounds are
the same as that of a maximized window, OnNativeWindowChanged does not
end up being called.

This CL ensures that OnNativeWindowChanged is called whenever the app
maximizes or restores.

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

[benwells, 2014-08-04 06:55:31]

Is this bug because we only get notified when the bounds change but not when the
state changes? What I'm really wondering is if we should be listening to more
stuff happening on the native window.

[jackhou1, 2014-08-05 04:29:30]

On 2014/08/04 06:55:31, benwells wrote:
> Is this bug because we only get notified when the bounds change but not when
the
> state changes? What I'm really wondering is if we should be listening to more
> stuff happening on the native window.

Ah good point. I previously thought it was working on Aura native frames
(frame:chrome), but it turns out that, while restoring works, but the javascript
context is not updated. On Windows, the bounds always change when maximizing for
some reason.

So I think the correct fix is to add aura::WindowDelegate::OnPropertyChanged.
The main change is in Patch Set 2, Patch Set 3 updates all subclasses of
aura::WindowDelegate.

[benwells, 2014-08-05 23:24:55]

On 2014/08/05 04:29:30, jackhou1 wrote:
> On 2014/08/04 06:55:31, benwells wrote:
> > Is this bug because we only get notified when the bounds change but not when
> the
> > state changes? What I'm really wondering is if we should be listening to
more
> > stuff happening on the native window.
> 
> Ah good point. I previously thought it was working on Aura native frames
> (frame:chrome), but it turns out that, while restoring works, but the
javascript
> context is not updated. On Windows, the bounds always change when maximizing
for
> some reason.
> 
> So I think the correct fix is to add aura::WindowDelegate::OnPropertyChanged.
> The main change is in Patch Set 2, Patch Set 3 updates all subclasses of
> aura::WindowDelegate.

lgtm. Let's see what owners say :)

[jackhou1, 2014-08-06 01:05:18]

sky, could you please review this for OWNERS?

[sky, 2014-08-06 17:23:01]

https://codereview.chromium.org/434343004/diff/40001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/434343004/diff/40001/ui/aura/window.cc#newcod...
ui/aura/window.cc:862: delegate_->OnPropertyChanged(key, old);
Make NativeWidgetAura a WindowObserver so that you don't need to add a new
method to WindowDelegate.

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/native_w...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:743:
delegate_->OnNativeWidgetWindowShowStateChanged(
Won't this result in double the notifications if the bounds do change? Also,
might this be invoked before the bounds have changed? That said, I can't think
of a better way to handle this without adding something on
OnAfterPropertyChanged, which I'm loathe to suggest right now.

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:1141: void
Widget::OnNativeWidgetWindowShowStateChanged(ui::WindowShowState old) {
You don't use old, so nuke it.

[jackhou1, 2014-08-11 05:41:06]

https://codereview.chromium.org/434343004/diff/40001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/434343004/diff/40001/ui/aura/window.cc#newcod...
ui/aura/window.cc:862: delegate_->OnPropertyChanged(key, old);
On 2014/08/06 17:23:00, sky wrote:
> Make NativeWidgetAura a WindowObserver so that you don't need to add a new
> method to WindowDelegate.

Done.

Ah, thanks, that's much simpler.

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/native_w...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:743:
delegate_->OnNativeWidgetWindowShowStateChanged(
On 2014/08/06 17:23:00, sky wrote:
> Won't this result in double the notifications if the bounds do change? Also,
> might this be invoked before the bounds have changed? That said, I can't think
> of a better way to handle this without adding something on
> OnAfterPropertyChanged, which I'm loathe to suggest right now.

Yeah, SaveWindowPlacement gets called twice. I think the expensive part is
writing to prefs. I don't have any better ideas on how to avoid it though =S.

The bounds are correct in the JS context, so I assume it's being called after
the bounds change, unless there's an asynchronous step.

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/434343004/diff/40001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:1141: void
Widget::OnNativeWidgetWindowShowStateChanged(ui::WindowShowState old) {
On 2014/08/06 17:23:00, sky wrote:
> You don't use old, so nuke it.

Done.

[sky, 2014-08-11 16:09:19]

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:85: window_->AddObserver(this);
nit: move to init.

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
File ui/views/widget/native_widget_delegate.h (right):

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
ui/views/widget/native_widget_delegate.h:94: // Called when the NativeWidget
changed its window state.
Also document that a size change may also include a show state change.

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:1130: // Size changed notifications can fire prior to
full initialization
Make your new code and this call into a SaveWindowPlacementIfNecessary() (or
similarly named method) and update the comment.

[jackhou1, 2014-08-12 05:23:44]

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
File ui/views/widget/native_widget_aura.cc (right):

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
ui/views/widget/native_widget_aura.cc:85: window_->AddObserver(this);
On 2014/08/11 16:09:19, sky wrote:
> nit: move to init.

Done.

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
File ui/views/widget/native_widget_delegate.h (right):

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/native_w...
ui/views/widget/native_widget_delegate.h:94: // Called when the NativeWidget
changed its window state.
On 2014/08/11 16:09:19, sky wrote:
> Also document that a size change may also include a show state change.

Done.

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/434343004/diff/80001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:1130: // Size changed notifications can fire prior to
full initialization
On 2014/08/11 16:09:19, sky wrote:
> Make your new code and this call into a SaveWindowPlacementIfNecessary() (or
> similarly named method) and update the comment.

Done.

[sky, 2014-08-12 13:21:28]

LGTM

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/native_...
File ui/views/widget/native_widget_delegate.h (right):

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/native_...
ui/views/widget/native_widget_delegate.h:96: // Called when the NativeWidget
changed its window state.
changed->changes

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget....
ui/views/widget/widget.cc:1130: // Size changed notifications can fire prior to
full initialization
1130-1131 and 1140-1141 can now be nuked since covered by description of
SaveWindowPlacementIfInitialized in header.

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget....
ui/views/widget/widget.h:821: // Avoid saving session state during startup
procedures.
This comment is cryptic. Maybe:
// Invokes SaveWindowPlacement() if the native widget has been initialized.
// This is called at times when the native widget may not have been initialized.

And then you can remove the two comments in the code since this comment covers
them.

[jackhou1, 2014-08-13 01:57:51]

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/native_...
File ui/views/widget/native_widget_delegate.h (right):

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/native_...
ui/views/widget/native_widget_delegate.h:96: // Called when the NativeWidget
changed its window state.
On 2014/08/12 13:21:28, sky wrote:
> changed->changes

Done.

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget....
ui/views/widget/widget.cc:1130: // Size changed notifications can fire prior to
full initialization
On 2014/08/12 13:21:28, sky wrote:
> 1130-1131 and 1140-1141 can now be nuked since covered by description of
> SaveWindowPlacementIfInitialized in header.

Done.

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/434343004/diff/100001/ui/views/widget/widget....
ui/views/widget/widget.h:821: // Avoid saving session state during startup
procedures.
On 2014/08/12 13:21:28, sky wrote:
> This comment is cryptic. Maybe:
> // Invokes SaveWindowPlacement() if the native widget has been initialized.
> // This is called at times when the native widget may not have been
initialized.
> 
> And then you can remove the two comments in the code since this comment covers
> them.

Done.

[jackhou1, 2014-08-13 01:57:56]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-08-13 02:02:48]

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

[commit-bot: I haz the power, 2014-08-13 10:05:55]

Change committed as 289246