Add always-on-top property to app windows

Add always-on-top property to app windows

The chrome.app.window API has been extended to include:
- A new alwaysOnTop option for the create() function.
- An AppWindow.isAlwaysOnTop() function to query the state of this
property.
- An AppWindow.setAlwaysOnTop() function to change this property after
creation of the window.

Changes involving native app windows:
- Added apps::NativeAppWindow::SetAlwaysOnTop().
- Implemented IsAlwaysOnTop(), a function inherited from ui::BaseWindow
but was left unimplemented for native app windows.

Changes involving views:
- Added IsAlwaysOnTop(). SetAlwaysOnTop() already existed.

BUG=171597
TEST=browser_tests (PlatformAppBrowserTest.WindowsApiAlwaysOnTop).
Test manually by creating an app that enables the alwaysOnTop option
on window creation and changes the property after creation.
Test Windows, Mac, ChromeOS and Linux.

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

[tmdiep, 2013-10-08 05:04:58]

This is a patch for chrome.app.window alwaysOnTop property.  API proposal:
https://docs.google.com/document/d/1F54IJ4nDQGU8hWwOOcYopyXXQirBFPjvZih4zOyfK...

Would you mind reviewing this patch (or suggest someone else)? Thanks.

benwells: For native app windows
sky: For views
erg: For X11 files in views

[sky, 2013-10-08 15:46:09]

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

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/widget.cc#...
ui/views/widget/widget.cc:662: return native_widget_->IsAlwaysOnTop();
Can we make widget cache the value of SetAlwaysOnTop so that we don't need to
always ask the underlying NativeWidget?

[Elliot Glaysher, 2013-10-08 18:05:00]

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/desktop_au...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/desktop_au...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:928:
XChangeProperty (xdisplay_,
This will clobber the XChangeProperty immediately above. _NET_WM_STATE takes an
array, and you're possibly overwriting the previous one length array. We have a
nice utility function, ui::SetAtomArrayProperty, in x11_util.h which can handle
that.

Documentation:
http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936

[benwells, 2013-10-09 02:00:49]

https://codereview.chromium.org/26427002/diff/27001/chrome/browser/extensions...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/26427002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:209:
if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
Can we use _api_features.json for this check?

https://codereview.chromium.org/26427002/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/26427002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/app_window/app_window_api.cc:248: if
(GetCurrentChannel() <= chrome::VersionInfo::CHANNEL_DEV &&
It's a pity we can't use _api_features.json for this :(

https://codereview.chromium.org/26427002/diff/27001/chrome/common/extensions/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/26427002/diff/27001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:112: [nodoc] boolean? alwaysOnTop;
Why are these nodoc?

https://codereview.chromium.org/26427002/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/windows_api_always_on_top/background.js
(right):

https://codereview.chromium.org/26427002/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/windows_api_always_on_top/background.js:19:
setTimeout(callbackPass(function() {
Is there any way to test this without relying on a timeout of 500ms? This will
be flaky on the bots.

As we'd only be testing whether setting is set and retrieved properly, not that
the window is actually stay on top, maybe it isn't worth testing.

[tmdiep, 2013-10-09 02:15:04]

Thanks Elliot and Sky. Review comments addressed in patch 4.

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/desktop_au...
File ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc (right):

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/desktop_au...
ui/views/widget/desktop_aura/desktop_root_window_host_x11.cc:928:
XChangeProperty (xdisplay_,
On 2013/10/08 18:05:00, Elliot Glaysher wrote:
> This will clobber the XChangeProperty immediately above. _NET_WM_STATE takes
an
> array, and you're possibly overwriting the previous one length array. We have
a
> nice utility function, ui::SetAtomArrayProperty, in x11_util.h which can
handle
> that.
> 
> Documentation:
> http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#id2731936

Thanks for that. Fixed.

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

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/widget.cc#...
ui/views/widget/widget.cc:662: return native_widget_->IsAlwaysOnTop();
On 2013/10/08 15:46:10, sky wrote:
> Can we make widget cache the value of SetAlwaysOnTop so that we don't need to
> always ask the underlying NativeWidget?

In Patch 4 I've retrieved and cached the value from the underlying NativeWidget
in SetAlwaysOnTop.  But did you mean just cache the argument passed to
SetAlwaysOnTop and remove IsAlwaysOnTop from all the views classes entirely?

Is there any case where the actual value would not match the argument? The only
reason I can think of is when the underlying platform doesn't implement an
always-on-top flag.

[tmdiep, 2013-10-09 06:18:07]

Thanks Ben. Patch 5 addresses review comments. As discussed, SetAlwaysOnTop()
was moved from NativeAppWindow to ui::BaseWindow. It is not implemented by
BrowserWindows, but it is implemented by Panels.

https://codereview.chromium.org/26427002/diff/27001/chrome/browser/extensions...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/26427002/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:209:
if (GetCurrentChannel() > chrome::VersionInfo::CHANNEL_DEV) {
On 2013/10/09 02:00:50, benwells wrote:
> Can we use _api_features.json for this check?

Done.

https://codereview.chromium.org/26427002/diff/27001/chrome/common/extensions/...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/26427002/diff/27001/chrome/common/extensions/...
chrome/common/extensions/api/app_window.idl:112: [nodoc] boolean? alwaysOnTop;
On 2013/10/09 02:00:50, benwells wrote:
> Why are these nodoc?

Removed [nodoc].

https://codereview.chromium.org/26427002/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/windows_api_always_on_top/background.js
(right):

https://codereview.chromium.org/26427002/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/windows_api_always_on_top/background.js:19:
setTimeout(callbackPass(function() {
On 2013/10/09 02:00:50, benwells wrote:
> Is there any way to test this without relying on a timeout of 500ms? This will
> be flaky on the bots.
> 
> As we'd only be testing whether setting is set and retrieved properly, not
that
> the window is actually stay on top, maybe it isn't worth testing.

It is tricky to test whether the window is actually on top, so I didn't really
try.

This was an end-to-end test to verify that after calling SetAlwaysOnTop(),
IsAlwaysOnTop() will return the new value. I know this isn't a good solution,
but I increased the timeout to 2 secs.

[sky, 2013-10-09 13:23:37]

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

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/widget.cc#...
ui/views/widget/widget.cc:662: return native_widget_->IsAlwaysOnTop();
On 2013/10/09 02:15:04, tmdiep wrote:
> On 2013/10/08 15:46:10, sky wrote:
> > Can we make widget cache the value of SetAlwaysOnTop so that we don't need
to
> > always ask the underlying NativeWidget?
> 
> In Patch 4 I've retrieved and cached the value from the underlying
NativeWidget
> in SetAlwaysOnTop.  But did you mean just cache the argument passed to
> SetAlwaysOnTop and remove IsAlwaysOnTop from all the views classes entirely?
> 
> Is there any case where the actual value would not match the argument? The
only
> reason I can think of is when the underlying platform doesn't implement an
> always-on-top flag.

Yes. I mean cache the value from InitParams and SetAlwaysOnTop. I'm assuming the
platform always honor it, if not you would not be able to detect it with my
approach. If you need to really know what the native platform is doing keep your
code.

[tmdiep, 2013-10-10 02:35:08]

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

https://codereview.chromium.org/26427002/diff/3001/ui/views/widget/widget.cc#...
ui/views/widget/widget.cc:662: return native_widget_->IsAlwaysOnTop();
On 2013/10/09 13:23:38, sky wrote:
> Yes. I mean cache the value from InitParams and SetAlwaysOnTop. I'm assuming
the
> platform always honor it, if not you would not be able to detect it with my
> approach. If you need to really know what the native platform is doing keep
your
> code.

I'm inclined to query IsAlwaysOnTop() from the native platform, as it just feels
like a more complete implementation. In the event that we do not to implement
this flag for a particular platform, I prefer the native widget returning false
over #ifdefs in Widget. But it's done for all platforms currently, so either
approach would work really.

[sky, 2013-10-10 02:45:57]

Ok, go with the implementation that always queries the native widget.

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

https://codereview.chromium.org/26427002/diff/57001/ui/views/widget/widget.h#...
ui/views/widget/widget.h:898: bool is_always_on_top_;
This can be nuked now, right?

https://codereview.chromium.org/26427002/diff/57001/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/26427002/diff/57001/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:690: return (GetWindowLong(hwnd(),
GWL_EXSTYLE) & WS_EX_TOPMOST) > 0;
== WS_EX_TOPMOST?

[sky, 2013-10-10 02:47:51]

Couple more

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel.h (left):

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.h:249: void SetAlwaysOnTop(bool on_top);
move implementation to match position.

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.cc:705: NOTIMPLEMENTED();
Why not make this and IsALwaysOnTop real?

[tmdiep, 2013-10-10 04:02:00]

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel.h (left):

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.h:249: void SetAlwaysOnTop(bool on_top);
On 2013/10/10 02:47:52, sky wrote:
> move implementation to match position.

Done.

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/26427002/diff/57001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view.cc:705: NOTIMPLEMENTED();
On 2013/10/10 02:47:52, sky wrote:
> Why not make this and IsALwaysOnTop real?

Can we leave unimplemented for browser windows until there is a need?

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

https://codereview.chromium.org/26427002/diff/57001/ui/views/widget/widget.h#...
ui/views/widget/widget.h:898: bool is_always_on_top_;
On 2013/10/10 02:45:57, sky wrote:
> This can be nuked now, right?

Yes. Done.

https://codereview.chromium.org/26427002/diff/57001/ui/views/win/hwnd_message...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/26427002/diff/57001/ui/views/win/hwnd_message...
ui/views/win/hwnd_message_handler.cc:690: return (GetWindowLong(hwnd(),
GWL_EXSTYLE) & WS_EX_TOPMOST) > 0;
On 2013/10/10 02:45:57, sky wrote:
> == WS_EX_TOPMOST?

Changed to != 0.

[sky, 2013-10-10 16:00:31]

LGTM

[Elliot Glaysher, 2013-10-10 17:59:40]

lgtm

[benwells, 2013-10-11 03:07:06]

lgtm

[commit-bot: I haz the power, 2013-10-11 04:40:39]

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

[commit-bot: I haz the power, 2013-10-11 04:40:48]

Failed to apply patch for
chrome/browser/extensions/extension_function_histogram_value.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/extension_function_histogram_value.h
  Hunk #1 FAILED at 647.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_histogram_value.h.rej

Patch:       chrome/browser/extensions/extension_function_histogram_value.h
Index: chrome/browser/extensions/extension_function_histogram_value.h
diff --git a/chrome/browser/extensions/extension_function_histogram_value.h
b/chrome/browser/extensions/extension_function_histogram_value.h
index
541b9dd220d03d4ab8fca93d6c7ef5f7f331f783..2692eb1d6b24d626875a5d27f41b3c53fa31ed7a
100644
--- a/chrome/browser/extensions/extension_function_histogram_value.h
+++ b/chrome/browser/extensions/extension_function_histogram_value.h
@@ -647,6 +647,7 @@ enum HistogramValue {
   CAST_CHANNEL_SEND,
   CAST_CHANNEL_CLOSE,
   RUNTIME_RESTART,
+  APP_CURRENTWINDOWINTERNAL_SETALWAYSONTOP,
   ENUM_BOUNDARY // Last entry: Add new entries above.
 };

[commit-bot: I haz the power, 2013-10-11 04:51:48]

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

[commit-bot: I haz the power, 2013-10-11 11:27:27]

Change committed as 228160