977904 - [e10s] Get native key bindings working

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: native-key-events

We don't support native key bindings in electrolysis for two reasons:

1. Key events are handled in the child process, which isn't supposed to be able to access OS services.
2. The native key binding code uses a pointer to the GdkEvent* or NSEvent*, which can't be sent to the child over IPC.

This patch addresses both these problems. It exposes a special native key bindings proxy service in the child process. This service just sends a sync message to the parent whenever it's called. The parent process then gets the platform-specific key binding service, gets the list of commands that the key maps to, and sends the list back to the child.

Since the platform-specific key binding service needs access to the event's mNativeKeyEvent field, we save this field in the parent process before sending key events to the child. Then when the proxy service asks for the key bindings, the parent can reuse the cached value for mNativeKeyEvent.

To deal with the memory management for saving mNativeKeyEvent, I wrapped it in a refcounted abstract class. This class also exposes an ID field, which allows us to decide whether the cached mNativeKeyEvent is actually for the event that the proxy service is asking about.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8383381 [details] [diff] [review]
native-key-events

Well, I wonder if child process is busy, isn't ContentParent::mSavedNativeKeyEvent overwritten by following key event?

And I worry about the performance. Does this work fine with auto key repeat on slow machines?

NativeKeyBindings returns the command list for the focused text/HTML editor. AFAIK, KeyPress() of NativeKeyBindings is now only used. And widget can know if a single text editor, multiline text editor or HTML editor has focus or nobody of them has focus with nsIWidget::GetInputContext().mHTMLInputType and nsIWidget::GetInputContext().mIMEState.mEnabled. So, it might be enough we to set command list only for every keypress event.

Currently, NativeKeyBindings is used here:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#839
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorEventListener.cpp#539

I think that even if JS changes focus by keypress event. If JS sets focus to an editor, the editor won't receive the keypress event because the event target has already been decided. If JS steals focus from an editor, the command list will be ignored.

Any ideas, Ehsan and/or Neil?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> Well, I wonder if child process is busy, isn't ContentParent::mSavedNativeKeyEvent
> overwritten by following key event?

Yes, that's a problem with this patch. I guess we could keep multiple native events around if that's a big issue.

> And I worry about the performance. Does this work fine with auto key repeat on slow machines?

Are you worried about the extra allocation? I'm pretty skeptical that one allocation per keystroke will hurt us on even older desktop hardware.

> So, it might be enough we to set command list only for every keypress event.

I don't understand how we would know which kind of key binding to use (input, editor, or textarea). Even if the child tells us whenever the focus changes, it seems like we would have the same problem with a slow child as before. What if you hit tab (which switches to a different kind of field) and then another key? If we need to get the commands for the second key before the child has processed the tab character, how will be know what kind of field is in focus?

I guess we could get the commands for all three kinds of fields and send that to the child. That would be guaranteed to work I think, but it seems a little excessive.

Comment 5

[(no longer active)]

Masayuki, sorry but I'm not sure if I understand the question you're asking me completely, can you please clarify?

This is actually the first time that I'm hearing about mHTMLInputType!  Looking at nsIMEStateManager, I think it's unaware of HTML editable fields.  Is that correct?

About the call sites you listed, the nsTextEditorState call site handles plaintext fields (input/textareas) and the nsEditorEventListener call site handles HTML editable fields (designMode/contenteditable).  The former seems to care about keydown, keypress and keyup, and the latter only cares about keypress for now.  So I think we may need to keep doing the IPC for all three elements.

Can you please help me understand your concern about focus?  nsEditorEventListener installs its event handler on the root element of the editor (which should be the contenteditable element or the document element in the case of designMode documents) so I think we should receive all of the events that we're interested in.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Bill McCloskey (:billm) from comment #4)
> > And I worry about the performance. Does this work fine with auto key repeat on slow machines?
> 
> Are you worried about the extra allocation? I'm pretty skeptical that one
> allocation per keystroke will hurt us on even older desktop hardware.

I worry about the cost of cross process communication which is being added by the patch.

Allocation already occurs with other reasons. So, you don't mind about that.

However, it's true we should prevent allocation as far as possible. If we can store command into WidgetKeyboardEvent, that's enough to add an int member like mKeyNameIndex.

> > So, it might be enough we to set command list only for every keypress event.
> 
> I don't understand how we would know which kind of key binding to use
> (input, editor, or textarea).

As I said, widget stores InputContext which is set by nsIWidget::SetInputContext(). This is called at focus move. widget changes some behavior with this.

> Even if the child tells us whenever the focus
> changes, it seems like we would have the same problem with a slow child as
> before. What if you hit tab (which switches to a different kind of field)
> and then another key? If we need to get the commands for the second key
> before the child has processed the tab character, how will be know what kind
> of field is in focus?

Hmm, it may be possible. However, it's true, we have already had the problem. As I said, focus move causes a call of nsIWidget::SetInputContext() and changes the behavior. For example, if first native key event in editor occurs before a call of nsIWidget::SetInputContext(), the native key event isn't sent to IME or sent to IME with "wrong" disabled state. This could cause very annoying bug for users. How does Chromium solve this??

> I guess we could get the commands for all three kinds of fields and send
> that to the child. That would be guaranteed to work I think, but it seems a
> little excessive.

If NativeKeyBinding caches the result at first time of every key combination and can clear them at system settings are changed, we can reduce the cost. Although, I don't know if it's possible on both Cocoa and GTK. Note that we can ignore the native key bindings at causing text input by the keypress.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #5)
> This is actually the first time that I'm hearing about mHTMLInputType! 
> Looking at nsIMEStateManager, I think it's unaware of HTML editable fields. 
> Is that correct?

Partially, yes. But IIRC, when mHTMLInputType is empty but IME isn't disabled, HTML editor has focus.

> About the call sites you listed, the nsTextEditorState call site handles
> plaintext fields (input/textareas) and the nsEditorEventListener call site
> handles HTML editable fields (designMode/contenteditable).  The former seems
> to care about keydown, keypress and keyup, and the latter only cares about
> keypress for now.

Sure. But KeyDown() and KeyUp() always return false :-(

http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsNativeKeyBindings.cpp#244
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsNativeKeyBindings.cpp#332
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/NativeKeyBindings.mm#170
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/NativeKeyBindings.mm#268

We should remove the methods completely (in other bug).

> So I think we may need to keep doing the IPC for all
> three elements.
> 
> Can you please help me understand your concern about focus? 
> nsEditorEventListener installs its event handler on the root element of the
> editor (which should be the contenteditable element or the document element
> in the case of designMode documents) so I think we should receive all of the
> events that we're interested in.

I didn't have any concern about focus issue at posting the comment since event target is decided at dispatching the event into DOM tree and editor checks the event target is still focused AFAIK.

However, billm's comment 4 suggests new issue, though...

My question for you is, how do you think the best approach for fixing this bug? Sorry for my unclear question.

I'm still thinking which approach is better and looking for new better approach.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I filed bug 977959. I think that if we make the native key bindings handler just a method of nsIWidget, the patch for this bug becomes simpler.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't think that using nsIWidget::GetInputContext() will work. I tried three different kinds of fields:

1. In cases where the code asks for the "input" native key bindings, GetInputContext returns these values:

mHTMLInputType = "text" or ""
mIMEState.mEnabled = true

2. In cases where the code asks for the "textarea" native key bindings, GetInputContext returns these values:

mHTMLInputType = "textarea"
mIMEState.mEnabled = true

3. In cases where the code asks for the "editor" native key bindings, GetInputContext returns these values:

mHTMLInputType = ""
mIMEState.mEnabled = true

So I can't figure out how to distinguish "input" from "editor".


Given that the native key bindings code almost never returns any commands, I am thinking maybe the parent process should just get the commands for all possible editor types. Then we can send those commands to the child process, and it can use whichever list it needs. Since the lists will usually be empty, we won't actually be sending much data. Does that sound okay?

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Bill McCloskey (:billm) from comment #9)
> 1. In cases where the code asks for the "input" native key bindings,
> GetInputContext returns these values:
> 
> mHTMLInputType = "text" or ""
> mIMEState.mEnabled = true

Oh, the case |mHTMLInputTYpe == ""| is a bug. I'll fix it.

> Given that the native key bindings code almost never returns any commands, I
> am thinking maybe the parent process should just get the commands for all
> possible editor types. Then we can send those commands to the child process,
> and it can use whichever list it needs. Since the lists will usually be
> empty, we won't actually be sending much data. Does that sound okay?

Yeah. Sounds OK to me.

But I think that the all results should be represented with one enum value. Then, it doesn't cause any allocation. If you want me to change current code like that, I'll work on it next week.

Comment 11

[[e-mail]]

(In reply to Bill McCloskey from comment #9)
> Given that the native key bindings code almost never returns any commands, I
> am thinking maybe the parent process should just get the commands for all
> possible editor types. Then we can send those commands to the child process,
> and it can use whichever list it needs. Since the lists will usually be
> empty, we won't actually be sending much data. Does that sound okay?

Presumably you'd do this in DispatchCrossProcessEvent or thereabouts?

Comment 12

[[e-mail]]

Comment on attachment 8383381 [details] [diff] [review]
native-key-events

Cancelling feedback because I don't think I can usefully contribute further at this stage.

Comment 13

[(no longer active)]

Comment on attachment 8383381 [details] [diff] [review]
native-key-events

What is suggested at the end of comment 9 seems like a better idea to me.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> But I think that the all results should be represented with one enum value. Then, it doesn't
> cause any allocation. If you want me to change current code like that, I'll work on it next
> week.

Thanks, I would appreciate that.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8383381 [details] [diff] [review]
native-key-events

After bug 977959, please recreate the patch. It's waiting remaining one review.

Comment 16

[Tom S [:evilpie]]

Attachment: WIP

So I started working on this, based on Masayuki patches. Those indeed made this a lot easier, thanks! We now request the native-key-binding for every key-press. Sadly ExecuteNativeKeyBinding is asynchronous, which makes this a little bit harder. I assign a unique-id to every key-press event, which is already done on windows, but still needs to be moved to some better place. Once the callback is invoked we send the unique id, type and the command to the child. In the child we cache the command in a hashtable with unique id + type as key. I think I am leaking the callback data, so I need to find some better way to do this. The cache in the child should maybe have a limited size.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks, Tom!

I'm pretty sure that ExecuteNativeKeyBinding is not asynchronous. I think it uses a callback because it wants to be able to return multiple commands without using a dynamically sized array. However, the callback is always called while ExecuteNativeKeyBinding is still on the stack. The only two implementations are here:

http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/NativeKeyBindings.mm#176
http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsNativeKeyBindings.cpp#304

So I think you should be able get rid of the unique ID stuff and the hashtable.

Comment 18

[Tom S [:evilpie]]

So gtk_bindings_activate in KeyPressInternal synchronously invokes the callback? If that's the case this should be even easier.

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Tom Schuster [:evilpie] from comment #18)
> So gtk_bindings_activate in KeyPressInternal synchronously invokes the
> callback? If that's the case this should be even easier.

Given how the code sets that gCurrentCallback global and then nulls it out after gtk_bindings_activate returns, I think it must be synchronous.

Comment 20

[Chris Peterson [:cpeterson]]

evilpie: Don't forget to add some asserts to test your assumption! :)

Comment 21

[Tom S [:evilpie]]

Attachment: v2

Yup, really nice.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8390014 [details] [diff] [review]
v2

The callback may be called two or more times for a key event.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

After bug 977959, you can stop using callback on each NativeKeyBindigns. I think that it makes the patch simpler you to make NativeKeyBindigns::Execute() return its result with nsTArray<mozilla::Command> argument. I.e., NativeKeyBindigns::Execute(mozilla::WidgetKeyboardEvent, nsTArray<mozilla::Command>& aCommands); and caller passes nsAutoTArray<mozilla::Command, 4>.

Comment 24

[Tom S [:evilpie]]

Okay I can do that as well. I was just thinking about how to avoid creating useless arrays for IPDL, but this should probably fix this.

Comment 25

[Tom S [:evilpie]]

Attachment: v3

I didn't bother implementing that change you suggested, but I can do that in a followup.

Comment 26

[(no longer active)]

Comment on attachment 8390170 [details] [diff] [review]
v3

Review of attachment 8390170 [details] [diff] [review]:
-----------------------------------------------------------------

Tom, is there a specific part of this patch you'd like me to review?  I think Masayuki is a much better person to review this stuff than I am.  I don't really know much about native key event handling.

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8390170 [details] [diff] [review]
v3

Please use -U 8 -p...

>+struct NativeKeyBinding {

The "{" should be in next line.

>+  uint32_t[] singleLineCommands;
>+  uint32_t[] multiLineCommands;
>+  uint32_t[] richTextCommands;
>+};

You should use m prefix the members. And you can use mozilla::CommandInt instead of uint32_t.

>+
>+union MaybeNativeKeyBinding {

The "{" should be in next line.

>+  NativeKeyBinding;
>+  void_t;
>+};

> bool
>+TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& event,
>+                           const MaybeNativeKeyBinding& aBindings)
>+ {
>+  if (event.message == NS_KEY_PRESS) {
>+    PuppetWidget* widget = static_cast<PuppetWidget*>(mWidget.get());
>+    if (aBindings.type() == MaybeNativeKeyBinding::TNativeKeyBinding) {
>+      const NativeKeyBinding& bindings = aBindings;
>+      widget->CacheNativeKeyCommands(bindings.singleLineCommands(),
>+                                     bindings.multiLineCommands(),
>+                                     bindings.richTextCommands());
>+    } else {
>+      widget->ClearNativeKeyCommands();
>+    }
>+  }

I have worry about this approach. I think that when you detect coming nested key event, you should call MOZ_CRASH(). It may be possible of a bug of automated tests.

>@@ -797,7 +803,30 @@
>   if (!MapEventCoordinatesForChildProcess(&event)) {
>     return false;
>   }
>-  return PBrowserParent::SendRealKeyEvent(event);
>+
>+
>+  MaybeNativeKeyBinding bindings;
>+  bindings = void_t();
>+  if (event.message == NS_KEY_PRESS) {
>+    nsCOMPtr<nsIWidget> widget = GetWidget();
>+
>+    InfallibleTArray<uint32_t> singleLine;
>+    InfallibleTArray<uint32_t> multiLine;
>+    InfallibleTArray<uint32_t> richText;

Why don't use AutoInfallibleTArray<mozilla::CommandInt, 4>? It must prevent to make damage with reallocation.

>diff -r 9f508e937bd3 widget/xpwidgets/PuppetWidget.h
>--- a/widget/xpwidgets/PuppetWidget.h	Thu Mar 13 00:01:39 2014 +0100
>+++ b/widget/xpwidgets/PuppetWidget.h	Thu Mar 13 01:07:44 2014 +0100

>@@ -225,6 +247,11 @@
>   // The DPI of the screen corresponding to this widget
>   float mDPI;
>   double mDefaultScale;
>+
>+  // Precomputed answers for ExecuteNativeKeyBinding
>+  InfallibleTArray<uint32_t> mSingleLineCommands;
>+  InfallibleTArray<uint32_t> mMultiLineCommands;
>+  InfallibleTArray<uint32_t> mRichTextCommands;

I think you should use AutoInfallibleTArray<mozilla::CommandInt, 4> here too.

Comment 28

[Tom S [:evilpie]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #27)
> Comment on attachment 8390170 [details] [diff] [review]
> v3
> 
> Please use -U 8 -p...
> 
Oh sorry, forgot that after my last re-installation.
> >+struct NativeKeyBinding {
> 
> The "{" should be in next line.
> 
> >+  uint32_t[] singleLineCommands;
> >+  uint32_t[] multiLineCommands;
> >+  uint32_t[] richTextCommands;
> >+};
> 
> You should use m prefix the members. And you can use mozilla::CommandInt
> instead of uint32_t.
CommandInt is a good idea, but we don't prefix members in IPDL.
> 
> >+
> >+union MaybeNativeKeyBinding {
> 
> The "{" should be in next line.
> 
> >+  NativeKeyBinding;
> >+  void_t;
> >+};
> 
> > bool
> >+TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& event,
> >+                           const MaybeNativeKeyBinding& aBindings)
> >+ {
> >+  if (event.message == NS_KEY_PRESS) {
> >+    PuppetWidget* widget = static_cast<PuppetWidget*>(mWidget.get());
> >+    if (aBindings.type() == MaybeNativeKeyBinding::TNativeKeyBinding) {
> >+      const NativeKeyBinding& bindings = aBindings;
> >+      widget->CacheNativeKeyCommands(bindings.singleLineCommands(),
> >+                                     bindings.multiLineCommands(),
> >+                                     bindings.richTextCommands());
> >+    } else {
> >+      widget->ClearNativeKeyCommands();
> >+    }
> >+  }
> 
> I have worry about this approach. I think that when you detect coming nested
> key event, you should call MOZ_CRASH(). It may be possible of a bug of
> automated tests.
What are "coming nested key event"?
> 
> >@@ -797,7 +803,30 @@
> >   if (!MapEventCoordinatesForChildProcess(&event)) {
> >     return false;
> >   }
> >-  return PBrowserParent::SendRealKeyEvent(event);
> >+
> >+
> >+  MaybeNativeKeyBinding bindings;
> >+  bindings = void_t();
> >+  if (event.message == NS_KEY_PRESS) {
> >+    nsCOMPtr<nsIWidget> widget = GetWidget();
> >+
> >+    InfallibleTArray<uint32_t> singleLine;
> >+    InfallibleTArray<uint32_t> multiLine;
> >+    InfallibleTArray<uint32_t> richText;
> 
> Why don't use AutoInfallibleTArray<mozilla::CommandInt, 4>? It must prevent
> to make damage with reallocation.
> 
I haven't heard of AutoInfallibleTArray before. Do you think 4 is a sane value? What "damage with reallocation" are you talking about? I feel like most of the time there are even less entries in the array.
> >diff -r 9f508e937bd3 widget/xpwidgets/PuppetWidget.h
> >--- a/widget/xpwidgets/PuppetWidget.h	Thu Mar 13 00:01:39 2014 +0100
> >+++ b/widget/xpwidgets/PuppetWidget.h	Thu Mar 13 01:07:44 2014 +0100
> 
> >@@ -225,6 +247,11 @@
> >   // The DPI of the screen corresponding to this widget
> >   float mDPI;
> >   double mDefaultScale;
> >+
> >+  // Precomputed answers for ExecuteNativeKeyBinding
> >+  InfallibleTArray<uint32_t> mSingleLineCommands;
> >+  InfallibleTArray<uint32_t> mMultiLineCommands;
> >+  InfallibleTArray<uint32_t> mRichTextCommands;
> 
> I think you should use AutoInfallibleTArray<mozilla::CommandInt, 4> here too.
Ok

Comment 29

[(no longer active)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #26)
> Comment on attachment 8390170 [details] [diff] [review]
> v3
> 
> Review of attachment 8390170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tom, is there a specific part of this patch you'd like me to review?  I
> think Masayuki is a much better person to review this stuff than I am.  I
> don't really know much about native key event handling.

ni?evilpie!

Comment 30

[Tom S [:evilpie]]

Sorry, I don't think there is anything you have to review.

Comment 31

[Tom S [:evilpie]]

>I think you should use AutoInfallibleTArray<mozilla::CommandInt, 4> here too.
Actually there is no point in doing this, because we get InfallibleTArray from IPDL.

Comment 32

[Tom S [:evilpie]]

Attachment: v4

So I fixed stuff that I think make sense. I don't understand what you meant with "nested events" so this isn't included yet.

Comment 33

[Tom S [:evilpie]]

Can you please explain "nested events" and answer my question about AutoInfallibleTArray?

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Tom Schuster [:evilpie] from comment #32)
> Created attachment 8390693 [details] [diff] [review]
> v4
> 
> So I fixed stuff that I think make sense. I don't understand what you meant
> with "nested events" so this isn't included yet.

If automated tests call nsIDOMWindowUtils::Send(Native)KeyEvent() from keydown or keypress event handler, key event can be nested. Of course, this is the bug of automated tests.

(In reply to Tom Schuster [:evilpie] from comment #31)
> >I think you should use AutoInfallibleTArray<mozilla::CommandInt, 4> here too.
> Actually there is no point in doing this, because we get InfallibleTArray
> from IPDL.

Although, I'm not familiar with actual implementation of nsTArray, I guess that copying instance doesn't reallocate its array buffer if the capacity isn't larger than already allocated size. If it's true, my suggestion can prevent reallocation in most environment (If users customized some shortcut keys with OS settings and one key caused two or more actions, it would need to reallocate the array, though).

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Well, sorry for the delay to review it. I feel something bad today. So, I'll review it tomorrow or next Monday because I want to review patches when my brain is clear.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8390693 [details] [diff] [review]
v4

>diff -r 9f508e937bd3 dom/ipc/PBrowser.ipdl
>--- a/dom/ipc/PBrowser.ipdl	Thu Mar 13 00:01:39 2014 +0100
>+++ b/dom/ipc/PBrowser.ipdl	Thu Mar 13 19:58:19 2014 +0100
>@@ -49,10 +49,22 @@
> using struct mozilla::layers::TextureFactoryIdentifier from "mozilla/layers/CompositorTypes.h";
> using mozilla::CSSPoint from "Units.h";
> using mozilla::CSSToScreenScale from "Units.h";
>+using mozilla::CommandInt from "mozilla/EventForwards.h";
> 
> namespace mozilla {
> namespace dom {
> 
>+struct NativeKeyBinding {

Move { to the next line.

>+  CommandInt[] singleLineCommands;
>+  CommandInt[] multiLineCommands;
>+  CommandInt[] richTextCommands;
>+};

Hmm, I don't like raw array if there is no demerit to use nsTArray... But I'm not familiar with IPC related code, so, I don't have strong objection.

And please use "m" prefix for members of struct.

>+
>+union MaybeNativeKeyBinding {

Same.

>diff -r 9f508e937bd3 dom/ipc/TabChild.cpp
>--- a/dom/ipc/TabChild.cpp	Thu Mar 13 00:01:39 2014 +0100
>+++ b/dom/ipc/TabChild.cpp	Thu Mar 13 19:58:19 2014 +0100
>@@ -1917,8 +1917,20 @@
> }
> 
> bool
>-TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& event)
>-{
>+TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& event,
>+                           const MaybeNativeKeyBinding& aBindings)
>+ {
>+  if (event.message == NS_KEY_PRESS) {
>+    PuppetWidget* widget = static_cast<PuppetWidget*>(mWidget.get());
>+    if (aBindings.type() == MaybeNativeKeyBinding::TNativeKeyBinding) {
>+      const NativeKeyBinding& bindings = aBindings;
>+      widget->CacheNativeKeyCommands(bindings.singleLineCommands(),
>+                                     bindings.multiLineCommands(),
>+                                     bindings.richTextCommands());
>+    } else {
>+      widget->ClearNativeKeyCommands();
>+    }
>+  }
>   // If content code called preventDefault() on a keydown event, then we don't
>   // want to process any following keypress events.
>   if (event.message == NS_KEY_PRESS && mIgnoreKeyPressEvent) {

>diff -r 9f508e937bd3 widget/xpwidgets/PuppetWidget.h
>--- a/widget/xpwidgets/PuppetWidget.h	Thu Mar 13 00:01:39 2014 +0100
>+++ b/widget/xpwidgets/PuppetWidget.h	Thu Mar 13 19:58:19 2014 +0100
>@@ -225,6 +248,11 @@
>   // The DPI of the screen corresponding to this widget
>   float mDPI;
>   double mDefaultScale;
>+
>+  // Precomputed answers for ExecuteNativeKeyBinding
>+  InfallibleTArray<mozilla::CommandInt> mSingleLineCommands;
>+  InfallibleTArray<mozilla::CommandInt> mMultiLineCommands;
>+  InfallibleTArray<mozilla::CommandInt> mRichTextCommands;
> };
> 
> class PuppetScreen : public nsBaseScreen

I still think that they should be Auto*... Then, they must be allocated with fixed array when they are constructed. Or, another possible idea is, you should call SetCapacity() in constructor of PuppetWidget? Anyway, the bad scenario which I think is, the array allocates 1 item, then, reallocates 2 items, finally, allocates 3 or more items.

Comment 37

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just wanted to clarify a few things about IPDL that Tom mentioned already.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> Hmm, I don't like raw array if there is no demerit to use nsTArray... But
> I'm not familiar with IPC related code, so, I don't have strong objection.

When you write "Type[] field" in IPDL, it translates that to "InfallibleTArray<Type> field". So there isn't any raw array here.

> And please use "m" prefix for members of struct.

We don't use "m" prefixes in IPDL because it would lead to very awkward names for some of the auto-generated accessor functions. If you grep through all *.ipdl files, you can see that no "m" prefixes are used.

Comment 38

[Tom S [:evilpie]]

Attachment: v5

You can't assign InfallibleTArrays to AutoInfallibleTArray, so I instead used the SetCapacity trick.

Comment 39

[Tom S [:evilpie]]

Attachment: v5

Right version of the patch this time.

Comment 40

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da72442a5a3c

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da72442a5a3c