Implement pepper flash fullscreen on Mac

This CL adds pepper flash fullscreen support on Mac.

Pepper has two fullscreen APIs:
  src/ppapi/cpp/fullscreen.h pp::FullScreen::SetFullscreen()
  src/ppapi/cpp/private/flash_fullscreen.h pp::FlashFullScreen::SetFullscreen()

The first fullscreen API simply causes the browser window to enter fullscreen as normal. This means that the menu bar and tab strip is auto-shown when the mouse is moved to the top of the screen. This feature is already working on the Mac and this CL doesn't change anything with respect to that API.

The second fullscreen API creates a new web content that is hosted in its own window. To keep the architecture similar to the Windows and Linux implementation I decided not to host the web content inside a browser window. Instead I'm using a custom borderless window that's similar to  windows that NPAPI plugins use today.

The chrome browser fullscreen window has three interesting functionality:
  1. allow OpenGL surface under the window to be visible to the user
  2. Setup the background color for the tab strip / toolbar drawing
  3. Auto-show the menu bar / tab strip when the mouse is at the top of the screen
Of these the flash fullscreen window only needs the first. For this reason I factored out that functionality into a new common base class, UnderlayOpenGLHostingWindow.

BUG=
TEST=call pp::FlashFullscreen::SetFullscreen(true). Verify that the plugin goes fullscreen.

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

[Nico, 2012-03-23 22:13:26]

Can you explain the motivation behind this design in the CL description a bit.
For example, we already have some fullscreen code that you're not using (I'm
guessing the reason is that that code has stuff like a dropdown tab strip, and
uses Lion's fullscreen code path if on Lion etc, which you don't want?).

I would guess that the content owners would prefer comments to mention "pepper
fullscreen" instead of "pepper flash fullscreen" – i guess this would work for
other pepper plugins that request fullscreen mode.

I haven't looked closely at the code yet, I'd like to read a real CL description
first :-) After you've written one, you'd might to want to loop in a contents
owner early to check that they're ok with the general approach.

https://chromiumcodereview.appspot.com/9838071/diff/4001/content/browser/rend...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/9838071/diff/4001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_mac.mm:326: NSRect
content_rect = NSMakeRect(0, 0, NSWidth(rect), NSHeight(rect));
Doesn't (x, y) have to be NSMinX(), NSMinY()?

https://chromiumcodereview.appspot.com/9838071/diff/4001/ui/base/cocoa/fullsc...
File ui/base/cocoa/fullscreen_window_manager.mm (right):

https://chromiumcodereview.appspot.com/9838071/diff/4001/ui/base/cocoa/fullsc...
ui/base/cocoa/fullscreen_window_manager.mm:24: return 0;
nil

[sail, 2012-03-23 23:05:17]

On 2012/03/23 22:13:26, Nico wrote:
> Can you explain the motivation behind this design in the CL description a bit.
> For example, we already have some fullscreen code that you're not using (I'm
> guessing the reason is that that code has stuff like a dropdown tab strip, and
> uses Lion's fullscreen code path if on Lion etc, which you don't want?).

I've expanded my CL description to explain why I'm not using more of the chrome
browser code.

> I would guess that the content owners would prefer comments to mention "pepper
> fullscreen" instead of "pepper flash fullscreen" – i guess this would work for
> other pepper plugins that request fullscreen mode.

It's weird, even though the function is called
RenderWidgetHostViewMac::InitAsFullscreen() it's only ever used for the
FlashFullscreen() API. I found this confusing which is why I specifically say
"flash pepper" all other the place.

> I haven't looked closely at the code yet, I'd like to read a real CL
description
> first :-) After you've written one, you'd might to want to loop in a contents
> owner early to check that they're ok with the general approach.

Sounds good. Added avi and brettw.

[sail, 2012-03-23 23:05:32]

https://chromiumcodereview.appspot.com/9838071/diff/4001/content/browser/rend...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/9838071/diff/4001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_mac.mm:326: NSRect
content_rect = NSMakeRect(0, 0, NSWidth(rect), NSHeight(rect));
On 2012/03/23 22:13:27, Nico wrote:
> Doesn't (x, y) have to be NSMinX(), NSMinY()?

Ooops. Fixed.

[sail, 2012-03-27 17:54:10]

By the way, trung says that silence counts as an LGTM

[Nico, 2012-03-27 18:12:26]

On 2012/03/27 17:54:10, sail wrote:
> By the way, trung says that silence counts as an LGTM

Might explain why you don't see Trung on codereview a lot these days. If you
meant "ping", say that. I do admit I had forgotten about this change.

Nico says that passive-aggressive notes on codereviews count as "please wait
with reviewing this another 2 days".

[sail, 2012-03-27 18:15:46]

> Nico says that passive-aggressive notes on codereviews count as "please wait
> with reviewing this another 2 days".

I know right. Trung, stop being so passive aggressive!

[viettrungluu, 2012-03-27 19:22:18]

On 2012/03/27 17:54:10, sail wrote:
> By the way, trung says that silence counts as an LGTM

Lies! I said no such thing.

[Robert Sesek, 2012-03-27 19:47:49]

http://codereview.chromium.org/9838071/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/9838071/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_view_mac.mm:328:
backing:NSBackingStoreBuffered
nit: align colons

http://codereview.chromium.org/9838071/diff/6001/ui/base/cocoa/fullscreen_win...
File ui/base/cocoa/fullscreen_window_manager.h (right):

http://codereview.chromium.org/9838071/diff/6001/ui/base/cocoa/fullscreen_win...
ui/base/cocoa/fullscreen_window_manager.h:24: bool fullscreenEnabled_;
ObjC BOOL?

http://codereview.chromium.org/9838071/diff/6001/ui/base/cocoa/fullscreen_win...
ui/base/cocoa/fullscreen_window_manager.h:30: - (void)enterFullscreenMode;
What do these do?

http://codereview.chromium.org/9838071/diff/6001/ui/base/cocoa/fullscreen_win...
File ui/base/cocoa/fullscreen_window_manager.mm (right):

http://codereview.chromium.org/9838071/diff/6001/ui/base/cocoa/fullscreen_win...
ui/base/cocoa/fullscreen_window_manager.mm:39: }
   // namespace

http://codereview.chromium.org/9838071/diff/6001/ui/base/cocoa/fullscreen_win...
ui/base/cocoa/fullscreen_window_manager.mm:66: [[NSNotificationCenter
defaultCenter]
Why not listen to this in init and not remove/add it each time?

http://codereview.chromium.org/9838071/diff/6001/ui/base/cocoa/fullscreen_win...
ui/base/cocoa/fullscreen_window_manager.mm:68:
selector:@selector(onScreenChanged:)
Where is this selector implemented?

[sail, 2012-03-27 20:19:51]

https://chromiumcodereview.appspot.com/9838071/diff/6001/content/browser/rend...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/9838071/diff/6001/content/browser/rend...
content/browser/renderer_host/render_widget_host_view_mac.mm:328:
backing:NSBackingStoreBuffered
On 2012/03/27 19:47:50, rsesek wrote:
> nit: align colons

Done.

https://chromiumcodereview.appspot.com/9838071/diff/6001/ui/base/cocoa/fullsc...
File ui/base/cocoa/fullscreen_window_manager.h (right):

https://chromiumcodereview.appspot.com/9838071/diff/6001/ui/base/cocoa/fullsc...
ui/base/cocoa/fullscreen_window_manager.h:24: bool fullscreenEnabled_;
On 2012/03/27 19:47:50, rsesek wrote:
> ObjC BOOL?

Done.

https://chromiumcodereview.appspot.com/9838071/diff/6001/ui/base/cocoa/fullsc...
ui/base/cocoa/fullscreen_window_manager.h:30: - (void)enterFullscreenMode;
On 2012/03/27 19:47:50, rsesek wrote:
> What do these do?

Done.

https://chromiumcodereview.appspot.com/9838071/diff/6001/ui/base/cocoa/fullsc...
File ui/base/cocoa/fullscreen_window_manager.mm (right):

https://chromiumcodereview.appspot.com/9838071/diff/6001/ui/base/cocoa/fullsc...
ui/base/cocoa/fullscreen_window_manager.mm:39: }
On 2012/03/27 19:47:50, rsesek wrote:
>    // namespace

Done.

https://chromiumcodereview.appspot.com/9838071/diff/6001/ui/base/cocoa/fullsc...
ui/base/cocoa/fullscreen_window_manager.mm:66: [[NSNotificationCenter
defaultCenter]
On 2012/03/27 19:47:50, rsesek wrote:
> Why not listen to this in init and not remove/add it each time?

Done.

https://chromiumcodereview.appspot.com/9838071/diff/6001/ui/base/cocoa/fullsc...
ui/base/cocoa/fullscreen_window_manager.mm:68:
selector:@selector(onScreenChanged:)
On 2012/03/27 19:47:50, rsesek wrote:
> Where is this selector implemented?

Oops, good catch. Accidentally dropped this during the refactoring. Re-added.

[sail, 2012-03-30 19:19:36]

> Nico says that passive-aggressive notes on codereviews count as "please wait
> with reviewing this another 2 days".

Alright, I think trung learned his lesson now. Time to start LGTMing.

[Nico, 2012-03-30 19:33:57]

Sorry, I forgot about this CL again 9_9

Looks mostly good. A few nits, one real comment.

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
File chrome/browser/ui/cocoa/chrome_browser_window.h (right):

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
chrome/browser/ui/cocoa/chrome_browser_window.h:9: #import <Cocoa/Cocoa.h>
Remove

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
chrome/browser/ui/cocoa/chrome_browser_window.h:15: @private
Remove { @private }

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:17: @interface
ChromeEventProcessingWindow : UnderlayOpenGLHostingWindow {
This makes InfoBubbleWindows UnderlayOpenGLHostingWindows as well. That's
probably a good thing since we might not have supported composited extension
popups before.

Can you test if an extension popup with 3d content (3d css or even better webgl)
works before this change / after this change?

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.h:351: NSWindow*
fullscreen_window() const { return fullscreen_window_; }
pepper_fullscreen_window()?

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:317:
RenderWidgetHostView* reference_host_view) {
Comment that this is only used for plugin fullscreen (copy parts of the cl
description as comment)

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:1526: return;
Is flash responsible for the "hit esc to exit" message? Should it be responsible
for handling this event then too?

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
File ui/base/cocoa/fullscreen_window_manager.h (right):

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
ui/base/cocoa/fullscreen_window_manager.h:24: BOOL fullscreenEnabled_;
fullscreenActive_

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
File ui/base/cocoa/fullscreen_window_manager.mm (right):

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
ui/base/cocoa/fullscreen_window_manager.mm:65: removeObserver:self];
unwrap

[sail, 2012-03-30 23:06:06]

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
File chrome/browser/ui/cocoa/chrome_browser_window.h (right):

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
chrome/browser/ui/cocoa/chrome_browser_window.h:9: #import <Cocoa/Cocoa.h>
On 2012/03/30 19:33:57, Nico wrote:
> Remove

Done.

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
chrome/browser/ui/cocoa/chrome_browser_window.h:15: @private
On 2012/03/30 19:33:57, Nico wrote:
> Remove { @private }

Done.

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
File chrome/browser/ui/cocoa/chrome_event_processing_window.h (right):

http://codereview.chromium.org/9838071/diff/13001/chrome/browser/ui/cocoa/chr...
chrome/browser/ui/cocoa/chrome_event_processing_window.h:17: @interface
ChromeEventProcessingWindow : UnderlayOpenGLHostingWindow {
On 2012/03/30 19:33:57, Nico wrote:
> This makes InfoBubbleWindows UnderlayOpenGLHostingWindows as well. That's
> probably a good thing since we might not have supported composited extension
> popups before.
> 
> Can you test if an extension popup with 3d content (3d css or even better
webgl)
> works before this change / after this change?

On Canary the extension popup works both 3d CSS and webgl.
I verified that everything still works with my changes. I've emailed you my test
extension incase it's helpful.

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.h:351: NSWindow*
fullscreen_window() const { return fullscreen_window_; }
On 2012/03/30 19:33:57, Nico wrote:
> pepper_fullscreen_window()?

Done.

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:317:
RenderWidgetHostView* reference_host_view) {
On 2012/03/30 19:33:57, Nico wrote:
> Comment that this is only used for plugin fullscreen (copy parts of the cl
> description as comment)

Done.

http://codereview.chromium.org/9838071/diff/13001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_mac.mm:1526: return;
On 2012/03/30 19:33:57, Nico wrote:
> Is flash responsible for the "hit esc to exit" message? Should it be
responsible
> for handling this event then too?

This additional check is needed incase the render is hung.
I copy/pasted a comment from the GTK code that explains this.

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
File ui/base/cocoa/fullscreen_window_manager.h (right):

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
ui/base/cocoa/fullscreen_window_manager.h:24: BOOL fullscreenEnabled_;
On 2012/03/30 19:33:57, Nico wrote:
> fullscreenActive_

Done.

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
File ui/base/cocoa/fullscreen_window_manager.mm (right):

http://codereview.chromium.org/9838071/diff/13001/ui/base/cocoa/fullscreen_wi...
ui/base/cocoa/fullscreen_window_manager.mm:65: removeObserver:self];
On 2012/03/30 19:33:57, Nico wrote:
> unwrap

Done.

[Nico, 2012-04-02 18:44:31]

LGTM

Thanks for checking. Sorry for the atrocious latency :-(

[commit-bot: I haz the power, 2012-04-02 19:18:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9838071/17002

[commit-bot: I haz the power, 2012-04-02 19:18:23]

Can't apply patch for file ui/base/cocoa/underlay_opengl_hosting_window.h.
While running patch -p1 --forward --force;
patching file ui/base/cocoa/underlay_opengl_hosting_window.h
Hunk #1 FAILED at 2.
Hunk #2 succeeded at 26 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file
ui/base/cocoa/underlay_opengl_hosting_window.h.rej

[sail, 2012-04-02 19:23:03]

avi: could I get OWNERS review for content/browser/*
ben: could I get OWNERS review for ui/*

[Ben Goodger (Google), 2012-04-02 19:29:03]

LGTM for ui/base. Please in a separate CL work with Nico to add an OWNERS file
to ui/base/cocoa.

[sail, 2012-04-02 19:34:03]

On 2012/04/02 19:29:03, Ben Goodger (Google) wrote:
> LGTM for ui/base. Please in a separate CL work with Nico to add an OWNERS file
> to ui/base/cocoa.

Done! https://chromiumcodereview.appspot.com/9963069/

[sail, 2012-04-02 20:47:17]

Merged in avi's addition of UnderlayableSurface protocol.

[Avi (use Gerrit), 2012-04-02 20:55:20]

LGTM

https://chromiumcodereview.appspot.com/9838071/diff/15004/content/shell/shell...
File content/shell/shell_mac.mm (right):

https://chromiumcodereview.appspot.com/9838071/diff/15004/content/shell/shell...
content/shell/shell_mac.mm:20: 
Yes!

[commit-bot: I haz the power, 2012-04-02 21:44:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9838071/17045

[commit-bot: I haz the power, 2012-04-02 22:16:40]

Try job failure for 9838071-17045 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-04-02 22:20:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9838071/17045

[commit-bot: I haz the power, 2012-04-03 00:59:27]

Try job failure for 9838071-17045 (retry) on win_rel for step "browser_tests"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-04-03 01:11:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/9838071/17045

[commit-bot: I haz the power, 2012-04-03 04:38:59]

Change committed as 130311