1339543 - PBrowser::Msg_RequestNativeKeyBindings is too slow

Status: RESOLVED FIXED

(Core :: DOM: Events, defect, P1)

Description

[(no longer active)]

Per <https://docs.google.com/spreadsheets/d/1x_BWVlnQPg0DHbsrvPFX7g89lnFGa3lAIHWD_pLa_dE/edit#gid=738048355> this is our 5th worst sync IPC.

smaug, can we do something about this?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

It was added in bug 993714

Comment 2

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

Ehsan, I don't see that message in your spreadsheet at all. Is there a mistake?

Originally this message was only sent during tests. The RequestNativeKeyBindings call in PuppetWidget.cpp used to be gated on mIsSynthesizedForTests. Now it seems to be running in other cases. That happened either in bug 1271125 or bug 1321245, I think. Also, I don't understand this stuff very well. Masayuki, maybe you know what is going on here?

Comment 3

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

We're using native key bindings (e.g., Emacs like shortcut keys on macOS) as far as possible. For user convenience, we retrieve native key binding information at runtime instead of declaring with XUL.

When the keyboard event is synthesized for tests, we need to to this via IPC because keyboard event is synthesized in the content process.

On the other hand, I'm not sure about mIsSuppressedOrDelayed case. I think that the original event of delayed event comes from parent process. Then, we must have native key binding information in the widget because TabParent::SendRealKeyEvent() sends it.
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/dom/ipc/TabParent.cpp#1456,1465,1472-1480

So, perhaps, DelayedKeyEvent should cache native key bindings stored in the widget and set it to widget when PresShell dispatches it and the process is not a main process. (Or WidgetKeyboardEvent should have the information instead of widget?)

Comment 4

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #2)
> Ehsan, I don't see that message in your spreadsheet at all. Is there a
> mistake?

The spreadsheet was edited by Harald while I was commenting on these bugs, and honestly I can't really trust it any more, not sure why this probe doesn't even show up.  The raw data is still in the "auto-import" sheet, but I cannot figure out how to reproduce the view I was looking at yesterday.  :/  I guess for now you should look at telemetry.m.o raw data.

Comment 5

[Andrew Overholt [:overholt]]

Do we have a plan here?

Comment 6

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

Should be not so hard to fix.

* WidgetKeyboardEvent should have commands.
* TextEventDispatcher should set them before dispatching keypress events.
* WidgetKeyboardEvent should have a method to execute the stored command.
* nsIWidget::ExecuteNativeKeyBinding() shouldn't be used from EditorEventListener::KeyPress() nor nsTextInputListener::HandleEvent().
* Some additional hack for delayed keyboard events, perhaps.

Comment 7

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

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) (PTO: 5/3-5/5) from comment #6)
> Should be not so hard to fix.
> 
> * WidgetKeyboardEvent should have commands.
> * TextEventDispatcher should set them before dispatching keypress events.
> * WidgetKeyboardEvent should have a method to execute the stored command.
> * nsIWidget::ExecuteNativeKeyBinding() shouldn't be used from
> EditorEventListener::KeyPress() nor nsTextInputListener::HandleEvent().
> * Some additional hack for delayed keyboard events, perhaps.

Oops, I misunderstood because forgot that this is not a problem when actual user input. So, those approaches are wrong.

About the issue for delayed events, we need to improve it. However, do we need to work on for synthesized events? I think that for solving it, child process should cache key bindings, however, I think that it just wants memory in real use.

For fixing the former issue, taking this.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a91e5311b89dcdfc4d37449a36ef7dc744505fd

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=045dfe58d442b62f86ba543f5cdf916cb328f493

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fae85bb6796dc59ea6472615a2edd9fa9f2ff122

Comment 11

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b28935e16a1488ecb241637fe045867d03472739

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1d6cae7ecd803708690494c20eff0fccd42f97d

Comment 13

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c21f95feafa3531c6c8d8b7d7d8cc6096c90a88

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0288bb0adc889fb265466a2f997896068200a8f2

Comment 15

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8890b8cdd3a0928898944ef6fd95d5e8b261e839

Comment 16

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d03478c11f6217b3a8a27650c22943e6ea86092

Comment 17

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f48d7bee2970a9acf6479456b65bb6c3d3fa90b2

Comment 18

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46519d402d501d5406c9a170a7b02d24054cd4a0

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=614eca6fb39cba7d2013de9565ab7b05dafa4fd8

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5952d0d29d1acbc61d22bcc3dcee28bea23d9b35

Comment 21

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

Attachment: Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

Currently, edit commands for native key bindings are stored in widget.  This is
stateful and really complicated in content process because it needs to cache
them.

We can make this simpler if we make WidgetKeyboardEvent store edit commands for
the key combination.  Then, child process can handle it even if it's delayed
event or it's a nested event.

This patch adds WidgetKeyboardEvent::ExecuteEditCommands() for the wrapper and
makes it use stored edit commands which are initialized by
WidgetKeyboardEvent::InitEditCommandsFor().

Review commit: https://reviewboard.mozilla.org/r/140554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140554/

Comment 22

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

Attachment: Bug 1339543 part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process

When eKeyPress event is dispatched from TabParent to a remote process, it
should store edit command for all editor types.  Then, copied
WidgetKeyboardEvent in the remote process doesn't need to request the edit
commands when its ExecuteEditCommands() is called.

Note that this patch also changes a automated test, browser_bug1316330.js,
that uses nsIDOMWindowUtils.dispatchDOMEventViaPresShell() to dispatch
repeated keyboard events in the tab.  However, it should use synthesizeKey()
to emulate everything of native keyboard events and the API can dispatch
repeated keyboard events too.  (And the test has a bug.  It tries to wait 0.5
sec when every keydown or keypress event.  However, it fails since startTime
is never initialized.  This patch fixes this bug too.)

Review commit: https://reviewboard.mozilla.org/r/140556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140556/

Comment 23

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

Attachment: Bug 1339543 part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests

Synthesized keyboard events in child process doesn't have edit commands when it's being dispatched.  With the old design, PuppetWidget needed to store all edit commands for dispatching keyboard event but it's not necessary now because WidgetKeyboardEvent will get only necessary edit commands when WidgetKeyboardEvent::ExecuteEditCommands() is called.

So, PuppetWidget should stop calling TabChild::RequestNativeKeyBindings() before dispatching keyboard events.

This patch changes browser_audioTabIcon.js which becomes permanent orange with this change.

The test has odd asyc method, wait_for_tab_playing_event(), which never returns Promise but is used with await. Additionally, this runs immediately after |await ContentTask.spawn()| in pause(), i.e., before |await awaitTabPausedAttrModified;|, and the last check of pause() causes orange.  Therefore, this patch makes wait_for_tab_playing_event() may return Promise when it needs to wait TabAttrModified event.  Additionally, makes pause() use |await wait_for_tab_playing_event(tab, false);| directly before checking if tab has soundplaying attribute.

Review commit: https://reviewboard.mozilla.org/r/140558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140558/

Comment 24

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

Attachment: Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type

Now, nsIWidget::ExecuteNativeKeyBinding() isn't used by anybody for executing edit commands.  Instead, they need array of edit commands for the key combination.  So, the method should be renamed to GetEditCommands() and just return edit commands as an array.

Review commit: https://reviewboard.mozilla.org/r/140560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140560/

Comment 25

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

Attachment: Bug 1339543 part 5 Remove unnecessary stuff from PuppetWidget

Review commit: https://reviewboard.mozilla.org/r/140562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140562/

Comment 26

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

Attachment: Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

PBrowser::RequestNativeKeyBindings() is used only when somebody tries to execute native key bindings for synthesized keyboard events.  Therefore, it doesn't need to retrieve edit commands for all editor types.  Instead, it should take the editor type and just return the edit commands for it.

Review commit: https://reviewboard.mozilla.org/r/140564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140564/

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8868921 [details]
Bug 1339543 part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests

https://reviewboard.mozilla.org/r/140558/#review144138

::: commit-message-12e9b:9
(Diff revision 1)
> +
> +So, PuppetWidget should stop calling TabChild::RequestNativeKeyBindings() before dispatching keyboard events.
> +
> +This patch changes browser_audioTabIcon.js which becomes permanent orange with this change.
> +
> +The test has odd asyc method, wait_for_tab_playing_event(), which never returns Promise but is used with await. Additionally, this runs immediately after |await ContentTask.spawn()| in pause(), i.e., before |await awaitTabPausedAttrModified;|, and the last check of pause() causes orange.  Therefore, this patch makes wait_for_tab_playing_event() may return Promise when it needs to wait TabAttrModified event.  Additionally, makes pause() use |await wait_for_tab_playing_event(tab, false);| directly before checking if tab has soundplaying attribute.

This kind of comments shouldn't IMO go to commit message, but be just a comment in bugzilla or something

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

ok, this all is complicated. Takes some time to figure out what is changed and why... but review is coming.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8868919 [details]
Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

https://reviewboard.mozilla.org/r/140554/#review144140

::: commit-message-7cc61:11
(Diff revision 1)
> +
> +We can make this simpler if we make WidgetKeyboardEvent store edit commands for
> +the key combination.  Then, child process can handle it even if it's delayed
> +event or it's a nested event.
> +
> +This patch adds WidgetKeyboardEvent::ExecuteEditCommands() for the wrapper and

for the wrapper? What wrapper is this comment talking about?

::: dom/html/nsTextEditorState.cpp:1004
(Diff revision 1)
>    if (!widget) {
>      widget = mFrame->GetNearestWidget();
>      NS_ENSURE_TRUE(widget, NS_OK);
>    }
> -                                         
> -  if (widget->ExecuteNativeKeyBinding(nativeKeyBindingsType,
> +
> +  AutoRestore<nsCOMPtr<nsIWidget>> saveWidget(keyEvent->mWidget);

I don't understand this change.
This definitely needs a comment why we need to change the widget.

I guess the reason is that ExecuteEditCommands wants some widget. But in which case do we not have a widget? When using events for testing? Should we make them have widget?

::: editor/libeditor/EditorEventListener.cpp:630
(Diff revision 1)
>      widget = pc ? pc->GetNearestWidget() : nullptr;
>      NS_ENSURE_TRUE(widget, NS_OK);
>    }
>  
>    nsCOMPtr<nsIDocument> doc = editorBase->GetDocument();
> -  bool handled = widget->ExecuteNativeKeyBinding(
> +  AutoRestore<nsCOMPtr<nsIWidget>> saveWidget(aKeyboardEvent->mWidget);

ditto

::: widget/TextEvents.h:469
(Diff revision 1)
>    static CodeNameIndexHashtable* sCodeNameIndexHashtable;
> +
> +  // mEditCommandsFor*Editor store edit commands.  This should be initialized
> +  // with InitEditCommandsFor().
> +  // XXX Ideally, this should be array of Command rather than CommandInt.
> +  //     However, ParamTraits isn't aware with enum array.

aware of

::: widget/TextEvents.h:470
(Diff revision 1)
> +
> +  // mEditCommandsFor*Editor store edit commands.  This should be initialized
> +  // with InitEditCommandsFor().
> +  // XXX Ideally, this should be array of Command rather than CommandInt.
> +  //     However, ParamTraits isn't aware with enum array.
> +  InfallibleTArray<CommandInt> mEditCommandsForSingleLineEditor;

Just nsTArray

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8868923 [details]
Bug 1339543 part 5 Remove unnecessary stuff from PuppetWidget

https://reviewboard.mozilla.org/r/140562/#review144148

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8868920 [details]
Bug 1339543 part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process

https://reviewboard.mozilla.org/r/140556/#review144162

::: widget/TextEvents.h:299
(Diff revision 1)
> +   * process.
> +   */
> +  void InitAllEditCommands();
> +
> +  /**
> +   * PreventNativeKeyBindings() makes the instance doesn't cause any edit

makes the instance to not cause ...

::: widget/TextEvents.h:328
(Diff revision 1)
> +
> +  /**
> +   * IsAllEditCommandsInitialized() returns true if edit commands for all types
> +   * were already initialized.  Otherwise, false.
> +   */
> +  bool IsAllEditCommandsInitialized() const

I think this should be
AreAllEditCommandsInitialized

::: widget/WidgetEventImpl.cpp:608
(Diff revision 1)
>  WidgetKeyboardEvent::CodeNameIndexHashtable*
>    WidgetKeyboardEvent::sCodeNameIndexHashtable = nullptr;
>  
> +void
> +WidgetKeyboardEvent::InitAllEditCommands()
> +{

Is it ok to re-initialize? If not, should there be an assertion for that.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8868922 [details]
Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type

https://reviewboard.mozilla.org/r/140560/#review144168

::: commit-message-60e4f:1
(Diff revision 1)
> +Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieve edit commands for the type r?smaug

s/retrieve/retrieves/

::: dom/ipc/TabChild.h:516
(Diff revision 1)
>  
>    void NotifyPainted();
>  
> -  void RequestNativeKeyBindings(mozilla::widget::AutoCacheNativeKeyCommands* aAutoCache,
> -                                const WidgetKeyboardEvent* aEvent);
> +  void RequestNativeKeyBindings(nsIWidget::NativeKeyBindingsType aType,
> +                                const WidgetKeyboardEvent& aEvent,
> +                                InfallibleTArray<CommandInt>& aCommands);

I think I'd still prefer use of nsTArray everywhere, but up to you.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8868924 [details]
Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

https://reviewboard.mozilla.org/r/140564/#review144184

::: commit-message-5203c:3
(Diff revision 1)
> +Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type r?smaug
> +
> +PBrowser::RequestNativeKeyBindings() is used only when somebody tries to execute native key bindings for synthesized keyboard events.  Therefore, it doesn't need to retrieve edit commands for all editor types.  Instead, it should take the editor type and just return the edit commands for it.

Given that this is testing only, why do we need to optimize here at all? Why not just send all the commands?

But since you already wrote the patch, I guess it is fine :)

::: dom/ipc/TabChild.cpp:1881
(Diff revision 1)
>    }
>    return IPC_OK();
>  }
>  
>  void
> -TabChild::RequestNativeKeyBindings(nsIWidget::NativeKeyBindingsType aType,
> +TabChild::RequestEditCommands(nsIWidget::NativeKeyBindingsType aType,

is there any way we could assert here that this method is called only when doing testing?

::: dom/ipc/TabParent.cpp:1233
(Diff revision 1)
> +    case nsIWidget::NativeKeyBindingsForSingleLineEditor:
> +    case nsIWidget::NativeKeyBindingsForMultiLineEditor:
> +    case nsIWidget::NativeKeyBindingsForRichTextEditor:
> +      break;
> +    default:
> +      return IPC_OK();

Shouldn't this be IPC_FAIL or some such

Comment 34

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

Comment on attachment 8868919 [details]
Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

https://reviewboard.mozilla.org/r/140554/#review144140

> for the wrapper? What wrapper is this comment talking about?

Ah, I meant "as a wrapper of nsIWidget::ExecuteNativeKeyBinding()".

I'll update the commit message.

> I don't understand this change.
> This definitely needs a comment why we need to change the widget.
> 
> I guess the reason is that ExecuteEditCommands wants some widget. But in which case do we not have a widget? When using events for testing? Should we make them have widget?

If event is created by chrome script, it doesn't have widget even it returns true when calling IsTrusted().

I'll add comment to there.

Comment 35

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

Comment on attachment 8868919 [details]
Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

https://reviewboard.mozilla.org/r/140554/#review144140

> Ah, I meant "as a wrapper of nsIWidget::ExecuteNativeKeyBinding()".
> 
> I'll update the commit message.

Modified the last paragraph to:

> This patch adds arrays to WidgetKeyboardEvent to store edit commands which are
> initialized with nsIWidget::ExecuteNativeKeyBinding() and adds
> WidgetKeyboardEvent::ExecuteEditCommands() to execute stored edit commands as
> same as nsIWidget::ExecutenativeKeyBinding().

Comment 36

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

Comment on attachment 8868920 [details]
Bug 1339543 part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process

https://reviewboard.mozilla.org/r/140556/#review144162

> Is it ok to re-initialize? If not, should there be an assertion for that.

Although, it's checked in InitEditCommandsFor(), I added MOZ_ASSERT() with !AreAllEditCommandsInitialized().

Comment 37

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

Comment on attachment 8868921 [details]
Bug 1339543 part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests

https://reviewboard.mozilla.org/r/140558/#review144138

> This kind of comments shouldn't IMO go to commit message, but be just a comment in bugzilla or something

Okay, I dropped this paragraph:

> The test has odd asyc method, wait_for_tab_playing_event(), which never returns
> Promise but is used with await. Additionally, this runs immediately after
> |await ContentTask.spawn()| in pause(), i.e., before |await awaitTabPausedAttrModified;|,
> and the last check of pause() causes orange.  Therefore, this patch makes
> wait_for_tab_playing_event() may return Promise when it needs to wait TabAttrModified event.
> Additionally, makes pause() use |await wait_for_tab_playing_event(tab, false);| directly
> before checking if tab has soundplaying attribute.

Comment 38

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

Comment on attachment 8868922 [details]
Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type

https://reviewboard.mozilla.org/r/140560/#review144168

> I think I'd still prefer use of nsTArray everywhere, but up to you.

I like nsTArray better (because of really shorter name), but the old code used InfallibleTArray explicitly. Therefore, I tried to keep it.

But I change them to nsTArray.

Comment 39

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

Comment on attachment 8868924 [details]
Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

https://reviewboard.mozilla.org/r/140564/#review144184

> Given that this is testing only, why do we need to optimize here at all? Why not just send all the commands?
> 
> But since you already wrote the patch, I guess it is fine :)

If there were some add-ons which is like IME or software keyboard, it can synthesize keyboard events not for tests (mIsSynthesizedForTests becomes false).
https://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/widget/TextEventDispatcher.cpp#161,165
https://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/widget/TextEventDispatcher.h#374,378,385,390,392-393

Although, I guess there are no such add-ons.

> is there any way we could assert here that this method is called only when doing testing?

Sure. Like PuppetWidget::DispatchEvent(), I'll add MOZ_ASSERT() here.

> Shouldn't this be IPC_FAIL or some such

Okay. But I usually see IPC_OK() even if it obviously unexpected case.

Comment 40

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

Comment on attachment 8868924 [details]
Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

https://reviewboard.mozilla.org/r/140564/#review144184

> Sure. Like PuppetWidget::DispatchEvent(), I'll add MOZ_ASSERT() here.

Hmm, unfortunately, it's impossible for now because we cannot check if it's created by chrome script (bug 1345374).

Comment 41

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

Comment on attachment 8868919 [details]
Bug 1339543 part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140554/diff/1-2/

Comment 42

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

Comment on attachment 8868920 [details]
Bug 1339543 part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140556/diff/1-2/

Comment 43

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

Comment on attachment 8868921 [details]
Bug 1339543 part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140558/diff/1-2/

Comment 44

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

Comment on attachment 8868922 [details]
Bug 1339543 part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140560/diff/1-2/

Comment 45

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

Comment on attachment 8868923 [details]
Bug 1339543 part 5 Remove unnecessary stuff from PuppetWidget

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140562/diff/1-2/

Comment 46

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

Comment on attachment 8868924 [details]
Bug 1339543 part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140564/diff/1-2/

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68c474271ba5
part 1 Wrap nsIWidget::ExecuteNativeKeyBinding() with a WidgetKeyboardEvent method and users of the method should use it r=smaug
https://hg.mozilla.org/integration/autoland/rev/198e58f7052f
part 2 eKeyPress event should have edit commands for all editor types when it's dispatched to a remote process r=smaug
https://hg.mozilla.org/integration/autoland/rev/a950fbfa6d8c
part 3 PuppetWidget should stop getting all edit commands before dispatching keyboard events which are synthesized for tests r=smaug
https://hg.mozilla.org/integration/autoland/rev/b75c111837a8
part 4 Change nsIWidget::ExecuteNativeKeyBinding() to nsIWidget::GetEditCommands() which just retrieves edit commands for the type r=smaug
https://hg.mozilla.org/integration/autoland/rev/8d32e150bf7f
part 5 Remove unnecessary stuff from PuppetWidget r=smaug
https://hg.mozilla.org/integration/autoland/rev/bf71ece03a29
part 6 PBrowser::RequestNativeKeyBindings() should retrieves edit commands only for specified type r=smaug

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/68c474271ba5
https://hg.mozilla.org/mozilla-central/rev/198e58f7052f
https://hg.mozilla.org/mozilla-central/rev/a950fbfa6d8c
https://hg.mozilla.org/mozilla-central/rev/b75c111837a8
https://hg.mozilla.org/mozilla-central/rev/8d32e150bf7f
https://hg.mozilla.org/mozilla-central/rev/bf71ece03a29