Move the NSTextInput Implementation in nsChildView.mm to the nsCocoaTextInputHandler class. The text input (and key events) related code is very large and they will grow up. And they are related to the IME statement which is managed in nsCocoaIMEHandler (nsCocoaTextInputHandler's parent class). After we remove the code for 10.4, we should do this.

We don't need the methods IME_* of nsChildView. They were used for #ifdef hell for 10.4 support. But 10.4 support has been dropped, so, we can remove them now. # mChildView->TextInputHandler()->*() is long, but it will be shrunken when this bug is completely fixed because the callers will be moved into the nsTextInputHandler.

Comment on attachment 462310 [details] [diff] [review] Patch part.1 v1.0 Remove IME method wrapper Just removes several methods which are not dropped by bug 548021. No risk but this blocks my next work.

I should fix this bug before bug 631165.

changed the order of patches.

Comment on attachment 513695 [details] [diff] [review] Patch part.2 v1.1 Move dispatching text event code to nsCocoaIMEHandler Part 2, 3, and 4 are just moving the code from nsChildView.mm to nsCocoaTextInputHandler.mm. And the patches are stable for me now. Steven, would you review these patches? They will be landed after branched. So, if you have some work for Fx4, you don't need to review these patches for now. UnderlineAttributeToTextRangeType() -> nsCocoaIMEHandler::ConvertToTextRangeType() CountRanges() -> nsCocoaIMEHandler::GetRangeCount() ConvertAttributeToGeckoRange() -> nsCocoaIMEHandler::SetTextRangeList() ChildView::sendTextEvent and FillTextRangeInTextEvent() -> nsCocoaIMEHandler::DispatchTextEvent() Note that don't mind about the debug log, I'll clean up them in final patch of this bug because IME related code should be able to print logs even on release build. So, we should use our logging API rather than #ifdef and NSLog().

Comment on attachment 513696 [details] [diff] [review] Patch part.3 v1.2 Move marked text handling to nsCocoaIMEHandler ChildView::mMarkedRange is moved to nsCocoaIMEHandler. ChildView::sendCompositionEvent is removed. Composition commit handler in NSTextInput::inserText is moved to nsCocoaIMEHandler::InsertTextAsCommitComposition() NSTextInput::setMarkedText: -> nsCocoaIMEHandler::SetMarkedText().

Comment on attachment 513697 [details] [diff] [review] Patch part.4 v1.0 Move NSTextInput implementation to nsCocoaIMEHandler (except insertText) NSTextInput::conversionIdentifier -> nsCocoaIMEHandler::ConvertionIdentifier() NSTextInput::attributedSubstringFromRange -> nsCocoaIMEHandler::GetAttributedSubstringFromRange() NSTextInput::selectedRange -> nsCocoaIMEHandler::SelectedRange() NSTextInput::firstRectForCharacterRange -> nsCocoaIMEHandler::FirstRectForCharacterRange() NSTextInput::characterIndexForPoint -> nsCocoaIMEHandler::CharacterIndexForPoint NSTextInput::validAttributesForMarkedText nsCocoaIMEHandler::GetValidAttributesForMarkedText()

When I worked on part.7 which moves plugin text input handlers to nsCocoaTextInputHandler.mm, I realized that the code should be in a new class which is PluginTextInputHandler. The class is a super class of nsCocoaIMEHandler. Then, it needs some members of nsCocoaIMEHandler and nsCocoaTextInputHandler. For resolving this issue, I should make a root class as TextInputHandlerBase and a skeleton class for plugin as PluginTextInputHandler. This patch also renames ns* classes in nsCocoaTextInputHandler.h with namespace mozilla::widget and their files to TextInputHandler.*. This is modern rules of current Mozilla source code.

fix a mistake.

oops, sorry for the spam.

I found some memory broken bugs by automated tests with part.7 patch. It's the cause of the delay of posting new patches. Now, I realized the cause of the bug which is a design issue of the handler class life cycle. I'll fix it next part 6 patch and current part 6 and later patches will be increased the number. I'm going to land the patches until I finish the part 6 work. So, I'd like Steven to review the current review requested patches.

> So, I'd like Steven to review the current review requested patches. I should be able to review them tomorrow (Friday) or Monday.

Comment on attachment 518610 [details] [diff] [review] Patch part.2 v1.2 Move text event dispatcher to nsCocoaIMEHandler This patch basically looks fine to me. But I did find one problem with the code. And I have suggested changes to several of the comments (to make them clearer). Here's the code change: +PRBool +nsCocoaIMEHandler::DispatchTextEvent(const nsString& aText, + NSAttributedString* aAttrString, + NSRange& aSelectedRange, + PRBool aDoCommit) +{ +#ifdef DEBUG_IME_HANDLER + NSLog(@"****in DispatchTextEvent; string = '%@'", aAttrString.get()); + NSLog(@" aSelectedRange = %d, %d", + aSelectedRange.location, aSelectedRange.length); +#endif An NSAttributedString object doesn't have a get() method, and aAttrString is a pointer, not a reference. Here are suggested changes to the comments: + /** + * DispatchTextEvent() dispatches a text event on mOwnerWidget. + * + * @param aText Inputted text by current event or + * composition string. + * @param aAttrString An NSAttributedString instance which indicates + * current composition string. + * @param aSelectedRange Current selected range (or caret position). + * @param aDoCommit If the text event means committing the + * composition string, TRUE. Otherwise, FALSE. + */ The comment on @param aText should be something like "User text input". The comment on @param aDoCommit should be something like "Should the composition string be committed?" Or: "TRUE if the composition string should be committed. Otherwise FALSE." + /** + * GetRangeCount() computes the range count of aAttrString. + * + * @param aAttrString An NSAttributedString instance you want to + * know the count of ranges. + * @return The count of ranges of aAttrString. + */ The comment on @param aAttrString should be something like: "An NSAttributedString instance whose number of NSUnderlineStyleAttributeName ranges you wish to know." The comment on @return should be something like: "The count of NSUnderlineStyleAttributeName ranges in aAttrString." + /** + * SetTextRangeList() appends text ranges to aTextRangeList. + * + * @param aTextRangeList Result of the ranges. Note that if you can + * set an enough auto-range instance for most + * cases (e.g., nsAutoTArray<nsTextRange, 4>), + * it prevents memory fragmentation. + * @param aAttrString An NSAttributedString instance which indicates + * current composition string. + * @param aSelectedRange Current selected range (or caret position). + */ The comment on @param aTextRangeList should be something like: "When SetTextRangeList() returns, aTextRangeList will be set to the NSUnderlineStyleAttributeName ranges in aAttrString." +PRUint32 +nsCocoaIMEHandler::ConvertToTextRangeType(PRUint32 aUnderlineStyle, + NSRange& aSelectedRange) +{ +#ifdef DEBUG_IME_HANDLER + NSLog(@"****in UnderlineAttributeToTextRangeType = %d", aUnderlineStyle); +#endif The NSLog() statement should be: NSLog(@"****in ConvertToTextRangeType = %d", aUnderlineStyle); + if (aSelectedRange.length == 0) { + switch (aUnderlineStyle) { + case NSUnderlineStyleSingle: + return NS_TEXTRANGE_RAWINPUT; + case NSUnderlineStyleThick: + return NS_TEXTRANGE_SELECTEDRAWTEXT; + default: + NS_WARNING("Unexpected line style comes"); + return NS_TEXTRANGE_SELECTEDRAWTEXT; + } + } The NS_WARNING() statement should be: NS_WARNING("Unexpected underline style"); + switch (aUnderlineStyle) { + case NSUnderlineStyleSingle: + return NS_TEXTRANGE_CONVERTEDTEXT; + case NSUnderlineStyleThick: + return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT; + default: + NS_WARNING("Unexpected line style comes"); + return NS_TEXTRANGE_SELECTEDCONVERTEDTEXT; + } The NS_WARNING() statement should be: NS_WARNING("Unexpected underline style");

Comment on attachment 518610 [details] [diff] [review] Patch part.2 v1.2 Move text event dispatcher to nsCocoaIMEHandler >+ * @param aTextRangeList Result of the ranges. Note that if you can >+ * set an enough auto-range instance for most >+ * cases (e.g., nsAutoTArray<nsTextRange, 4>), >+ * it prevents memory fragmentation. > > The comment on @param aTextRangeList should be something like: > > "When SetTextRangeList() returns, aTextRangeList will be set to the > NSUnderlineStyleAttributeName ranges in aAttrString." I should have said that the comment should be something like: "When SetTextRangeList() returns, aTextRangeList will be set to the NSUnderlineStyleAttributeName ranges in aAttrString. Note that if you pass in a large enough auto-range instance for most cases (e.g., nsAutoTArray<nsTextRange, 4>), it prevents memory fragmentation."

Comment on attachment 518612 [details] [diff] [review] Patch part.4 v1.1 Move NSTextInput handler to nsCocoaIMEHandler (except insertText) This looks fine to me. I just have one suggestion for a comment change: + /** + * SetMarkedText() is a handler of setMarkedText of NSTextInput. + * + * @param aAttrString Set the id of setMarkedText. If the instance + * isn't NSAttributedString, please create + * an NSAttributedString instance for the id. + * @param aSelectedRange Current selected range (or caret position). + */ The comment to @param aAttrString should be something like: "This must be an instance of NSAttributedString. If the aString parameter to [ChildView setMarkedText:setSelectedRange:] isn't an instance of NSAttributedString, create an NSAttributedString from it and pass that instead."

Comment on attachment 518612 [details] [diff] [review] Patch part.4 v1.1 Move NSTextInput handler to nsCocoaIMEHandler (except insertText) Oops, commented on (and r+ed) wrong patch.

Comment on attachment 518642 [details] [diff] [review] Patch part.3 v1.4 Move marked text handler to nsCocoaIMEHandler This looks fine to me. I just have one suggestion for a comment change: + /** + * SetMarkedText() is a handler of setMarkedText of NSTextInput. + * + * @param aAttrString Set the id of setMarkedText. If the instance + * isn't NSAttributedString, please create + * an NSAttributedString instance for the id. + * @param aSelectedRange Current selected range (or caret position). + */ The comment to @param aAttrString should be something like: "This must be an instance of NSAttributedString. If the aString parameter to [ChildView setMarkedText:setSelectedRange:] isn't an instance of NSAttributedString, create an NSAttributedString from it and pass that instead."

Comment on attachment 518612 [details] [diff] [review] Patch part.4 v1.1 Move NSTextInput handler to nsCocoaIMEHandler (except insertText) This basically looks fine to me. I'll suggest some cosmetic changes, some of which are changes to comments. Throughout the patch, ConvertionIdentifier() should be changed to ConversationIdentifier(). This must have been a typo :-) + /** + * ConvertionIdentifier() returns an ID for current editor. The ID is + * guaranteed that another editor doesn't have same ID same time. + * I.e., the ID might be same as another editor which was already destroyed. + * + * @return An identifier of current focused editor. + */ The comment should be something like: "ConversationIdentifier() returns an ID for the current editor. The ID is guaranteed to be unique among currently existing editors. But it might be the same as the ID of an editor that has already been destroyed." + /** + * FirstRectForCharacterRange() returns first *character* rect in the range. + * Cocoa needs the first line rect in the range, but we cannot compute it + * on current implementation. + * + * @param The range of text which you want. The + * position is offset from focused editor or + * focused document. + * @return The first character rect of the range whose + * position is offset from screen. + * If the length of aRange is 0, the width will + * be 0. + */ The comment on @param should be something like: "A range of text to examine. Its position is an offset from the beginning of the focused editor or document." The comment on @return should be something like: "A NSRect containing the first character in aRange, in screen coordinates. If the length of aRange is 0, the width will be 0." + /** + * CharacterIndexForPoint() returns an offset of a character at aPoint. + * XXX This isn't implemented, always returns 0. + * + * @param The point in screen coordinates. + * @return The offset of a character at aPoint from + * focused editor or focused document. + */ The comment on @return should be something like: "The offset of the character at aPoint from the beginning of the focused editor or document." +NSInteger +nsCocoaIMEHandler::ConvertionIdentifier() +{ + // NOTE: The size of NSInteger is same as pointer size. + nsQueryContentEvent textContent(PR_TRUE, NS_QUERY_TEXT_CONTENT, mOwnerWidget); + textContent.InitForQueryTextContent(0, 0); + mOwnerWidget->DispatchWindowEvent(textContent); + if (!textContent.mSucceeded) { + return reinterpret_cast<NSInteger>(mView); + } + // XXX This might return same ID for different editor if another editor is + // created at same point as old editor. Is there a better way? + return reinterpret_cast<NSInteger>(textContent.mReply.mContentsRoot); +} The XXX comment should be something like: "XXX This might return the same ID as a previously existing editor if the deleted editor was created at the same address. Is there a better way?"

Comment on attachment 518613 [details] [diff] [review] Patch part.5 v1.2 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename nsCocoa*Hander to *InputHandler with namespace This looks fine to me. Once again I've got some suggested changes to the comments: + /** + * OnDestroyView must be called when mOwnerWidget is destroying and detaching + * mView. + * + * @param aDestroingView Destroing view. This may not be mView. + * @return This result doesn't have any meaning for + * callers. When the aDstroingView isn't same + * as mView, FALSE. Then, inherited methods in + * sub classes should return from this method + * without cleaning up. + */ "@param aDestroingView" should be "@param aDestroyingView". The comment on @param aDestroyingView should be: "Destroying view. This might not be mView." In the comment on @return, "mDstroingView" should be "mDestroyingView". + // The native focused view, this is the native NSView of mOwnerWidget. + // This view handling the actual text inputting. + NSView<mozView>* mView; The second sentence of the comment should be: "This view handles the actual text inputting." +/** + * PluginTextInputHandler is going to handle text input events for plugins. + */ This should be: "PluginTextInputHandler handles text input events for plugins." /** - * nsCocoaTextInputHandler is going to implement the NSTextInput protocol. + * TextInputHandler is going to implement the NSTextInput protocol. */ This should be: "TextInputHandler implements the NSTextInput protocol."

This patch changes the owner of TextInputHandler from nsChildView to refpointer. Basically, TextInputHandler is grabbed by nsChildView until called Destroy(). ChildView stores the pointer but it's weak. This is automatically done by TextInputHandler at initializing. This makes simpler code. TextInputHandler grabs ChildView. And all native event handlers grabs TextInputHandler themselves. (But this is By these approach, nsChildView's life time isn't changed by this bug's patches. E.g., even if a dispatched event causes destroying the widget, the widget isn't grabbed by TextInputHandler's any methods. So, the can be gone. But this is just to be safe. ChildView must not be destroyed during nsChildView life time. TextInputHandler grabs itself. Therefore, it can check its member safely (i.e., can call Destroy() safely). I want to land part.2 - part.6 if you r+ this patch before part.7.

Steven: ping

> Steven: ping I started reviewing part 6 of your patch today. But then I discovered that I need to do more work on my patch for bug 621117, which is more urgent. I'll probably have to put off the review until early next week.

Okay, don't mind. I have a lot of other work, you too. Thank you for explanation about your schedule.

Comment on attachment 526398 [details] [diff] [review] Patch part.6 v1.0 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events I've been reviewing this patch, and will have more detailed comments later. But for now I'll just say that it has a serious problem -- OnDestroyWidget() is never called! I suggest you change your patch to [ChildView widgetDestroyed] as follows: > - (void)widgetDestroyed > { >- mGeckoChild->TextInputHandler()->OnDestroyView(self); >- mGeckoChild = nsnull; >+ if (mTextInputHandler) { >+ mTextInputHandler->OnWidgetDestroyed(mGeckoChild); >+ mTextInputHandler = nsnull; >+ } >+ mGeckoChild = nsnull; Or actually the following should also be safe: > - (void)widgetDestroyed > { >- mGeckoChild->TextInputHandler()->OnDestroyView(self); >+ mTextInputHandler->OnWidgetDestroyed(mGeckoChild); >+ mTextInputHandler = nsnull; > mGeckoChild = nsnull;

Comment on attachment 526398 [details] [diff] [review] Patch part.6 v1.0 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events Additional comments: >+ nsrefcnt AddRef() > ... >+ nsrefcnt AddRef() > ... You haven't tried to make these thread-safe. But I imagine they're always called on the main thread -- which would make this unnecessary. >+ /** >+ * OnDestroyWidget must be called when mWidget is destroying The following text would be better: mWidget must not be destroyed without OnDestroyWidget being called. >+ * callers. When the aDstroyingWidget isn't same >+ * as mWidget, FALSE. Then, inherited methods in The sentence in the middle should be: When aDestroyingWidget isn't the same as mWidget, FALSE. >-IMEInputHandler::OnDestroyView(NSView<mozView> *aDestroyingView) >+IMEInputHandler::OnDestroyWidget(nsChildView* aDestroyingWidget) > { > // If we're not focused, the destroying view might be composing with it in > // another instance. > if (sFocusedIMEHandler && sFocusedIMEHandler != this) { This is a driveby, but I found this comment very difficult to understand. Something like the following would be better: If we're not focused, the focused IMEInputHandler may have been created by another widget/nsChildView. >+ // If your view wanted Gecko's default text input handler, you could use >+ // aHandler for handling key events and other NSTextInput protocol >+ // implementation. Don't use strong reference for aHandler because it >+ // causes memory leak. >+- (void)installTextInputHandler:(mozilla::widget::TextInputHandler*)aHandler; >+- (void)uninstallTextInputHandler; The comment should be something like the following: aHandler is Gecko's default text input handler: It implements the NSTextInput protocol to handle key events. Don't make aHandler a strong reference -- that causes a memory leak. >+ // Text input handler for mGeckoChild and the view. Note that this is weak >+ // reference. Ideally, this should be strong reference but the life time of >+ // NSView instances are too long for our memory leak detector. >+ // This is initialized by [mozView installTextInputHandler:aHandler] and >+ // cleared by [mozView uninstallTextInputHandler]. >+ mozilla::widget::TextInputHandler* mTextInputHandler; // [WEAK] The comment should be something like the following. (If I've misunderstood your comment, please let me know.) Text input handler for mGeckoChild and us. Note that this is a weak reference. Ideally this should be a strong reference. But a ChildView object can live longer than the mGeckoChild that owns it. And if mTextInputHandler were a strong reference, this would make it difficult for Gecko's leak detector to detect leaked TextInputHandler objects.

(In reply to comment #54) > I've been reviewing this patch, and will have more detailed comments > later. But for now I'll just say that it has a serious problem -- > OnDestroyWidget() is never called! Oops!! I removed the necessary code before diff. (In reply to comment #55) > >+ nsrefcnt AddRef() > > ... > >+ nsrefcnt AddRef() > > ... > > You haven't tried to make these thread-safe. But I imagine they're > always called on the main thread -- which would make this unnecessary. Yes. The model of this class is nsGtkIMModule which is IME event handler for GTK. It is owned by top level nsWindow and created/referenced by every child nsWindow (i.e., nsChildView in widget/src/cocoa). But we don't have any problems on GTK. So, we can say that nsWindow is always created and destroyed in main thread. > >+ // Text input handler for mGeckoChild and the view. Note that this is weak > >+ // reference. Ideally, this should be strong reference but the life time of > >+ // NSView instances are too long for our memory leak detector. > >+ // This is initialized by [mozView installTextInputHandler:aHandler] and > >+ // cleared by [mozView uninstallTextInputHandler]. > >+ mozilla::widget::TextInputHandler* mTextInputHandler; // [WEAK] > > The comment should be something like the following. (If I've > misunderstood your comment, please let me know.) > > Text input handler for mGeckoChild and us. Note that this is a weak > reference. Ideally this should be a strong reference. But a > ChildView object can live longer than the mGeckoChild that owns it. > And if mTextInputHandler were a strong reference, this would make it > difficult for Gecko's leak detector to detect leaked TextInputHandler > objects. Hm, it's not a problem that ChildView can live longer than the mGeckoChild. If ChildView must be destroyed before running the memory leak detector on tinderbox, we can reference TextInputHandler strongly. If so, we can log all behavior of NSTextInput protocol by TextInputHandler. If we could store all instances of TextInputHandler, we could release all instances from nsAppShell forcibly, though.

# updated for latest trunk.

Comment on attachment 529397 [details] [diff] [review] Patch part.6 v1.1 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events Review of attachment 529397 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me.

http://hg.mozilla.org/mozilla-central/rev/85f82a8d3ed8 http://hg.mozilla.org/mozilla-central/rev/b3a799ac62c9 http://hg.mozilla.org/mozilla-central/rev/ec6d99f512a5 http://hg.mozilla.org/mozilla-central/rev/eceb7533a5e7 http://hg.mozilla.org/mozilla-central/rev/b6a4eb4ec5d4

Sorry for the delay. I'll post all patches for this bug soon. This patch moves low level key event handling code from ChildView to TextInputHandler or TISInputSourceWrapper.

This patch moves keydown and insertText implementation to TextInputHandler. By this, IsPrintableChar(), ComputeGeckoKeyCodeFromChar(), IsNormalCharInputtingEvent(), IsModifierKey() and InsertTextAsCommittingComposition() can be protected. And I created KeyEventState struct for storing the handling key event state. It should be stored an array in the future.

Moves the plugin key event handlers to PluginTextInputHandler.

Moves keyup event handler and flagsChanged handler to TextInputHandler. By this patch, ActivatePluginTSMDocument() and HandleCarbonPluginKeyEvent() can be private and ConvertCocoaKeyEventToCarbonEvent() can be protected. This is final patch to move the code from ChildView to TextInputHandler. But I'll post some patches for cleaning up.

This is needed for next patch.

Comment on attachment 539109 [details] [diff] [review] Part.11 nsEvent.h should have forward declarations of classes and structs in nsGUIEvent.h Review of attachment 539109 [details] [diff] [review]: -----------------------------------------------------------------

part.13 fixes bug 524865 too.

Comment on attachment 539089 [details] [diff] [review] Part.7 Move low level key event handling code from ChildView to TextInputHandler and TISInputSourceWrapper This basically looks fine to me. But I've got a couple of questions, and (as usual) some suggested text changes to the comments, to make them easier to understand. + /** + * InitByLayoutID() initializes the keyboard layout by the layout ID. + * Some layout IDs are same as Mac's legacy keyboard layout IDs which were + * obsoleted. However, you don't need to look for the legacy layout ID + * when you add new keyboard layout support for this method. The number + * doesn't have any meaning now. + * + * @param aLayoutID An ID of keyboard layout. + * 0: US + * -18944: Greek + * 3: German + * 224: Swedish-Pro + * @param aOverride When you're testing, set TRUE. Otherwise, + * set FALSE. If you set TRUE, the instance + * always refers ANSI keyboard rather than actual + * keyboard. + */ + void InitByLayoutID(SInt32 aLayoutID, PRBool aOverride = PR_FALSE); The overall comment should read something like: InitByLayoutID() initializes the keyboard layout by layout ID. The KeyboardLayoutIdentifier (SInt32), used by Apple's now-deprecated Keyboard Layout Services, is no longer used by its replacement -- Apple's Text Input Source Services (TIS). All the layout IDs currently supported by InitByLayoutID() are backwards-compatible with the layout IDs used by Keyboard Layout Services. But there's no need to continue maintaining backwards compatibility as support for new IDs is added. The comment on @param aOverride should read something like: When testing set to TRUE, otherwise set to FALSE. When TRUE, we use an ANSI keyboard instead of the actual keyboard. Also, I think "aOverride" should be "aOverrideKeyboard". + /** + * InitKeyEvent() initializes aKeyEvent for aNativeKeyEvent. + * + * @param aNativeKeyEvent A native key event which you want to dispatch + * our key event for. + * @param aKeyEvent The result. I.e., initialized our key event + * which you want to dispatch for the + * aNativeKeyEvent. + */ + void InitKeyEvent(NSEvent *aNativeKeyEvent, nsKeyEvent& aKeyEvent); The comments on @param aNativeKeyEvent @param aKeyEvent should be something like: A native key event for which you want to dispatch a Gecko key event. The result -- a Gecko key event initialized from the native key event. + /** + * TranslateToString() computes the inputted text by the keyCode with the + * modifier state on the keyboard type. + * + * @param aKeyCode A native keyCode. + * @param aModifiers Combination of native modifier flags. + * @param aKbType A native Keyboard Type value. Typically, + * this is a result of ::LMGetKbdType(). + * @param aStr Result, i.e., inputted text. + * The result can be two or more characters. + * @return If succeeded, TRUE. Otherwise, FALSE. + * Even if TRUE, aStr can be empty string. + */ + PRBool TranslateToString(UInt32 aKeyCode, UInt32 aModifiers, + UInt32 aKbType, nsAString &aStr); The overall comment should be something like: TranslateToString() computes the inputted text from the native keyCode, modifier flags and keyboard type. + /** + * TranslateToChar() computes the inputted character by the keyCode with the + * modifier state on the keyboard type. + * If inputted text is two or more characters, this returns 0. + * + * @param aKeyCode A native keyCode. + * @param aModifiers Combination of native modifier flags. + * @param aKbType A native Keyboard Type value. Typically, + * this is a result of ::LMGetKbdType(). + * @return If succeeded and the result is one character, + * returns the charCode of it. Otherwise, + * returns 0. + */ + PRUint32 TranslateToChar(UInt32 aKeyCode, UInt32 aModifiers, UInt32 aKbdType); The overall comment should be something like: TranslateToChar() computes the inputted character from the native keyCode, modifier flags and keyboard type. If two or more characters would be input, this returns 0. + /** + * InitKeyPressEvent() initializes aKeyEvent for aNativeKeyEvent. + * Don't call this method when aKeyEvent isn't NS_KEY_PRESS. + * + * @param aNativeKeyEvent A native key event which you want to dispatch + * our key event for. + * @param aKeyEvent The result. I.e., initialized our key event + * which you want to dispatch for the + * aNativeKeyEvent. This must be NS_KEY_PRESS + * event. + */ + void InitKeyPressEvent(NSEvent *aNativeKeyEvent, nsKeyEvent& aKeyEvent); The comments on @param aNativeKeyEvent and @param aKeyEvent should be something like: A native key event for which you want to dispatch a Gecko key event. The result -- a Gecko key event initialized from the native key event. + PRPackedBool mOverride; Change the name to mOverrideKeyboard? + /** + * DispatchEvent() dispatches the aEvent on mWidget. + * + * @param aEvent An event which you want to dispatch. + * @return TRUE if the event is consumed by web contents + * or chrome contents. Otherwise, FALSE. + */ + PRBool DispatchEvent(nsGUIEvent& aEvent); The overall comment should be: DispatchEvent() dispatches aEvent on mWidget. + /** + * InitKeyEvent() initializes aKeyEvent for aNativeKeyEvent. + * + * @param aNativeKeyEvent A native key event which you want to dispatch + * our key event for. + * @param aKeyEvent The result. I.e., initialized our key event + * which you want to dispatch for the + * aNativeKeyEvent. + */ + void InitKeyEvent(NSEvent *aNativeKeyEvent, nsKeyEvent& aKeyEvent); The comments on @param aNativeKeyEvent and @param aKeyEvent should be: A native key event for which you want to dispatch a Gecko key event. The result -- a Gecko key event initialized from the native key event. + /** + * ComputeGeckoKeyCode() computes Gecko defined keyCode from the native + * keyCode or the characters. + * + * @param aNativeKeyCode A native keyCode. + * @param aCharacters Characters which are typed by the key. + * If the key inputs one or more characters, + * the result is computed from this. + * @return Gecko keyCode value for the aNativeKeyCode + * if the key doesn't input characters. + * Otherwise, for aCharacters. Or zero if the + * key inputs a Unicode character or the key + * cannot be mapped to a Gecko keyCode. + */ + static PRUint32 ComputeGeckoKeyCode(UInt32 aNativeKeyCode, + NSString *aCharacters); The comments on @param aCharacters and @return should be something like: Characters from the native key event (obtained using charactersIgnoringModifiers). If the native event contains one or more characters, the result is computed from this. Gecko keyCode value for aNativeKeyCode (if aCharacters is empty), otherwise for aCharacters (if aCharacters is non-empty). Or zero if aCharacters contains one or more Unicode characters, or if aNativeKeyCode cannot be mapped to a Gecko keyCode. + /** + * IsSpecialGeckoKey() checks whether the given native keycode is mapped to + * a special keyCode of Gecko. The special key means that they are not used + * for text input. + * + * @param aNativeKeyCode A native keycode. + * @return If the keycode is mapped to a special key, + * TRUE. Otherwise, FALSE. + */ + static PRBool IsSpecialGeckoKey(UInt32 aNativeKeyCode); The overall comment should be something like: IsSpecialGeckoKey() checks whether aNativeKeyCode is mapped to a special Gecko keycode. A key is "special" if it isn't used for text input. + KeyboardLayoutOverride mOverrideKeyboardLayout; Rename mOverrideKeyboardLayout to mKeyboardOverride? + aKeyEvent.isChar = PR_FALSE; // XXX not used in XP level + aKeyEvent.isChar = PR_TRUE; // this is not a special key XXX not used in XP What do "not used in XP level" and "not used in XP" mean? +void +TISInputSourceWrapper::InitKeyPressEvent(NSEvent *aNativeKeyEvent, + nsKeyEvent& aKeyEvent) +... aKeyEvent.keyCode is sometimes uninitialized ... though I'm not sure this matters.

Comment on attachment 539102 [details] [diff] [review] Part.8 Move keydown and insertText implementation to TextInputHandler This is basically fine. But you broke the null-widget checking in the code you moved from nsChildView.mm to TextInputHandler.mm! I can't r+ this patch until that problem's fixed. You generally replaced null checks on ChildView's mGeckoChild variable with null checks on TextInputHandlerBase's mView. But mView is a strong reference, so that's unneeded. What we really need to check is whether TextInputHandlerBase.mWidget has become null, and there's a Destroyed() method you can call to do this. You also added some kungFuDeathGrips (extra references) to TextInputHandlerBase.mWidget. But this unneeded, because it doesn't prevent the object mWidget points to from being "destroyed" (and TextInputHandlerBase.mWidget from being reset to null). I've also made some other comments and suggestions, and asked some questions. + /** + * mHandlingKeyEvent indicates what key event we are handling. While + * handling a native keydown event, we need to store the event for insertText, + * doCommandBySelector and various action message handlers of NSResponder + * such as [NSResponder insertNewline:sender]. + */ + KeyEventState mHandlingKeyEvent; I'd rename this mCurrentKeyEvent. +#ifndef NP_NO_CARBON + EventRecord carbonEvent; + if ([mView pluginEventModel] == NPEventModelCarbon) { + // XXX This makes keyup event for NS_KEY_DOWN. Is this correct?? + ConvertCocoaKeyEventToCarbonEvent(aNativeEvent, carbonEvent, PR_FALSE); + keydownEvent.pluginEvent = &carbonEvent; + } +#endif // #ifndef NP_NO_CARBON +#ifndef NP_NO_CARBON + if ([mView pluginEventModel] == NPEventModelCarbon) { + // XXX This makes keyup event for NS_KEY_PRESS. Is this correct?? + ConvertCocoaKeyEventToCarbonEvent(keyEvent, carbonEvent, PR_FALSE); + keypressEvent.pluginEvent = &carbonEvent; + } +#endif // #ifndef NP_NO_CARBON Why did you make these code snippets create a keyup EventRecord for an NS_KEY_DOWN and an NS_KEY_PRESS event? +PRBool +TextInputHandler::HandleKeyDownEvent(NSEvent* aNativeEvent) +{ + NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN; + + nsRefPtr<nsChildView> kungFuDeathGrip(mWidget); Get rid of the kungFuDeathGrip and in its place add: if (Destroyed()) return PR_FALSE; An extra reference to mWidget is unneeded -- it might get "destroyed" without getting deleted, and it's being "destroyed" is what we need to check for. The orginal code in nsChildView created a kungFuDeathGrip (an extra reference) on the native view (a ChildView object). But that, too, is unneeded -- we hold a strong reference to the native view in mView. +PRBool +TextInputHandler::HandleKeyDownEvent(NSEvent* aNativeEvent) +... + mHandlingKeyEvent.mKeyDownHandled = DispatchEvent(keydownEvent); + if (!mView) { + return mHandlingKeyEvent.KeyDownOrPressHandled(); + } "if (!mView)" should be "if (Destroyed())". + NSView<mozView>* view = mView; + nsAutoRetainCocoaObject kungFuDeathGrip(view); + PRBool cmEventHandled = DispatchEvent(contextMenuEvent); + [view maybeInitContextMenuTracking]; The kungFuDeathGrip on mView is unneeded -- we already hold a strong reference to it. +PRBool +TextInputHandler::HandleKeyDownEvent(NSEvent* aNativeEvent) +... + mHandlingKeyEvent.mKeyPressHandled = DispatchEvent(keypressEvent); + mHandlingKeyEvent.mKeyPressDispatched = PR_TRUE; + if (!mView) { + return mHandlingKeyEvent.KeyDownOrPressHandled(); + } "if (!mView)" should be "if (Destroyed())". +PRBool +TextInputHandler::HandleKeyDownEvent(NSEvent* aNativeEvent) +... + if (!mView) { + return mHandlingKeyEvent.KeyDownOrPressHandled(); + } "if (!mView)" should be "if (Destroyed())". +PRBool +TextInputHandler::HandleKeyDownEvent(NSEvent* aNativeEvent) +... + // Note: mView might have become null here. Don't count on it from here on. mWidget might have become null here. ... +void +TextInputHandler::InsertText(NSAttributedString *aAttrString) +{ + NS_OBJC_BEGIN_TRY_ABORT_BLOCK; Add the following here: if (Destroyed()) return; +void +TextInputHandler::InsertText(NSAttributedString *aAttrString) +{ + NS_OBJC_BEGIN_TRY_ABORT_BLOCK; +... + nsRefPtr<nsChildView> kungFuDeathGrip(mWidget); This kungFuDeathGrip on mWidget is unneeded, and won't prevent it being "destroyed" (i.e. set to null). +void +TextInputHandler::InsertText(NSAttributedString *aAttrString) +{ + NS_OBJC_BEGIN_TRY_ABORT_BLOCK; +... + // Note: mView might have become null here. Don't count on it from here on. mWidget might have become null here. ...

(Following up comment #71) Oops, I just realized that your kungFuDeathGrips on TextInputHandlerBase.mWidget *are* needed, since mWidget holds a strong reference to the TextInputHandler object. But you still need to replace the null checks on TextInputHandlerBase.mWidget with calls to TextInputHandlerBase.Destroyed().

> But you still need to replace the null checks on > TextInputHandlerBase.mWidget with calls to > TextInputHandlerBase.Destroyed(). But you still need to replace the null checks on TextInputHandlerBase.mView with calls to TextInputHandlerBase.Destroyed().

(In reply to comment #70) > Comment on attachment 539089 [details] [diff] [review] [review] > Part.7 Move low level key event handling code from ChildView to > TextInputHandler and TISInputSourceWrapper > + aKeyEvent.isChar = PR_FALSE; // XXX not used in XP level > > + aKeyEvent.isChar = PR_TRUE; // this is not a special key XXX not used in > XP > > What do "not used in XP level" and "not used in XP" mean? isChar isn't used in XP level code (e.g., layout, content). It's referred only by nsDOMUIEvent::GetIsChar() but isChar isn't set by other platforms and it's not in DOM3 Events spec. So, we could remove it completely after this bug. http://mxr.mozilla.org/mozilla-central/ident?i=isChar > +void > +TISInputSourceWrapper::InitKeyPressEvent(NSEvent *aNativeKeyEvent, > + nsKeyEvent& aKeyEvent) > +... > > aKeyEvent.keyCode is sometimes uninitialized ... though I'm not sure > this matters. Oh, right. The keyCode must be initialized by InitKeyEvent() before InitKeyPressEvent() is called. I changed as so in this patch.

(In reply to comment #71) > Comment on attachment 539102 [details] [diff] [review] [review] > Part.8 Move keydown and insertText implementation to TextInputHandler > > +#ifndef NP_NO_CARBON > + EventRecord carbonEvent; > + if ([mView pluginEventModel] == NPEventModelCarbon) { > + // XXX This makes keyup event for NS_KEY_DOWN. Is this correct?? > + ConvertCocoaKeyEventToCarbonEvent(aNativeEvent, carbonEvent, > PR_FALSE); > + keydownEvent.pluginEvent = &carbonEvent; > + } > +#endif // #ifndef NP_NO_CARBON > > +#ifndef NP_NO_CARBON > + if ([mView pluginEventModel] == NPEventModelCarbon) { > + // XXX This makes keyup event for NS_KEY_PRESS. Is this correct?? > + ConvertCocoaKeyEventToCarbonEvent(keyEvent, carbonEvent, PR_FALSE); > + keypressEvent.pluginEvent = &carbonEvent; > + } > +#endif // #ifndef NP_NO_CARBON > > Why did you make these code snippets create a keyup EventRecord for an > NS_KEY_DOWN and an NS_KEY_PRESS event? Because current trunk build did so. If you want me to fix this in this bug, I'll fix it on part.14. I wouldn't change the logic at moving the methods from nsChildView to TextInputHandler. Separating to two or more patches makes clear the reason of change (especially, intentional vs. unintentional).

(In reply to comment #75) >> Why did you make these code snippets create a keyup EventRecord for >> an NS_KEY_DOWN and an NS_KEY_PRESS event? > > Because current trunk build did so. How can you tell? I couldn't when I looked at current trunk code :-)

(In reply to comment #76) > (In reply to comment #75) > > >> Why did you make these code snippets create a keyup EventRecord for > >> an NS_KEY_DOWN and an NS_KEY_PRESS event? > > > > Because current trunk build did so. > > How can you tell? I couldn't when I looked at current trunk code :-) Ah, I was confused by [NSEvent type] vs keyType. I'll update it.

Comment on attachment 546978 [details] [diff] [review] Patch part.10 Move keyup and flagsChanged handler to TextInputHnadler This looks fine, but I have one suggestion: +void +TextInputHandler::DispatchKeyEventForFlagsChanged(NSEvent* aNativeEvent, + PRBool aDispatchKeyDown) +{ + NS_OBJC_BEGIN_TRY_ABORT_BLOCK; It's probably best to add a Destroyed() check here.

http://hg.mozilla.org/mozilla-central/rev/0eba68e1c6ea http://hg.mozilla.org/mozilla-central/rev/c2766c695ca7 http://hg.mozilla.org/mozilla-central/rev/3fcccd893550 http://hg.mozilla.org/mozilla-central/rev/dd8c12b66e9f http://hg.mozilla.org/mozilla-central/rev/8f3c6449758f http://hg.mozilla.org/mozilla-central/rev/8cb9a61ef251 http://hg.mozilla.org/mozilla-central/rev/dd5648b22802 Thank you Steven, for your reviews for a lot of patches! About 2000 lines are removed from nsChildView.mm (part.1 - 13).