Use Carbon Events for IME instead of Apple Events

RESOLVED INVALID

Status

()

RESOLVED INVALID
17 years ago
10 years ago

People

(Reporter: ftang, Assigned: katsuhiromihara)

Tracking

({intl})

Trunk
PowerPC
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [eta 9/3])

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

17 years ago
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

Updated

17 years ago
Keywords: intl
(Reporter)

Updated

17 years ago
Blocks: 157673
Status: NEW → ASSIGNED
Keywords: nsbeta1

Updated

17 years ago
Hardware: PC → Macintosh
(Reporter)

Comment 1

17 years ago
*** Bug 107473 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

17 years ago
Whiteboard: [eta 9/3]
Target Milestone: --- → mozilla1.2alpha

Comment 2

16 years ago
i18n triage team: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
(Reporter)

Comment 3

14 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → WONTFIX

Comment 4

14 years ago
Mass Reassign Please excuse the spam
Assignee: ftang → nobody

Comment 5

14 years ago
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
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Comment 6

14 years ago
Reassigning Franks old bugs to Jungshik Shin for triage - Sorry for spam
Assignee: nobody → jshin1987
Status: REOPENED → NEW
(Assignee)

Comment 7

14 years ago
Created attachment 177361 [details] [diff] [review]
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.
(Assignee)

Comment 8

14 years ago
Created attachment 177366 [details] [diff] [review]
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.
Attachment #177361 - Attachment is obsolete: true

Updated

14 years ago
Summary: use carbone IME event instead of apple event for MacOS X in mozilla → Use Carbon Events for IME instead of Apple Events

Comment 9

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

Updated

14 years ago
Blocks: 106689
Target Milestone: mozilla1.2alpha → ---

Comment 10

14 years ago
Patch looks OK, but please remove all tabs. Who should review this?
(Assignee)

Comment 11

14 years ago
Created attachment 177483 [details] [diff] [review]
Carbon Event Handler for TSM (remove tabs)

remove tabs and debug printf()s.
(Assignee)

Comment 12

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

Comment 13

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

Unless you ask for review expliclty, nothing will happen
Attachment #177483 - Flags: review?(pinkerton)
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.
Attachment #177483 - Flags: review?(pinkerton) → review+
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...
(Assignee)

Comment 16

14 years ago
Created attachment 182842 [details] [diff] [review]
TSM patch (updated assertions)

updated assertions.
Attachment #177366 - Attachment is obsolete: true
Attachment #177483 - Attachment is obsolete: true
Attachment #182842 - Flags: review?(pinkerton)
(Assignee)

Updated

14 years ago
Attachment #182842 - Flags: review?(pinkerton) → review?(bugs.mano)
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.
Assignee: jshin1987 → katsuhiromihara
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
Attachment #182842 - Flags: superreview?(sfraser_bugs)
Attachment #182842 - Flags: review?(bugs.mano)
Attachment #182842 - Flags: review+

Comment 20

14 years ago
Any chance of a diff -w here, so that I don't see all the detabbing changes?
(Assignee)

Comment 21

14 years ago
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.
(Assignee)

Comment 22

14 years ago
Created attachment 185572 [details] [diff] [review]
TSM patch (cvs diff -uw)

Sorry. I re-attach new patch.

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

Updated

14 years ago
Attachment #182842 - Attachment is obsolete: true
Attachment #185572 - Flags: review?(bugs.mano)
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.
Attachment #185572 - Attachment is obsolete: true
Attachment #185572 - Flags: review?(bugs.mano)
(Assignee)

Comment 24

14 years ago
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.
Attachment #182842 - Attachment is obsolete: true
(Assignee)

Comment 25

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

This patch has the same bug.
Attachment #185572 - Attachment is obsolete: true
(Assignee)

Comment 26

14 years ago
Created attachment 186005 [details] [diff] [review]
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.
Attachment #186005 - Flags: review?(bugs.mano)
and please also attach a patch diffed w/o -w...
(Assignee)

Comment 28

14 years ago
Created attachment 186015 [details] [diff] [review]
TSM patch (update nsMacTSMMessagePump::UnicodeUpdateHandler(), w/o -w)
Attachment #186015 - Flags: review?(bugs.mano)
Comment on attachment 186005 [details] [diff] [review]
TSM patch (update nsMacTSMMessagePump::UnicodeUpdateHandler())

assuming that was the only change you did, r=me.
Attachment #186005 - Flags: review?(bugs.mano) → review+

Comment 30

14 years ago
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.
Attachment #186005 - Flags: superreview?(sfraser_bugs) → superreview-
(Assignee)

Comment 31

14 years ago
Created attachment 187999 [details] [diff] [review]
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.
Attachment #186005 - Attachment is obsolete: true
Attachment #187999 - Flags: review?(bugs.mano)
(Assignee)

Comment 32

14 years ago
Created attachment 188000 [details] [diff] [review]
Fix stack allocation (-uw)
Attachment #186015 - Attachment is obsolete: true
Attachment #188000 - Flags: review?(bugs.mano)
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).
Attachment #187999 - Flags: review?(bugs.mano)

Comment 34

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

Comment 35

13 years ago
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

12 years ago
In trunk, it moved Cocoa.
Is this bug still effective?
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.