aura: Some changes in Window and Desktop.

aura: Some changes to Desktop.

. Add Observer support to Desktop.
. On X11, make sure the correct desktop size is always maintained.

Use these changes to fix keyboard positioning in touchui when the desktop window is resized.

BUG=none
TEST=aura_unittests (ObserverTest.*)

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

[sadrul, 2011-10-12 20:45:58]

Window::Observer's aren't used yet, but I suspect it will be useful soon.

Should all these different kinds of AddObserver/RemoveObserver/HasObserver be
moved into a base::Observable<Observer> class?

[sky, 2011-10-12 21:11:27]

http://codereview.chromium.org/8253001/diff/1/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8253001/diff/1/ui/aura/desktop.h#newcode37
ui/aura/desktop.h:37: virtual void OnDesktopResized(const gfx::Size& new_size)
{}
I think we should make these pure virtual. Also, add a protected virtual
destructor and move this into its own header.

http://codereview.chromium.org/8253001/diff/1/ui/aura/desktop.h#newcode112
ui/aura/desktop.h:112: bool HasObserver(Observer* observer);
Unless we need HasObserver, don't add it.

http://codereview.chromium.org/8253001/diff/1/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/8253001/diff/1/ui/aura/window.h#newcode52
ui/aura/window.h:52: virtual void OnWindowClosing(Window* widget) {}
Same comments as for Desktop::Observer.

[sadrul, 2011-10-12 21:33:22]

http://codereview.chromium.org/8253001/diff/1/ui/aura/desktop.h
File ui/aura/desktop.h (right):

http://codereview.chromium.org/8253001/diff/1/ui/aura/desktop.h#newcode37
ui/aura/desktop.h:37: virtual void OnDesktopResized(const gfx::Size& new_size)
{}
On 2011/10/12 21:11:27, sky wrote:
> I think we should make these pure virtual. Also, add a protected virtual
> destructor and move this into its own header.

Done. I kept the empty implementation for WindowObserver: an observer may only
be interested in one particular event, so it shouldn't be forced to provide
implementations for all the possible events.

http://codereview.chromium.org/8253001/diff/1/ui/aura/desktop.h#newcode112
ui/aura/desktop.h:112: bool HasObserver(Observer* observer);
On 2011/10/12 21:11:27, sky wrote:
> Unless we need HasObserver, don't add it.

It's not needed right now. Removed.

[sky, 2011-10-12 22:05:38]

Adds some tests to make sure observers are getting notified correctly.

http://codereview.chromium.org/8253001/diff/1007/ui/aura/desktop_observer.h
File ui/aura/desktop_observer.h (right):

http://codereview.chromium.org/8253001/diff/1007/ui/aura/desktop_observer.h#n...
ui/aura/desktop_observer.h:16: virtual void OnDesktopResized(const gfx::Size&
new_size) = 0;
I think if you keep the methods in WindowObserver non-pure virtual you should do
the same here. After all, this is an observer and observers are likely only to
care about a couple of methods.

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window.cc#newcode120
ui/aura/window.cc:120: FOR_EACH_OBSERVER(WindowObserver, observers_,
This should be up in the Desktop, not here (there are a number of reasons why
this could fail, which would result in a bad notification).

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window.cc#newcode126
ui/aura/window.cc:126: FOR_EACH_OBSERVER(WindowObserver, observers_,
Same comment, this should be in Desktop.

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:16: virtual void OnWindowVisibilityChanged(Window*
widget, bool visible) {}
I don't like passing in visible or active here. The observer can ask the window
if it wants this state.

[tfarina, 2011-10-12 22:23:43]

http://codereview.chromium.org/8253001/diff/3002/ui/aura/desktop_observer.h
File ui/aura/desktop_observer.h (right):

http://codereview.chromium.org/8253001/diff/3002/ui/aura/desktop_observer.h#n...
ui/aura/desktop_observer.h:18: virtual void OnDesktopResized(const gfx::Size&
new_size) {}
forward declare gfx::Size too.

[tfarina, 2011-10-12 22:25:26]

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:15: virtual void OnWindowClosing(Window* window) {}
forward declare Window.

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:24: #endif  // UI_AURA_DESKTOP_OBSERVER_H_
UI_AURA_DESKTOP_OBSERVER_H_ -> UI_AURA_WINDOW_OBSERVER_H_

above too.

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:25: 
remove this empty blank line here

[sadrul, 2011-10-12 22:59:58]

Added some tests.

http://codereview.chromium.org/8253001/diff/1007/ui/aura/desktop_observer.h
File ui/aura/desktop_observer.h (right):

http://codereview.chromium.org/8253001/diff/1007/ui/aura/desktop_observer.h#n...
ui/aura/desktop_observer.h:16: virtual void OnDesktopResized(const gfx::Size&
new_size) = 0;
On 2011/10/12 22:05:38, sky wrote:
> I think if you keep the methods in WindowObserver non-pure virtual you should
do
> the same here. After all, this is an observer and observers are likely only to
> care about a couple of methods.

Done.

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window.cc#newcode120
ui/aura/window.cc:120: FOR_EACH_OBSERVER(WindowObserver, observers_,
On 2011/10/12 22:05:38, sky wrote:
> This should be up in the Desktop, not here (there are a number of reasons why
> this could fail, which would result in a bad notification).

Removed this, added DesktopObserver::OnActiveWindowChanged instead.

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window.cc#newcode126
ui/aura/window.cc:126: FOR_EACH_OBSERVER(WindowObserver, observers_,
On 2011/10/12 22:05:38, sky wrote:
> Same comment, this should be in Desktop.

Done.

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

http://codereview.chromium.org/8253001/diff/1007/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:16: virtual void OnWindowVisibilityChanged(Window*
widget, bool visible) {}
On 2011/10/12 22:05:38, sky wrote:
> I don't like passing in visible or active here. The observer can ask the
window
> if it wants this state.

Done.

http://codereview.chromium.org/8253001/diff/3002/ui/aura/desktop_observer.h
File ui/aura/desktop_observer.h (right):

http://codereview.chromium.org/8253001/diff/3002/ui/aura/desktop_observer.h#n...
ui/aura/desktop_observer.h:18: virtual void OnDesktopResized(const gfx::Size&
new_size) {}
On 2011/10/12 22:23:44, tfarina wrote:
> forward declare gfx::Size too.

Done.

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h
File ui/aura/window_observer.h (right):

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:15: virtual void OnWindowClosing(Window* window) {}
On 2011/10/12 22:25:27, tfarina wrote:
> forward declare Window.

Done.

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:24: #endif  // UI_AURA_DESKTOP_OBSERVER_H_
On 2011/10/12 22:25:27, tfarina wrote:
> UI_AURA_DESKTOP_OBSERVER_H_ -> UI_AURA_WINDOW_OBSERVER_H_
> 
> above too.

D'oh! Copy pasta. Done.

http://codereview.chromium.org/8253001/diff/3002/ui/aura/window_observer.h#ne...
ui/aura/window_observer.h:25: 
On 2011/10/12 22:25:27, tfarina wrote:
> remove this empty blank line here

Done.

[sky, 2011-10-12 23:20:52]

http://codereview.chromium.org/8253001/diff/7003/ui/aura/desktop_observer.h
File ui/aura/desktop_observer.h (right):

http://codereview.chromium.org/8253001/diff/7003/ui/aura/desktop_observer.h#n...
ui/aura/desktop_observer.h:24: // Invoked when the active window is changed.
Document that |active| may be NULL if there are no active windows.

http://codereview.chromium.org/8253001/diff/7003/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8253001/diff/7003/ui/aura/window.cc#newcode386
ui/aura/window.cc:386: FOR_EACH_OBSERVER(WindowObserver, observers_,
Shouldn't this be moved inside the if statement to avoid unnecessary
notifications?

[sadrul, 2011-10-13 00:44:18]

http://codereview.chromium.org/8253001/diff/7003/ui/aura/desktop_observer.h
File ui/aura/desktop_observer.h (right):

http://codereview.chromium.org/8253001/diff/7003/ui/aura/desktop_observer.h#n...
ui/aura/desktop_observer.h:24: // Invoked when the active window is changed.
On 2011/10/12 23:20:52, sky wrote:
> Document that |active| may be NULL if there are no active windows.

Done.

http://codereview.chromium.org/8253001/diff/7003/ui/aura/window.cc
File ui/aura/window.cc (right):

http://codereview.chromium.org/8253001/diff/7003/ui/aura/window.cc#newcode386
ui/aura/window.cc:386: FOR_EACH_OBSERVER(WindowObserver, observers_,
On 2011/10/12 23:20:52, sky wrote:
> Shouldn't this be moved inside the if statement to avoid unnecessary
> notifications?

Indeed. Good point. Made the change (and updated the test accordingly).

http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:854: w3->Hide();
This does not trigger the observer, because the parent window, w1, is already
hidden. Is this expectation correct?

[sky, 2011-10-13 00:51:08]

http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:854: w3->Hide();
On 2011/10/13 00:44:18, sadrul wrote:
> This does not trigger the observer, because the parent window, w1, is already
> hidden. Is this expectation correct?

I think so. But it does mean you'll want to notify all descendants if an
ancestor is made visible/invisible.

[sadrul, 2011-10-13 02:24:54]

http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc
File ui/aura/window_unittest.cc (right):

http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc#n...
ui/aura/window_unittest.cc:854: w3->Hide();
On 2011/10/13 00:51:08, sky wrote:
> On 2011/10/13 00:44:18, sadrul wrote:
> > This does not trigger the observer, because the parent window, w1, is
already
> > hidden. Is this expectation correct?
> 
> I think so. But it does mean you'll want to notify all descendants if an
> ancestor is made visible/invisible.

I personally think the observer should trigger whenever the Window's local
visibility state changes (not necessarily onscreen visibility). There was a very
brief discussion on this here:
http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc#newcode373
This will be more in-line with how this works at least on GTK. It's of course
not necessary that we follow GTK, but in case there's consistency among other
toolkits (I do not know how this works on windows/other toolkits), perhaps it
would make sense for this to do the same.

Or an observer could just observe both local state change and onscreen
visibility change.

[sky, 2011-10-13 04:01:50]

On 2011/10/13 02:24:54, sadrul wrote:
> http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc
> File ui/aura/window_unittest.cc (right):
> 
>
http://codereview.chromium.org/8253001/diff/9001/ui/aura/window_unittest.cc#n...
> ui/aura/window_unittest.cc:854: w3->Hide();
> On 2011/10/13 00:51:08, sky wrote:
> > On 2011/10/13 00:44:18, sadrul wrote:
> > > This does not trigger the observer, because the parent window, w1, is
> already
> > > hidden. Is this expectation correct?
> > 
> > I think so. But it does mean you'll want to notify all descendants if an
> > ancestor is made visible/invisible.
> 
> I personally think the observer should trigger whenever the Window's local
> visibility state changes (not necessarily onscreen visibility). There was a
very
> brief discussion on this here:
> http://codereview.chromium.org/8202026/diff/12010/ui/aura/window.cc#newcode373

I think windows does the same. But it's a little weird for us to do this as we
only expose whether all ancestors are visible. It's definitely simpler to do as
you propose, so lets do that.

> This will be more in-line with how this works at least on GTK. It's of course
> not necessary that we follow GTK, but in case there's consistency among other
> toolkits (I do not know how this works on windows/other toolkits), perhaps it
> would make sense for this to do the same.
> 
> Or an observer could just observe both local state change and onscreen
> visibility change.

I tend to think folks care more about onscreen visibility changes, but lets keep
it simple for now.

  -Scott

[sky, 2011-10-13 04:03:31]

One more question. Do you have use cases that require the other changes you're
doing? It seems like you only care about the desktop notification.

  -Scott

[sadrul, 2011-10-13 14:28:35]

On 2011/10/13 04:03:31, sky wrote:
> One more question. Do you have use cases that require the other changes you're
> doing? It seems like you only care about the desktop notification.

Yep. For now, only the desktop notifications are needed. I can shelve all of the
window notifications for now, or just the visibility change until we have an
actual use case. WDYT?

> 
>   -Scott

[sky, 2011-10-13 14:50:14]

On Thu, Oct 13, 2011 at 7:28 AM,  <sadrul@chromium.org> wrote:
> On 2011/10/13 04:03:31, sky wrote:
>>
>> One more question. Do you have use cases that require the other changes
>> you're
>> doing? It seems like you only care about the desktop notification.
>
> Yep. For now, only the desktop notifications are needed. I can shelve all of
> the
> window notifications for now, or just the visibility change until we have an
> actual use case. WDYT?

I'm not keen on adding features until we know we'll need them. I say
shelve it until we need it. That said, I do need a WindowObserver for
a different patch, but I'll pull the changes from your patch.

  -Scott

[sadrul, 2011-10-13 15:38:31]

On 2011/10/13 14:50:14, sky wrote:
> On Thu, Oct 13, 2011 at 7:28 AM,  <mailto:sadrul@chromium.org> wrote:
> > On 2011/10/13 04:03:31, sky wrote:
> >>
> >> One more question. Do you have use cases that require the other changes
> >> you're
> >> doing? It seems like you only care about the desktop notification.
> >
> > Yep. For now, only the desktop notifications are needed. I can shelve all of
> > the
> > window notifications for now, or just the visibility change until we have an
> > actual use case. WDYT?
> 
> I'm not keen on adding features until we know we'll need them. I say
> shelve it until we need it. That said, I do need a WindowObserver for
> a different patch, but I'll pull the changes from your patch.

Sounds good. Done.

[sky, 2011-10-13 16:26:01]

LGTM