Build Identifier: http://hg.mozilla.org/mozilla-central/rev/50b63701fc01 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110620 Firefox/7.0a1 ID:20110620121237 Using Japanese IME, response of input is painfully slow. *This happens Firefox4.0 and later. *This does not happen Firefox3.6.x and Google Chrome 12.0.742.100. Reproducible: Always Steps to Reproduce: 1. Start Nightly with new profile 2. Open Wikipedia editing page ( Ex. http://ja.wikipedia.org/w/index.php?title=Wikipedia:%E3%82%B5%E3%83%B3%E3%83%89%E3%83%9C%E3%83%83%E3%82%AF%E3%82%B9&action=edit ) 3. IME on 4. Key in some text Actual Results: Response of input is painfully slow. Expected Results: it should be at usual speed. Regression window: Works: http://hg.mozilla.org/mozilla-central/rev/68033b993ed7 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre ID:20100913165011 Fails: http://hg.mozilla.org/mozilla-central/rev/ccaffbc6a970 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100913 Firefox/4.0b6pre ID:20100913162558 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68033b993ed7&tochange=ccaffbc6a970 Suspected bug: https://bugzilla.mozilla.org/show_bug.cgi?id=240933#c117 Relanded Bug 240933 - Plaintext editor should stop using <br> all over

*This happens at the time of the editing of the huge sentence and texts. *And it does not happen if IME off.

I will profile this today.

This happens in not only Wikipedia but also ordinary TEXTAREA. [STR] 1. Start Nightly with New profile 2. Open https://bugzilla.mozilla.org/show_bug.cgi?id=240933 3. Select All and Copy 4. Paste in to TEXTAREA at the bottom of the page. (Paste the huge texts on TEXTAREA) 6. IME ON 7. Key in some text with IME

So, I couldn't actually reproduce the bug. Could you please let me know exactly which keyboard layout to use and what to do in order to reproduce this? (Are you able to reproduce this on multiple platforms? I tried to reproduce this on Mac)

(Note that I don't speak Japanase, so telling me exactly which keys to press would be most helpful) :-)

(In reply to comment #4) > So, I couldn't actually reproduce the bug. Could you please let me know > exactly which keyboard layout to use and what to do in order to reproduce > this? (Are you able to reproduce this on multiple platforms? I tried to > reproduce this on Mac) This does not happen on Ubuntu10.04+Ibus. So,I guess this happens only on Windows platform. (In reply to comment #5) > (Note that I don't speak Japanase, so telling me exactly which keys to press > would be most helpful) :-) 1. Using Microsoft IME on Windows7 Japanese Edition and Japanese 109keyboard, 2. Press ALT+半角/全角漢字 key(to start IME ON) 3. Press "w a t a s h i h a" keys without quotation and space 4. Displaying of one by one character is very slow. ** I think you should contact Masayuki Nakano (Mozilla Japan).

I can reproduce this too. When I test this bug on debug build, I can see some warnings but I'm not sure this is related or not. I suspected the nsContentEventHandler performance. But according to the log, it seems not so. When I type 'A' on the wikipedia's text area, I got following widget log: > 0[51f220]: IMM32: OnKeyDownEvent, hWnd=00041f20, wParam=000000e5, lParam=00290001 > 0[51f220]: IMM32: OnIMENotify, hWnd=00041f20, IMN_SETOPENSTATUS > 0[51f220]: IMM32: nsIMM32Handler is created > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 283, y: 646, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED log for normal case (not reproduced this perf regression) > 0[51f220]: IMM32: OnKeyDownEvent, hWnd=00041f20, wParam=000000e5, lParam=00040001 > 0[51f220]: IMM32: OnIMEStartComposition, hWnd=00041f20, mIsComposing=FALSE > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 976, y: 170, width: 2, height: 14 } > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 976, y: 170, width: 2, height: 14 } > 0[51f220]: IMM32: SetIMERelatedWindowsPos, mNativeCaretIsCreated=TRUE, width=2 height=14 > 0[51f220]: IMM32: SetIMERelatedWindowsPos, Set candidate window > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 976, y: 170, width: 2, height: 14 } > 0[51f220]: IMM32: OnIMENotify, hWnd=00041f20, IMN_SETCANDIDATEPOS, lParam=00000001 > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 976, y: 170, width: 2, height: 14 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: HandleStartComposition, START composition, mCompositionStart=0 > 0[51f220]: IMM32: OnIMEComposition, hWnd=00041f20, lParam=000001b9, mIsComposing=TRUE > 0[51f220]: IMM32: OnIMEComposition, GCS_RESULTSTR=no, GCS_COMPSTR=YES, GCS_COMPATTR=YES, GCS_COMPCLAUSE=YES, GCS_CURSORPOS=YES > 0[51f220]: IMM32: HandleComposition, GCS_COMPSTR > 0[51f220]: IMM32: GetCompositionString, SUCCEEDED mCompositionString="あ" > 0[51f220]: IMM32: HandleComposition, GCS_COMPCLAUSE, useA_API=FALSE > 0[51f220]: IMM32: HandleComposition, GCS_COMPCLAUSE, mClauseLength=2 > 0[51f220]: IMM32: HandleComposition, GCS_COMPATTR, mAttributeLength=1 > 0[51f220]: IMM32: HandleComposition, GCS_CURSORPOS, mCursorPosition=1 > 0[51f220]: IMM32: DispatchTextEvent, aCheckAttr=TRUE > 0[51f220]: IMM32: SetTextRangeList, index=0, rangeType=NS_TEXTRANGE_RAWINPUT, range=[0-1] > 0[51f220]: IMM32: SetTextRangeList, caret position=1 > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 976, y: 170, width: 12, height: 14 } > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 987, y: 170, width: 2, height: 14 } > 0[51f220]: IMM32: SetIMERelatedWindowsPos, Set candidate window > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 976, y: 170, width: 12, height: 14 } > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 976, y: 170, width: 12, height: 14 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMENotify, hWnd=00041f20, IMN_SETCANDIDATEPOS, lParam=00000001 > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 976, y: 170, width: 12, height: 14 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 976, y: 170, width: 12, height: 14 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED

Oops, first log is wrong, correct log is here: > 0[51f220]: IMM32: OnKeyDownEvent, hWnd=00041f20, wParam=000000e5, lParam=001e0001 > 0[51f220]: IMM32: OnIMEStartComposition, hWnd=00041f20, mIsComposing=FALSE > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 235, y: 719, width: 1, height: 18 } > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 235, y: 719, width: 1, height: 18 } > 0[51f220]: IMM32: SetIMERelatedWindowsPos, mNativeCaretIsCreated=TRUE, width=1 height=18 > 0[51f220]: IMM32: SetIMERelatedWindowsPos, Set candidate window > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 235, y: 719, width: 1, height: 18 } > 0[51f220]: IMM32: OnIMENotify, hWnd=00041f20, IMN_SETCANDIDATEPOS, lParam=00000001 > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 235, y: 719, width: 1, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: HandleStartComposition, START composition, mCompositionStart=3138 > 0[51f220]: IMM32: OnIMEComposition, hWnd=00041f20, lParam=000001b9, mIsComposing=TRUE > 0[51f220]: IMM32: OnIMEComposition, GCS_RESULTSTR=no, GCS_COMPSTR=YES, GCS_COMPATTR=YES, GCS_COMPCLAUSE=YES, GCS_CURSORPOS=YES > 0[51f220]: IMM32: HandleComposition, GCS_COMPSTR > 0[51f220]: IMM32: GetCompositionString, SUCCEEDED mCompositionString="ち" > 0[51f220]: IMM32: HandleComposition, GCS_COMPCLAUSE, useA_API=FALSE > 0[51f220]: IMM32: HandleComposition, GCS_COMPCLAUSE, mClauseLength=2 > 0[51f220]: IMM32: HandleComposition, GCS_COMPATTR, mAttributeLength=1 > 0[51f220]: IMM32: HandleComposition, GCS_CURSORPOS, mCursorPosition=1 > 0[51f220]: IMM32: DispatchTextEvent, aCheckAttr=TRUE > 0[51f220]: IMM32: SetTextRangeList, index=0, rangeType=NS_TEXTRANGE_RAWINPUT, range=[0-1] > 0[51f220]: IMM32: SetTextRangeList, caret position=1 > 0[51f220]: WARNING: Ended font run in the middle of a cluster: 'end == aSource->GetLength() || aSource->IsClusterStart(end)', file m:/mozilla-l/src/gfx/thebes/gfxFont.cpp, line 4229 > 0[51f220]: WARNING: Started font run in the middle of a cluster: 'aSource->IsClusterStart(start)', file m:/mozilla-l/src/gfx/thebes/gfxFont.cpp, line 4227 > 0[51f220]: WARNING: Ended font run in the middle of a cluster: 'end == aSource->GetLength() || aSource->IsClusterStart(end)', file m:/mozilla-l/src/gfx/thebes/gfxFont.cpp, line 4229 > 0[51f220]: WARNING: Started font run in the middle of a cluster: 'aSource->IsClusterStart(start)', file m:/mozilla-l/src/gfx/thebes/gfxFont.cpp, line 4227 > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 234, y: 719, width: 17, height: 18 } > 0[51f220]: IMM32: GetCaretRect, SUCCEEDED, aCaretRect={ x: 251, y: 719, width: 1, height: 18 } > 0[51f220]: IMM32: SetIMERelatedWindowsPos, Set candidate window > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 234, y: 719, width: 17, height: 18 } > 0[51f220]: IMM32: OnIMENotify, hWnd=00041f20, IMN_SETCANDIDATEPOS, lParam=00000001 > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 234, y: 719, width: 17, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED > 0[51f220]: IMM32: OnIMERequest, hWnd=00041f20, IMR_QUERYCHARPOSITION > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aOffset=0, SUCCEEDED > 0[51f220]: IMM32: GetCharacterRectOfSelectedTextAt, aCharRect={ x: 234, y: 719, width: 17, height: 18 } > 0[51f220]: IMM32: HandleQueryCharPosition, SUCCEEDED

FYI, In local build(windows7): build from c611f052ec92 :very slow build from 0f5a2dce1aa5 : normal speed Triggered: c611f052ec92 Ehsan Akhgari — Bug 240933 - Part 1: Do not split multiline text into textframes separated by BR elements; r=roc a=dbaron

OK, I managed to reproduce this. Thanks for your help. I'll investigate.

Here is what's happening. During nsIMM32Handler::DispatchTextEvent, we end up calling nsContentEventHandler::SetRangeFromFlatTextOffset <http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsContentEventHandler.cpp#374> (see the stack below). xul.dll!nsContentEventHandler::SetRangeFromFlatTextOffset(nsIRange * aRange=0x0e1c0620, unsigned int aNativeOffset=86266, unsigned int aNativeLength=1, int aExpandToClusterBoundaries=1) Line 374 + 0x1b bytes C++ xul.dll!nsContentEventHandler::OnQueryTextRect(nsQueryContentEvent * aEvent=0x0034ce24) Line 551 C++ xul.dll!nsEventStateManager::PreHandleEvent(nsPresContext * aPresContext=0x0dc23110, nsEvent * aEvent=0x0e07083c, nsIFrame * aTargetFrame=0x12dc1eb0, nsEventStatus * aStatus=0x0034cce0, nsIView * aView=0x0e0754f0) Line 1342 C++ xul.dll!PresShell::HandleEventInternal(nsEvent * aEvent=0x0e070820, nsIView * aView=0x0e0754f0, nsEventStatus * aStatus=0x0034cce0) Line 7127 C++ xul.dll!PresShell::HandleEvent(nsIView * aView=0x0e0754f0, nsGUIEvent * aEvent=0x0034ce24, int aDontRetargetEvents=239428272, nsEventStatus * aEventStatus=0x0034cce0) Line 6884 + 0x11 bytes C++ xul.dll!PresShell::HandleEvent(nsIView * aView=0x0a1b17f8, nsGUIEvent * aEvent=0x0e050d08, int aDontRetargetEvents=175507928, nsEventStatus * aEventStatus=0x0034cce0) Line 6561 + 0x10 bytes C++ xul.dll!nsViewManager::HandleEvent(nsView * aView=0x00000000, nsGUIEvent * aEvent=0x0034ce24) Line 1056 C++ xul.dll!nsViewManager::DispatchEvent(nsGUIEvent * aEvent=0x0034ce24, nsIView * aView=0x0a1b17f8, nsEventStatus * aStatus=0x0034cd4c) Line 1031 + 0x9 bytes C++ xul.dll!AttachedHandleEvent(nsGUIEvent * aEvent=0x09bcabc0) Line 193 C++ xul.dll!nsWindow::DispatchEvent(nsGUIEvent * event=0x0034ce24, nsEventStatus & aStatus=nsEventStatus_eIgnore) Line 3545 + 0x4 bytes C++ xul.dll!nsWindow::DispatchWindowEvent(nsGUIEvent * event=0x00000000) Line 3572 C++ xul.dll!nsIMM32Handler::GetCharacterRectOfSelectedTextAt(nsWindow * aWindow=0x00000007, unsigned int aOffset=0, nsIntRect & aCharRect={...}) Line 1871 C++ xul.dll!nsIMM32Handler::SetIMERelatedWindowsPos(nsWindow * aWindow=0x09bf7b30, const nsIMEContext & aIMEContext={...}) Line 1926 C++ xul.dll!nsIMM32Handler::DispatchTextEvent(nsWindow * aWindow=0x09bf7b30, const nsIMEContext & aIMEContext={...}, int aCheckAttr=1) Line 1677 C++ xul.dll!nsIMM32Handler::HandleComposition(nsWindow * aWindow=0x09bf7b30, const nsIMEContext & aIMEContext={...}, long lParam=441) Line 1273 C++ xul.dll!nsIMM32Handler::OnIMEComposition(nsWindow * aWindow=0x09bf7b30, unsigned int wParam=12431, long lParam=441) Line 571 + 0xe bytes C++ xul.dll!nsIMM32Handler::ProcessMessage(nsWindow * aWindow=0x09bf7b30, unsigned int msg=271, unsigned int & wParam=12431, long & lParam=441, long * aRetValue=0x0034d270, int & aEatMessage=2004231718) Line 404 C++ xul.dll!nsWindow::ProcessMessage(unsigned int msg=271, unsigned int & wParam=12431, long & lParam=441, long * aRetValue=0x0034d270) Line 4525 + 0x15 bytes C++ xul.dll!nsWindow::WindowProcInternal(HWND__ * hWnd=0x09bf7b30, unsigned int msg=271, unsigned int wParam=12431, long lParam=441) Line 4402 + 0x17 bytes C++ xul.dll!CallWindowProcCrashProtected(long (HWND__ *, unsigned int, unsigned int, long)* wndProc=0x67641049, HWND__ * hWnd=0x000c0086, unsigned int msg=271, unsigned int wParam=12431, long lParam=441) Line 65 + 0xf bytes C++ xul.dll!nsWindow::WindowProc(HWND__ * hWnd=0x000c0086, unsigned int msg=271, unsigned int wParam=12431, long lParam=441) Line 4344 + 0x16 bytes C++ ConvertToXPOffset is really inefficient. It first copies the text node string, then truncates it, then goes ahead and replaces native newlines to XP newlines. This means that if the textnode has N newlines, we perform N memmoves, making an O(N^2) algorithm, which hurts us badly now that textareas can contain huge text nodes. Actually none of this is necessary at all! On Mac (where the length of the native newline is equal to the length of the XP newline), we can just return the native offset. On Windows, we can calculate the number of newlines in the string, and return aNativeOffset - numberOfNewLines.

nsContentEventHandler::GetFlatTextOffset also showed pathological performance characteristics. This function too is O(N^2), and also does a bunch of unnecessary copying, which can be avoided because we're only interested in the length, and not the actual strings.

With these two patches, the performance of entering Japanese text with IME turned on is the same as entering English text for me. In the xperf profile, painting seems to be a larger chunk of the time spent while typing characters, which makes me quite happy!

Comment on attachment 541185 [details] [diff] [review] Part 1 - Optimize the conversion of native and cross platform text offsets Review of attachment 541185 [details] [diff] [review]: -----------------------------------------------------------------

Comment on attachment 541199 [details] [diff] [review] Part 2: Optimize nsContentEventHandler::GetFlatTextOffset Review of attachment 541199 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/events/src/nsContentEventHandler.cpp @@ +262,5 @@ > return 0; > + if (aMaxOffset == 0xffffffff) { > + aMaxOffset = 0; > + } > + return text->GetLength() - textLengthDifference - aMaxOffset; I don't understand this line. You seem to be returning the length of the text after aMaxOffset, but shouldn't you be returning the length before aMaxOffset? At least document what aMaxOffset means.

(In reply to comment #16) > ::: content/events/src/nsContentEventHandler.cpp > @@ +262,5 @@ > > return 0; > > + if (aMaxOffset == 0xffffffff) { > > + aMaxOffset = 0; > > + } > > + return text->GetLength() - textLengthDifference - aMaxOffset; > > I don't understand this line. You seem to be returning the length of the > text after aMaxOffset, but shouldn't you be returning the length before > aMaxOffset? > > At least document what aMaxOffset means. Good catch, you're right!

GetNativeTextLength can accept BR nodes in addition to text nodes, so asserting that the node parameter is a text node is a mistake.

Comment on attachment 541235 [details] [diff] [review] Part 2: Optimize nsContentEventHandler::GetFlatTextOffset Review of attachment 541235 [details] [diff] [review]: -----------------------------------------------------------------

Comment on attachment 541251 [details] [diff] [review] Part 1 - Optimize the conversion of native and cross platform text offsets Review of attachment 541251 [details] [diff] [review]: -----------------------------------------------------------------

Sorry for the numerous iterations here, but I discovered two bugs with the previous version of this patch. The first bug was with the newline counting. I incorrectly had assumed that the input to CountNewlinesIn has native newlines, so I was looking for them. But it's actually the other way around, so I just need to look for linefeed characters.

The second bug was in GetNativeTextLength. Since we're getting the native length, we should be adding the adjustment value calculated instead of subtracting it from the original (XP) length.

This part has only changed to apply on top of part 1. It's probably a good idea for Masayuki to review these patches too, since it's now apparent that my coding skills are less than perfect. ;-)

Comment on attachment 541521 [details] [diff] [review] Part 2: Optimize nsContentEventHandler::GetFlatTextOffset ...so are my bugzilla usage skills!

Comment on attachment 541517 [details] [diff] [review] Part 1 - Optimize the conversion of native and cross platform text offsets Review of attachment 541517 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/events/src/nsContentEventHandler.cpp @@ +216,5 @@ > + "aContent is not a text node!"); > + const nsTextFragment* text = aContent->GetText(); > + if (!text) > + return 0; > + if (aMaxOffset == 0xffffffff) { Why don't you define name const or macro for 0xffffffff? @@ +246,5 @@ > + CountNewlinesIn(aContent, 0xffffffff); > +#else > + // On other platforms, the native and XP newlines are the same. > + 0; > +#endif I think the lines in this #if/#elif/#else should be indented one more level.

(In reply to comment #25) > @@ +246,5 @@ > > + CountNewlinesIn(aContent, 0xffffffff); > > +#else > > + // On other platforms, the native and XP newlines are the same. > > + 0; > > +#endif > > I think the lines in this #if/#elif/#else should be indented one more level. This is about GetNativeTextLength().

> Why don't you define name const or macro for 0xffffffff? PR_UINT32_MAX?

Comment on attachment 541521 [details] [diff] [review] Part 2: Optimize nsContentEventHandler::GetFlatTextOffset Review of attachment 541521 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/events/src/nsContentEventHandler.cpp @@ +254,5 @@ > return 0; > + PRUint32 length = text->GetLength(); > + if (aMaxOffset != 0xffffffff) { > + length = aMaxOffset; > + } If aMaxOffset is larger than actual length but aMaxOffset isn't 0xffffffff, it causes some bugs. Maybe, it should be: PRUint32 length = NS_MIN(text->GetLength(), aMaxOffset); And aMaxOffset may be misleadable. Isn't aMaxLength better? (CountNewlinesIn()'s aMaxOffset too.)

Comment on attachment 541517 [details] [diff] [review] Part 1 - Optimize the conversion of native and cross platform text offsets Review of attachment 541517 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/events/src/nsContentEventHandler.cpp @@ +216,5 @@ > + "aContent is not a text node!"); > + const nsTextFragment* text = aContent->GetText(); > + if (!text) > + return 0; > + if (aMaxOffset == 0xffffffff) { as bz said, PR_UINT32_MAX

Try run for 5c4767395a8b is complete. Detailed breakdown of the results available here: http://tbpl.mozilla.org/?tree=Try&rev=5c4767395a8b Results: exception: 3 success: 142 warnings: 16 Total buildrequests: 161

Review comments addressed.

Comment on attachment 541845 [details] [diff] [review] Part 2: Optimize nsContentEventHandler::GetFlatTextOffset Thank you.

http://hg.mozilla.org/mozilla-central/rev/58655e365c91 http://hg.mozilla.org/mozilla-central/rev/d7ed8936e9f8 Please file a new bug if you still see more performance regressions. Thanks!

Verified as fixed on: Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 Build ID: 20110831124126 To verify this fix I used the steps in comment #3. The response time is good now when using the Japanese IME.