Rewrote full-screen support on Windows. O3D now always creates its own...

Rewrote full-screen support on Windows. O3D now always creates its own
window in which to render, rather than rendering into either the
browser's window or a separate full-screen window. The O3D window is
removed from the browser's hierarchy and made top-level in order to go
to full-screen mode via Direct3D. This solves fundamental focus
fighting problems seen on Windows Vista.

This change allowed the event forwarding code in the plugin's window
message loop to be deleted, but a new workaround for a flicker upon
the first mouse click in O3D in Firefox was required.

Split the Renderer's fullscreen API into GoFullscreen and
CancelFullscreen to solve chicken-and-egg problems with coming out of
full-screen mode.

Changed how the plugin detects resize events. Rather than responding
to WM_SIZE messages, NPP_SetWindow is now responsible for propagating
resize events to the client. Changed the ActiveX host control to call
NPP_SetWindow in response to SetObjectRects.

Fixed RendererGL::IsCurrent() on non-Mac platforms. Removed the bogus
current_renderer_ static variable.

Tested the following scenarios in IE and Firefox on Windows:
 - Full-screen involving display mode change, Escape to exit.
 - Full-screen involving display mode change, Alt-Tab to exit.
 - Full-screen involving display mode change, Alt-F4 to exit.
 - Full-screen involving display mode change, timeout to exit.
 - Full-screen with no display mode change, Escape to exit.
 - Full-screen with no display mode change, Alt-Tab to exit.
 - Full-screen with no display mode change, Alt-F4 to exit.
 - Full-screen with no display mode change, timeout to exit.
 - Beach demo, particle demo, other tests.

Tested the following scenarios on the Mac in Safari (for which the
code path didn't change):
 - Full-screen, escape to exit.
 - Full-screen, Alt-Tab to exit.
 - Full-screen, timeout to exit.

When http://crbug.com/21921 is fixed, full-screen mode will work on
Windows Vista with Aero on in Chrome.

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

[vangelis, 2009-09-17 01:56:22]

nicely done.  A few inline comments and thoughts.

http://codereview.chromium.org/210005/diff/3007/3020
File plugin/win/main_win.cc (right):

http://codereview.chromium.org/210005/diff/3007/3020#newcode668
Line 668: void ReplaceContentWindow(HWND content_hwnd,
Would be useful to add a comment describing what the method does.  It looks like
it (re)parents the content window under the plugin window.

http://codereview.chromium.org/210005/diff/3007/3020#newcode825
Line 825: obj->Resize(window->width, window->height);
should this be ->Resize or ->SetPrevSize ?

http://codereview.chromium.org/210005/diff/3007/3020#newcode849
Line 849: HWND content_window =
It would be nice to insert a comment somewhere here that explains the new window
hierarchy.  You've done a good job explaining it in the description for the CL
but that explanation will get lost.  For posterity, better to have a short
description of it somewhere in the code.

http://codereview.chromium.org/210005/diff/3007/3020#newcode929
Line 929: display.set_hwnd(GetContentHWnd());
Do we need to call this here?  I thought the ContentHWnd doesn't change anymore.

http://codereview.chromium.org/210005/diff/3007/3020#newcode940
Line 940: ReplaceContentWindow(GetContentHWnd(), GetPluginHWnd(), 0, 0);
why are you using the current window size here instead of calling
ReplaceContentWindow with 0, 0 and letting the content window be as large as its
parent?  I suppose these two should match but I was curious if there's a more
subtle point that needs to be described here.

[apatrick, 2009-09-17 18:32:35]

A few additional things...

http://codereview.chromium.org/210005/diff/3007/3013
File core/win/d3d9/renderer_d3d9.cc (right):

http://codereview.chromium.org/210005/diff/3007/3013#newcode1337
Line 1337: if (true != fullscreen_) {
if (!fullscreen_)
?

http://codereview.chromium.org/210005/diff/3007/3014
File plugin/npapi_host_control/win/np_plugin_proxy.cc (right):

http://codereview.chromium.org/210005/diff/3007/3014#newcode290
Line 290: if (NPERR_NO_ERROR != plugin_funcs_.setwindow(
I'd check if plugin_funcs_.setwindow != NULL first.

http://codereview.chromium.org/210005/diff/3007/3020
File plugin/win/main_win.cc (right):

http://codereview.chromium.org/210005/diff/3007/3020#newcode595
Line 595: ::ValidateRect(hWnd, NULL);
I know this works. But isn't the "proper" way to validate the window from
WM_PAINT to call BeginPaint and EndPaint?

http://codereview.chromium.org/210005/diff/3007/3020#newcode614
Line 614: WNDCLASSEX window_class;
I rather like WNDCLASSEX window_class = { sizeof(WNDCLASSEX) };

That zeros all the other members as well and eliminates the need for ZeroMemory.

If you prefer ZeroMemory, the style guide recommends using a variable name in
preference to a type name in sizeof:

ZeroMemory(&window_class, sizeof(window_class));

[Ken Russell (Google), 2009-09-17 19:03:07]

http://codereview.chromium.org/210005/diff/3007/3013
File core/win/d3d9/renderer_d3d9.cc (right):

http://codereview.chromium.org/210005/diff/3007/3013#newcode1337
Line 1337: if (true != fullscreen_) {
On 2009/09/17 18:32:35, apatrick wrote:
> if (!fullscreen_)
> ?

Done.

http://codereview.chromium.org/210005/diff/3007/3014
File plugin/npapi_host_control/win/np_plugin_proxy.cc (right):

http://codereview.chromium.org/210005/diff/3007/3014#newcode290
Line 290: if (NPERR_NO_ERROR != plugin_funcs_.setwindow(
On 2009/09/17 18:32:35, apatrick wrote:
> I'd check if plugin_funcs_.setwindow != NULL first.

This isn't done in any of the other routines in this file, but OK. Fixed.

http://codereview.chromium.org/210005/diff/3007/3020
File plugin/win/main_win.cc (right):

http://codereview.chromium.org/210005/diff/3007/3020#newcode595
Line 595: ::ValidateRect(hWnd, NULL);
On 2009/09/17 18:32:35, apatrick wrote:
> I know this works. But isn't the "proper" way to validate the window from
> WM_PAINT to call BeginPaint and EndPaint?

It's cheaper to avoid fetching the DC.

http://codereview.chromium.org/210005/diff/3007/3020#newcode614
Line 614: WNDCLASSEX window_class;
On 2009/09/17 18:32:35, apatrick wrote:
> I rather like WNDCLASSEX window_class = { sizeof(WNDCLASSEX) };
> 
> That zeros all the other members as well and eliminates the need for
ZeroMemory.
> 
> If you prefer ZeroMemory, the style guide recommends using a variable name in
> preference to a type name in sizeof:
> 
> ZeroMemory(&window_class, sizeof(window_class));
> 

I really do not like the partial structure initializer idiom. I can never
remember whether the unreferenced members are zeroed or left uninitialized. Web
searches turned up conflicting information. I've changed the code to use
sizeof(window_class).

http://codereview.chromium.org/210005/diff/3007/3020#newcode668
Line 668: void ReplaceContentWindow(HWND content_hwnd,
On 2009/09/17 01:56:22, vangelis wrote:
> Would be useful to add a comment describing what the method does.  It looks
like
> it (re)parents the content window under the plugin window.

Done.

http://codereview.chromium.org/210005/diff/3007/3020#newcode825
Line 825: obj->Resize(window->width, window->height);
On 2009/09/17 01:56:22, vangelis wrote:
> should this be ->Resize or ->SetPrevSize ? 

In this context it should have been Resize, since SetPrevSize should only have
been called during initialization. However, it turns out Resize had the right
semantics for the only use of SetPrevSize so I've removed the latter.

http://codereview.chromium.org/210005/diff/3007/3020#newcode849
Line 849: HWND content_window =
On 2009/09/17 01:56:22, vangelis wrote:
> It would be nice to insert a comment somewhere here that explains the new
window
> hierarchy.  You've done a good job explaining it in the description for the CL
> but that explanation will get lost.  For posterity, better to have a short
> description of it somewhere in the code.

Done.

http://codereview.chromium.org/210005/diff/3007/3020#newcode929
Line 929: display.set_hwnd(GetContentHWnd());
On 2009/09/17 01:56:22, vangelis wrote:
> Do we need to call this here?  I thought the ContentHWnd doesn't change
anymore.
> 

It is needed to set up the hDeviceWindow in the D3DPRESENT_PARAMETERS. We don't
keep any other record of that window in the Renderer.

http://codereview.chromium.org/210005/diff/3007/3020#newcode940
Line 940: ReplaceContentWindow(GetContentHWnd(), GetPluginHWnd(), 0, 0);
On 2009/09/17 01:56:22, vangelis wrote:
> why are you using the current window size here instead of calling
> ReplaceContentWindow with 0, 0 and letting the content window be as large as
its
> parent?  I suppose these two should match but I was curious if there's a more
> subtle point that needs to be described here. 

I assume you were referring to the call to ReplaceContentWindow below which
passed non-zero values.

In an earlier version of this code it was inconvenient to find the previous
size. At this point it is readily available so I've changed this site to pass it
and removed the logic in ReplaceContentWindow which queries the size of the
containing window.

[apatrick, 2009-09-17 19:12:39]

I'll leave it to vangelis. LGTM for my comments.

[vangelis, 2009-09-17 19:55:49]

LGTM for me too.

[maf, 2009-09-17 20:25:24]

LGTM

[gman, 2009-09-17 21:30:55]

LGTM

http://codereview.chromium.org/210005/diff/3007/3012
File core/win/d3d9/renderer_d3d9.h (right):

http://codereview.chromium.org/210005/diff/3007/3012#newcode95
Line 95: virtual bool CancelFullscreen(const DisplayWindow& display,
Personal style but I'm a big fan of not documenting things twice which makes
more work (have to maintain multiple sets of docs) and makes it more likely for
one to get out of sync. So, I usually just say 

// Overridden from Renderer

That way I know where to find the one and only correct docs.

http://codereview.chromium.org/210005/diff/3007/3020
File plugin/win/main_win.cc (right):

http://codereview.chromium.org/210005/diff/3007/3020#newcode555
Line 555: // TODO(kbr): consider doing this somehow more asynchronously;
Is this important? You can send yourself a message which will happen async.

[Ken Russell (Google), 2009-09-17 21:42:15]

http://codereview.chromium.org/210005/diff/3007/3012
File core/win/d3d9/renderer_d3d9.h (right):

http://codereview.chromium.org/210005/diff/3007/3012#newcode95
Line 95: virtual bool CancelFullscreen(const DisplayWindow& display,
On 2009/09/17 21:30:55, gman wrote:
> Personal style but I'm a big fan of not documenting things twice which makes
> more work (have to maintain multiple sets of docs) and makes it more likely
for
> one to get out of sync. So, I usually just say 
> 
> // Overridden from Renderer
> 
> That way I know where to find the one and only correct docs.

Sounds good. Done here and in renderer_gl.h.

http://codereview.chromium.org/210005/diff/3007/3020
File plugin/win/main_win.cc (right):

http://codereview.chromium.org/210005/diff/3007/3020#newcode555
Line 555: // TODO(kbr): consider doing this somehow more asynchronously;
On 2009/09/17 21:30:55, gman wrote:
> Is this important? You can send yourself a message which will happen async.

It doesn't appear to be important -- the code works -- and would complicate the
logic. For that reason I left it as is.