Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18741006:
[GTK] Report isMinimized and correctly restore app windows. (Closed)
|
Created:
7 years, 5 months ago by zhchbin Modified:
7 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, jeremya+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionIn the unity window manager in ubuntu, when you minimize a window it doesn't
let the application know via the normal GTK API's. In this patch, it will use
XEvent to catch the PropertyNotify event and check |_NET_WM_STATE| of the window
to determine whether the state of the window is minimized.
gtk_window_present is used in Maximize and Restore to cause windows to show,
matching the behavior on windows.
BUG=180268, 162794
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213028
Patch Set 1 #Patch Set 2 : Enable PlatformAppBrowserTest.WindowsApiProperties on GTK. #
Total comments: 18
Patch Set 3 : Update as comments. #Patch Set 4 : Add comments and fix some nits. #Patch Set 5 : Ignore PlatformAppBrowserTest.WindowsApiProperties. It can pass in local. #
Total comments: 7
Patch Set 6 : Fix nit as comments. #
Total comments: 4
Patch Set 7 : Check _NET_SUPPORTED first of all to determine whether the absence of _NET_WM_STATE_HIDDEN infers t… #Patch Set 8 : Update comment. #
Total comments: 2
Patch Set 9 : Add a bool member for safer remove filter. #
Messages
Total messages: 22 (0 generated)
Please review. Test on Ubuntu 12.04 Unity and Ubuntu 13.04 Gnome Shell 3. Will add a test case as soon as I can. By the way, I recreate a code review for better description.
https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:245: gtk_window_present(window_); I'm not sure why this is needed here? https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:259: gtk_window_present(window_); We should only present if the window isn't hidden by us, and only if it is minimized and we are relying on present to do the job deiconify is failing to do. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:281: GdkFilterReturn NativeAppWindowGtk::OnXEvent(GdkXEvent* xevent, Please add a comment explaining why this method is here and what it is doing. e.g. ... a work around for gdk not reporting minimize state changes, listening for property notify events for state and checking to see if the state includes hidden, and from that setting or clearing the iconified bit. Probably reference https://bugs.launchpad.net/unity/+bug/998073 https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:283: XEvent* xev = static_cast<XEvent*>(xevent); name it xevent, and name the method parameter gdkxevent https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:284: ::Atom state = atom_cache_.GetAtom("_NET_WM_STATE"); variable only used once, right? Consider calling GetAtom in the location it's needed to reduce lines of code and indirection required to read it. Same for state_hidden. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:295: state_hidden); One thing I don't understand is why the hidden atom is not set when a window is hidden with gtk_widget_hide. I do not want to report isMinimized when we have hidden a window.
Note that it failed on the "linux_rel", can you please offer me the information of environment about "linux_rel" and try it again. I also updated the test case. What's more, does it work on your local ubuntu? It works fine on my side. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:245: gtk_window_present(window_); On 2013/07/16 22:11:21, scheib wrote: > I'm not sure why this is needed here? After minimize the app window, calling maximize should show the window. Without gtk_window_present, the window will still hide. I add these code to make sure the behavior is the same on Windows platform. If I'm wrong, please let me know and I will remove these. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:259: gtk_window_present(window_); On 2013/07/16 22:11:21, scheib wrote: > We should only present if the window isn't hidden by us, and only if it is > minimized and we are relying on present to do the job deiconify is failing to > do. However, I find these behavior are different on the windows and linux platform. So I add it to keep consistency. For example, on the windows platform, calling restore after hide will show the window, but on the linux, the window will still hide. Is this by design? https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:281: GdkFilterReturn NativeAppWindowGtk::OnXEvent(GdkXEvent* xevent, On 2013/07/16 22:11:21, scheib wrote: > Please add a comment explaining why this method is here and what it is doing. > e.g. ... a work around for gdk not reporting minimize state changes, listening > for property notify events for state and checking to see if the state includes > hidden, and from that setting or clearing the iconified bit. Probably reference > https://bugs.launchpad.net/unity/+bug/998073 > Done. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:283: XEvent* xev = static_cast<XEvent*>(xevent); On 2013/07/16 22:11:21, scheib wrote: > name it xevent, and name the method parameter gdkxevent Done. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:284: ::Atom state = atom_cache_.GetAtom("_NET_WM_STATE"); On 2013/07/16 22:11:21, scheib wrote: > variable only used once, right? Consider calling GetAtom in the location it's > needed to reduce lines of code and indirection required to read it. Same for > state_hidden. Done. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:295: state_hidden); On 2013/07/16 22:11:21, scheib wrote: > One thing I don't understand is why the hidden atom is not set when a window is > hidden with gtk_widget_hide. I do not want to report isMinimized when we have > hidden a window. Glance through the source code of "gtk_widget_hide", I didn't find anything that it sets _NET_WM_STATE_HIDDEN. And on the implementation of DesktopRootWindowHostX11::IsMinimized, it also depend on "_NET_WM_STATE_HIDDEN" to check. Code: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...
lgtm. You will need an owners review as well. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:245: gtk_window_present(window_); On 2013/07/17 02:39:32, zhchbin wrote: > On 2013/07/16 22:11:21, scheib wrote: > > I'm not sure why this is needed here? > > After minimize the app window, calling maximize should show the window. Without > gtk_window_present, the window will still hide. I add these code to make sure > the behavior is the same on Windows platform. If I'm wrong, please let me know > and I will remove these. > Ah! I agree that we should move the different systems to be coherent with each other, and see that Windows does this, and so agree with this change. Add a comment to explain it. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:259: gtk_window_present(window_); On 2013/07/17 02:39:32, zhchbin wrote: > On 2013/07/16 22:11:21, scheib wrote: > > We should only present if the window isn't hidden by us, and only if it is > > minimized and we are relying on present to do the job deiconify is failing to > > do. > > However, I find these behavior are different on the windows and linux platform. > So I add it to keep consistency. For example, on the windows platform, calling > restore after hide will show the window, but on the linux, the window will still > hide. Is this by design? Hmm, on win7 with window state sample (from github and on web store) if I hide and then restore the window comes back all black -- another bug. (http://crbug.com/261013). If you are able to have windows hide and then restore shows the window then keep the code this way on linux as well but add a comment explaining it (consistent with windows). https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:281: GdkFilterReturn NativeAppWindowGtk::OnXEvent(GdkXEvent* xevent, On 2013/07/17 02:39:32, zhchbin wrote: > On 2013/07/16 22:11:21, scheib wrote: > > Please add a comment explaining why this method is here and what it is doing. > > e.g. ... a work around for gdk not reporting minimize state changes, listening > > for property notify events for state and checking to see if the state includes > > hidden, and from that setting or clearing the iconified bit. Probably > reference > > https://bugs.launchpad.net/unity/+bug/998073 > > > > Done. Thank you, though the comment needs a bit more clean up: // Work around gtk+ not reporting minimize state changes, by listening for // property notify events for state and checking to see if the state includes // hidden. Then from that setting or clearing the iconified bit. // http://crbug.com/162794.
Also, I've updated the issue description some to be more inclusive of the changes, please review. (you can change on codereview website via 'edit issue' in top left)
https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:245: gtk_window_present(window_); On 2013/07/17 04:47:39, scheib wrote: > On 2013/07/17 02:39:32, zhchbin wrote: > > On 2013/07/16 22:11:21, scheib wrote: > > > I'm not sure why this is needed here? > > > > After minimize the app window, calling maximize should show the window. > Without > > gtk_window_present, the window will still hide. I add these code to make sure > > the behavior is the same on Windows platform. If I'm wrong, please let me know > > and I will remove these. > > > > Ah! I agree that we should move the different systems to be coherent with each > other, and see that Windows does this, and so agree with this change. Add a > comment to explain it. Done. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:259: gtk_window_present(window_); On 2013/07/17 04:47:39, scheib wrote: > On 2013/07/17 02:39:32, zhchbin wrote: > > On 2013/07/16 22:11:21, scheib wrote: > > > We should only present if the window isn't hidden by us, and only if it is > > > minimized and we are relying on present to do the job deiconify is failing > to > > > do. > > > > However, I find these behavior are different on the windows and linux > platform. > > So I add it to keep consistency. For example, on the windows platform, calling > > restore after hide will show the window, but on the linux, the window will > still > > hide. Is this by design? > > Hmm, on win7 with window state sample (from github and on web store) if I hide > and then restore the window comes back all black -- another bug. > (http://crbug.com/261013). If you are able to have windows hide and then restore > shows the window then keep the code this way on linux as well but add a comment > explaining it (consistent with windows). Done. https://codereview.chromium.org/18741006/diff/3001/chrome/browser/ui/gtk/exte... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:281: GdkFilterReturn NativeAppWindowGtk::OnXEvent(GdkXEvent* xevent, On 2013/07/17 04:47:39, scheib wrote: > On 2013/07/17 02:39:32, zhchbin wrote: > > On 2013/07/16 22:11:21, scheib wrote: > > > Please add a comment explaining why this method is here and what it is > doing. > > > e.g. ... a work around for gdk not reporting minimize state changes, > listening > > > for property notify events for state and checking to see if the state > includes > > > hidden, and from that setting or clearing the iconified bit. Probably > > reference > > > https://bugs.launchpad.net/unity/+bug/998073 > > > > > > > Done. > > Thank you, though the comment needs a bit more clean up: > > // Work around gtk+ not reporting minimize state changes, by listening for > // property notify events for state and checking to see if the state includes > // hidden. Then from that setting or clearing the iconified bit. > // http://crbug.com/162794. Done.
Regarding PlatformAppBrowserTest.WindowsApiProperties failing with a time out on linux_rel: I believe you should disable the automated test in app_window_apitest.cc by removing "|| defined(TOOLKIT_GTK)". The test is already flaky and disabled on other platforms and configurations. We have learned from other tests related to window manager control that they do not hold up to automation's requirements of at least 99.99% uptime. We will rely on manual testing for this change. I've included you on an email to infrastructure requesting the details of the linux_rel configuration to assist in us checking it out locally.
On 2013/07/18 02:02:21, zhchbin wrote: +estade for owner review.
this is a bit over my head, +derat for review. https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:306: if (it != atom_list.end()) I prefer use of the ternary operator here. If no ternary, you need curlies because the else has 2 lines.
the general approach here looks fine to me https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:266: gtk_window_present(window_); i don't understand this call. http://crbug.com/261013 is about unhiding a window. i think that the "Restore" in this method's name refers to unmaximizing it -- see ui::BaseWindow::Restore(). i guess that this method does also include code to un-minimize a window, but can't this gtk_window_present() call be moved to the IfMinimized() case, then? it doesn't seem like it should be needed after you unmaximize a window. https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:292: // hidden. Then from that setting or clearing the iconified bit. nit: reword this as: Work around GTK+ not reporting minimization state changes. Listen for _NET_WM_STATE property changes and use _NET_WM_STATE_HIDDEN's presence to set or clear the iconified bit. https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:306: if (it != atom_list.end()) On 2013/07/18 18:08:26, Evan Stade wrote: > I prefer use of the ternary operator here. If no ternary, you need curlies > because the else has 2 lines. agreed
https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:266: gtk_window_present(window_); On 2013/07/18 20:18:39, Daniel Erat wrote: > i don't understand this call. http://crbug.com/261013 is about unhiding a > window. i think that the "Restore" in this method's name refers to unmaximizing > it -- see ui::BaseWindow::Restore(). i guess that this method does also include > code to un-minimize a window, but can't this gtk_window_present() call be moved > to the IfMinimized() case, then? it doesn't seem like it should be needed after > you unmaximize a window. From our API docs, "Restore" means: "Restore the window, exiting a maximized, minimized, or fullscreen state.", not only unmaximizing. Yes, the gtk_window_present() method include un-minimization of window. However, this code is added to keep restoration behavior consistency with Windows platform as my comments here. On Windows, no matter what state the window stays, calling restore will show the window. Though @ scheib said it's a bug if the window is hidden, he said that we can keep this call temporarily. So I left a TODO here. Another case, minimize -> maximize -> restore, without this call, the window will not show up. Test on GNOME Shell 3.8.3. https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:292: // hidden. Then from that setting or clearing the iconified bit. On 2013/07/18 20:18:39, Daniel Erat wrote: > nit: reword this as: > > Work around GTK+ not reporting minimization state changes. Listen > for _NET_WM_STATE property changes and use _NET_WM_STATE_HIDDEN's > presence to set or clear the iconified bit. Done. https://codereview.chromium.org/18741006/diff/34001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:306: if (it != atom_list.end()) On 2013/07/18 20:18:39, Daniel Erat wrote: > On 2013/07/18 18:08:26, Evan Stade wrote: > > I prefer use of the ternary operator here. If no ternary, you need curlies > > because the else has 2 lines. > > agreed Done.
https://codereview.chromium.org/18741006/diff/40001/chrome/browser/ui/gtk/ext... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/40001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:304: atom_cache_.GetAtom("_NET_WM_STATE_HIDDEN")); i didn't notice it at first, but i think that this is wrong on window managers that support _NET_WM_STATE but don't support _NET_WM_STATE_HIDDEN. i think that you need to check _NET_SUPPORTED (see http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#idp3331912) to determine whether the absence of _NET_WM_STATE_HIDDEN infers that the window is not iconified. https://codereview.chromium.org/18741006/diff/40001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:306: (it != atom_list.end()) ? GDK_WINDOW_STATE_ICONIFIED : nit: move this to the previous line: state_ = (it != atom_list.end()) ? GDK_WINDOW_STATE_ICONIFIED : static_cast<GdkWindowState>(state_ & ~GDK_WINDOW_STATE_ICONIFIED);
https://codereview.chromium.org/18741006/diff/40001/chrome/browser/ui/gtk/ext... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/40001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:304: atom_cache_.GetAtom("_NET_WM_STATE_HIDDEN")); On 2013/07/19 14:03:46, Daniel Erat wrote: > i didn't notice it at first, but i think that this is wrong on window managers > that support _NET_WM_STATE but don't support _NET_WM_STATE_HIDDEN. i think that > you need to check _NET_SUPPORTED (see > http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#idp3331912) to > determine whether the absence of _NET_WM_STATE_HIDDEN infers that the window is > not iconified. Done. Sorry for my mistake. Now I check it in the constructor, is it OK? https://codereview.chromium.org/18741006/diff/40001/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:306: (it != atom_list.end()) ? GDK_WINDOW_STATE_ICONIFIED : On 2013/07/19 14:03:46, Daniel Erat wrote: > nit: move this to the previous line: > > state_ = (it != atom_list.end()) ? GDK_WINDOW_STATE_ICONIFIED : > static_cast<GdkWindowState>(state_ & ~GDK_WINDOW_STATE_ICONIFIED); Done.
generally LGTM with one more suggestion https://codereview.chromium.org/18741006/diff/25002/chrome/browser/ui/gtk/ext... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/25002/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:172: gdk_window_remove_filter(NULL, is this safe to call when the filter wasn't added? https://developer.gnome.org/gdk/stable/gdk-Windows.html#gdk-window-remove-filter doesn't make a statement one way or the other. unless you've confirmed in the source that this is explicitly permitted, it might be safer to add a bool member to track whether the filter was added
On 2013/07/20 01:29:29, Daniel Erat wrote: > generally LGTM with one more suggestion > > https://codereview.chromium.org/18741006/diff/25002/chrome/browser/ui/gtk/ext... > File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): > > https://codereview.chromium.org/18741006/diff/25002/chrome/browser/ui/gtk/ext... > chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:172: > gdk_window_remove_filter(NULL, > is this safe to call when the filter wasn't added? > https://developer.gnome.org/gdk/stable/gdk-Windows.html#gdk-window-remove-filter > doesn't make a statement one way or the other. unless you've confirmed in the > source that this is explicitly permitted, it might be safer to add a bool member > to track whether the filter was added Thanks, it is safe after I checked the source code of GTK+ again and did an experiment. However, I will add a bool member as your suggestion before committed.
https://codereview.chromium.org/18741006/diff/25002/chrome/browser/ui/gtk/ext... File chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc (right): https://codereview.chromium.org/18741006/diff/25002/chrome/browser/ui/gtk/ext... chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc:172: gdk_window_remove_filter(NULL, On 2013/07/20 01:29:30, Daniel Erat wrote: > is this safe to call when the filter wasn't added? > https://developer.gnome.org/gdk/stable/gdk-Windows.html#gdk-window-remove-filter > doesn't make a statement one way or the other. unless you've confirmed in the > source that this is explicitly permitted, it might be safer to add a bool member > to track whether the filter was added Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/18741006/55001
Ping Evan Stade for
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/browser/ui/gtk/extensions/native_app_window_gtk.cc
chrome/browser/ui/gtk/extensions/native_app_window_gtk.h
rslgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/18741006/55001
Message was sent while issue was closed.
Change committed as 213028 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
