To feed the NPAPI the nsWindow implementation store some native key event data on the nsGUIEvent. With e10s this native data gets lost and never reaches the nsObjectFrame thus disabling the key events for windowless plugins. I observed the behaviour with a Qt/Fennec setup, but it should break in the same manner for at least GTK (dunno about Windows and Mac).

I'll look into this.

Still working on this. So far lower and upper case letters work. Special keys and symbols still need investigation. The idea is to *not* use the void* pluginEvent of nsKeyEvent, since this doesn't make it through ipc, but to use the provided state and key information instead.

This is a work in progress. On the platform I tested it with it at least works for latin characters, shift, control and some punctuation. This should be tested on a desktop build, I guess.

Some more insights: I notice non-US keyboard layouts seem to be broken in respect to key events on not only mozilla, but also chromium and opera. On the German keyboard # and 3 are two different keys, yet pressing them yields the same keycode. Lacking the native event, we can only synthesize a new event from the keycode, making it impossible to distinguish 3 and #. I feel like using the charcode will be the best plan of action, but for some reason it is nulled in keyup/down events, where the keycode is nulled in keypress events. According to http://www.quirksmode.org/js/keys.html we are wrong in nulling charcode on up/down events anyways.

Addendum: the above observations hold on linux. On windows firefox, safari and chrome generate different keycodes for # (191) and 3 (51).

Could you be more specific about what we're doing differently? There should be no differences at all in the nsWindow -> nsGUIEvent -> nsObjectFrame code for e10s or not.

nsKeyEvent contains a void* pluginEvent which contains the native Gdk/Qt/Windows?/Mac? Event as received by nsWindow. This is used to get to the native keycode to generate the nsapi event. e10s events go through javascript which is where the void* gets lost.

heeen and I discussed on IRC. I think we should provide a way to duplicate a DOM event from js. I think we should create 2 methods for this: nsIDOMEvent nsIDOMWindowUtils::duplicateEvent(nsIDOMEvent); [noscript] nsIDOMEvent nsIDOMNSEvent::duplicate(); The former method just calls the latter method. It hides the method from content's script. And the latter method is implemented on each nsDOM*Event. Then, we can dispatch a lossless event to content process. How about you, smaug?

IMO this all should happen without any JS, and since the event contains platform specific and event non-DOM specific data, couldn't we just duplicate nsKeyEvent and send it to the right process?

Here's the fennec part for adding nativeKeyCodes to key events. I spent a day trying to figure out how to send the event directly to the right process instead of through js, but alas I couldn't find a solution. So for this patch, the event still goes through js as before. In the nativeKeyCode I store the keycode as received by X11/the platform.

mozilla part

> I spent a day > trying to figure out how to send the event directly to the right process > instead of through js, but alas I couldn't find a solution. you should register DOMEvent "keypress/keydown/keyrelease" listener in in TabParent, catch events, extract nsKeyEvent, serialize it (see example probably NPWindow serialization). Send it to TabChild (probably better to create new IPC method) On tab child you just do whatever you do in existing patch

Changed as per romaxa's request, not routing key events through js anymore.

Comment on attachment 476817 [details] [diff] [review] not through js anymore Just few random comments. >@@ -261,16 +261,17 @@ interface nsIDOMWindowUtils : nsISupport You must update iid when modifying an interface > * > * @return false if the event had preventDefault() called on it, > * true otherwise. In other words, true if and only if the > * default action was taken. > */ > boolean sendKeyEvent(in AString aType, > in long aKeyCode, > in long aCharCode, >+ in long aNativeKeyCodes, > in long aModifiers, > [optional] in boolean aPreventDefault); This change breaks probably all the key event handling tests. You could add the new parameter as an optional parameter after aPreventDefault. >@@ -179,25 +179,27 @@ interface nsIDOMKeyEvent : nsIDOMUIEvent Update iid > const unsigned long DOM_VK_BACK_SLASH = 0xDC; > const unsigned long DOM_VK_CLOSE_BRACKET = 0xDD; > const unsigned long DOM_VK_QUOTE = 0xDE; > > const unsigned long DOM_VK_META = 0xE0; > > readonly attribute unsigned long charCode; > readonly attribute unsigned long keyCode; >+ readonly attribute unsigned long nativeKeyCode; We certainly don't want to expose this to web content this way. If this can't be [noscript], then at least mozNativeKeyCode > void initKeyEvent(in DOMString typeArg, > in boolean canBubbleArg, > in boolean cancelableArg, > in nsIDOMAbstractView viewArg, > in boolean ctrlKeyArg, > in boolean altKeyArg, > in boolean shiftKeyArg, > in boolean metaKeyArg, > in unsigned long keyCodeArg, >- in unsigned long charCodeArg); >+ in unsigned long charCodeArg, >+ in unsigned long nativeKeyCodeArg); No way. This would break all sorts of web sites. Use [optional] or add new mozInitKeyEvent or something. >+nsresult >+TabParent::HandleEvent(nsIDOMEvent *aEvent) >+{ >+ nsString str_event; >+ aEvent->GetType(str_event); >+ if(str_event.EqualsLiteral("keydown") >+ || str_event.EqualsLiteral("keypress") >+ || str_event.EqualsLiteral("keyup")) >+ { Strange indentation. And || should be in the end of a line. '{' shouldn't start a new line after 'if' >+ nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aEvent); >+ PRUint32 modifiers=0; Missing space before and after 0 >+ SendKeyEvent( >+ str_event, >+ keyCode, >+ charCode, >+ nativeKeyCode, >+ modifiers, >+ false //?? no prevent defaukt in nsIDOMKeyEvent >+ ); Strange indentation

Florian, when you want a review from someone, you must specify them in the reviewer field. Otherwise, no one sees it.

Comment on attachment 477104 [details] [diff] [review] moz part - incorporated smaug's suggestions maybe smaug?

Comment on attachment 476816 [details] [diff] [review] not through js anymore Ben - want your opinion here

Can't argue with any of that! I haven't looked to closely at how preventDefault or stopPropagation could possibly work... are keyboard shortcuts going to work after this fix?

Maybe it would be wiser to use the X SymKey to send through the dom and translating it back to a X keycode at the last possible step, so someone could make some sense from the value. The regular keycode is broken by design: on a German keyboard " and 2 generate two different dom keycodes since on the english keyboard they are different keys, while 3 and # generate the same code since they are the same key, but are seperate keys on a German layout. Ä,Ö and Ü generate a zero keycode since they are nonexistant on a US Layout. Noone can make any sense from this mess.

Comment on attachment 477104 [details] [diff] [review] moz part - incorporated smaug's suggestions >diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl >--- a/content/base/public/nsIFrameLoader.idl >+++ b/content/base/public/nsIFrameLoader.idl >@@ -118,17 +118,18 @@ interface nsIFrameLoader : nsISupports You change an interface, so update the iid. > void sendCrossProcessKeyEvent(in AString aType, > in long aKeyCode, > in long aCharCode, > in long aModifiers, >- [optional] in boolean aPreventDefault); >+ [optional] in boolean aPreventDefault, >+ [optional] in long aNativeKeyCode); > NS_IMETHODIMP > nsFrameLoader::SendCrossProcessKeyEvent(const nsAString& aType, > PRInt32 aKeyCode, > PRInt32 aCharCode, >+ PRInt32 aNativeKeyCode, > PRInt32 aModifiers, > PRBool aPreventDefault) Huh, I assume this compiles just because PRBool is internally PRInt32 or something. In the .idl you're adding new parameter as the last parameter, here you're doing something else. I assume this all wasn't tested properly. >+nsDOMKeyboardEvent::GetMozNativeKeyCode(PRUint32* aKeyCode) >+{ >+ NS_ENSURE_ARG_POINTER(aKeyCode); >+ switch (mEvent->message) { >+ case NS_KEY_UP: >+ case NS_KEY_PRESS: >+ case NS_KEY_DOWN: >+ *aKeyCode = ((nsKeyEvent*)mEvent)->nativeKeyCode; >+ break; >+ default: >+ ReportWrongPropertyAccessWarning("nativeKeyCode"); I think this is not really needed. mozNativeKeyCode isn't anything for web developers, I hope. > NS_IMETHODIMP > nsDOMKeyboardEvent::InitKeyEvent(const nsAString& aType, PRBool aCanBubble, PRBool aCancelable, > nsIDOMAbstractView* aView, PRBool aCtrlKey, PRBool aAltKey, > PRBool aShiftKey, PRBool aMetaKey, >- PRUint32 aKeyCode, PRUint32 aCharCode) >+ PRUint32 aKeyCode, PRUint32 aCharCode, PRUint32 aNativeKeyCode) Actually, I think I'd prefer a new Init method for this. Similar to what MouseEvent has. > NS_IMETHODIMP > nsDOMWindowUtils::SendKeyEvent(const nsAString& aType, > PRInt32 aKeyCode, > PRInt32 aCharCode, > PRInt32 aModifiers, > PRBool aPreventDefault, >+ PRInt32 aNativeKeyCode, > PRBool* aDefaultActionTaken) > { > if (!IsUniversalXPConnectCapable()) { > return NS_ERROR_DOM_SECURITY_ERR; > } > > // get the widget to send the event to > nsCOMPtr<nsIWidget> widget = GetWidget(); >@@ -262,17 +262,18 @@ interface nsIDOMWindowUtils : nsISupport > * @return false if the event had preventDefault() called on it, > * true otherwise. In other words, true if and only if the > * default action was taken. > */ > boolean sendKeyEvent(in AString aType, > in long aKeyCode, > in long aCharCode, > in long aModifiers, >- [optional] in boolean aPreventDefault); >+ [optional] in boolean aPreventDefault, >+ [optional] in long aNativeKeyCodes); Again, .idl attributes don't map to .cpp method properly > void initKeyEvent(in DOMString typeArg, > in boolean canBubbleArg, > in boolean cancelableArg, > in nsIDOMAbstractView viewArg, > in boolean ctrlKeyArg, > in boolean altKeyArg, > in boolean shiftKeyArg, > in boolean metaKeyArg, > in unsigned long keyCodeArg, >- in unsigned long charCodeArg); >+ in unsigned long charCodeArg, >+ [optional] in unsigned long mozNativeKeyCodeArg); So if you could add a new method. And if possible, make it [noscript] Also, could .mozNativeKeyCode be [noscript]? > TabParent::TabParent() > : mSecurityState(nsIWebProgressListener::STATE_IS_INSECURE) > { >+ nsCOMPtr<nsIWindowWatcher> ww = do_GetService(NS_WINDOWWATCHER_CONTRACTID); >+ nsCOMPtr<nsIDOMWindow> window; >+ ww->GetActiveWindow( getter_AddRefs(window) ); >+ nsCOMPtr<nsIDOMEventTarget> target=do_QueryInterface(window); So you add event listeners to somewhat random window?!? And why window? What if focus is in the location bar. Do you really want to send event to content process? >+TabParent::HandleEvent(nsIDOMEvent *aEvent) >+{ >+ nsString str_event; nsAutoString eventType; or something like that. using '_' in the variable names isn't common. >+ false); //?? no prevent defaukt in nsIDOMKeyEvent 'defaukt' -> 'default' >+QtKeyCodeToXKeySym(int qtkey) So QT doesn't provide this mapping anywhere? >+static bool sAltGrModifier=false; //quick fix for pmo 189373 space before and after = >+ if(sAltGrModifier) { >+ aEvent.isControl=PR_TRUE; >+ aEvent.isAlt=PR_TRUE; >+ } ditto Someone more familiar with QT key events should look at this too.

Olli, How close is this platform patch to what we want in https://bugzilla.mozilla.org/show_bug.cgi?id=583976?

Yoink.

for bjacob - windowed and windowless flash test cases, including a text input applet.

This recreates XKeyEvent.keycode based on the mCodeNameIndex, which from my understanding isn't always possible. I will probably have to add a field like mNativeKeyCode to WidgetKeyboardEvent and handle that in ipdl.

Thanks for the patch. Somehow, it doesn't apply on current mozilla-central. If you could elaborate in some more detail on the multiple things that this patch is changing, that would be very interesting.

Evilpie: sorry about any duplication of effort; we had sort of informally assigned this bug to me during the last e10s meeting, and I forgot to update the assignee field. We want this fixed asap though, so it doesn't hurt to have more hands on it! Especially as I'm a n00b in this area. This fixes the bug for me on linux/GTK: Jim's above "editor - windowless.html" works in an e10s tab, with this patch. The problem was that in the child process we were hitting this warning: http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/dom/plugins/base/nsPluginInstanceOwner.cpp#l2162 That was because anEvent.pluginEvent was null. That field is how we pass the fields of a GdkEventKey (https://developer.gnome.org/gdk3/stable/gdk3-Event-Structures.html#GdkEventKey) that are not reflected by any field in WidgetKeyboardEvent (http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/widget/TextEvents.h#l68). Indeed, see this code and comment in the place where we encode this event info on GTK: http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/widget/gtk/nsGtkKeyUtils.cpp#l1004 1004 // The transformations above and in gdk for the keyval are not invertible 1005 // so link to the GdkEvent (which will vanish soon after return from the 1006 // event callback) to give plugins access to hardware_keycode and state. 1007 // (An XEvent would be nice but the GdkEvent is good enough.) 1008 aKeyEvent.pluginEvent = (void *)aGdkKeyEvent; Indeed, in the code that we fail to execute (for lack of pluginEvent serialization) on deserialization on the child side, we do try to use that pluginEvent field to retrieve the hardware_keycode, state and also "is_modifier" field of the GdkEventKey: http://hg.mozilla.org/mozilla-central/file/161fadfcbadc/dom/plugins/base/nsPluginInstanceOwner.cpp#l2120 Thus it seems that as far as GTK is concerned, we can solve our problems by dropping the hack that consists in using the pluginEvent field to pass the GdkEventKey, and instead just add these 3 fields from GdkEvent to WidgetKeyboardEvent: hardware_keycode, state and is_modifier. The attached patch does that. It adds a "guard" boolean field, mHasExtraTastyBits, to tell whether these 3 extra fields are present. The silly name was intentionally chosen so that we would feel compelled to think of a better name. If this looks reasonable, then I'll look at what fields need to be serialized on Windows, and hopefully we'll be able to settle on a small number of extra fields to add to WidgetKeyboardEvent to be able to cover all platforms.

Was not properly recording GdkEventKey::is_modifier in KeymapWrapper::InitKeyEvent.

Nice work! I actually have an other patch that takes almost the same approach. The patch looks pretty good to me, I just wouldn't do that conditional serialization. I would also put these fields in some native data struct. You also correctly have mIsModifier, which I haven't.

Comment on attachment 8455032 [details] [diff] [review] WIP: properly serialize key events, GTK-only strawman for now Code looks ok, for the event changes I'm interested in getting masayuki's feedback.

I think we'll probably want to isolate those platform specific fields somehow, I don't think we want those exposed to the rest of the code base. Maybe we should wrap those in some sort of "pluginEvent" type struct within WidgetKeyboardEvent?

Thanks for the feedback. Meanwhile, I'm setting up a Windows dev env on my new laptop, and trying to fix the issue there (I can already confirm that this reproduces). Once we know what the Windows-specific patch would look like, we can think of how to combine that with the above Linux-specific patch and get the cross-platformness right.

Comment on attachment 8455032 [details] [diff] [review] WIP: properly serialize key events, GTK-only strawman for now First, why don't you use mNativeKeyEvent? I think that it's safe to copy its data across process boundary. So, I think that you don't need to add additional members to WidgetKeyboardEvent. Anyway, I comment my suggestions below: >diff --git a/widget/gtk/nsGtkKeyUtils.cpp b/widget/gtk/nsGtkKeyUtils.cpp >--- a/widget/gtk/nsGtkKeyUtils.cpp >+++ b/widget/gtk/nsGtkKeyUtils.cpp >@@ -997,20 +997,22 @@ KeymapWrapper::InitKeyEvent(WidgetKeyboa > GetBoolName(aKeyEvent.IsShift()), GetBoolName(aKeyEvent.IsControl()), > GetBoolName(aKeyEvent.IsAlt()), GetBoolName(aKeyEvent.IsMeta()))); > > if (aKeyEvent.message == NS_KEY_PRESS) { > keymapWrapper->InitKeypressEvent(aKeyEvent, aGdkKeyEvent); > } > > // The transformations above and in gdk for the keyval are not invertible >- // so link to the GdkEvent (which will vanish soon after return from the >- // event callback) to give plugins access to hardware_keycode and state. >- // (An XEvent would be nice but the GdkEvent is good enough.) >- aKeyEvent.pluginEvent = (void *)aGdkKeyEvent; >+ // so write extra fields to give access to the hardware_keycode and state. >+ aKeyEvent.mHasExtraTastyBits = true; Why do we need this flag? Don't you set it in all key event dispatcher? >+ aKeyEvent.mIsModifier = aGdkKeyEvent->is_modifier; For emulating all key events strictly, we need this flag, indeed. However, is this really necessary in actual usage? Isn't it enough to check mKeyNameIndex value? >+ aKeyEvent.mHardwareKeyCode = aGdkKeyEvent->hardware_keycode; Why don't you use mCodeNameIndex? It does not cover all keys of unknown keyboards. However, I guess that current support level is enough for the most users (except Sun keyboard users, see bug 1020139). If it were not enough, we could add original values for specific keys with empty string as its code value. If you will use it on other platforms too, mScanCode sounds better to me. >+ aKeyEvent.mState = aGdkKeyEvent->state; Unfortunately, we need this at least for GTK. However, as far as possible. So, I believe that you should copy the data of mNativeKeyEvent. >@@ -321,18 +327,27 @@ struct ParamTraits<mozilla::WidgetKeyboa > ReadParam(aMsg, aIter, &codeNameIndex) && > ReadParam(aMsg, aIter, &aResult->mKeyValue) && > ReadParam(aMsg, aIter, &aResult->mCodeValue) && > ReadParam(aMsg, aIter, &aResult->keyCode) && > ReadParam(aMsg, aIter, &aResult->charCode) && > ReadParam(aMsg, aIter, &aResult->isChar) && > ReadParam(aMsg, aIter, &aResult->mIsRepeat) && > ReadParam(aMsg, aIter, &aResult->location) && >- ReadParam(aMsg, aIter, &aResult->mUniqueId)) >+ ReadParam(aMsg, aIter, &aResult->mUniqueId) && >+ ReadParam(aMsg, aIter, &aResult->mHasExtraTastyBits) ) nit: extra space between ")"s. > { >+ if (aResult->mHasExtraTastyBits) { >+ if (!(ReadParam(aMsg, aIter, &aResult->mIsModifier) && >+ ReadParam(aMsg, aIter, &aResult->mHardwareKeyCode) && >+ ReadParam(aMsg, aIter, &aResult->mState))) >+ { nit: "{" should be the end of the previous line.

Many thanks for the feedback Masayuki. Notice that the existing code that I touched, was not using mNativeKeyEvent, mCodeNameIndex or mKeyName. Instead it was using the pluginEvent pointer to reference the GdkEventKey directly. That's what I was starting from. I do understand that the fields that you point out (mNativeKeyEvent, mCodeNameIndex, mKeyName) sound like fields that I should be using, instead of introducing new fields redundant with these. Patch coming. Though I'll first try to get something working on Windows.

Comment on attachment 8455032 [details] [diff] [review] WIP: properly serialize key events, GTK-only strawman for now I think it would be nice if we could recreate keycode and state with just the current information, specifically mCodeNameIndex (or what it's called). I however kind of doubt this is possible, because that conversion isn't loseless and there is a comment that hints at that.

This new version handles Windows as well as Linux. We had to propagate 3 fields from the Windows MSG struct: message, wParam and lParam. For this strawman patch, I just added new fields for that to WidgetKeyboardEvent. Could any of these fields reuse existing WidgetKeyboardEvent fields instead? In widget/windows/KeyboardLayout.cpp, I had to remove the condition on mIMEState.mEnabled being PLUGIN, as that was not the case - instead, it was DISABLED. Should I worry about that?

Comment on attachment 8456520 [details] [diff] [review] WIP proof of concept, working on Windows and Linux Review of attachment 8456520 [details] [diff] [review]: ----------------------------------------------------------------- Have you tested on OSX yet? Looks like we use that pluginEvent struct there as well. ::: dom/plugins/base/nsPluginInstanceOwner.cpp @@ +2126,5 @@ > //XXX case NS_MOUSE_SCROLL_EVENT: not received. > > + case NS_KEY_EVENT: { > + const WidgetKeyboardEvent& keyEvent = *anEvent.AsKeyboardEvent(); > + if (keyEvent.mHasExtraTastyBits) since this replaces a valid pointer check on 'pluginEvent', lets name mHasExtraTastyBits a bit better, how about something like mValidNativeEvent or mValidPluginEvent? ::: widget/TextEvents.h @@ +156,5 @@ > + uint16_t mHardwareKeyCode; > + uint32_t mState; > + uint32_t mWindowsMessage; > + uintptr_t mWindowsWParam; > + uintptr_t mWindowsLParam; If we can't share these among platforms, we should ifdef this by platform and define only the fields we need so we don't have a bunch of loose platform event related variables hanging around that are unused. (note there's also a type different between wparam and lparam.). typedef unsigned int UINT; typedef UINT_PTR WPARAM; typedef LONG_PTR LPARAM; typedef _W64 unsigned int UINT_PTR, *PUINT_PTR; // A 32-bit unsigned integer. typedef _W64 long LONG_PTR, *PLONG_PTR; // A 32-bit signed integer. ::: widget/gtk/nsGtkKeyUtils.cpp @@ +1005,5 @@ > + // so write extra fields to give access to the hardware_keycode and state. > + aKeyEvent.mHasExtraTastyBits = true; > + aKeyEvent.mIsModifier = aGdkKeyEvent->is_modifier; > + aKeyEvent.mHardwareKeyCode = aGdkKeyEvent->hardware_keycode; > + aKeyEvent.mState = aGdkKeyEvent->state; We also appear to be storing the raw gdk event in WidgetKeyboardEvent's mNativeKeyEvent. Can we get rid of mNativeKeyEvent and coalesce the native storage into a single set of variables? ::: widget/windows/KeyboardLayout.cpp @@ -1094,5 @@ > KeyboardLayout::NotifyIdleServiceOfUserActivity(); > > - NPEvent pluginEvent; > - if (aMsgSentToPlugin && > - mWidget->GetInputContext().mIMEState.mEnabled == IMEState::PLUGIN) { Honestly I don't understand the IME check here either, masayuki would be a good reviewer for this change.

So I spent the day on OSX, but not even writing the OSX key events patch: at the moment, on OSX e10s, we can't even set the focus on a windowless plugin by clicking on it. So I couldn't even get to where I could reproduce the bug. This patches fixes that for me, but I have no idea whether it's the right approach. What it does is handle NS_FOCUS_CONTENT (which was unhandled on OSX) and make it use some Cocoa Objective C call found in nsChildView.mm that sets the focus. With that I can finally reproduce, so I'll be writing the OSX patch for key events tomorrow...

Here is a much simpler patch to handle focus events on Mac, that naively looks like it might be ready for review already? Masayuki / Steven , I don't know who would be the right reviewer for this?

Comment on attachment 8458267 [details] [diff] [review] Handle plugin focus events on Mac / e10s I think that this should be reviewed by josh.

Comment on attachment 8458267 [details] [diff] [review] Handle plugin focus events on Mac / e10s Review of attachment 8458267 [details] [diff] [review]: ----------------------------------------------------------------- Definitely want Steven to review this.

Comment on attachment 8458267 [details] [diff] [review] Handle plugin focus events on Mac / e10s I will take a look at this next week.

(In reply to Steven Michaud from comment #46) > Comment on attachment 8458267 [details] [diff] [review] > Handle plugin focus events on Mac / e10s > > I will take a look at this next week. Note that this patch doesn't work as intended; your help welcome to understand why. This patch does visually give the focus to the plugin area, e.g. in Jimm's editor testcase it gives the focus with the blinking cursor.... BUT key events are still not directed to the plugin view. As such, I still can't get key events working in plugins on Mac, even with the platform-agnostic patch which I'm going to attach below.

This time, here is a really cross-platform patch that really "should" fix this entire class of problems (not just in key events but all GUI events) and indeed it does fix this bug on Windows and on Linux. (On Mac, I still can't properly handle focus, as noted above, and also another patch seems to be needed at least, see next patch). With this approach, we are not anymore looking at changing fields in WidgetKeyboardEvent. Instead, we embrace the existing approach of the pluginEvent generic field, and just make it serializable, so it works in e10s just as well as in non-e10s. Please refer to the comment on the nested PluginEvent struct in WidgetGUIEvent.

Comment on attachment 8460197 [details] [diff] [review] Turn the pluginEvent field into a buffer and serialize it Review of attachment 8460197 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good. Not a fan of the copying of local pluginEvents for set up, but since this is keyboard handling the overhead shouldn't be an issue. ::: dom/plugins/base/nsPluginInstanceOwner.cpp @@ +1878,5 @@ > > int16_t response = kNPEventNotHandled; > void* window = FixUpPluginWindow(ePluginPaintEnable); > if (window || (eventModel == NPEventModelCocoa)) { > + mInstance->HandleEvent(const_cast<NPCocoaEvent*>(event), &response, NS_PLUGIN_CALL_SAFE_TO_REENTER_GECKO); nit - wrap me

Here is an update on my attempts to debug the focus issues on Mac. I've added some logging and compared what happened between e10s and non-e10s. Here is the basic difference: In non-e10s (where things work), when I click on the plugin area, I get the becomeFirstResponder callback called (as defined in nsChildView.mm) on the plugin's view. That in turn generates the focus events. In e10s (where things do not work), that becomeFirstResponder callback (in nsChildView.mm) is never called - neither for the plugin view nor for any other view, it seems. I'm failing to understand that so I think I'll work on other stuff until someone with Mac widget knowledge can help me understand that.

I'm hoping to help Benoit with this later this week. But it will involve hours of work, and I can't spare the time right now.

Comment on attachment 8460197 [details] [diff] [review] Turn the pluginEvent field into a buffer and serialize it Looks like that this patch (almost?) all of the users of WidgetGUIEvent::pluginEvent. So, this is a good change to rename it. Could you rename it to mPluginEvent? And I think that mBuffer should be private and ParamTraits<WidgetGUIEvent> should be a friend of PluginEvent. Although, looks like that PluginEvent struct isn't good name since it can work with anything, i.e., very general. However, I have no idea for any other points. So, It's okay to keep this here in this bug. PluginEvent could have AsNPEvent(), AsNPCocoaEvent() and AsANPEvent() for making callers simpler. But I don't want them so strongly.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #52) > Comment on attachment 8460197 [details] [diff] [review] > Turn the pluginEvent field into a buffer and serialize it > > Looks like that this patch (almost?) all of the users of > WidgetGUIEvent::pluginEvent. So, this is a good change to rename it. Could > you rename it to mPluginEvent? Done. > > And I think that mBuffer should be private and ParamTraits<WidgetGUIEvent> > should be a friend of PluginEvent. Done. > > Although, looks like that PluginEvent struct isn't good name since it can > work with anything, i.e., very general. However, I have no idea for any > other points. So, It's okay to keep this here in this bug. OK, let me know if you can think of a better name. Kept the name PluginEvent for now. > > PluginEvent could have AsNPEvent(), AsNPCocoaEvent() and AsANPEvent() for > making callers simpler. But I don't want them so strongly. Ideally, PluginEvent would remember of type of the data structure that was given to Copy(), and would only allow getting that type back. In that case, As*() would be a good name for the getter. For now though, PluginEvent does not remember any type information and only stores an untyped buffer; so it does not perform any type checking when getting the structure back; I would be afraid of exposing something so dangerous under an innocuous name like As*(). As long as it's unsafe, I'd rather have it feel like an unsafe pointer.

Landed the serialization patch, so we now have e10s plugin events working on Linux and Windows at least. On Mac, they might well be working too, but the above-discussed focus bug prevents checking that. Leaving open until that is resolved. https://hg.mozilla.org/integration/mozilla-inbound/rev/8581ea023552

Filed bug 1044182 for the mac focus issue.

Mac focus work is going on on bug 1044182; the original issue discussed here is fixed, so let's close this bug. Of course, until bug 1044182 is fixed, we can't confirm that it's actually fixed on Mac, but the fact that the same cross-platform patch did fix it on Linux and Windows makes it reasonable to default to assuming so.

Comment on attachment 8458267 [details] [diff] [review] Handle plugin focus events on Mac / e10s I now think we need to deal with bug 1044182 before we can tackle this bug.