Closed Bug 1180564 Opened 4 years ago Closed 4 years ago

don't implement NSTextInput any more, just NSTextInputClient

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
firefox45 --- fixed

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(3 files, 5 obsolete files)

Attached patch rm-NSTextInput-1.patch (obsolete) — Splinter Review
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.
Assignee: nobody → jaas
Attachment #8629768 - Flags: review?(smichaud)
4 files changed, 35 insertions(+), 139 deletions(-)
Attachment #8629768 - Flags: review?(masayuki)
Attached patch fix v1.1 (obsolete) — Splinter Review
Updated patch to current trunk.
Attachment #8629768 - Attachment is obsolete: true
Attachment #8629768 - Flags: review?(smichaud)
Attachment #8629768 - Flags: review?(masayuki)
Attachment #8629794 - Flags: review?(smichaud)
Attachment #8629794 - Flags: review?(masayuki)
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.
Flags: needinfo?(jaas)
I strongly agree that we need to be able to turn off this patch's changes with a pref.
Attached patch pref off methods v1.0 (obsolete) — Splinter Review
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.
Attachment #8629794 - Attachment is obsolete: true
Attachment #8629794 - Flags: review?(smichaud)
Attachment #8629794 - Flags: review?(masayuki)
Flags: needinfo?(jaas)
Attachment #8630477 - Flags: review?(masayuki)
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.
Attachment #8630477 - Flags: review?(masayuki) → review+
Updated per your comment, want to run this by you again.
Attachment #8630477 - Attachment is obsolete: true
Attachment #8631392 - Flags: review?(masayuki)
https://hg.mozilla.org/mozilla-central/rev/b0ae85311a6d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Let's keep this open until we actually remove the interface.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Remove code v1.0 (obsolete) — Splinter Review
Attachment #8682952 - Flags: review?(masayuki)
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.
Attachment #8682952 - Flags: review?(masayuki)
Attachment #8682952 - Attachment is obsolete: true
Attached patch Remove code v1.1 (obsolete) — Splinter Review
Attachment #8682954 - Flags: review?(masayuki)
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 :-(
Attachment #8682954 - Flags: review?(masayuki) → review-
Attached patch Remove code v1.2Splinter Review
Nice catch!
Attachment #8682954 - Attachment is obsolete: true
Attachment #8682978 - Flags: review?(masayuki)
(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).
(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?
Flags: needinfo?(suishouen)
(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.
Flags: needinfo?(suishouen)
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!
Flags: needinfo?(jaas)
Flags: needinfo?(jaas)
Attachment #8684812 - Flags: review?(masayuki)
Comment on attachment 8684812 [details] [diff] [review]
insertNewline: Fix v1.0

Thanks!!
Attachment #8684812 - Flags: review?(masayuki) → review+
https://hg.mozilla.org/mozilla-central/rev/8707d23c0be0
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
(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!
(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?
Flags: needinfo?(suishouen)
Oops, please ignore the previous comment. The additional patch hasn't been landed yet.
Flags: needinfo?(suishouen)
(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.
I think the additinal patch needs to land on Inbound and on Central.
But why is this bug marked fixed?
Because there was no "leave-open" in the whiteboard.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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/.
https://hg.mozilla.org/mozilla-central/rev/a9da8af10871
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Thanks for landing that Masayuki.
Depends on: 1223966
Duplicate of this bug: 1191283
You need to log in before you can comment on or make changes to this bug.