522122 - Electrolysis: Forward windows events

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: rough version of what this could look like

This gets basic event forwarding working on win32. It doesn't support any IME events.

It looks like windowless plugins and IME are a bit of disaster story:
- We pretend to support them but fail to get the proper characters into their places
- Chrome and Opera don't even pretend to support IME (IME is disabled when the plugin has focus)
- Safari doesn't disable IME but no characters show up at all
- IE has the most usable implementation but screws up the drawing: it puts the characters temporarily into content or doesn't display them at all.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Chris, I haven't ported linux to NPRemoteEvent yet, but does the patch above look basically sane?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looks good!

Linux port can be a one-liner in PluginMessageUtils.h, |typedef NPEvent NPRemoteEvent;|.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Forward events on windows

This should basically work. It doesn't forward any IME events for the reasons explained above.

Linux doesn't use a typedef to NPEvent because we assign to the event element of NPEvent. It could be possible to fix this by making NPRemoteEvent a subclass of NPEvent. Is this prefered?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 407097 [details] [diff] [review]
Forward events on windows

>diff --git a/dom/plugins/NPEventWindows.h b/dom/plugins/NPEventWindows.h
>--- a/dom/plugins/NPEventWindows.h
>+++ b/dom/plugins/NPEventWindows.h
>     static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>     {
>+        const char* bytes = 0;
>+
>+        if (!aMsg->ReadBytes(aIter, &bytes, sizeof(paramType))) {
>+            return false;
>+        }
>+        memcpy(aResult, bytes, sizeof(paramType));
>+
>+        if (aResult->event.event == WM_PAINT) {
>+            // restore the lParam to point at the NPRect
>+            aResult->event.lParam = reinterpret_cast<uint32>(&aResult->rect);
>+        }

What happens here for 64-bit Windows?  Or does that not exist and/or we don't support it?

> #endif // ifndef mozilla_dom_plugins_NPEventWindows_h
>diff --git a/dom/plugins/NPEventX11.h b/dom/plugins/NPEventX11.h
>--- a/dom/plugins/NPEventX11.h
>+++ b/dom/plugins/NPEventX11.h
>@@ -43,16 +43,29 @@
> #if defined(MOZ_WIDGET_GTK2)
> #  include <gdk/gdkx.h>
> #else
> #  error Implement me for your toolkit
> #endif
> 
> #include "npapi.h"
> 
>+namespace mozilla {
>+
>+namespace plugins {
>+
>+typedef NPRemoteEvent NPEvent;
>+    NPEvent event;

Oops, typo?

> namespace IPC {
> 
> template <>
>-struct ParamTraits<NPEvent>     // synonym for XEvent
>+struct ParamTraits<mozilla::plugins::NPRemoteEvent>     // synonym for XEvent
> {
>     static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
[...]
>         memcpy(aResult, bytes, sizeof(paramType));
>-        SetXDisplay(*aResult);
>+        SetXDisplay(aResult->event);

This looks like the same typo as above.

>diff --git a/dom/plugins/PluginInstanceParent.cpp b/dom/plugins/PluginInstanceParent.cpp
>--- a/dom/plugins/PluginInstanceParent.cpp
>+++ b/dom/plugins/PluginInstanceParent.cpp
>@@ -427,31 +427,33 @@ PluginInstanceParent::NPP_GetValue(NPPVa
> }
> 
> int16_t
> PluginInstanceParent::NPP_HandleEvent(void* event)
> {
>     _MOZ_LOG(__FUNCTION__);
> 
>     NPEvent* npevent = reinterpret_cast<NPEvent*>(event);
>+    NPRemoteEvent npremoteevent;
>+    npremoteevent.event = *npevent;
> 

Same inconsistency.

r+ with the X11 stuff fixed up.

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to comment #4)
> What happens here for 64-bit Windows?  Or does that not exist and/or we don't
> support it?

Makoto Kato is working on Windows x64 support in bug 471090, so it would be good to not create new compatibility issues.

This intel portability guide says that LPARAM is 64-bit on x64:
http://www.intel.com/cd/ids/developer/asmo-na/eng/197664.htm?page=3

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Fix problems with the last patch

uses LPARAM instead of uint32 and fixes the linux build

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 407400 [details] [diff] [review]
Fix problems with the last patch

Karl,

My hg repo got itself all corrupt. In the mean time, if you have a chance, do you mind taking a look at this patch?

Comment 8

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 407400 [details] [diff] [review]
Fix problems with the last patch

An observation regarding interaction with ipdl in general:

  Often/usually there is a transformation of data from a format suitable for
  in-process use (e.g. NPEvent) to a format suitable for cross-process
  communication (NPRemoteEvent).

  It feels nice and tidy when the transformation can be performed within a
  ParamTraits<> specification, but sometimes, additional information is
  necessary for the transformation or there isn't enough flexibility in
  ParamTraits<> to perform the transformation.  So we end up sometimes doing
  the transformation at the ParamTraits level, and sometimes at the level
  above (PluginInstanceParent/Child).

  I could imagine situations where the transformation initially happens at one level, then something changes in the parameter or additional functionality is added, and then the transformation code needs to be moved from one level to another.

  This isn't a comment about this patch, but the general approach.
  I write it down because I'm interested to hear others' views on what the general approach should be.

In this patch, most of the transformation happens at the ParamTraits level,
but the NPRemoteEvent is also exposed to the next level.

This is a useful trick to ensure that ipdl provides the necessary memory on
the stack (that is still valid after ParamTraits<>::Read()).  The symmetry of
ParamTraits means that this additional memory is also provided for Write(),
even though it's not needed.  That's fine I think: it means more copies of the data, but there's elegance in the symmetry.

>+        // Make a non-const copy of aParam so that we can muck with
>+        // its insides for tranport
>+        paramType aParamCopy = aParam;

It might be worthwhile only copying the |paramType::event| member to make it
clearer that |aParam.rect| doesn't get used.  I would call the local variable
|paramCopy|.

>     // plugins might be fooling with these, make a copy
>-    NPEvent evcopy = event;
>+    NPRemoteEvent evcopy = event;

Similarly here, the plugin expects an NPEvent, so its clearer to pass an
NPEvent.  (The plugin has a pointer to event.rect either way, but I can't see
any evil that could result from that.)

>+            case WM_PAINT:

If the conclusion was that the HDC wasn't valid in the other process, then
perhaps this event shouldn't be passed until we have bug 522299.  That would
mean we wouldn't need a NPRemoteEvent type yet, but maybe it will be easier to see the best way to do that once we know how the drawable will be passed.
Maybe you already have something in mind, but I don't know whether that will
have an influence on this.

The other interesting event is WM_WINDOWPOSCHANGED, which also includes a
pointer to a rect (WINDOWPOS this time).

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Attachment: event forwarding v3

Fixes the issues you pointed out. Good catch with WINDOWPOS.

Comment 10

[Benjamin Smedberg]

This landed in electrolysis:f6c3ddff4306. Is this bug FIXED?

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Sure.