gtk: Improve fullscreen RenderWidgetHostViewGtk.

gtk: Improve fullscreen RenderWidgetHostViewGtk.

This was previously using the same code as for popup windows
(i.e. <select> and autocomplete), which creates an
override-redirect/popup window and grabs the pointer and
keyboard.  This is the wrong way to make a window
fullscreen: since the window is override-redirect, the
window manager will ignore the fullscreen request, and since
the input is grabbed, other X clients' key grabs (such as
Alt-Tab) won't work.

This change makes us instead open a regular toplevel window.

BUG=chromium-os:11545
TEST=checked that brightness and volume keys work while Pepper Flash is fullscreen and that <select> popups still work

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

[Daniel Erat, 2011-02-10 01:58:26]

Bo, what's the best way for me to check that this didn't cause any regressions
in the original (<video>-related?) functionality of
http://codereview.chromium.org/3055009/?

[Daniel Erat, 2011-02-10 01:59:58]

(Side note: browser/renderer_host/render_widget_host_view_gtk.cc is
the only file with any substantial changes -- the rest is all just
removing popup type parameters from fullscreen-related methods.)

On Wed, Feb 9, 2011 at 5:58 PM,  <derat@chromium.org> wrote:
> Reviewers: piman, brettw,
>
> Message:
> Bo, what's the best way for me to check that this didn't cause any
> regressions
> in the original (<video>-related?) functionality of
> http://codereview.chromium.org/3055009/?
>
> Description:
> gtk: Improve fullscreen RenderWidgetHostViewGtk.
>
> This was previously using the same code as for popup windows
> (i.e. <select> and autocomplete), which creates an
> override-redirect/popup window and grabs the pointer and
> keyboard.  This is the wrong way to make a window
> fullscreen: since the window is override-redirect, the
> window manager will ignore the fullscreen request, and since
> the input is grabbed, other X clients' key grabs (such as
> Alt-Tab) won't work.
>
> This change makes us instead open a regular toplevel window.
>
> BUG=chromium-os:11545
> TEST=checked that brightness and volume keys work while Pepper Flash is
> fullscreen and that <select> popups still work
>
> Please review this at http://codereview.chromium.org/6462017/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/extensions/extension_host.h
>  M chrome/browser/extensions/extension_host.cc
>  M chrome/browser/notifications/balloon_host.h
>  M chrome/browser/prerender/prerender_contents.h
>  M chrome/browser/prerender/prerender_contents.cc
>  M chrome/browser/renderer_host/render_message_filter.h
>  M chrome/browser/renderer_host/render_message_filter.cc
>  M chrome/browser/renderer_host/render_view_host.h
>  M chrome/browser/renderer_host/render_view_host.cc
>  M chrome/browser/renderer_host/render_view_host_delegate.h
>  M chrome/browser/renderer_host/render_widget_helper.h
>  M chrome/browser/renderer_host/render_widget_helper.cc
>  M chrome/browser/renderer_host/render_widget_host_view.h
>  M chrome/browser/renderer_host/render_widget_host_view_gtk.h
>  M chrome/browser/renderer_host/render_widget_host_view_gtk.cc
>  M chrome/browser/renderer_host/render_widget_host_view_mac.h
>  M chrome/browser/renderer_host/render_widget_host_view_mac.mm
>  M chrome/browser/renderer_host/render_widget_host_view_views.h
>  M chrome/browser/renderer_host/render_widget_host_view_views.cc
>  M chrome/browser/renderer_host/render_widget_host_view_win.h
>  M chrome/browser/renderer_host/render_widget_host_view_win.cc
>  M chrome/browser/renderer_host/test/render_view_host_unittest.cc
>  M chrome/browser/renderer_host/test/test_render_view_host.h
>  M chrome/browser/tab_contents/background_contents.h
>  M chrome/browser/tab_contents/background_contents.cc
>  M chrome/browser/tab_contents/interstitial_page.cc
>  M chrome/browser/tab_contents/render_view_host_delegate_helper.h
>  M chrome/browser/tab_contents/render_view_host_delegate_helper.cc
>  M chrome/browser/tab_contents/tab_contents_view.h
>  M chrome/browser/tab_contents/tab_contents_view.cc
>  M chrome/common/render_messages_internal.h
>  M chrome/renderer/render_view.h
>  M chrome/renderer/render_view.cc
>  M chrome/renderer/render_widget_fullscreen.h
>  M chrome/renderer/render_widget_fullscreen.cc
>  M chrome/renderer/render_widget_fullscreen_pepper.cc
>
>
>

[piman, 2011-02-10 02:28:45]

Thanks for the big cleanup. It mostly looks good, aside of one comment.

http://codereview.chromium.org/6462017/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6462017/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:152: GDK_Escape ==
event->keyval) {
Does "Esc" still close the fullscreen window ? I would expect some code here to
deal with it - unless GTK handles it already but I'd be surprised.

[Daniel Erat, 2011-02-10 04:45:16]

http://codereview.chromium.org/6462017/diff/9001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6462017/diff/9001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:152: GDK_Escape ==
event->keyval) {
On 2011/02/10 02:28:45, piman wrote:
> Does "Esc" still close the fullscreen window ? I would expect some code here
to
> deal with it - unless GTK handles it already but I'd be surprised.

Yes, it does.  I'd assumed that Flash was handling this.  Do you think that
Chrome should also be watching for it?  I'm not sure of the order in which Flash
vs. Chrome handles these events, but I'll do some experimenting tomorrow to see
if this code ever even gets reached in the fullscreen case or if Flash always
eats the event first.

[piman, 2011-02-10 05:07:49]

On 2011/02/10 04:45:16, Daniel Erat wrote:
>
http://codereview.chromium.org/6462017/diff/9001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):
> 
>
http://codereview.chromium.org/6462017/diff/9001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:152: GDK_Escape ==
> event->keyval) {
> On 2011/02/10 02:28:45, piman wrote:
> > Does "Esc" still close the fullscreen window ? I would expect some code here
> to
> > deal with it - unless GTK handles it already but I'd be surprised.
> 
> Yes, it does.  I'd assumed that Flash was handling this.  Do you think that
> Chrome should also be watching for it?  I'm not sure of the order in which
Flash
> vs. Chrome handles these events, but I'll do some experimenting tomorrow to
see
> if this code ever even gets reached in the fullscreen case or if Flash always
> eats the event first.

Ah, right, Flash has some code to handle that. LGTM then.

[boliu, 2011-02-10 16:56:12]

Hi Daniel,

My original patch was only to create a full screen pop up window similar to a
pop up menu that the renderer can draw in. It was originally intended for full
screen html5 video but that part was never done. So I'm not sure there is an
easy way to test this. You could try creating a full screen pop up menu to test
that it still works locally.

I think pepper is using this code that for creating full screen video as well,
so perhaps you can use that to test. See http://codereview.chromium.org/3352019/

Bo

On 2011/02/10 01:58:26, Daniel Erat wrote:
> Bo, what's the best way for me to check that this didn't cause any regressions
> in the original (<video>-related?) functionality of
> http://codereview.chromium.org/3055009/?

[Timur Iskhodzhanov, 2011-02-12 19:11:34]

FYI, this patch has made Valgrind bots red for a couple of days.
Please watch the memory waterfall after commiting