1180564 - don't implement NSTextInput any more, just NSTextInputClient

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

Attachment: rm-NSTextInput-1.patch

The NSTextInput protocol was deprecated in favor of NSTextInputClient in OS X 10.6. We use NSTextInputClient now (bug 875674), but we still implement NSTextInput as well. This isn't necessary and we can remove some old code.

There is quite a bit of overlap in their methods. This patch trims what we're not using from NSTextInput and the associated TextInputHandler code, and re-organizes what's left that overlaps with NSTextInputClient so that it lives with the associated protocol methods.

Comment 1

[Josh Aas]

4 files changed, 35 insertions(+), 139 deletions(-)

Comment 2

[Josh Aas]

Attachment: fix v1.1

Updated patch to current trunk.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

If old IME only supports NSTextInput protocol, this patch will break the compatibility with them. Although, I'm not sure if IMEs which only support NSTextInput protocol are available on OS X 10.6 (or later). So, when this causes the compatibility regression, we need to back this out. But if it will be reported too late, we may not be able to back this out from Beta or 42.0.x. In such case we'll lose some users.

So, I'd like to suggest that, in this bug, just we kill the methods with prefs (make the methods does nothing in default settings). If it doesn't cause any problem even after released, we will drop the code completely. How about this approach?

# Of course, if there is some evidence such IME doesn't work on 10.6, we should take this patch.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

I strongly agree that we need to be able to turn off this patch's changes with a pref.

Comment 5

[Josh Aas]

Attachment: pref off methods v1.0

Sounds good. This patch organizes the NSTextInput and NSTextInputClient methods so that they are all in their own sections and then disables all of the NSTextInput methods via a pref.

If this doesn't cause any problems then we can just remove all of the methods in the NSTextInput section.

There is also an unrelated compiler warning fix in here.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8630477 [details] [diff] [review]
pref off methods v1.0

>@@ -1656,17 +1656,17 @@ nsChildView::StartPluginIME(const mozill
>   // obtain the NSEvent* we pass to InterpretKeyEvent().  This works fine in
>   // non-e10s mode.  But in e10s mode TextInputHandler::HandleKeyDownEvent()
>   // has already returned, so the relevant KeyEventState* (and its NSEvent*)
>   // is already out of scope.  Furthermore we don't *need* to use it.
>   // StartPluginIME() is only ever called to start a new IME session when none
>   // currently exists.  So nested IME should never reach here, and so it should
>   // be fine to use the last key-down event received by -[ChildView keyDown:]
>   // (as we currently do).
>-  ctiPanel->InterpretKeyEvent([mView lastKeyDownEvent], aCommitted);
>+  ctiPanel->InterpretKeyEvent([(ChildView*)mView lastKeyDownEvent], aCommitted);

Thank you for the fix of warning!

>+- (void)unmarkText
>+{
>+  NS_ENSURE_TRUE(mTextInputHandler, );

Could you use NS_ENSURE_TRUE_VOID or use NS_WARN_IF with |if|?


And please add the hidden pref to modules/libpref/init/all.js? And according to our current pref names, |intl.ime.nstextinput.enable| is better.

With those fixes, r=me.

Comment 7

[Josh Aas]

Attachment: pref off methods v1.1

Updated per your comment, want to run this by you again.

Comment 8

[Pulsebot]

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

Comment 9

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

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

Comment 10

[Josh Aas]

Let's keep this open until we actually remove the interface.

Comment 11

[Josh Aas]

Attachment: Remove code v1.0

Comment 12

[Josh Aas]

Comment on attachment 8682952 [details] [diff] [review]
Remove code v1.0

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

Ugh, I forgot to fully remove the pref.

Comment 13

[Josh Aas]

Attachment: Remove code v1.1

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8682954 [details] [diff] [review]
Remove code v1.1

>-- (void)insertNewline:(id)sender
>-{
>-  // We're considering not implementing NSTextInput. Start by just
>-  // preffing its methods off.
>-  if (!Preferences::GetBool("intl.ime.nstextinput.enable", false)) {
>-    NSLog(@"Set intl.ime.nstextinput.enable to true in about:config to fix input.");
>-    return;
>-  }
>-
>-  [self insertText:@"\n"];
>-}

Don't remove this, this is not a part of NSTextInput protocol. Although, this is a mistake of previous landing. I hope that this won't cause any regression with Korean IME... (bug 376093)

# So, checking the pref is wrong :-(

Comment 15

[Josh Aas]

Attachment: Remove code v1.2

Nice catch!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8707d23c0be0

Comment 17

[Eiichi]

(In reply to Pulsebot from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8707d23c0be0

I'm using OS X El Capitan.
Every time writing some words in Search bar or in Url bar and hitting return key, beeps regardless of IME (on or off).

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Eiichi from comment #17)
> (In reply to Pulsebot from comment #16)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/8707d23c0be0
> 
> I'm using OS X El Capitan.
> Every time writing some words in Search bar or in Url bar and hitting return
> key, beeps regardless of IME (on or off).

When you make "intl.ime.nstextinput.enable" true in Firefox 42, does it beep?

Comment 19

[Eiichi]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #18)
> When you make "intl.ime.nstextinput.enable" true in Firefox 42, does it beep?

No, it doesn't beep.
It beeps only after https://hg.mozilla.org/integration/mozilla-inbound/rev/8707d23c0be0.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8682978 [details] [diff] [review]
Remove code v1.2

> +- (void)insertNewline:(id)sender
> +{
> +  [self insertText:@"\n"];
> +}

Ah, I see. insertText: is now removed by this patch itself!

We should directly call mTextInputHandler->InsertText(attrStr); instead!

Comment 21

[Josh Aas]

Attachment: insertNewline: Fix v1.0

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Comment on attachment 8684812 [details] [diff] [review]
insertNewline: Fix v1.0

Thanks!!

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8707d23c0be0

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8707d23c0be0

Comment 25

[Eiichi]

(In reply to Carsten Book [:Tomcat] from comment #24)
> https://hg.mozilla.org/mozilla-central/rev/8707d23c0be0

After landing https://hg.mozilla.org/mozilla-central/rev/8707d23c0be0, I tested mozilla-inbound-macosx64/1447093292/.
But it still beeps!

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

(In reply to Eiichi from comment #25)
> (In reply to Carsten Book [:Tomcat] from comment #24)
> > https://hg.mozilla.org/mozilla-central/rev/8707d23c0be0
> 
> After landing https://hg.mozilla.org/mozilla-central/rev/8707d23c0be0, I
> tested mozilla-inbound-macosx64/1447093292/.
> But it still beeps!

Then, probably it's not caused by simple mistake. Could you file a new bug which blocks this bug?

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Oops, please ignore the previous comment. The additional patch hasn't been landed yet.

Comment 28

[Eiichi]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #27)
> Oops, please ignore the previous comment. The additional patch hasn't been
> landed yet.

Ah, I see.
The patch landed only on Central, and not landed on Inbound.
Sorry my mistake.

Comment 29

[Eiichi]

I think the additinal patch needs to land on Inbound and on Central.
But why is this bug marked fixed?

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Because there was no "leave-open" in the whiteboard.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9da8af1087161565a4812cf770d1a3cb00e6855
Bug 1180564: insertNewline should use TextInputHandler::InsertText() instead of using insertText of NSTextInput protocol r=masayuki

Comment 32

[Eiichi]

Thanks a lot for your quick response.
I confirmed that the beeping issue has been fixed on /pub/firefox/tinderbox-builds/mozilla-inbound-macosx64/1447125090/.

Comment 33

[Carsten Book [:Tomcat]]

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

Comment 34

[Josh Aas]

Thanks for landing that Masayuki.