bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

XIM performance improvement for over-the-spot mode

VERIFIED FIXED

Status

()

Core
Internationalization
VERIFIED FIXED
17 years ago
8 years ago

People

(Reporter: Masaki Katakai, Assigned: Masaki Katakai)

Tracking

({inputmethod})

Trunk
x86
Linux
inputmethod
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

17 years ago
This bug came from bug 109723. It seems that on over-the-spot mode, some
unnecessary traffic happens between Mozilla and XIM server.

- nsIMEGtkIC::SetPreeditArea() is called even if the area is not changed.
- nsIMEGtkIC::GetPreeditFont() is called on every spot location is called.
- if (height == mXICFontSize) return; does not work at all in
  nsWindow::SetXICBaseFontSize() because we change height before storing
  mXICFontSize;

We should remove those calls to reduce the traffic for performance.
(Assignee)

Comment 1

17 years ago
- In nsIMEGtkIC::SetFocusWindow(), we can keep the previous width and height and
  avoid the dup calling.

  We can keep the previous values as static in SetPreeditArea() as Tung-Han Hsieh
  suggested in bug 109723, but we already keep such values in nsWindow and compare
  in nsWindow::UpdateICSpot() so I made changes with the same manner.

- GetPreeditFont() is not needed in nsWindow::GetXYFromPosition() because we keep
  gPreeditFontset as global and we can use gPreeditFontset->descent

- In nsWindow::SetXICBaseFontSize(), we should compare height with mXICFontSize
  *after* height-=1 and remember the value as mXICFontSize

I'll attach the patch.

Tung-Han Hsieh, can you try on your environment?
Status: NEW → ASSIGNED
(Assignee)

Comment 2

17 years ago
Created attachment 58070 [details] [diff] [review]
proposed patch

Comment 3

17 years ago
I have tested your patch. It has several typos:

--- nsGtkIMEHelper.cpp	2001/11/14 12:35:45	1.35
+++ nsGtkIMEHelper.cpp	2001/11/16 05:01:36
@@ -1021,9 +1021,15 @@
   gdk_im_begin((GdkIC *) mIC, gdkWindow);
 
   if (mInputStyle & GDK_IM_PREEDIT_POSITION) {
-    SetPreeditArea(0, 0,
-
	   (int)((GdkWindowPrivate*)gdkWindow)->width,
-
	   (int)((GdkWindowPrivate*)gdkWindow)->height);
+    static int oldw=0;
+    static int oldh=0;
+    int neww=(int)((GdkWindowPrivate*)gdkWindow)->width;
+    int newh=(int)((GdkWindowPrivate*)gdkWindow)->width;

Note here, I think "newh" should be
     (int)((GdkWindowPrivate*)gdkWindow)->height;

Another one:

 nsWindow::SetXICSpotLocation(nsIMEGtkIC* aXIC, nsPoint aPoint)
 {
-  unsigned long x, y;
-  x = aPoint.x, y = aPoint.y;
-  GetXYFromPosition(aXIC, &x, &y);
-  aXIC->SetPreeditSpotLocation(x, y);
+  if (gPreeditFontset) {
+    unsigned long x, y;
+    x = aPoint.x, y = aPoint.y;
+//    GetXYFromPosition(aXIC, &x, &y);
+    aXIC->SetPreeditSpotLocation(x, y);
+  }
 }
Note: Since you have removed the function GetXYFromPosition(), so I think
you should not call it there.

One question: The correct fix is to add both of your patch here, and my
patch proposed in bug 109723, right? I have merged both of the patches,
and it seems that mozilla works fine in my environment. If only add your
patch, mozilla will crash when I terminate my XIM server --- xcin (which
is operating in the OverTheSpot mode), because the crash point is in
 nsIMEGtkIC::SetPreeditArea(), where my patch fixed it.
(Assignee)

Comment 4

17 years ago
Let's think about only performance issue here. Don't include crash problem
(bug 109723).

> Note here, I think "newh" should be
>     (int)((GdkWindowPrivate*)gdkWindow)->height;

Thank you for correction.

> Note: Since you have removed the function GetXYFromPosition(), so I think
> you should not call it there.

I'm sorry I don't understand. GetXYFromPosition() is removed by the patch.

Comment 5

17 years ago
Oh, Very sorry, I may looked into a wrong place. So there is no problem
for GetXYFromPosition(). Very sorry for boring .... :-)
(Assignee)

Comment 6

17 years ago
Created attachment 63774 [details] [diff] [review]
proposed patch for current tree

Comment 7

17 years ago
Comment on attachment 63774 [details] [diff] [review]
proposed patch for current tree

I'm probably not the best person to review this (Since I don't really know
XIM), but the code looks fine to me. r=pavlov
Attachment #63774 - Flags: review+
Comment on attachment 63774 [details] [diff] [review]
proposed patch for current tree

sr=blizzard
Attachment #63774 - Flags: superreview+
(Assignee)

Comment 9

17 years ago
Thank you very much for review.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

17 years ago
QA Contact: teruko → ylong

Comment 10

17 years ago
Brian, Frank, Katakai-san:
Can either of you help to verify this bug?  cause I have no way to verify this
code change, and I don't know where can I get a XIM server at least to verify
the crash on bug 109723.   Thanks!
   
(Assignee)

Comment 11

17 years ago
Yuying, thanks for testing.

Yes, it's hard to verify this so at least we
need to verify over-the-spot is working fine.

1. exit Mozilla
2. user_pref("xim.input_style", "over-the-spot");
    in your prefs
3. start Mozilla
4. turn conversion mode on
5. type something

The locale of composed text should be correct.

Comment 12

17 years ago
Thank you very much! after I added: user_pref("xim.input_style", "over-the-spot");
in the prefs.js file of my profile, and I didn't have problem with typing and
display the text.

I'll mark it as verified for now, please re-open if there is still something wrong.
Status: RESOLVED → VERIFIED
Keywords: inputmethod
You need to log in before you can comment on or make changes to this bug.