[Aura] Support always-on-top top level window.

[Aura] Support always-on-top top level window.

BUG=97256, 102582
TEST=AlwaysOnTop related tests in ShellTest should pass.


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

[oshima, 2011-11-01 18:35:21]

can you add test for this?

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

http://codereview.chromium.org/8387043/diff/1/ui/aura/window.cc#newcode28
ui/aura/window.cc:28: const char Window::kPropAlwaysOnTop[] = "AlwaysOnTop";
move this to ui/aura/aura_constants.cc

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

http://codereview.chromium.org/8387043/diff/1/ui/aura/window.h#newcode60
ui/aura/window.h:60: static const char kPropAlwaysOnTop[];  // boolean, default
is false.
move this to ui/aura/aura_contents.h

[mazda, 2011-11-02 13:43:44]

Does this solve http://crbug.com/97256?
If yes, please add the number to BUG line.

[xiyuan, 2011-11-02 23:26:32]

CL updated. Please take another look.

- Moved property key to auro_constants;
- Added unittests;
- Quick fix for XCursorCache problem;

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

http://codereview.chromium.org/8387043/diff/1/ui/aura/window.cc#newcode28
ui/aura/window.cc:28: const char Window::kPropAlwaysOnTop[] = "AlwaysOnTop";
On 2011/11/01 18:35:21, oshima wrote:
> move this to ui/aura/aura_constants.cc

Done.

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

http://codereview.chromium.org/8387043/diff/1/ui/aura/window.h#newcode60
ui/aura/window.h:60: static const char kPropAlwaysOnTop[];  // boolean, default
is false.
On 2011/11/01 18:35:21, oshima wrote:
> move this to ui/aura/aura_contents.h 

Done.

[tfarina, 2011-11-02 23:34:35]

http://codereview.chromium.org/8387043/diff/7001/ui/aura/aura_constants.cc
File ui/aura/aura_constants.cc (right):

http://codereview.chromium.org/8387043/diff/7001/ui/aura/aura_constants.cc#ne...
ui/aura/aura_constants.cc:14: }
please add  // namespace aura

[oshima, 2011-11-04 18:14:11]

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:88: if (instance_ == this)
There should be only one Shell instance, so DCHECK.

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest.cc
File ui/aura_shell/shell_unittest.cc (right):

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:28: views::Widget* CreateTestWindow(const
views::Widget::InitParams& params) {
const or maybe static

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:31: widget->SetContentsView(new views::View);
do you need this? If so, include views.h. (I wonder who is including it)

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:36: aura_shell::Shell* shell_;
do you need this?

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:70:
aura_shell::internal::kShellWindowId_DefaultContainer),
Add method (like default_container()) to get container in ShellTest or
GetDefaultContainer() in anonymous ns. That would make the code simpler.

[xiyuan, 2011-11-04 18:39:30]

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell.cc#newco...
ui/aura_shell/shell.cc:88: if (instance_ == this)
On 2011/11/04 18:14:11, oshima wrote:
> There should be only one Shell instance, so DCHECK.

Done.

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest.cc
File ui/aura_shell/shell_unittest.cc (right):

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:28: views::Widget* CreateTestWindow(const
views::Widget::InitParams& params) {
On 2011/11/04 18:14:11, oshima wrote:
> const or maybe static

Moved to anonymous namespace.

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:31: widget->SetContentsView(new views::View);
On 2011/11/04 18:14:11, oshima wrote:
> do you need this? If so, include views.h. (I wonder who is including it)
Removed SetContentsView.

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:36: aura_shell::Shell* shell_;
On 2011/11/04 18:14:11, oshima wrote:
> do you need this?

Removed.

http://codereview.chromium.org/8387043/diff/9002/ui/aura_shell/shell_unittest...
ui/aura_shell/shell_unittest.cc:70:
aura_shell::internal::kShellWindowId_DefaultContainer),
On 2011/11/04 18:14:11, oshima wrote:
> Add method (like default_container()) to get container in ShellTest or
> GetDefaultContainer() in anonymous ns. That would make the code simpler.

Done. Added helper functions to get default container and always on top
container.

[oshima, 2011-11-04 20:40:08]

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
File ui/aura_shell/shell_unittest.cc (right):

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:6: #include "ui/aura/desktop.h"
i believe you don't need these two.

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:8: #include "ui/aura/window.h"
you probably don't need this either, although i'm ok to keep it if you like.

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:24: aura::Window* DefaultContainer() {
GetDefaultContainer/GetAlwaysOnTopContainer

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:222: const int kCursorClearXCursorCache = -1;
move this to the top (if you want to keep it extern)

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.h#newcode73
ui/base/x/x11_util.h:73: extern const int kCursorClearXCursorCache;
just using const int is more common

http://www.google.com/codesearch#search/&exact_package=chromium&q=%5Econst%5C...
http://www.google.com/codesearch#search/&exact_package=chromium&q=%5Eextern%5...

it's up to you though.

http://codereview.chromium.org/8387043/diff/12002/views/widget/native_widget_...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8387043/diff/12002/views/widget/native_widget_...
views/widget/native_widget_aura.cc:449: }
Sorry I missed this part in previous review.
This should be handled in shell, but not in widget.
You can use WindowObserver::PropertyChanged to reparent.

[xiyuan, 2011-11-04 22:40:21]

Please take another look. Thanks

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
File ui/aura_shell/shell_unittest.cc (right):

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:6: #include "ui/aura/desktop.h"
On 2011/11/04 20:40:08, oshima wrote:
> i believe you don't need these two.

Done.

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:8: #include "ui/aura/window.h"
On 2011/11/04 20:40:08, oshima wrote:
> you probably don't need this either, although i'm ok to keep it if you like.

We need it because we call on parent() to get container.

http://codereview.chromium.org/8387043/diff/12002/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:24: aura::Window* DefaultContainer() {
On 2011/11/04 20:40:08, oshima wrote:
> GetDefaultContainer/GetAlwaysOnTopContainer

Done.

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:222: const int kCursorClearXCursorCache = -1;
On 2011/11/04 20:40:08, oshima wrote:
> move this to the top (if you want to keep it extern)

Removed since I followed your suggestion to put the value in header.

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/8387043/diff/12002/ui/base/x/x11_util.h#newcode73
ui/base/x/x11_util.h:73: extern const int kCursorClearXCursorCache;
On 2011/11/04 20:40:08, oshima wrote:
> just using const int is more common
> 
>
http://www.google.com/codesearch#search/&exact_package=chromium&q=%255Econst%...
>
http://www.google.com/codesearch#search/&exact_package=chromium&q=%255Eextern...
> 
> it's up to you though.

Done.

http://codereview.chromium.org/8387043/diff/12002/views/widget/native_widget_...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8387043/diff/12002/views/widget/native_widget_...
views/widget/native_widget_aura.cc:449: }
On 2011/11/04 20:40:08, oshima wrote:
> Sorry I missed this part in previous review.
> This should be handled in shell, but not in widget.
> You can use WindowObserver::PropertyChanged to reparent.

Done.

[oshima, 2011-11-08 23:34:24]

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
File ui/aura_shell/always_on_top_controller.cc (right):

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
ui/aura_shell/always_on_top_controller.cc:33:
DCHECK(default_container->children().size() == 0);
empty()

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
File ui/aura_shell/always_on_top_controller.h (right):

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
ui/aura_shell/always_on_top_controller.h:24: class AlwaysOnTopController :
public aura::WindowObserver {
I still feel that we may want to have one controller for toplevel window. You
better ask ben or sky for their opinion now, than later.

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/shell_unittes...
File ui/aura_shell/shell_unittest.cc (right):

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:60: EXPECT_EQ(GetAlwaysOnTopContainer(),
widget->GetNativeWindow()->parent());
can you add a few more test cases such as
keep_on_top == false, with different window types?

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
views/widget/native_widget_aura.cc:149: SetAlwaysOnTop(true);
SetAlwaysOnTop(params.keep_on_top);

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
views/widget/native_widget_aura.cc:424: switch (window_->type()) {
I think we want the shell handle these cases, not in widget.

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
views/widget/native_widget_aura.cc:428: reinterpret_cast<void*>(on_top));
window->SetIntProperty(aura::kAlwaysOnTopKey, on_top); is simpler.

[xiyuan, 2011-11-09 00:01:41]

+sky, ben for comments

Oshima and I are wonder where is the best place to have shell monitor windows
property changes and act accordingly.

Right now, I have put all the logic into a AlwaysOnTopController that only
handles the always-on-top property. Maybe I should name it to something more
generically, such as TopLevelWindowController so that we could put more similar
property handling code in it?

Thanks.

[Ben Goodger (Google), 2011-11-09 17:18:08]

Approach LGTM.

http://codereview.chromium.org/8387043/diff/26002/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8387043/diff/26002/ui/aura_shell/shell.cc#newc...
ui/aura_shell/shell.cc:114:
GetContainer(internal::kShellWindowId_DefaultContainer),
nit: 4-space indent

[xiyuan, 2011-11-09 17:42:18]

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
File ui/aura_shell/always_on_top_controller.cc (right):

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
ui/aura_shell/always_on_top_controller.cc:33:
DCHECK(default_container->children().size() == 0);
On 2011/11/08 23:34:24, oshima wrote:
> empty()

Done.

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
File ui/aura_shell/always_on_top_controller.h (right):

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/always_on_top...
ui/aura_shell/always_on_top_controller.h:24: class AlwaysOnTopController :
public aura::WindowObserver {
On 2011/11/08 23:34:24, oshima wrote:
> I still feel that we may want to have one controller for toplevel window. You
> better ask ben or sky for their opinion now, than later.

Ben seems okay with the approach. Maybe we can have this for now and change it
later when needed?

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/shell_unittes...
File ui/aura_shell/shell_unittest.cc (right):

http://codereview.chromium.org/8387043/diff/21001/ui/aura_shell/shell_unittes...
ui/aura_shell/shell_unittest.cc:60: EXPECT_EQ(GetAlwaysOnTopContainer(),
widget->GetNativeWindow()->parent());
On 2011/11/08 23:34:24, oshima wrote:
> can you add a few more test cases such as
> keep_on_top == false, with different window types?

Done.

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
File views/widget/native_widget_aura.cc (right):

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
views/widget/native_widget_aura.cc:149: SetAlwaysOnTop(true);
On 2011/11/08 23:34:24, oshima wrote:
> SetAlwaysOnTop(params.keep_on_top);

Done.

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
views/widget/native_widget_aura.cc:424: switch (window_->type()) {
On 2011/11/08 23:34:24, oshima wrote:
> I think we want the shell handle these cases, not in widget.

Code is removed. Set always-on-top property on menu and tooltip will be ignored.

http://codereview.chromium.org/8387043/diff/21001/views/widget/native_widget_...
views/widget/native_widget_aura.cc:428: reinterpret_cast<void*>(on_top));
On 2011/11/08 23:34:24, oshima wrote:
> window->SetIntProperty(aura::kAlwaysOnTopKey, on_top); is simpler.

Done.

http://codereview.chromium.org/8387043/diff/26002/ui/aura_shell/shell.cc
File ui/aura_shell/shell.cc (right):

http://codereview.chromium.org/8387043/diff/26002/ui/aura_shell/shell.cc#newc...
ui/aura_shell/shell.cc:114:
GetContainer(internal::kShellWindowId_DefaultContainer),
On 2011/11/09 17:18:09, Ben Goodger (Google) wrote:
> nit: 4-space indent

Done.

[oshima, 2011-11-10 00:00:48]

LGTM

[commit-bot: I haz the power, 2011-11-10 00:46:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/8387043/29001

[commit-bot: I haz the power, 2011-11-10 00:46:43]

Can't apply patch for file ui/aura/aura_constants.cc.
While running patch -p1 --forward --force;
patching file ui/aura/aura_constants.cc
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file ui/aura/aura_constants.cc.rej

[commit-bot: I haz the power, 2011-11-10 19:30:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/8387043/32002

[commit-bot: I haz the power, 2011-11-10 20:48:54]

Can't apply patch for file ui/aura_shell/aura_shell.gyp.
While running patch -p1 --forward --force;
patching file ui/aura_shell/aura_shell.gyp
Hunk #2 FAILED at 122.
1 out of 2 hunks FAILED -- saving rejects to file
ui/aura_shell/aura_shell.gyp.rej

[xiyuan, 2011-11-10 23:51:27]

CL updated with Ben's StackingController change:
- Moved AlwaysOnTopController from Shell into StackingController;
- Update test so that Shell is properly teared down;

Please take another look. Thanks.

[oshima, 2011-11-11 21:08:07]

http://codereview.chromium.org/8387043/diff/32003/ui/aura/test/aura_test_base.h
File ui/aura/test/aura_test_base.h (right):

http://codereview.chromium.org/8387043/diff/32003/ui/aura/test/aura_test_base...
ui/aura/test/aura_test_base.h:33: void FlushMessageLoop();
Can you rename to RunAllPendingInMessageLoop()?
(to keep consistency with ui_tests_utils::RunAllPendingInMessageLoop)

[xiyuan, 2011-11-14 21:47:38]

CL updated:
- Added a AuraShellTestBase that creates Shell in Setup and release it in
Teardown;
- Use AuraShellTestBase for ShellTest and StackingControllerTest that need Shell
instance in test.

This should fix win_aura crash also (Thanks Oshima).

[Ben Goodger (Google), 2011-11-14 22:07:58]

lgtm

http://codereview.chromium.org/8387043/diff/45001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/8387043/diff/45001/ui/base/x/x11_util.h#newcode71
ui/base/x/x11_util.h:71: // TODO(xiyuan): Fix the stale XCursorCache problem
properly.
I haven't followed what "properly" means... perhaps you could explain or
reference a bug.

[oshima, 2011-11-14 22:45:42]

LGTM

[xiyuan, 2011-11-14 22:47:45]

Thanks guys.

Will try to land it now. :)

http://codereview.chromium.org/8387043/diff/45001/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/8387043/diff/45001/ui/base/x/x11_util.h#newcode71
ui/base/x/x11_util.h:71: // TODO(xiyuan): Fix the stale XCursorCache problem
properly.
On 2011/11/14 22:07:58, Ben Goodger (Google) wrote:
> I haven't followed what "properly" means... perhaps you could explain or
> reference a bug.

Done. Updated comment to include bug number.

[commit-bot: I haz the power, 2011-11-14 22:48:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/8387043/45005

[commit-bot: I haz the power, 2011-11-14 22:52:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/8387043/45008

[commit-bot: I haz the power, 2011-11-15 00:20:19]

Change committed as 109990