Abstract fullscreen exit bubble logic to bring Linux's behaviour in line with

Abstract fullscreen exit bubble logic to bring Linux's behaviour in line with
Windows.


BUG=30743
TEST=

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

[jeremya, 2011-08-02 05:41:38]

Hi Tony,

I've built & tested this on win & lin, and there shouldn't be any code changes
on mac.

The shared logic is in ui/FullscreenExitBubble.cc;
ui/views/FullscreenExitBubbleViews.cc and ui/gtk/FullscreenExitBubbleGtk.cc
derive from it and contain the platform-specific code.

Thanks :)

[tony, 2011-08-02 17:04:25]

Looks ok overall, mostly style nits.  Can you find/borrow a second monitor to
test as well?

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
File chrome/browser/ui/fullscreen_exit_bubble.cc (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
chrome/browser/ui/fullscreen_exit_bubble.cc:6: #include "ui/gfx/rect.h"
Blank line after the fullscreen_exit_bubble.h include, then sort the rest.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
chrome/browser/ui/fullscreen_exit_bubble.h:13: #include
"base/memory/scoped_ptr.h"
Sort these includes.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
chrome/browser/ui/fullscreen_exit_bubble.h:19: namespace gfx {
Sort the namespaces (ui comes after gfx).

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
chrome/browser/ui/fullscreen_exit_bubble.h:46: virtual bool
WindowContainsPoint(gfx::Point pos) = 0;
const gfx::Point& pos

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:92:
gdk_display_get_pointer(display, NULL, &x, &y, NULL);
Don't you need to get screen to make sure you're on the right monitor?  Can you
test with multiple monitors?

Also, it looks like gdk_display_get_pointer hits the X server.  On remote X,
this might be a bit janky.  That's fine for now, but maybe you can add a TODO to
consider changing the kPositionCheckHz value if we detect we're using a remote X
server.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h:22: explicit
FullscreenExitBubbleGtk(
Drop explicit since it doesn't do anything anymore.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h:27: protected:
Nit: I would make the interface methods protected and keep the other methods
private since there are no plans of inheriting from this class.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h:41: virtual bool
IsAnimating();
Please add Override to these methods.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (left):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:4: 
Did you mean to remove this line?

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:19: #include
"ui/gfx/screen.h"
Looks like we already have this include.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:37: static const int
kPaddingPixels;  // Number of pixels around all sides of link
Nit: I wonder if we should move this into fullscreen_exit_bubble.h since the
GTK+ code also declares this value.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:106: //
FullscreenExitBubbleViews
--------------------------------------------------------
80 cols

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:114: popup_(NULL) {
I'm surprised you're able to reorder these.  I thought there was a compiler
warning for making sure that members variables are initialized in the same order
as they are declared.  Maybe that's gcc only (you might want to run this by the
linux_view try bot)?

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.h (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:9: #include
"base/basictypes.h"
Do we still need basictypes.h?

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:26: explicit
FullscreenExitBubbleViews(
Drop explicit.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:52: virtual bool
IsAnimating();
Please add OVERRIDE to these methods.

http://codereview.chromium.org/7549005/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7549005/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:2716: 'browser/ui/fullscreen_exit_bubble.cc',
.cc before .h (unix sort)

[jeremya, 2011-08-03 07:32:29]

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
File chrome/browser/ui/fullscreen_exit_bubble.cc (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/fullscreen_ex...
chrome/browser/ui/fullscreen_exit_bubble.cc:6: #include "ui/gfx/rect.h"
On 2011/08/02 17:04:25, tony wrote:
> Blank line after the fullscreen_exit_bubble.h include, then sort the rest.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:92:
gdk_display_get_pointer(display, NULL, &x, &y, NULL);
On 2011/08/02 17:04:25, tony wrote:
> Don't you need to get screen to make sure you're on the right monitor?  Can
you
> test with multiple monitors?

I've tested with multiple monitors, and didn't see any broken behaviour.
Specifically, I tried fullscreening on one screen and moving the mouse to the
top of the other screen, and the bubble did not appear. The two monitors have
the same y-origin.

> Also, it looks like gdk_display_get_pointer hits the X server.  On remote X,
> this might be a bit janky.  That's fine for now, but maybe you can add a TODO
to
> consider changing the kPositionCheckHz value if we detect we're using a remote
X
> server.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h:22: explicit
FullscreenExitBubbleGtk(
On 2011/08/02 17:04:25, tony wrote:
> Drop explicit since it doesn't do anything anymore.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h:27: protected:
On 2011/08/02 17:04:25, tony wrote:
> Nit: I would make the interface methods protected and keep the other methods
> private since there are no plans of inheriting from this class.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/gtk/fullscree...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.h:41: virtual bool
IsAnimating();
On 2011/08/02 17:04:25, tony wrote:
> Please add Override to these methods.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (left):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:4: 
On 2011/08/02 17:04:25, tony wrote:
> Did you mean to remove this line?

Nope! Oops :)

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:19: #include
"ui/gfx/screen.h"
On 2011/08/02 17:04:25, tony wrote:
> Looks like we already have this include.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:37: static const int
kPaddingPixels;  // Number of pixels around all sides of link
On 2011/08/02 17:04:25, tony wrote:
> Nit: I wonder if we should move this into fullscreen_exit_bubble.h since the
> GTK+ code also declares this value.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:106: //
FullscreenExitBubbleViews
--------------------------------------------------------
On 2011/08/02 17:04:25, tony wrote:
> 80 cols

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:114: popup_(NULL) {
On 2011/08/02 17:04:25, tony wrote:
> I'm surprised you're able to reorder these.  I thought there was a compiler
> warning for making sure that members variables are initialized in the same
order
> as they are declared.  Maybe that's gcc only (you might want to run this by
the
> linux_view try bot)?

Since I didn't reorder them for any particular reason, I've just put them back
to the same order they were declared in.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.h (right):

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:9: #include
"base/basictypes.h"
On 2011/08/02 17:04:25, tony wrote:
> Do we still need basictypes.h?

Guess not. Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:26: explicit
FullscreenExitBubbleViews(
On 2011/08/02 17:04:25, tony wrote:
> Drop explicit.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/browser/ui/views/fullscr...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:52: virtual bool
IsAnimating();
On 2011/08/02 17:04:25, tony wrote:
> Please add OVERRIDE to these methods.

Done.

http://codereview.chromium.org/7549005/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/7549005/diff/1/chrome/chrome_browser.gypi#newc...
chrome/chrome_browser.gypi:2716: 'browser/ui/fullscreen_exit_bubble.cc',
On 2011/08/02 17:04:25, tony wrote:
> .cc before .h (unix sort)

Done.

[tony, 2011-08-03 17:33:14]

LGTM with the following comments addressed.

Also, can you verify that quitting while the animation is running doesn't crash?
 On Linux, you can press ctrl+shift+q while the popup is sliding up to test
this.

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_exit_bubble.cc (right):

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_exit_bubble.cc:8: #include
"chrome/app/chrome_command_ids.h"
Nit: Sort includes.

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_exit_bubble.h:13: #include
"base/memory/scoped_ptr.h"
Nit: Sort includes.

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_exit_bubble.h:71:
CommandUpdater::CommandUpdaterDelegate* delegate_;
Can we make most of these member variables private?

[tony, 2011-08-03 17:53:21]

Also, I'm not a huge fan of the polling every 100ms in general.  Another way to
do this would be to get a notification on mouse movements.  You can do this by
hooking into the MessageLoopForUI on GTK+ (closing a tab does this, see
MessageLoopForUI::AddObserver) and using views::MouseWatcher on views.  Anyway,
this would be something to consider for a follow up patch.

[jeremya, 2011-08-04 05:09:51]

It does crash... though I'm not sure why. Here's the stack trace:

[20131:20131:4126999948749:FATAL:browser_main.cc(1026)] Gtk:
gtk_widget_size_request: assertion `GTK_IS_WIDGET (widget)' failed
Backtrace:
	base::debug::StackTrace::StackTrace() [0x7fe743f763ca]
	logging::LogMessage::~LogMessage() [0x7fe743f98306]
	(anonymous namespace)::GLibLogHandler() [0x7fe7432a13f0]
	0x7fe73f0d5fb9
	0x7fe73f0d63d3
	SlideAnimatorGtk::AnimationProgressed() [0x7fe743654c0a]
	ui::LinearAnimation::Step() [0x7fe74426a6c1]
	ui::AnimationContainer::Run() [0x7fe744268149]
	DispatchToMethod<>() [0x7fe74426a13c]
	base::BaseTimer<>::TimerTask::Run() [0x7fe744269fcc]
	base::subtle::TaskClosureAdapter::Run() [0x7fe743fe3eeb]
	base::internal::Invoker1<>::DoInvoke() [0x7fe743fa04bc]
	base::Callback<>::Run() [0x7fe743a3b649]
	MessageLoop::RunTask() [0x7fe743f9d5bd]
	MessageLoop::DeferOrRunPendingTask() [0x7fe743f9d6c5]
	MessageLoop::DoDelayedWork() [0x7fe743f9e0bb]
	base::MessagePumpGlib::RunWithDispatcher() [0x7fe744001a92]
	base::MessagePumpGlib::Run() [0x7fe744001e12]
	MessageLoop::RunInternal() [0x7fe743f9d3b1]
	MessageLoop::RunHandler() [0x7fe743f9d264]
	MessageLoopForUI::Run() [0x7fe743f9e506]
	(anonymous namespace)::RunUIMessageLoop() [0x7fe7432a0405]
	BrowserMain() [0x7fe7432a3528]
	(anonymous namespace)::RunNamedProcessTypeMain() [0x7fe7431f4b03]
	ChromeMain [0x7fe7431f5553]
	main [0x7fe7431f6188]
	0x7fe73b160c4d
	0x7fe7431f3b79

[1]    20131 trace trap  ./out/Debug/chrome

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_exit_bubble.cc (right):

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_exit_bubble.cc:8: #include
"chrome/app/chrome_command_ids.h"
On 2011/08/03 17:33:15, tony wrote:
> Nit: Sort includes.

Done.

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_exit_bubble.h:13: #include
"base/memory/scoped_ptr.h"
On 2011/08/03 17:33:15, tony wrote:
> Nit: Sort includes.

Done.

http://codereview.chromium.org/7549005/diff/7001/chrome/browser/ui/fullscreen...
chrome/browser/ui/fullscreen_exit_bubble.h:71:
CommandUpdater::CommandUpdaterDelegate* delegate_;
On 2011/08/03 17:33:15, tony wrote:
> Can we make most of these member variables private?

Done.

[Peter Kasting, 2011-08-04 18:28:38]

On 2011/08/03 17:53:21, tony wrote:
> Also, I'm not a huge fan of the polling every 100ms in general.  Another way
to
> do this would be to get a notification on mouse movements.

If you're interested in reducing CPU load, checking the mouse position every
100ms is much less load in general than reacting to each mouse move.  Also, at
the time I wrote this, the polling approach was the only method that worked (on
Windows) because we didn't get mouse movements to track at all if the window was
not the activated window.  I don't know if these mouse observers address that
issue.

[tony, 2011-08-04 18:37:55]

On 2011/08/04 05:09:51, jeremya wrote:
> It does crash... though I'm not sure why.

I think you need to convert link_container_ in fullscreen_exit_bubble_gtk.h to
an OwnedWidgetGtk.  What's happening is the browser window is closed, so all the
GtkWidget's are destroyed and SlideAnimatorGtk keeps a raw pointer to the object
(child_).  Even though link_container_'s parent (widget_ in SlideAnimatorGtk) is
kept alive by an OwnWidgetGtk, it doesn't keep alive all children.  That is,
OwnedWidgetGtk will only keep alive itself, not any of it's children.

[tony, 2011-08-04 18:42:18]

On 2011/08/04 18:28:38, Peter Kasting wrote:
> On 2011/08/03 17:53:21, tony wrote:
> > Also, I'm not a huge fan of the polling every 100ms in general.  Another way
> to
> > do this would be to get a notification on mouse movements.
> 
> If you're interested in reducing CPU load, checking the mouse position every
> 100ms is much less load in general than reacting to each mouse move.

Maybe we could do every mouse move but only check the position if there's been
more than 100ms since the last check?  Specifically, it would be nice if being
in fullscreen didn't eat CPU if you walked away from your computer.

[Peter Kasting, 2011-08-04 18:47:23]

On 2011/08/04 18:42:18, tony wrote:
> On 2011/08/04 18:28:38, Peter Kasting wrote:
> > On 2011/08/03 17:53:21, tony wrote:
> > > Also, I'm not a huge fan of the polling every 100ms in general.  Another
way
> > to
> > > do this would be to get a notification on mouse movements.
> > 
> > If you're interested in reducing CPU load, checking the mouse position every
> > 100ms is much less load in general than reacting to each mouse move.
> 
> Maybe we could do every mouse move but only check the position if there's been
> more than 100ms since the last check?

Meh, the actual check isn't any more costly than just running any code at all in
the first place.

>  Specifically, it would be nice if being
> in fullscreen didn't eat CPU if you walked away from your computer.

I agree, but like I said, at least when I wrote the code we got no messages when
not the active window, so I had no choice.  You're welcome to change things if
there's a way to work around that.

[tony, 2011-08-04 20:41:08]

Oops, committed a change with the wrong description.  Forgot that git cl patch
overwrites my change description.

[jeremya, 2011-08-05 03:08:56]

http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/slide_...
File chrome/browser/ui/gtk/slide_animator_gtk.cc (right):

http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/slide_...
chrome/browser/ui/gtk/slide_animator_gtk.cc:122: if (widget_.get()->parent) {
This seems to stop the other crash I described (gtk_fixed_move_internal:
assertion `widget->parent == GTK_WIDGET (fixed)' failed), though I'm not sure
it's the right way to fix it...

[tony, 2011-08-05 18:03:54]

On 2011/08/05 03:08:56, jeremya wrote:
>
http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/slide_...
> File chrome/browser/ui/gtk/slide_animator_gtk.cc (right):
> 
>
http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/slide_...
> chrome/browser/ui/gtk/slide_animator_gtk.cc:122: if (widget_.get()->parent) {
> This seems to stop the other crash I described (gtk_fixed_move_internal:
> assertion `widget->parent == GTK_WIDGET (fixed)' failed), though I'm not sure
> it's the right way to fix it...

Ah, you're right.  There might be a better way to check to see if the widget is
attached, but this seems OK to me (looping in estade in case he knows of a
better way).

[tony, 2011-08-05 18:23:08]

http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/fullsc...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/fullsc...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:99: GtkWindow* window =
GTK_WINDOW(
Nit: I had to check for widget()->parent here too or I can trigger a crash
(moving the mouse near the popup while quitting).

[jeremya, 2011-08-08 00:15:39]

http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/fullsc...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7549005/diff/6002/chrome/browser/ui/gtk/fullsc...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:99: GtkWindow* window =
GTK_WINDOW(
On 2011/08/05 18:23:08, tony wrote:
> Nit: I had to check for widget()->parent here too or I can trigger a crash
> (moving the mouse near the popup while quitting).

Done.

[tony, 2011-08-08 16:19:32]

LGTM with the following fixed.

http://codereview.chromium.org/7549005/diff/24001/chrome/browser/ui/gtk/fulls...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7549005/diff/24001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:99: if (!widget()->parent)
return false;
Put the return on a separate line.

[tony, 2011-08-12 17:00:53]

committed as r96568