POSIX: gfx::NativeViewId and CrossProcessEvent

POSIX: gfx::NativeViewId and CrossProcessEvent

Create a couple new typedefs for porting work. Firstly,
gfx::NativeViewId is a handle to a platform specific widget in the
renderer process. For Windows, this is just a HWND as before. However,
in other platforms the ids used in the renderer process will be
something else.

CrossProcessEvent is the type of a HANDLE to a Windows event object
which is used across processes. Since we aren't going to support these
sorts of events on non-Windows platforms, this will have to go away at
some point. For now, however, this lets us build code without too many
ifdefs all over the place.

[darin (slow to review), 2009-01-26 19:37:26]

http://codereview.chromium.org/18768/diff/1/2
File base/gfx/native_widget_types.h (right):

http://codereview.chromium.org/18768/diff/1/2#newcode47
Line 47: typedef HWND NativeViewId;
I would also just define this as intptr_t on windows.  Is there a reason not to
do that?  It would then be the same on all platforms.

What about the idea I mentioned of having conversion functions in this file?

http://codereview.chromium.org/18768/diff/1/3
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/18768/diff/1/3#newcode48
Line 48: typedef IPCNoData CrossProcessEvent;
maybe instead of IPCNoData, this should just be void*?  that way it is possible
to assign NULL to it.  the idea of an IPC parameter that is not a parameter is
kind of strange.

if you do want IPCNoData, then i would instead put it into ipc_message_utils.h,
which is where the more generic IPC data types are defined.

http://codereview.chromium.org/18768/diff/1/4
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/18768/diff/1/4#newcode48
Line 48: CrossProcessEvent, /* model dialog box event */
Can you name this SharedEventHandle instead?  (it better matches other naming
conventions: SharedMemoryHandle)

http://codereview.chromium.org/18768/diff/1/5
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/18768/diff/1/5#newcode203
Line 203: #if defined(OS_WIN)
how about just "supporting" WaitableEvent(SharedEventHandle) on non-Windows
platform in a similar manner so that this code doesn't have to be #ifdef'd?

http://codereview.chromium.org/18768/diff/1/6
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/18768/diff/1/6#newcode17
Line 17: #include "chrome/common/render_messages.h"
including this header all over the place is a bit icky since it drags in the
world.  can you instead just create base/shared_event_handle.h with a big NOTE
about how it should not be used in new code, etc.?

http://codereview.chromium.org/18768/diff/1/7
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/18768/diff/1/7#newcode1852
Line 1852: HANDLE modal_dialog_event = NULL;
you can use the new typedef here, right?  (assumes you make it possible to
initialize SharedEventHandle to NULL on all platforms.)

http://codereview.chromium.org/18768/diff/1/11
File webkit/glue/chrome_client_impl.cc (right):

http://codereview.chromium.org/18768/diff/1/11#newcode423
Line 423: PlatformWidget ChromeClientImpl::platformWindow() const {
please re-define PlatformWidget to be intptr_t as well.  or switch NativeViewId
to be void* on all platforms.

http://codereview.chromium.org/18768/diff/1/16
File webkit/glue/webwidget_impl.cc (right):

http://codereview.chromium.org/18768/diff/1/16#newcode244
Line 244: return reinterpret_cast<PlatformWidget>(
ditto

http://codereview.chromium.org/18768/diff/1/17
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/18768/diff/1/17#newcode671
Line 671: return reinterpret_cast<gfx::NativeViewId>(host->view_handle());
How about a conversion function from NativeView to NativeViewId?

[agl, 2009-01-27 01:09:16]

Updated with Darin's suggestions and merging in Brett's patch (which was
reverted on trunk). Changed substantially I'm afraid.


AGL

[brettw, 2009-01-27 04:12:35]

http://codereview.chromium.org/18768/diff/196/288
File base/gfx/native_widget_types.h (right):

http://codereview.chromium.org/18768/diff/196/288#newcode64
Line 64: typedef intptr_t NativeViewId;
I'm not sure when I would use NativeView and when I would use NativeViewId. Can
you elaborate?

http://codereview.chromium.org/18768/diff/196/290
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/18768/diff/196/290#newcode156
Line 156: Send(new ViewMsg_New(gfx::IdFromNativeView(view()->GetPluginHWND()),
It would be nice to rename HetPluginHWND< although you don't need to do it in
this patch.

http://codereview.chromium.org/18768/diff/196/290#newcode767
Line 767: (modal_dialog_event.event);
I think this is more readable putting the opening ( with the WaitableEvent.

http://codereview.chromium.org/18768/diff/196/296
File chrome/browser/resource_message_filter.cc (right):

http://codereview.chromium.org/18768/diff/196/296#newcode478
Line 478: gfx::Rect *rect) {
Since you touched these, can you move the * to the type to match the rest of the
file?

http://codereview.chromium.org/18768/diff/196/298
File chrome/common/modal_dialog_event.h (right):

http://codereview.chromium.org/18768/diff/196/298#newcode5
Line 5: #ifndef CHROME_COMMON_MODAL_DIALOG_EVENT_H__
Only one underscore at the end nowadays.

http://codereview.chromium.org/18768/diff/196/298#newcode13
Line 13: // sure to also remove the ParamTraits for it below.
below -> probably reference resource_messages.h

http://codereview.chromium.org/18768/diff/196/298#newcode14
Line 14: struct ModalDialogEvent {
You decided to move this out of render_messages. I assume this is because it's
being used in some places that don't use IPC?

http://codereview.chromium.org/18768/diff/196/301
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/18768/diff/196/301#newcode204
Line 204: (modal_dialog_event.event);
I also like the ( on the WaitableEvent here too.

http://codereview.chromium.org/18768/diff/196/311
File webkit/glue/webplugin.h (right):

http://codereview.chromium.org/18768/diff/196/311#newcode92
Line 92: virtual void SetWindow(gfx::NativeView window, HANDLE
pump_messages_event) = 0;
80 cols.

http://codereview.chromium.org/18768/diff/196/312
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/18768/diff/196/312#newcode331
Line 331: void WebPluginImpl::SetWindow(gfx::NativeView window, HANDLE
pump_messages_event) {
80 cols.

[agl, 2009-01-27 18:01:49]

(Note that PlatformWidget is getting changed to intptr_t as well - but that
doesn't show up here)

http://codereview.chromium.org/18768/diff/196/288
File base/gfx/native_widget_types.h (right):

http://codereview.chromium.org/18768/diff/196/288#newcode64
Line 64: typedef intptr_t NativeViewId;
On 2009/01/27 04:12:35, brettw wrote:
> I'm not sure when I would use NativeView and when I would use NativeViewId.
Can
> you elaborate?

Have expanded upon the comment at the top

http://codereview.chromium.org/18768/diff/196/290
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/18768/diff/196/290#newcode156
Line 156: Send(new ViewMsg_New(gfx::IdFromNativeView(view()->GetPluginHWND()),
On 2009/01/27 04:12:35, brettw wrote:
> It would be nice to rename HetPluginHWND< although you don't need to do it in
> this patch.

Agreed, but this patch is already too large.

http://codereview.chromium.org/18768/diff/196/290#newcode767
Line 767: (modal_dialog_event.event);
On 2009/01/27 04:12:35, brettw wrote:
> I think this is more readable putting the opening ( with the WaitableEvent.

Done.

http://codereview.chromium.org/18768/diff/196/296
File chrome/browser/resource_message_filter.cc (right):

http://codereview.chromium.org/18768/diff/196/296#newcode478
Line 478: gfx::Rect *rect) {
On 2009/01/27 04:12:35, brettw wrote:
> Since you touched these, can you move the * to the type to match the rest of
the
> file?

Done.

http://codereview.chromium.org/18768/diff/196/298
File chrome/common/modal_dialog_event.h (right):

http://codereview.chromium.org/18768/diff/196/298#newcode5
Line 5: #ifndef CHROME_COMMON_MODAL_DIALOG_EVENT_H__
On 2009/01/27 04:12:35, brettw wrote:
> Only one underscore at the end nowadays.

Done.

http://codereview.chromium.org/18768/diff/196/298#newcode13
Line 13: // sure to also remove the ParamTraits for it below.
On 2009/01/27 04:12:35, brettw wrote:
> below -> probably reference resource_messages.h

Done.

http://codereview.chromium.org/18768/diff/196/298#newcode14
Line 14: struct ModalDialogEvent {
On 2009/01/27 04:12:35, brettw wrote:
> You decided to move this out of render_messages. I assume this is because it's
> being used in some places that don't use IPC?

It was a Darin suggestion to avoid including render_messages.h in lots of
places.

http://codereview.chromium.org/18768/diff/196/301
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/18768/diff/196/301#newcode204
Line 204: (modal_dialog_event.event);
On 2009/01/27 04:12:35, brettw wrote:
> I also like the ( on the WaitableEvent here too.

Done.

http://codereview.chromium.org/18768/diff/196/311
File webkit/glue/webplugin.h (right):

http://codereview.chromium.org/18768/diff/196/311#newcode92
Line 92: virtual void SetWindow(gfx::NativeView window, HANDLE
pump_messages_event) = 0;
On 2009/01/27 04:12:35, brettw wrote:
> 80 cols.

Done.

http://codereview.chromium.org/18768/diff/196/312
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/18768/diff/196/312#newcode331
Line 331: void WebPluginImpl::SetWindow(gfx::NativeView window, HANDLE
pump_messages_event) {
On 2009/01/27 04:12:35, brettw wrote:
> 80 cols.

Done.

[brettw, 2009-01-27 19:50:13]

LGTM