Closed
Bug 1339543
Opened 7 years ago
Closed 7 years ago
PBrowser::Msg_RequestNativeKeyBindings is too slow
Categories
(Core :: DOM: Events, defect, P1)
Core
DOM: Events
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
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?
Flags: needinfo?(bugs)
Comment 1•7 years ago
|
||
It was added in bug 993714
Blocks: 993714
Flags: needinfo?(bugs) → needinfo?(wmccloskey)
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?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(masayuki)
Flags: needinfo?(ehsan)
Assignee | ||
Comment 3•7 years ago
|
||
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?)
Flags: needinfo?(masayuki)
Reporter | ||
Comment 4•7 years ago
|
||
(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.
Flags: needinfo?(ehsan)
Updated•7 years ago
|
Priority: -- → P1
Updated•7 years ago
|
Whiteboard: [qf:p1]
Assignee | ||
Comment 6•7 years ago
|
||
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.
Flags: needinfo?(masayuki)
Assignee | ||
Comment 7•7 years ago
|
||
(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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a91e5311b89dcdfc4d37449a36ef7dc744505fd
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=045dfe58d442b62f86ba543f5cdf916cb328f493
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fae85bb6796dc59ea6472615a2edd9fa9f2ff122
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b28935e16a1488ecb241637fe045867d03472739
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1d6cae7ecd803708690494c20eff0fccd42f97d
Assignee | ||
Comment 13•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c21f95feafa3531c6c8d8b7d7d8cc6096c90a88
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0288bb0adc889fb265466a2f997896068200a8f2
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8890b8cdd3a0928898944ef6fd95d5e8b261e839
Assignee | ||
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d03478c11f6217b3a8a27650c22943e6ea86092
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f48d7bee2970a9acf6479456b65bb6c3d3fa90b2
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46519d402d501d5406c9a170a7b02d24054cd4a0
Assignee | ||
Comment 19•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=614eca6fb39cba7d2013de9565ab7b05dafa4fd8
Assignee | ||
Comment 20•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5952d0d29d1acbc61d22bcc3dcee28bea23d9b35
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•7 years ago
|
||
mozreview-review |
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
Attachment #8868921 -
Flags: review?(bugs) → review+
Comment 28•7 years ago
|
||
ok, this all is complicated. Takes some time to figure out what is changed and why... but review is coming.
Comment 29•7 years ago
|
||
mozreview-review |
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
Attachment #8868919 -
Flags: review?(bugs) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8868923 [details] Bug 1339543 part 5 Remove unnecessary stuff from PuppetWidget https://reviewboard.mozilla.org/r/140562/#review144148
Attachment #8868923 -
Flags: review?(bugs) → review+
Comment 31•7 years ago
|
||
mozreview-review |
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.
Attachment #8868920 -
Flags: review?(bugs) → review+
Comment 32•7 years ago
|
||
mozreview-review |
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.
Attachment #8868922 -
Flags: review?(bugs) → review+
Comment 33•7 years ago
|
||
mozreview-review |
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
Attachment #8868924 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 34•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review-reply |
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().
Assignee | ||
Comment 36•7 years ago
|
||
mozreview-review-reply |
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().
Assignee | ||
Comment 37•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 38•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 39•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 40•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 47•7 years ago
|
||
Pushed by masayuki@d-toybox.com: 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•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•