Fix several issues around fullscreen Mac plugins:...

Fix several issues around fullscreen Mac plugins:
* Keystrokes are now properly sent to plugins in fullscreen mode
* When a plugin creates a fullscreen window, we hide the menu bar and restore it when the window is closed
BUG=19534, 21020
TEST=Open a page with plugins that can go full screen (example: flash video players).  Enter full screen mode and verify that esc, arrow keys, spacebar, etc. work as expected.  Verify that the menu bar is hidden when the plugin goes fullscreen and is restored when it exits fullscreen mode.

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

[Amanda Walker, 2009-09-30 20:11:35]

stuartmorgan: interposed library changes
pinkerton: general review

Note: I don't like the "extern "C" {...}" stuff for the notifications from the
interposed library back to the plugin thread, but it was how I could get it to
link properly.  I'll experiment some more with namespace settings, etc. before
committing the change, to see if I can make that a bit more elegant.

[stuartmorgan, 2009-09-30 20:23:45]

LGTM

http://codereview.chromium.org/257008/diff/1/4
File chrome/browser/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/257008/diff/1/4#newcode79
Line 79: &bounds);
Is it worth making a little WindowRef -> CGRect helper function, instead of
repeating these two lines?

http://codereview.chromium.org/257008/diff/1/5
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/257008/diff/1/5#newcode37
Line 37: SetSystemUIMode(kUIModeNormal, 0);
Do we need to have something watching for plugin process crashes and ending this
mode?

[Amanda Walker, 2009-09-30 20:25:53]

http://codereview.chromium.org/257008/diff/1/4
File chrome/browser/plugin_carbon_interpose_mac.cc (right):

http://codereview.chromium.org/257008/diff/1/4#newcode79
Line 79: &bounds);
On 2009/09/30 20:23:45, stuartmorgan wrote:
> Is it worth making a little WindowRef -> CGRect helper function, instead of
> repeating these two lines?

Well, yes.  Will fix :-).

http://codereview.chromium.org/257008/diff/1/5
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/257008/diff/1/5#newcode37
Line 37: SetSystemUIMode(kUIModeNormal, 0);
On 2009/09/30 20:23:45, stuartmorgan wrote:
> Do we need to have something watching for plugin process crashes and ending
this
> mode?

Excellent point.  Yes, we do.  Will fix.

[Amanda Walker, 2009-10-01 16:52:59]

New patch up.

[stuartmorgan, 2009-10-01 17:17:08]

http://codereview.chromium.org/257008/diff/4001/4005
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/257008/diff/4001/4005#newcode37
Line 37: SetSystemUIMode(kUIModeNormal, 0);
I wonder if we need to track full-screen and non-full-screen windows in two
different sets, so that this (and the crash handling) are more focused? I'm
thinking about Chrome being in full-screen mode and the user, say, attaching a
file in Gmail; we don't want Chrome to exit full-screen mode.

[Amanda Walker, 2009-10-01 17:21:02]

http://codereview.chromium.org/257008/diff/4001/4005
File chrome/browser/plugin_process_host_mac.cc (right):

http://codereview.chromium.org/257008/diff/4001/4005#newcode37
Line 37: SetSystemUIMode(kUIModeNormal, 0);
Ah, I'd forgotten about browser-side fullscreen mode.  Good point, will take a
look.

[Amanda Walker, 2009-10-01 19:39:41]

I discussed the edge cases with jrg--we're in agreement that a general solution
needs a refcount on the menubar visibility, but that's a bit beyond the scope of
this CL.  Will open a bug on that issue (since it also affects non-plugin
fullscreen issues).

[stuartmorgan, 2009-10-01 19:41:10]

LGTM

[darin (slow to review), 2009-10-01 22:19:53]

http://codereview.chromium.org/257008/diff/4001/4009
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/257008/diff/4001/4009#newcode168
Line 168: __attribute__((visibility("default")))
what's up with these GCC attributes?

[Amanda Walker, 2009-10-01 23:17:05]

http://codereview.chromium.org/257008/diff/4001/4009
File chrome/plugin/plugin_thread.cc (right):

http://codereview.chromium.org/257008/diff/4001/4009#newcode168
Line 168: __attribute__((visibility("default")))
On 2009/10/01 22:19:53, darin wrote:
> what's up with these GCC attributes?

They allow the interposed library that we use to intercept Carbon API calls in
the plugin process to link against these symbols in the main application.