Closed Bug 977959 Opened 10 years ago Closed 10 years ago

Redesign native key bindings handling

Categories

(Core :: Widget, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(6 files, 1 obsolete file)

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.
Why were KeyUp and KeyDown added in the first place?
(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
This is not necessary for this bug directly, though.

However, this is much helpful for bug 977904 because this reduces heap allocation.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #8387586 - Flags: review?(roc)
This patch kills nsINativeKeyBindings::KeyDown() and KeyUp() which don't have actual implementers.
Attachment #8387587 - Flags: review?(roc)
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)
Comment on attachment 8387589 [details] [diff] [review]
part.3 Hide nsINativeKeyBindings with nsIWidget::ExecuteNativeKeyBinding()

Oops, this needs sr too.
Attachment #8387589 - Flags: superreview?(roc)
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)
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+
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 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.
(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.)
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)
Indeed, this can be just a non-reftountable class.
Attachment #8388373 - Flags: review?(neil)
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-
(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 #8388371 - Flags: review?(smichaud) → review+
I'm in meetings all this week, sorry, so won't be able to review this week.
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 on attachment 8387590 [details] [diff] [review]
part.4 Remove nsINativeKeyBindings r=

r=karlt on the widget/gtk parts
Attachment #8387590 - Flags: review+
You need to log in before you can comment on or make changes to this bug.