158859 - Use Carbon Events for IME instead of Apple Events

Status: RESOLVED INVALID

(Core :: Internationalization, defect)

Description

[Frank Tang]

we currently still use Apple Event model for ime instead of Carbon event.
the embedding application use the Carbon event model.
We should make the MacOS X inside mozilla use the Carbon event model

Comment 1

[Frank Tang]

*** Bug 107473 has been marked as a duplicate of this bug. ***

Comment 2

[Samir Gehani]

i18n triage team: nsbeta1-

Comment 3

[Frank Tang]

what a hack. I have not touch mozilla code for 2 years. I didn't read these bugs
for 2 years. And they are still there. Just close them as won't fix to clean up.

Comment 4

[Travis Chase]

Mass Reassign Please excuse the spam

Comment 5

[Travis Chase]

Mass Re-opening Bugs Frank Tang Closed on Wensday March 02 for no reason, all
the spam is his fault feel free to tar and feather him

Comment 6

[Travis Chase]

Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam

Comment 7

[Katsuhiro MIHARA]

Attachment: Carbon Event Handlers for TSM

This patch is only porting AppleEvent Handlers for Carbon Event.
I didn't change Low-level event handling, and didn't port
a handler for kUnicodeNotFromInputMethod .

This patch's benefit is avoiding allocating Heap memory.
Carbon Event use no AEDesc. And, in daily use, text from TSM
and hilite range is small than 64 bytes.
So, this patch doesn't allocate Heap memory.

I don't have accurate answer about exec time performance.

Comment 8

[Katsuhiro MIHARA]

Attachment: Carbon Event Handler for TSM (diff -u)

Sorry.
I didn't care about format
and include another patch for Bug 283827 .

This is a same patch that format is corrected
and exclude another patch.

Comment 9

[Simon Fraser [no longer active]]

*** Bug 106693 has been marked as a duplicate of this bug. ***

Comment 10

[Simon Fraser [no longer active]]

Patch looks OK, but please remove all tabs. Who should review this?

Comment 11

[Katsuhiro MIHARA]

Attachment: Carbon Event Handler for TSM (remove tabs)

remove tabs and debug printf()s.

Comment 12

[Katsuhiro MIHARA]

If I can be too ambitious,
I want Josh Aas or Mike Pinkerton to review this patch.

Comment 13

[Jungshik Shin]

Comment on attachment 177483 [details] [diff] [review]
Carbon Event Handler for TSM (remove tabs)

Unless you ask for review expliclty, nothing will happen

Comment 14

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 177483 [details] [diff] [review]
Carbon Event Handler for TSM (remove tabs)

a cursory glance looks ok to me. I'm no expert in these events or their
parameters, but nothing jumped out at me

r=pink.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 177483 [details] [diff] [review]
Carbon Event Handler for TSM (remove tabs)

please update the assertations to assert on the new calls instead of the old
ones...

Comment 16

[Katsuhiro MIHARA]

Attachment: TSM patch (updated assertions)

updated assertions.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Mihara-san:

You should send mail to Asaf Romano(mozilla.mano@...) directly.
Maybe, he don't look your request.

Asaf:

If you look this comment, please hurry up to review the patch.

Comment 18

[Asaf Romano (gone)]

I will review it later this week.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 182842 [details] [diff] [review]
TSM patch (updated assertions)

There are some style nits here which I will fix on checkin. otherwise, r=mano

Comment 20

[Simon Fraser [no longer active]]

Any chance of a diff -w here, so that I don't see all the detabbing changes?

Comment 21

[Katsuhiro MIHARA]

I didn't know -w option and relied on editor's config and replacement...

Should I attach new patch? I feel that requesting reviewers to review again is a
burden, but this is my fault.

Comment 22

[Katsuhiro MIHARA]

Attachment: TSM patch (cvs diff -uw)

Sorry. I re-attach new patch.

This patch is output of 'cvs diff -uw' at current repository.

Comment 23

[Asaf Romano (gone)]

Comment on attachment 185572 [details] [diff] [review]
TSM patch (cvs diff -uw)

diff -w only means "easier for review" ;) The previous patch is what will be
checked in, so you don't need ask for another review.

Comment 24

[Katsuhiro MIHARA]

Comment on attachment 182842 [details] [diff] [review]
TSM patch (updated assertions)

I found a bug of this patch.

When a user move focus by TAB key, form control commit untranslated characters
until first tranlate finished.

Comment 25

[Katsuhiro MIHARA]

Comment on attachment 185572 [details] [diff] [review]
TSM patch (cvs diff -uw)

This patch has the same bug.

Comment 26

[Katsuhiro MIHARA]

Attachment: TSM patch (update nsMacTSMMessagePump::UnicodeUpdateHandler())

change in nsMacTSMMessagePump::UnicodeUpdateHandler()

-      res = eventHandler->UnicodeHandleUpdateInputArea(unicodeText.get(),
text_size, fixLength / 2, hiliteRangePtr);
+	 res = eventHandler->UnicodeHandleUpdateInputArea(unicodeTextPtr,
text_size, fixLength, hiliteRangePtr);

fixLength is byte offset. We don't need divide by 2 even if text is UTF-16.

Comment 27

[Asaf Romano (gone)]

and please also attach a patch diffed w/o -w...

Comment 28

[Katsuhiro MIHARA]

Attachment: TSM patch (update nsMacTSMMessagePump::UnicodeUpdateHandler(), w/o -w)

Comment 29

[Asaf Romano (gone)]

Comment on attachment 186005 [details] [diff] [review]
TSM patch (update nsMacTSMMessagePump::UnicodeUpdateHandler())

assuming that was the only change you did, r=me.

Comment 30

[Simon Fraser [no longer active]]

Comment on attachment 186005 [details] [diff] [review]
TSM patch (update nsMacTSMMessagePump::UnicodeUpdateHandler())

> 	//
> 	// Extract the nsMacEvenbtHandler for this TSMDocument.  It's stored as the refcon.
> 	//

Typo.

>+    {
>+        UInt32 range_size = 0;
>+        UInt32 text_size = 0;
>+        TextRangeArrayPtr hiliteRangePtr = 0;
>+        char *mbcsTextPtr = 0;
>+        
>+        err = ::GetEventParameter(theEvent, kEventParamTextInputSendHiliteRng,
>+                                    typeTextRangeArray, &returnedType,
>+                                    0, &range_size,
>+                                    NULL);
>+        NS_ASSERTION(err==noErr||err==eventParameterNotFoundErr, "nsMacTSMMessagePump::UpdateHandler: GetEventParameger[hiliteRangeArray(only size)] failed.");
>+        if (err == eventParameterNotFoundErr) {
>+            range_size = 0;
>+        }
>+        err = ::GetEventParameter(theEvent, kEventParamTextInputSendText,
>+                                    unicodeOrNot, &returnedType,
>+                                    0, &text_size,
>+                                    NULL);
>+        NS_ASSERTION(err==noErr, "nsMacTSMMessagePump::UpdateHandler: GetEventParameger[text(only size)] failed.");
>+
>+        PRInt8 dummyHiliteRangePtr[range_size];
>+        PRInt8 dummyMbcsTextPtr[text_size + 1];

Erm, how does this compile? range_size and text_size are variable, so cannot be
determined at compile time.

>+        if (range_size > 0) {
>+            hiliteRangePtr = (TextRangeArrayPtr)(&dummyHiliteRangePtr[0]);
>   	//
>   	// extract the hilite ranges (optional param)
>   	//
>-  	err = AEGetParamDesc(theAppleEvent, keyAEHiliteRange, typeTextRangeArray, &hiliteRangeArray);
>-	NS_ASSERTION(err==noErr||err==errAEDescNotFound, "nsMacTSMMessagePump::UpdateHandler: AEGetParamPtr[fixlen] failed.");
>-  	if (err==errAEDescNotFound) {
>-  		hiliteRangePtr=NULL;
>-  	} else if (err==noErr) { 
>-		Size hiliteRangeSize = ::AEGetDescDataSize(&hiliteRangeArray);
>-		hiliteRangePtr = (TextRangeArray *) NewPtr(hiliteRangeSize);
>-		if (!hiliteRangePtr)
>-			return MemError();
>-		err = AEGetDescData(&hiliteRangeArray, (void *) hiliteRangePtr, hiliteRangeSize);
>-		if (err!=noErr) {
>-			DisposePtr((Ptr) hiliteRangePtr);
>-			return err;
>+            err = ::GetEventParameter(theEvent, kEventParamTextInputSendHiliteRng,
>+                                        typeTextRangeArray, &returnedType,
>+                                        range_size, &range_size,
>+                                        hiliteRangePtr);
>+            NS_ASSERTION(err==noErr||err==eventParameterNotFoundErr, "nsMacTSMMessagePump::UpdateHandler: GetEventParameger[hiliteRangeArray] failed.");
>+            if (err == eventParameterNotFoundErr) {
>+                range_size = 0;
>+            }
>+            if (range_size == 0) {
>+                hiliteRangePtr = 0;
> 		}
>+        }
>+        if (text_size > 0) {
>+            mbcsTextPtr = (char *)dummyMbcsTextPtr;
>+            //
>+            // IME update text
>+            //
>+            err = ::GetEventParameter(theEvent, kEventParamTextInputSendText,
>+                                        unicodeOrNot, &returnedType,
>+                                        text_size, &text_size,
>+                                        mbcsTextPtr);
>+            NS_ASSERTION(err==noErr, "nsMacTSMMessagePump::UpdateHandler: GetEventParameger[text] failed.");
>+            if (text_size == 0) {
>+                mbcsTextPtr = 0;
>   	} else { 
>-  		return err;
>+                mbcsTextPtr[text_size] = '\0';
>   	}

The memory management here scares me a little. Can we make the ownership of
the memory pointed to by mbcsTextPtr more explicit?

>+    {
>+        UInt32 range_size = 0;
>+        UInt32 text_size = 0;
>+        TextRangeArrayPtr hiliteRangePtr = 0;
>+        PRUnichar *unicodeTextPtr = 0;
>+        
>+        err = ::GetEventParameter(theEvent, kEventParamTextInputSendHiliteRng,
>+                                    typeTextRangeArray, &returnedType,
>+                                    0, &range_size,
>+                                    NULL);
>+        NS_ASSERTION(err==noErr||err==eventParameterNotFoundErr, "nsMacTSMMessagePump::UnicodeUpdateHandler: GetEventParameger[hiliteRangeArray(only size)] failed.");
>+        if (err == eventParameterNotFoundErr) {
>+            range_size = 0;
>+        }
>+        err = ::GetEventParameter(theEvent, kEventParamTextInputSendText,
>+                                    typeUnicodeText, &returnedType,
>+                                    0, &text_size,
>+                                    NULL);
>+        NS_ASSERTION(err==noErr, "nsMacTSMMessagePump::UnicodeUpdateHandler: GetEventParameger[text(only size)] failed.");
>+
>+        PRInt8 dummyHiliteRangePtr[range_size];
>+        PRInt8 dummyUnicodeTextPtr[text_size + sizeof(PRUnichar)];

More weirdness, and ownership vagueness.

I'd also like to see the nsMacTSMMessagePump files be entirely detabbed.

Comment 31

[Katsuhiro MIHARA]

Attachment: Fix stack allocation (-u)

+	 PRInt8 stackHiliteRangePtr[MACTSMMESSAGEPUMP_LARGE_STACK_SIZE];
+	 PRInt8 stackUnicodeTextPtr[MACTSMMESSAGEPUMP_LARGE_STACK_SIZE];
+	 PRInt8 *heapHiliteRangePtr = 0;
+	 PRInt8 *heapUnicodeTextPtr = 0;
+	 if (range_size > 0) {
+	     SelectStackOrHeap((PRInt8 **)&hiliteRangePtr, stackHiliteRangePtr,
&heapHiliteRangePtr,
+				 sizeof(stackHiliteRangePtr), range_size);

Code allcates fix length array, and when data is larger than array code
allocate heap memory.

TextPtr and HiliteRangePtr is variable length. But, in many case only contain
one sentence. So I want to avoid allocating heap memory.

Array size is defined at widget/src/mac/nsMacTSMMessagePump.h
+#define MACTSMMESSAGEPUMP_LARGE_STACK_SIZE 128

P.S.
I surprised about transitioning to Cocoa widget will begin very soon. This
patch maybe too late.

Comment 32

[Katsuhiro MIHARA]

Attachment: Fix stack allocation (-uw)

Comment 33

[Asaf Romano (gone)]

Comment on attachment 187999 [details] [diff] [review]
Fix stack allocation (-u)

Canceling review requesrt - This isn't going to be landed for 1.8, and I doubt
we need it for 1.9 (assuming we'll actually switch to cocoa widget).

Comment 34

[Hiro]

It is rewritten from Apple Events with patch of bug337199. 
https://bugzilla.mozilla.org/attachment.cgi?id=221541&action=diff

Comment 35

[Mark Mentovai]

My independent implementation is in attachment 223068 [details] [diff] [review].  Bug 337199 comment 46 describes how it's broken and why I went back to the Apple Event handlers.

Comment 36

[Hiro]

In trunk, it moved Cocoa.
Is this bug still effective?

Comment 37

[Samuel Sidler (old account; do not CC)]

We now use Cocoa for IME. This bug is INVALID in that this is something we'll never do because it was usurped by something better.