Closed
Bug 977959
Opened 10 years ago
Closed 10 years ago
Redesign native key bindings handling
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(6 files, 1 obsolete file)
9.36 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
42.20 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
33.49 KB,
patch
|
roc
:
review+
karlt
:
review+
|
Details | Diff | Splinter Review |
32.44 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
42.20 KB,
patch
|
karlt
:
review+
smichaud
:
review+
|
Details | Diff | Splinter Review |
8.34 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
Currently, native key bindings is solved by services of nsINativeKeyBindings. However, its KeyDown() and KeyUp() are not implemented. So, we can just get rid of them. Then, there will be only KeyPress(). I don't think that it's good design. The callers can retrieve widget easily. Therefore, I'd like to suggest that we should remove nsINativeKeyBindings and add a new method to nsIWidget like HandleWithNativeKeyBindings(const WidgetKeyboardEvent& aEvent, uint8_t aEditorType, DoCommandCallback aCallback, void* aClosure). aEditorType can be removed if widget instance must refer its input context.
Comment 1•10 years ago
|
||
Why were KeyUp and KeyDown added in the first place?
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #1) > Why were KeyUp and KeyDown added in the first place? I don't know since it has been implemented as so in initial implementation. https://bugzilla.mozilla.org/attachment.cgi?id=157901&action=diff
Assignee | ||
Comment 3•10 years ago
|
||
This is not necessary for this bug directly, though. However, this is much helpful for bug 977904 because this reduces heap allocation.
Assignee | ||
Comment 4•10 years ago
|
||
This patch kills nsINativeKeyBindings::KeyDown() and KeyUp() which don't have actual implementers.
Attachment #8387587 -
Flags: review?(roc)
Assignee | ||
Comment 5•10 years ago
|
||
This hides nsINativeKeyBindings with nsIWidget. This is a big step to remove nsINativeKeyBindings completely. FYI: This patch cannot be built on Linux due to KeyPress macro hell of X's header. But this issue will be gone by next patch, please ignore this issue. I'll request reviews to karlt and smichaud after roc's review.
Attachment #8387589 -
Flags: review?(roc)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8387589 [details] [diff] [review] part.3 Hide nsINativeKeyBindings with nsIWidget::ExecuteNativeKeyBinding() Oops, this needs sr too.
Attachment #8387589 -
Flags: superreview?(roc)
Assignee | ||
Comment 7•10 years ago
|
||
Comment on attachment 8387587 [details] [diff] [review] part.2 Remove nsINativeKeyBindings::KeyDown() and nsINativeKeyBindings::KeyUp() This needs sr too. Although, this interface will be gone by the last patch.
Attachment #8387587 -
Flags: superreview?(roc)
Assignee | ||
Comment 8•10 years ago
|
||
This removes nsINativeKeyBindings completely. After roc's review, I'll request review to widget people.
Attachment #8387590 -
Flags: review?(roc)
Comment on attachment 8387586 [details] [diff] [review] part.1 Define constants for each command which may be caused by native key bindings Review of attachment 8387586 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/shared/WidgetEventImpl.cpp @@ +393,5 @@ > + return "cmd_selectWordPrevious"; > + case CommandWordNext: > + return "cmd_wordNext"; > + case CommandWordPrevious: > + return "cmd_wordPrevious"; How about using a similar pattern to nsGkAtomList, where we have a CommandList.h file containing a list of DEFINE_COMMAND(CommandWordNext, "cmd_wordNext") etc, which we #include here and in EventForwards.h?
Attachment #8387586 -
Flags: review?(roc) → review-
Attachment #8387587 -
Flags: superreview?(roc)
Attachment #8387587 -
Flags: superreview+
Attachment #8387587 -
Flags: review?(roc)
Attachment #8387587 -
Flags: review+
Attachment #8387589 -
Flags: superreview?(roc)
Attachment #8387589 -
Flags: superreview+
Attachment #8387589 -
Flags: review?(roc)
Attachment #8387589 -
Flags: review+
Attachment #8387590 -
Flags: review?(roc) → review+
Comment 10•10 years ago
|
||
Comment on attachment 8387586 [details] [diff] [review] part.1 Define constants for each command which may be caused by native key bindings >+typedef int8_t CommandInt; >+enum Command MOZ_ENUM_TYPE(CommandInt) >+{ >+ CommandDoNothing, >+ >+ CommandBeginLine, IMHO this looks a bit awkward. Would it be better to write this as an enum class, so that the values are namespaced? Something like this: #include "mozilla/TypedEnum.h" MOZ_BEGIN_ENUM_CLASS(Command, uint8_t) #define COMMAND(cmd) Command ## cmd #include "CommandList.h" #undef COMMAND doNothing MOZ_END_ENUM_CLASS(Command) >+/* static */ const char* >+WidgetKeyboardEvent::GetCommandStr(Command aCommand) Can we not use an array here? static const char* commands[] = { #define COMMAND(cmd) "cmd_" #cmd #include "CommandList.h" #undef COMMAND nullptr }; const char* WidgetKeyboardEvent::GetCommandStr(Command aCommand) { MOZ_ASSERT(aCommand <= Command::doNothing, "Illegal command enumeration value"); return commands[aCommand]; }
Comment 11•10 years ago
|
||
Comment on attachment 8387590 [details] [diff] [review] part.4 Remove nsINativeKeyBindings r= > // static > void >+NativeKeyBindings::Shutdown() > { > NS_IF_RELEASE(sInstanceForSingleLineEditor); > NS_IF_RELEASE(sInstanceForMultiLineEditor); > } ... >+ NS_INLINE_DECL_REFCOUNTING(NativeKeyBindings) ... >+ static NativeKeyBindings* sInstanceForSingleLineEditor; >+ static NativeKeyBindings* sInstanceForMultiLineEditor; These are your only references to NativeKeyBindings objects, so you don't actually need to count the number of references, just delete the object in the shutdown method.
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #10) > Comment on attachment 8387586 [details] [diff] [review] > part.1 Define constants for each command which may be caused by native key > bindings > > >+typedef int8_t CommandInt; > >+enum Command MOZ_ENUM_TYPE(CommandInt) > >+{ > >+ CommandDoNothing, > >+ > >+ CommandBeginLine, > > IMHO this looks a bit awkward. Would it be better to write this as an enum > class, so that the values are namespaced? Something like this: > > #include "mozilla/TypedEnum.h" > MOZ_BEGIN_ENUM_CLASS(Command, uint8_t) > #define COMMAND(cmd) Command ## cmd > #include "CommandList.h" > #undef COMMAND > doNothing > MOZ_END_ENUM_CLASS(Command) I don't like to use enum class now for this since it'll be arrayed members of WidgetKeyboardEvent in bug 977904. So, I think that until we drop the support of compilers which don't support enum class, we shouldn't use it. (Although, I'd like to use enum class, actually.)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8387586 -
Attachment is obsolete: true
Attachment #8388370 -
Flags: review?(roc)
Assignee | ||
Comment 14•10 years ago
|
||
For making NativeKeyBindings a non-service, i.e., just a singleton class, this patch makes each widget (Linux and Mac) directly retrieves the singleton instance of mozilla::widget::NativeKeyBindings. karlt: On Linux, this patch renames nsNativeKeyBindings to mozilla::widget::NativeKeyBindings (same as cocoa widget) and renames their file names are NativeKeyBindings.*. Note that please ignore the changes in each widget's factory since it will be completely removed by part.4.
Attachment #8388371 -
Flags: review?(smichaud)
Attachment #8388371 -
Flags: review?(karlt)
Assignee | ||
Comment 15•10 years ago
|
||
Indeed, this can be just a non-reftountable class.
Attachment #8388373 -
Flags: review?(neil)
Updated•10 years ago
|
Attachment #8388373 -
Flags: review?(neil) → review+
Comment on attachment 8388370 [details] [diff] [review] part.1 Define constants for each command which may be caused by native key bindings Review of attachment 8388370 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/shared/WidgetEventImpl.cpp @@ +311,5 @@ > +WidgetKeyboardEvent::GetCommandStr(Command aCommand) > +{ > +#define NS_DEFINE_COMMAND(aName, aCommandStr) , #aCommandStr > + static const char* kCommands[] = { > + "" // CommandDoNothing This array adds quite a few relocations, which slow startup time, which is why I didn't suggest it. I think you should carry on using the switch statement you had before, but of course you can generate it by #including CommandList.h.
Attachment #8388370 -
Flags: review?(roc) → review-
Comment 17•10 years ago
|
||
(In reply to Robert O'Callahan from comment #16) > This array adds quite a few relocations, which slow startup time So this would apply to any static structure that contains a pointer to a string, such as those in nsGlobalWindowCommands.cpp ? Would specifying a character array help? (I can't see how a switch would help in nsGlobalWindowCommands.cpp .)
(In reply to neil@parkwaycc.co.uk from comment #17) > (In reply to Robert O'Callahan from comment #16) > > This array adds quite a few relocations, which slow startup time > > So this would apply to any static structure that contains a pointer to a > string, such as those in nsGlobalWindowCommands.cpp ? Would specifying a > character array help? (I can't see how a switch would help in > nsGlobalWindowCommands.cpp .) I guess you're right. Don't worry about that comment.
Attachment #8388370 -
Flags: review- → review+
Updated•10 years ago
|
Attachment #8388371 -
Flags: review?(smichaud) → review+
Comment 19•10 years ago
|
||
I'm in meetings all this week, sorry, so won't be able to review this week.
Comment 20•10 years ago
|
||
Comment on attachment 8388371 [details] [diff] [review] part.3 Hide nsINativeKeyBindings with nsIWidget::ExecuteNativeKeyBinding() r=karlt on the GTK bits >+ case nsIWidget::NativeKeyBindingsForSingleLineEditor: >+ if (!sInstanceForSingleLineEditor) { >+ NS_ADDREF(sInstanceForSingleLineEditor = new NativeKeyBindings()); >+ sInstanceForSingleLineEditor->Init(aType); >+ } >+ NS_ADDREF(sInstanceForSingleLineEditor); >+ return sInstanceForSingleLineEditor; >+ case nsIWidget::NativeKeyBindingsForMultiLineEditor: >+ case nsIWidget::NativeKeyBindingsForRichTextEditor: >+ if (!sInstanceForMultiLineEditor) { >+ NS_ADDREF(sInstanceForMultiLineEditor = new NativeKeyBindings()); >+ sInstanceForMultiLineEditor->Init(aType); >+ } >+ NS_ADDREF(sInstanceForMultiLineEditor); >+ return sInstanceForMultiLineEditor; >+ default: >+ MOZ_CRASH("Not implemented"); >+ return nullptr; There's no advantage in having dead code to crash in release builds here. I would have a default case to return sInstanceForMultiLineEditor and assert there that aType is NativeKeyBindingsForMultiLineEditor or NativeKeyBindingsForRichTextEditor, but I've seen others prefer otherwise, so whatever you choose.
Attachment #8388371 -
Flags: review?(karlt) → review+
Comment 21•10 years ago
|
||
Comment on attachment 8387590 [details] [diff] [review] part.4 Remove nsINativeKeyBindings r= r=karlt on the widget/gtk parts
Attachment #8387590 -
Flags: review+
Assignee | ||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/74ba454a1821 https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcd85163a89 https://hg.mozilla.org/integration/mozilla-inbound/rev/704b766676be https://hg.mozilla.org/integration/mozilla-inbound/rev/b9daeea8712a https://hg.mozilla.org/integration/mozilla-inbound/rev/8e7dd50fde87
Comment 23•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/74ba454a1821 https://hg.mozilla.org/mozilla-central/rev/ffcd85163a89 https://hg.mozilla.org/mozilla-central/rev/704b766676be https://hg.mozilla.org/mozilla-central/rev/b9daeea8712a https://hg.mozilla.org/mozilla-central/rev/8e7dd50fde87
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•