Use inner HWND for accelerated rendering on windows

When accelerated compositing is enabled, we create a plugin-like pair of windows,
CompositorHostWindow and CompositorWindow inside the RenderWidgetHostView. The host-side
HWND is used to position the compositor output relative to plugins; the GPU process creates the
compositor window as a child of the CompositorHostWindow.

Once we land webkit bugfix 49396, ANGLE issue 3038042, Chromium issue 4671003, this will fix
bugs 54301 and 61516 for windows.


BUG=none
TEST=none


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

[apatrick_chromium, 2010-11-11 22:32:15]

I looked at all the files with "gpu" in the filename.

http://codereview.chromium.org/4815001/diff/1/chrome/browser/gpu_process_host.cc
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/4815001/diff/1/chrome/browser/gpu_process_host...
chrome/browser/gpu_process_host.cc:354: void SendDelayedReply(IPC::Message*
reply_msg) {
I think some of this code might belong in GpuProcessHostUIShim. You might also
be able to use RouteOnUIThreadTask and SendOnIOThreadTask to pass the messages
between threads,

http://codereview.chromium.org/4815001/diff/1/chrome/browser/gpu_process_host...
chrome/browser/gpu_process_host.cc:358: void
CreateCompositorHostWindowDispatcher(int32 renderer_id,
Wrap first argument to next line.

http://codereview.chromium.org/4815001/diff/1/chrome/common/gpu_messages_inte...
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/4815001/diff/1/chrome/common/gpu_messages_inte...
chrome/common/gpu_messages_internal.h:103:
IPC_SYNC_MESSAGE_CONTROL2_1(GpuHostMsg_CreateCompositorHostWindow,
Do these synchronous messages introduce the possibility of a deadlock?

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:42: static LRESULT CALLBACK
CompositorWindowProc(HWND hWnd, UINT message,
One parameter per line. hWnd, lParam and wParam should not be camel case.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:53: HANDLE h = GetProp(hWnd,
L"CompositorWindowGpuCommandBufferStub");
Can you make the property name a constant?

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:60: return 0;
Why handle WM_CHAR? Does setting WS_DISABLED not auomatically propagate input
events to the parent window?

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:73: gpu_thread->Send(new
GpuHostMsg_CreateCompositorHostWindow(renderer_id_,
One argument per line.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:82: wcex.cbSize         =
sizeof(WNDCLASSEX);
sizeof(wcex)

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:99: WS_EX_LEFT | WS_EX_LTRREADING |
WS_EX_RIGHTSCROLLBAR,
One argument per line unless they are related, like width and height. You might
also comment the zeroes since it isn't obvious what they are.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:104: SetProp(compositor_window,
L"CompositorWindowGpuCommandBufferStub",
Use a string constant.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:108: GetWindowRect(host_window,
&parent_rect);
GetWindowRect or GetClientRect? Also, if this function fails, parent_rect will
be uninitialized and bad things might happen below.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:112: SetWindowPos(compositor_window, NULL,
0, 0,
One argument per line unless they are related, like width / height.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:134:
DestroyWindow(static_cast<HWND>(compositor_window_));
Windows apparently lets properties leak when you destroy a window. Safer to call
RemoveProp before destroying.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
File chrome/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.h:55: public:
I would be safer to move "public:" out of the preprocessor conditional block.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.h:80: HWND compositor_window_;
If HWND was gfx::NativeView, would it be easier to share this with the linux
version?

[darin (slow to review), 2010-11-11 23:33:14]

I would really like jam to review this change.

-Darin


On Thu, Nov 11, 2010 at 1:14 PM, <nduca@chromium.org> wrote:

> Reviewers: darin, apatrick,
>
> Description:
> When accelerated compositing is enabled, we create a plugin-like pair of
> windows,
> CompositorHostWindow and CompositorWindow inside the RenderWidgetHostView.
> The
> host-side
> HWND is used to position the compositor output relative to plugins; the GPU
> process creates the
> compositor window as a child of the CompositorHostWindow.
>
> Once we land webkit bugfix 49396, ANGLE issue 3038042, Chromium issue
> 4671003,
> this will fix
> bugs 54301 and 61516 for windows.
>
> Please review this at http://codereview.chromium.org/4815001/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/gpu_process_host.h
>  M     chrome/browser/gpu_process_host.cc
>  M     chrome/browser/renderer_host/render_widget_host.h
>  M     chrome/browser/renderer_host/render_widget_host.cc
>  M     chrome/browser/renderer_host/render_widget_host_view.h
>  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/tab_contents/tab_contents.cc
>  M     chrome/common/gpu_messages_internal.h
>  M     chrome/common/render_messages_internal.h
>  M     chrome/gpu/gpu_channel.cc
>  M     chrome/gpu/gpu_command_buffer_stub.h
>  M     chrome/gpu/gpu_command_buffer_stub.cc
>  M     chrome/renderer/render_view.h
>  M     chrome/renderer/render_view.cc
>  M     chrome/renderer/render_widget.h
>  M     chrome/renderer/render_widget.cc
>
>
>

[nduca, 2010-11-11 23:53:33]

http://codereview.chromium.org/4815001/diff/1/chrome/browser/gpu_process_host.cc
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/4815001/diff/1/chrome/browser/gpu_process_host...
chrome/browser/gpu_process_host.cc:354: void SendDelayedReply(IPC::Message*
reply_msg) {
I don't have any religion here... this is largely copying the lead of the Mac &
Linux equivalent functions, which do basically the same thing all inline in this
file.

I think a cleanup might be nice, though, so if we can get to consensus about
what the clean way to do this is, I could fix my windows commit and then file
and own a follwup bug for cleanup of the other platforms.

On 2010/11/11 22:32:15, apatrick_chromium wrote:
> I think some of this code might belong in GpuProcessHostUIShim. You might also
> be able to use RouteOnUIThreadTask and SendOnIOThreadTask to pass the messages
> between threads,

http://codereview.chromium.org/4815001/diff/1/chrome/browser/gpu_process_host...
chrome/browser/gpu_process_host.cc:358: void
CreateCompositorHostWindowDispatcher(int32 renderer_id,
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> Wrap first argument to next line.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/common/gpu_messages_inte...
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/4815001/diff/1/chrome/common/gpu_messages_inte...
chrome/common/gpu_messages_internal.h:103:
IPC_SYNC_MESSAGE_CONTROL2_1(GpuHostMsg_CreateCompositorHostWindow,
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> Do these synchronous messages introduce the possibility of a deadlock?

I'm not 100% sure... when I was implementing this, I built the following
spreadsheet which shows is the init process during which this sync message
occurs:
https://spreadsheets.google.com/a/google.com/ccc?key=0ApoQxJwHXgm1dDVjV3B5V25...

When we issue the CreateCompositorHostWindow occurs, the renderer is blocked on
a sync ipc to the GPU process CreateViewCommandBuffer.

So, we are safe there. The condition that I worry about is the browser trying to
create another accelerated tab while still initializing this one.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:42: static LRESULT CALLBACK
CompositorWindowProc(HWND hWnd, UINT message,
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> One parameter per line. hWnd, lParam and wParam should not be camel case.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:53: HANDLE h = GetProp(hWnd,
L"CompositorWindowGpuCommandBufferStub");
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> Can you make the property name a constant?

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:60: return 0;
This is largely a C&P of the code in render_widget_host_view_win.cc's
PluginHostWindow implementation. I'm not sure the motivation for it there,
either...

On 2010/11/11 22:32:15, apatrick_chromium wrote:
> Why handle WM_CHAR? Does setting WS_DISABLED not auomatically propagate input
> events to the parent window?

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:73: gpu_thread->Send(new
GpuHostMsg_CreateCompositorHostWindow(renderer_id_,
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> One argument per line.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:82: wcex.cbSize         =
sizeof(WNDCLASSEX);
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> sizeof(wcex)

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:99: WS_EX_LEFT | WS_EX_LTRREADING |
WS_EX_RIGHTSCROLLBAR,
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> One argument per line unless they are related, like width and height. You
might
> also comment the zeroes since it isn't obvious what they are.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:104: SetProp(compositor_window,
L"CompositorWindowGpuCommandBufferStub",
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> Use a string constant.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:108: GetWindowRect(host_window,
&parent_rect);
Hahaha, I fail. Fixed.

I'm not convinced you can get in a situation where GetClientRect could fail here
--- short of the browser process itself exiting/crashing. We just made the
window about 15 lines above... I could put a guard on it, but we'd be hosed if
that happened anyway, no?

On 2010/11/11 22:32:15, apatrick_chromium wrote:
> GetWindowRect or GetClientRect? Also, if this function fails, parent_rect will
> be uninitialized and bad things might happen below.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:112: SetWindowPos(compositor_window, NULL,
0, 0,
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> One argument per line unless they are related, like width / height.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.cc:134:
DestroyWindow(static_cast<HWND>(compositor_window_));
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> Windows apparently lets properties leak when you destroy a window. Safer to
call
> RemoveProp before destroying.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
File chrome/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.h:55: public:
On 2010/11/11 22:32:15, apatrick_chromium wrote:
> I would be safer to move "public:" out of the preprocessor conditional block.

Done.

http://codereview.chromium.org/4815001/diff/1/chrome/gpu/gpu_command_buffer_s...
chrome/gpu/gpu_command_buffer_stub.h:80: HWND compositor_window_;
The linux version does not create a child window at this time. Jonathan is
pursuing a one-window solution right now that might just work. But, if he ends
up going for a nested window solution, we can definitely generalize this.

On 2010/11/11 22:32:15, apatrick_chromium wrote:
> If HWND was gfx::NativeView, would it be easier to share this with the linux
> version?

[jam, 2010-11-12 18:08:00]

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:362: 
nit:extra line

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:365: DCHECK(host);
this, and 368:  we normally don't add NULL dchecks.  in release builds, they're
compiled out, but we don't care since if it happens we'll get crash reports from
the line below them and it's easy to see what happened.  in debug builds,
developers will get a crash anyways so they'll investigate.  so the dcheck
doesn't add much value.

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:373: // Have to reply from IO thread.
you can actually just do host->Send directly

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.h:101: void OnCreateCompositorHostWindow(int32
renderer_id,
nit: I'm curious why int32 is used, instead of just int, when both
RenderWidgetHost and RenderProcessHost define their ids as ints, and the
FromID() function which you call takes in ints?

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:385: .width(),
size.height(),
nit: size.width() should be on the same line

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:757: if
(compositor_host_window_) {
nit: can you just early return if compositor_host_window_ is null?  it's nicer
to have less indentation because of the 80 character limit

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:766: BOOL b;
what's the point of b, it's not used?

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:778: 
nit: extra line

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:45: HWND hWnd,
nit: hwnd

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:50: case WM_ERASEBKGND:
nit: the case should be indented in

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:55: {
nit: i think you have extra indentation here

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:104: WS_EX_LEFT | WS_EX_LTRREADING |
WS_EX_RIGHTSCROLLBAR,
nit: need 4 spaces for indentation

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:108: 0/*x*/, 0/*y*/,
nit: i don't see this pattern of commenting input parameters much in the code. 
it seems that anybody can figure this out quickly by mousing over the function
name or looking at msdn, so this appears to fall under "don't comment something
that someone can figure out from reading the code"

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:133: {
nit: brace bracket on previous line

[nduca, 2010-11-12 19:45:32]

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:362: 
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit:extra line

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:365: DCHECK(host);
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> this, and 368:  we normally don't add NULL dchecks.  in release builds,
they're
> compiled out, but we don't care since if it happens we'll get crash reports
from
> the line below them and it's easy to see what happened.  in debug builds,
> developers will get a crash anyways so they'll investigate.  so the dcheck
> doesn't add much value.

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:373: // Have to reply from IO thread.
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> you can actually just do host->Send directly

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.h:101: void OnCreateCompositorHostWindow(int32
renderer_id,
Neat discovery. I chased this a bit deeper and it turns out that FromID
translates into a lookup into an IDMap lookup, and IDMap defines its key as
int32. Put another way, the issue is that the FromID signature is wrong.

I'm totally down with refactoring the FromID function and related. Is the right
policy to fix this within this CL, or should I do it as a separate CL?

On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: I'm curious why int32 is used, instead of just int, when both
> RenderWidgetHost and RenderProcessHost define their ids as ints, and the
> FromID() function which you call takes in ints?

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:385: .width(),
size.height(),
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: size.width() should be on the same line

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:757: if
(compositor_host_window_) {
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: can you just early return if compositor_host_window_ is null?  it's nicer
> to have less indentation because of the 80 character limit

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:766: BOOL b;
Oops. Leftover debugging. I fail. ;)
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> what's the point of b, it's not used?

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:778: 
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: extra line

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:45: HWND hWnd,
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: hwnd

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:50: case WM_ERASEBKGND:
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: the case should be indented in

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:55: {
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: i think you have extra indentation here

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:104: WS_EX_LEFT | WS_EX_LTRREADING |
WS_EX_RIGHTSCROLLBAR,
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: need 4 spaces for indentation

Done.

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:108: 0/*x*/, 0/*y*/,

On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: i don't see this pattern of commenting input parameters much in the code.

> it seems that anybody can figure this out quickly by mousing over the function
> name or looking at msdn, so this appears to fall under "don't comment
something
> that someone can figure out from reading the code"
I tend to agree.

http://codereview.chromium.org/4815001/diff/21001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:133: {
On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> nit: brace bracket on previous line

Done.

[jam, 2010-11-12 20:34:41]

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:373: // Have to reply from IO thread.
On 2010/11/12 19:45:32, nduca wrote:
> On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> > you can actually just do host->Send directly
> 
> Done.

I actually meant "host->Send", i.e. using the RenderViewHost.  GPUProcessHost
lives on the IO thread and so shouldn't be accessed on the UI thread

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.h:101: void OnCreateCompositorHostWindow(int32
renderer_id,
the IDMap can be using int32, but almost everywhere else, including where the
id's come from, uses int.  if anything, i'd prefer that the idmap uses ints as
well..

On 2010/11/12 19:45:32, nduca wrote:
> Neat discovery. I chased this a bit deeper and it turns out that FromID
> translates into a lookup into an IDMap lookup, and IDMap defines its key as
> int32. Put another way, the issue is that the FromID signature is wrong.
> 
> I'm totally down with refactoring the FromID function and related. Is the
right
> policy to fix this within this CL, or should I do it as a separate CL?
> 
> On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> > nit: I'm curious why int32 is used, instead of just int, when both
> > RenderWidgetHost and RenderProcessHost define their ids as ints, and the
> > FromID() function which you call takes in ints?
>

[jonathan.backer, 2010-11-19 15:01:31]

http://codereview.chromium.org/4815001/diff/80001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/4815001/diff/80001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:677: Send(new
ViewHostMsg_ShowCompositorHostWindow(routing_id(), show));
Need to #if defined(OS_WIN) this so that it builds on other platforms.

[nduca, 2010-11-19 18:52:36]

http://codereview.chromium.org/4815001/diff/80001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/4815001/diff/80001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:677: Send(new
ViewHostMsg_ShowCompositorHostWindow(routing_id(), show));
On 2010/11/19 15:01:31, jonathan.backer wrote:
> Need to #if defined(OS_WIN) this so that it builds on other platforms.

Done.

[nduca, 2010-11-19 19:02:23]

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.h:101: void OnCreateCompositorHostWindow(int32
renderer_id,
So, for this CL, I will leave the IPC handlers (OnX) using the Int32 since the
IPC message definitions are explicitly sized. That way, we are at least
consistent with the guidance given here.

After studying the issue in further detail, refactoring IDMap to use ints and
adjusting all of the various code that calls into it is a pretty broad change.
While I'm happy to volunteer to normalize that, I don't think that should block
this CL.


On 2010/11/12 20:34:41, John Abd-El-Malek wrote:
> the IDMap can be using int32, but almost everywhere else, including where the
> id's come from, uses int.  if anything, i'd prefer that the idmap uses ints as
> well..
> 
> On 2010/11/12 19:45:32, nduca wrote:
> > Neat discovery. I chased this a bit deeper and it turns out that FromID
> > translates into a lookup into an IDMap lookup, and IDMap defines its key as
> > int32. Put another way, the issue is that the FromID signature is wrong.
> > 
> > I'm totally down with refactoring the FromID function and related. Is the
> right
> > policy to fix this within this CL, or should I do it as a separate CL?
> > 
> > On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> > > nit: I'm curious why int32 is used, instead of just int, when both
> > > RenderWidgetHost and RenderProcessHost define their ids as ints, and the
> > > FromID() function which you call takes in ints?
> > 
>

[nduca, 2010-11-19 22:43:11]

Hey folks -

I need some LGTMs, or reasons we're not there yet.


- Nat

[apatrick_chromium, 2010-11-19 23:09:51]

I looked at all files with "gl" or "gpu" in the name.

http://codereview.chromium.org/4815001/diff/99001/app/gfx/gl/gl_context.h
File app/gfx/gl/gl_context.h (right):

http://codereview.chromium.org/4815001/diff/99001/app/gfx/gl/gl_context.h#new...
app/gfx/gl/gl_context.h:42: virtual void SetSize(gfx::Size) { }
You only made a whitespace change to this file.

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:367: RenderViewHost* host =
RenderViewHost::FromID(renderer_id,
What if the renderer process has been destroyed or has crashed. Won't this
return null?

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:375: BrowserThread::PostTask(
If this is running on the UI thread it should be in GpuProcessHostUIShim and it
should call GpuProcessHostUIShim::Send(reply_msg). The code in this file all
runs on the IO thread. That wan't the case until recently. You might need to
sync to see the fixes I made.

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:383: DCHECK(host);
See above about renderer process terminating.

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:400: void
GpuProcessHost::OnScheduleComposite(int renderer_id, int render_view_id) {
Messages received by GpuProcessHost but not handled are automatically posted to
GpuProcessHostUIShim on the UI thread. That won't work for messages that expect
a reply though I think so OnCreateCompositorHostWindow will probably have to
stay as is.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:17: #define kCompositorWindowOwner
L"kCompositorWindowOwner"
nit: make this const wchar_t[]

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:56: BeginPaint(hwnd, &paint);
Check this doesn't return null and don't call EndPaint if it does. It probably
doesn't matter if OnCompitorWindowPainted gets called in that case since
everything has gone horribly wrong.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:57: HANDLE h = GetProp(hwnd,
kCompositorWindowOwner);
Check this does not return null before calling OnCompositorWindowPainted.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:73: ChildThread* gpu_thread =
ChildThread::current();
nit: add a pointer from GpuCommandBufferStub to GpuThread. Having multiple
GpuThreads is something that might happen in the future.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:79: HWND host_window =
static_cast<HWND>(host_window_id);
If Send fails, host_window_id will be uninitialized here. You might want
CreateCompositorWindow to return a bool indicating whether it succeeded.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:113: DCHECK(compositor_window);
I think this might legitimately fail. It might be an idea to have
CreateCompositorWindow return false in that case.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:118: GetClientRect(host_window,
&parent_rect);
If the host window is destroyed for any reason, this will fail and parent_rect
will be uninitialized and potentially enormous if interpreted as valid below.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:122: SetWindowPos(compositor_window,
nit: can you drop this argument to the next line and line it up with the others.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:132: ChildThread* gpu_thread =
ChildThread::current();
See comment about GpuThread above.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:189: CreateCompositorWindow();
Check if this fails.

[nduca, 2010-11-20 00:28:49]

http://codereview.chromium.org/4815001/diff/99001/app/gfx/gl/gl_context.h
File app/gfx/gl/gl_context.h (right):

http://codereview.chromium.org/4815001/diff/99001/app/gfx/gl/gl_context.h#new...
app/gfx/gl/gl_context.h:42: virtual void SetSize(gfx::Size) { }
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> You only made a whitespace change to this file.

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:367: RenderViewHost* host =
RenderViewHost::FromID(renderer_id,
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> What if the renderer process has been destroyed or has crashed. Won't this
> return null?

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:375: BrowserThread::PostTask(
Summarizing verbal conversation: the reason that this approach works here is
that this function isn't mutating any of the host parameters.

On 2010/11/19 23:09:51, apatrick_chromium wrote:
> If this is running on the UI thread it should be in GpuProcessHostUIShim and
it
> should call GpuProcessHostUIShim::Send(reply_msg). The code in this file all
> runs on the IO thread. That wan't the case until recently. You might need to
> sync to see the fixes I made.

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:383: DCHECK(host);
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> See above about renderer process terminating.

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.cc:400: void
GpuProcessHost::OnScheduleComposite(int renderer_id, int render_view_id) {
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> Messages received by GpuProcessHost but not handled are automatically posted
to
> GpuProcessHostUIShim on the UI thread. That won't work for messages that
expect
> a reply though I think so OnCreateCompositorHostWindow will probably have to
> stay as is.

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:17: #define kCompositorWindowOwner
L"kCompositorWindowOwner"
Other code that does this follows the define pattern, e.g. plugin_constants_win.


(On 2010/11/19 23:09:51, apatrick_chromium wrote:
> nit: make this const wchar_t[]

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:56: BeginPaint(hwnd, &paint);
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> Check this doesn't return null and don't call EndPaint if it does. It probably
> doesn't matter if OnCompitorWindowPainted gets called in that case since
> everything has gone horribly wrong.

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:57: HANDLE h = GetProp(hwnd,
kCompositorWindowOwner);
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> Check this does not return null before calling OnCompositorWindowPainted.

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:73: ChildThread* gpu_thread =
ChildThread::current();
I don't see why that would affect this call. Are you saying that these threads
may not have their Send wired up to route to the GpuProcessHost?

On 2010/11/19 23:09:51, apatrick_chromium wrote:
> nit: add a pointer from GpuCommandBufferStub to GpuThread. Having multiple
> GpuThreads is something that might happen in the future.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:79: HWND host_window =
static_cast<HWND>(host_window_id);
If send fails, it means browser is dead. We're terribly horribly screwed. If
CreateCompositorWindow fails browser side, we're terribly horribly screwed. That
is to say, I'm not convinced error handling or recovery is needed here.

On 2010/11/19 23:09:51, apatrick_chromium wrote:
> If Send fails, host_window_id will be uninitialized here. You might want
> CreateCompositorWindow to return a bool indicating whether it succeeded.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:113: DCHECK(compositor_window);
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> I think this might legitimately fail. It might be an idea to have
> CreateCompositorWindow return false in that case.

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:118: GetClientRect(host_window,
&parent_rect);
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> If the host window is destroyed for any reason, this will fail and parent_rect
> will be uninitialized and potentially enormous if interpreted as valid below.

Done.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:132: ChildThread* gpu_thread =
ChildThread::current();
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> See comment about GpuThread above.

Not sold on this being necessary.

http://codereview.chromium.org/4815001/diff/99001/chrome/gpu/gpu_command_buff...
chrome/gpu/gpu_command_buffer_stub.cc:189: CreateCompositorWindow();
On 2010/11/19 23:09:51, apatrick_chromium wrote:
> Check if this fails.

Done.

[jam, 2010-11-20 00:36:59]

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.h:101: void OnCreateCompositorHostWindow(int32
renderer_id,
On 2010/11/19 19:02:29, nduca wrote:
> So, for this CL, I will leave the IPC handlers (OnX) using the Int32 since the
> IPC message definitions are explicitly sized. That way, we are at least
> consistent with the guidance given here.

I feel kind of strongly that these should be ints, to match the rest of the code
that handles IDs of RenderProcessHosts and RenderViewHosts.

I agree fixing IDMap shouldn't block this change.  But it also doesn't mean that
we should use int32 until it's done, since that's being inconsistent with a lot
of existing code.

> 
> After studying the issue in further detail, refactoring IDMap to use ints and
> adjusting all of the various code that calls into it is a pretty broad change.
> While I'm happy to volunteer to normalize that, I don't think that should
block
> this CL.
> 
> 
> On 2010/11/12 20:34:41, John Abd-El-Malek wrote:
> > the IDMap can be using int32, but almost everywhere else, including where
the
> > id's come from, uses int.  if anything, i'd prefer that the idmap uses ints
as
> > well..
> > 
> > On 2010/11/12 19:45:32, nduca wrote:
> > > Neat discovery. I chased this a bit deeper and it turns out that FromID
> > > translates into a lookup into an IDMap lookup, and IDMap defines its key
as
> > > int32. Put another way, the issue is that the FromID signature is wrong.
> > > 
> > > I'm totally down with refactoring the FromID function and related. Is the
> > right
> > > policy to fix this within this CL, or should I do it as a separate CL?
> > > 
> > > On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> > > > nit: I'm curious why int32 is used, instead of just int, when both
> > > > RenderWidgetHost and RenderProcessHost define their ids as ints, and the
> > > > FromID() function which you call takes in ints?
> > > 
> > 
>

[apatrick_chromium, 2010-11-22 20:43:03]

LGTM for my part with a few nits.

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
chrome/gpu/gpu_command_buffer_stub.cc:63: EndPaint(hwnd, &paint);
EndPaint needs to be matched with every successful BeginPaint so it should be
outside of the if (h) { block.

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
chrome/gpu/gpu_command_buffer_stub.cc:80: gpu_thread->Send(new
GpuHostMsg_CreateCompositorHostWindow(
indentation is out.

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
chrome/gpu/gpu_command_buffer_stub.cc:201: *ring_buffer = NULL;
We use base::SharedMemory::NULLHandle() instead of null because it works on all
platforms. I don't think you need this assignment though, it's initialized to
the null handle up top.

[nduca, 2010-11-22 22:37:44]

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/4815001/diff/21001/chrome/browser/gpu_process_...
chrome/browser/gpu_process_host.h:101: void OnCreateCompositorHostWindow(int32
renderer_id,
On 2010/11/20 00:37:00, John Abd-El-Malek wrote:
> On 2010/11/19 19:02:29, nduca wrote:
> > So, for this CL, I will leave the IPC handlers (OnX) using the Int32 since
the
> > IPC message definitions are explicitly sized. That way, we are at least
> > consistent with the guidance given here.
> 
> I feel kind of strongly that these should be ints, to match the rest of the
code
> that handles IDs of RenderProcessHosts and RenderViewHosts.
> 
> I agree fixing IDMap shouldn't block this change.  But it also doesn't mean
that
> we should use int32 until it's done, since that's being inconsistent with a
lot
> of existing code.
> 
> > 
> > After studying the issue in further detail, refactoring IDMap to use ints
and
> > adjusting all of the various code that calls into it is a pretty broad
change.
> > While I'm happy to volunteer to normalize that, I don't think that should
> block
> > this CL.
> > 
> > 
> > On 2010/11/12 20:34:41, John Abd-El-Malek wrote:
> > > the IDMap can be using int32, but almost everywhere else, including where
> the
> > > id's come from, uses int.  if anything, i'd prefer that the idmap uses
ints
> as
> > > well..
> > > 
> > > On 2010/11/12 19:45:32, nduca wrote:
> > > > Neat discovery. I chased this a bit deeper and it turns out that FromID
> > > > translates into a lookup into an IDMap lookup, and IDMap defines its key
> as
> > > > int32. Put another way, the issue is that the FromID signature is wrong.
> > > > 
> > > > I'm totally down with refactoring the FromID function and related. Is
the
> > > right
> > > > policy to fix this within this CL, or should I do it as a separate CL?
> > > > 
> > > > On 2010/11/12 18:08:00, John Abd-El-Malek wrote:
> > > > > nit: I'm curious why int32 is used, instead of just int, when both
> > > > > RenderWidgetHost and RenderProcessHost define their ids as ints, and
the
> > > > > FromID() function which you call takes in ints?
> > > > 
> > > 
> > 
> 

Done.

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
chrome/gpu/gpu_command_buffer_stub.cc:63: EndPaint(hwnd, &paint);
On 2010/11/22 20:43:03, apatrick_chromium wrote:
> EndPaint needs to be matched with every successful BeginPaint so it should be
> outside of the if (h) { block.

Done.

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
chrome/gpu/gpu_command_buffer_stub.cc:80: gpu_thread->Send(new
GpuHostMsg_CreateCompositorHostWindow(
On 2010/11/22 20:43:03, apatrick_chromium wrote:
> indentation is out.

Done.

http://codereview.chromium.org/4815001/diff/124001/chrome/gpu/gpu_command_buf...
chrome/gpu/gpu_command_buffer_stub.cc:201: *ring_buffer = NULL;
On 2010/11/22 20:43:03, apatrick_chromium wrote:
> We use base::SharedMemory::NULLHandle() instead of null because it works on
all
> platforms. I don't think you need this assignment though, it's initialized to
> the null handle up top.

Done.

[jam, 2010-11-24 19:51:13]

lgtm with the one comment

in another cl (and not necessarily you :) ), it would be great if
gpu_command_buffer_stub.h used ints everywhere instead of int32 for
render_process/render_view IDs

http://codereview.chromium.org/4815001/diff/161001/chrome/common/gpu_messages...
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/4815001/diff/161001/chrome/common/gpu_messages...
chrome/common/gpu_messages_internal.h:104: int32, /* renderer_id */
all the int32 here and below need to be int