Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10542134:
Rely on losing focus, not on becoming unfullscreened, to destroy a fullscreen window. (Closed)
|
Created:
8 years, 6 months ago by jbauman Modified:
8 years, 6 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, jochen+watch-content_chromium.org, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRely on losing focus, not on becoming unfullscreened, to destroy a fullscreen window.
On metacity we get spurious unfullscreen events that were preventing the window from going fullscreen, and alt-tab didn't cause the window to unfullscreen, so it wasn't destroyed by alt-tab. We should ignore the unfullscreen event and instead rely on FocusOut to determine when to destroy the window.
BUG=117021
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143306
Patch Set 1 #Patch Set 2 : use _NET_ACTIVE_WINDOW #
Total comments: 3
Patch Set 3 : Fix nits #
Total comments: 1
Patch Set 4 : also make made_active_ work for popups #
Total comments: 1
Patch Set 5 : fix asterisk #
Messages
Total messages: 19 (0 generated)
What triggers OnFocusOut() getting called? Is it X FocusOut events, or changes to the _NET_ACTIVE_WINDOW property? If it's the former, this approach may have issues, as FocusOut events get generated whenever the keyboard is briefly grabbed in response to a key grab being activated (e.g. window manager accelerators like Alt-Tab, and sometimes even volume up/down keys -- I believe that this is why NPAPI Flash had a bug for many years on Linux where it'd exit fullscreen mode when users adjusted their volume).
On 2012/06/13 02:40:22, Daniel Erat wrote: > What triggers OnFocusOut() getting called? Is it X FocusOut events, or changes > to the _NET_ACTIVE_WINDOW property? If it's the former, this approach may have > issues, as FocusOut events get generated whenever the keyboard is briefly > grabbed in response to a key grab being activated (e.g. window manager > accelerators like Alt-Tab, and sometimes even volume up/down keys -- I believe > that this is why NPAPI Flash had a bug for many years on Linux where it'd exit > fullscreen mode when users adjusted their volume). Thanks, I didn't realize the distinction between FocusOut and _NET_ACTIVE_WINDOW. I had been using FocusOut, but this patch switches it to using _NET_ACTIVE_WINDOW.
http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_gtk.cc (right): http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_gtk.cc:49: #include "ui/base/x/active_window_watcher_x.h" nit: fix alphabetization http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_gtk.cc:890: ui::ActiveWindowWatcherX::RemoveObserver(this); you're also unregistering if it's a popup, but it looks like you're only registering fullscreen windows http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_gtk.h (right): http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_gtk.h:132: // ActiveWindowWatcherXObserve implementation. nit: s/Observe/Observer/
On 2012/06/13 19:04:09, Daniel Erat wrote: > http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_gtk.cc (right): > > http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_gtk.cc:49: #include > "ui/base/x/active_window_watcher_x.h" > nit: fix alphabetization > > http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_gtk.cc:890: > ui::ActiveWindowWatcherX::RemoveObserver(this); > you're also unregistering if it's a popup, but it looks like you're only > registering fullscreen windows > > http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_gtk.h (right): > > http://codereview.chromium.org/10542134/diff/3002/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_gtk.h:132: // > ActiveWindowWatcherXObserve implementation. > nit: s/Observe/Observer/ Ok, these are all fixed.
https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... File content/browser/renderer_host/render_widget_host_view_gtk.cc (right): https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_gtk.cc:578: made_active_(false), actually, having this variable contain false information unless this is a fullscreen window makes me nervous. any reason why you can't register as an observer in DoSharedInit() and unregister in the d'tor (maybe with another variable to make sure that you don't unregister unless init was actually called)?
On 2012/06/13 19:18:43, Daniel Erat wrote: > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > File content/browser/renderer_host/render_widget_host_view_gtk.cc (right): > > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_gtk.cc:578: > made_active_(false), > actually, having this variable contain false information unless this is a > fullscreen window makes me nervous. any reason why you can't register as an > observer in DoSharedInit() and unregister in the d'tor (maybe with another > variable to make sure that you don't unregister unless init was actually > called)? It won't be meaningful in the non-fullscreen non-popup case anyway, because this code won't create a parent window for the view.
On 2012/06/13 19:23:13, jbauman wrote: > On 2012/06/13 19:18:43, Daniel Erat wrote: > > > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > > File content/browser/renderer_host/render_widget_host_view_gtk.cc (right): > > > > > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > > content/browser/renderer_host/render_widget_host_view_gtk.cc:578: > > made_active_(false), > > actually, having this variable contain false information unless this is a > > fullscreen window makes me nervous. any reason why you can't register as an > > observer in DoSharedInit() and unregister in the d'tor (maybe with another > > variable to make sure that you don't unregister unless init was actually > > called)? > > It won't be meaningful in the non-fullscreen non-popup case anyway, because this > code won't create a parent window for the view. Good point. Please document this in the comment. It's probably worthwhile to make it be accurate for popups too.
On 2012/06/13 19:25:17, Daniel Erat wrote: > On 2012/06/13 19:23:13, jbauman wrote: > > On 2012/06/13 19:18:43, Daniel Erat wrote: > > > > > > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > > > File content/browser/renderer_host/render_widget_host_view_gtk.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > > > content/browser/renderer_host/render_widget_host_view_gtk.cc:578: > > > made_active_(false), > > > actually, having this variable contain false information unless this is a > > > fullscreen window makes me nervous. any reason why you can't register as an > > > observer in DoSharedInit() and unregister in the d'tor (maybe with another > > > variable to make sure that you don't unregister unless init was actually > > > called)? > > > > It won't be meaningful in the non-fullscreen non-popup case anyway, because > this > > code won't create a parent window for the view. > > Good point. Please document this in the comment. It's probably worthwhile to > make it be accurate for popups too. Ok, done.
On 2012/06/13 20:18:22, jbauman wrote: > On 2012/06/13 19:25:17, Daniel Erat wrote: > > On 2012/06/13 19:23:13, jbauman wrote: > > > On 2012/06/13 19:18:43, Daniel Erat wrote: > > > > > > > > > > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > > > > File content/browser/renderer_host/render_widget_host_view_gtk.cc (right): > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10542134/diff/9002/content/browser/ren... > > > > content/browser/renderer_host/render_widget_host_view_gtk.cc:578: > > > > made_active_(false), > > > > actually, having this variable contain false information unless this is a > > > > fullscreen window makes me nervous. any reason why you can't register as > an > > > > observer in DoSharedInit() and unregister in the d'tor (maybe with another > > > > variable to make sure that you don't unregister unless init was actually > > > > called)? > > > > > > It won't be meaningful in the non-fullscreen non-popup case anyway, because > > this > > > code won't create a parent window for the view. > > > > Good point. Please document this in the comment. It's probably worthwhile to > > make it be accurate for popups too. > > Ok, done. Does this version of the patch look good?
lgtm http://codereview.chromium.org/10542134/diff/6003/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_gtk.h (right): http://codereview.chromium.org/10542134/diff/6003/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_gtk.h:133: virtual void ActiveWindowChanged(GdkWindow *active_window) OVERRIDE; nit: move asterisk to left side of space
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbauman@chromium.org/10542134/14002
Presubmit check for 10542134-14002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
content/browser/renderer_host
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
Oops, adding jam for OWNERS review.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jbauman@chromium.org/10542134/14002
Change committed as 143306
On 2012/06/21 00:02:18, I haz the power (commit-bot) wrote: > Change committed as 143306 Hi, John. With this change, we have different behavior on different platform, right? At least on Windows, we still rely on being unfullscreened to destroy the window. (I don't know about Mac.) IMHO, such difference should be avoided. What do you think?
On 2012/06/22 16:41:32, yzshen1 wrote: > On 2012/06/21 00:02:18, I haz the power (commit-bot) wrote: > > Change committed as 143306 > > Hi, John. > With this change, we have different behavior on different platform, right? At > least on Windows, we still rely on being unfullscreened to destroy the window. > (I don't know about Mac.) > > IMHO, such difference should be avoided. What do you think? In practice, for me the behavior is now exactly the same on Windows and Linux. The same events that cause an unfullscreen on Windows cause an unfocus on Linux. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
