Last Comment Bug 519972 - Move NSTextInput implementation to nsCocoaTextInputHandler
: Move NSTextInput implementation to nsCocoaTextInputHandler
Status: RESOLVED FIXED
[approved-patches-landed][not-ready-f...
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
:
:
Mentors:
Depends on: 805357
Blocks: 477291 524865 631165
  Show dependency treegraph
 
Reported: 2009-10-01 07:32 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
Modified: 2012-10-25 01:01 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part.1 v1.0 Remove IME method wrapper (17.25 KB, patch)
2010-08-02 19:49 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
jaas: approval2.0+
Details | Diff | Splinter Review
Patch part.2 v1.0 Move plugin key/IME handlers to nsCocoaIMEHandler #1 (35.58 KB, patch)
2011-02-15 00:58 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.2 v1.1 Move plugin key/IME handlers to nsCocoaIMEHandler #1 (34.52 KB, patch)
2011-02-15 18:15 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.3 v1.0 Move dispatching text event code to nsCocoaIMEHandler (19.71 KB, patch)
2011-02-16 00:27 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.4 v1.0 Move marked text handling to nsCocoaIMEHandler (22.56 KB, patch)
2011-02-16 02:54 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.2 v1.1 Move dispatching text event code to nsCocoaIMEHandler (19.75 KB, patch)
2011-02-18 20:57 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.3 v1.2 Move marked text handling to nsCocoaIMEHandler (23.34 KB, patch)
2011-02-18 20:58 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.4 v1.0 Move NSTextInput implementation to nsCocoaIMEHandler (except insertText) (15.99 KB, patch)
2011-02-18 21:00 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.0 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler (61.85 KB, patch)
2011-02-18 21:02 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.1 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler (89.88 KB, patch)
2011-02-23 06:22 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.2 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler (89.88 KB, patch)
2011-02-23 10:07 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.2 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler (90.04 KB, patch)
2011-02-23 10:08 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.6 v1.0 Move NSTextInput::insertText and ChildView::processKeyDownEvent to nsCocoaTextInputHandler (42.23 KB, patch)
2011-02-23 10:11 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.0 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename classes in nsCocoaTextInput.h to modern names (59.03 KB, patch)
2011-03-03 19:43 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.1 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename classes in nsCocoaTextInput.h to modern names (59.03 KB, patch)
2011-03-03 21:13 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.1 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename classes in nsCocoaTextInput.h to modern names (59.05 KB, patch)
2011-03-03 21:16 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.6 v1.3 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper (88.26 KB, patch)
2011-03-03 21:18 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.7 v1.1 Move [NSTextInput insertText] and [ChildView processKeyDownEvent] to TextInputHandler (42.63 KB, patch)
2011-03-03 22:47 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.8 v1.0 Move plugin text input handling to PluginTextInputHandler (58.51 KB, patch)
2011-03-03 23:40 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.8 v1.1 Move plugin text input handling to PluginTextInputHandler (59.78 KB, patch)
2011-03-04 00:27 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.6 v1.4 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper (88.45 KB, patch)
2011-03-08 21:30 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.7 v1.2 Move [NSTextInput insertText] and [ChildView processKeyDownEvent] to TextInputHandler (42.59 KB, patch)
2011-03-08 21:39 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.2 v1.2 Move text event dispatcher to nsCocoaIMEHandler (19.68 KB, patch)
2011-03-10 18:48 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.3 v1.3 Move marked text handler to nsCocoaIMEHandler (23.32 KB, patch)
2011-03-10 18:50 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.4 v1.1 Move NSTextInput handler to nsCocoaIMEHandler (except insertText) (15.95 KB, patch)
2011-03-10 18:52 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.5 v1.2 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename nsCocoa*Hander to *InputHandler with namespace (58.92 KB, patch)
2011-03-10 18:56 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.6 v1.5 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper (88.42 KB, patch)
2011-03-10 18:58 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.7 v1.3 Move [NSTextInput insertText] and [ChildView processKeyDownEvent] to TextInputHandler (42.85 KB, patch)
2011-03-10 19:00 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.8 v1.2 Move plugin key/IME handling code to PluginTextInputHandler (59.95 KB, patch)
2011-03-10 19:01 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.3 v1.4 Move marked text handler to nsCocoaIMEHandler (23.43 KB, patch)
2011-03-10 21:23 PST, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.2 v1.3 Move text event dispatcher to nsCocoaIMEHandler (mq) (16.93 KB, patch)
2011-04-15 14:43 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.3 v1.5 Move marked text handler to nsCocoaIMEHandler (mq) (18.20 KB, patch)
2011-04-15 14:44 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.4 v1.2 Move NSTextInput handler to nsCocoaIMEHandler (except insertText) (mq) (15.15 KB, patch)
2011-04-15 14:45 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.3 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename nsCocoa*Hander to *InputHandler with namespace (mq) (41.33 KB, patch)
2011-04-15 14:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.6 v1.0 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events (46.43 KB, patch)
2011-04-15 15:02 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.5 v1.3 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename nsCocoa*Hander to *InputHandler with namespace (mq) (41.33 KB, patch)
2011-05-01 16:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.6 v1.1 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events (46.72 KB, patch)
2011-05-01 16:53 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Part.7 Move low level key event handling code from ChildView to TextInputHandler and TISInputSourceWrapper (88.06 KB, patch)
2011-06-13 19:29 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Part.8 Move keydown and insertText implementation to TextInputHandler (46.14 KB, patch)
2011-06-13 20:14 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review-
Details | Diff | Splinter Review
Part.9 Move plugin key event handling code to PluginTextInputHandler (61.33 KB, patch)
2011-06-13 20:19 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Part.10 Move keyup and flagsChanged handler to TextInputHnadler (22.08 KB, patch)
2011-06-13 20:44 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Part.11 nsEvent.h should have forward declarations of classes and structs in nsGUIEvent.h (1.29 KB, patch)
2011-06-13 20:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
roc: review+
Details | Diff | Splinter Review
Part.12 Move duplicated static methods in nsChildView.mm and TextInputHandler.mm to nsCocoaUtils (31.70 KB, patch)
2011-06-13 20:54 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Part.13 Log behavior of TextInputHandler (82.33 KB, patch)
2011-06-13 23:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.7 Move low level key event handling code from ChildView to TextInputHandler and TISInputSourceWrapper (mq) (77.11 KB, patch)
2011-07-19 17:48 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.8 Move keydown and insertText implementation to TextInputHandler (38.57 KB, patch)
2011-07-19 18:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
no flags Details | Diff | Splinter Review
Patch part.8 Move keydown and insertText implementation to TextInputHandler (38.72 KB, patch)
2011-07-19 19:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.9 Move plugin key event handling code to PluginTextInputHandler (53.21 KB, patch)
2011-07-19 19:46 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.10 Move keyup and flagsChanged handler to TextInputHnadler (16.99 KB, patch)
2011-07-19 20:18 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.12 Move duplicated static methods in nsChildView.mm and TextInputHandler.mm to nsCocoaUtils (19.68 KB, patch)
2011-07-20 01:51 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review
Patch part.13 Log behavior of TextInputHandler (62.72 KB, patch)
2011-07-20 01:52 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28))
smichaud: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2009-10-01 07:32:12 PDT
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.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-08-02 19:49:06 PDT
Created attachment 462310 [details] [diff] [review]
Patch part.1 v1.0 Remove IME method wrapper

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 2 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-08-03 14:35:37 PDT
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.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2010-08-08 02:29:46 PDT
http://hg.mozilla.org/mozilla-central/rev/a0ea32596abd
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-11 20:28:25 PST
I should fix this bug before bug 631165.
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-15 00:58:47 PST
Created attachment 512413 [details] [diff] [review]
Patch part.2 v1.0 Move plugin key/IME handlers to nsCocoaIMEHandler #1
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-15 18:15:12 PST
Created attachment 512678 [details] [diff] [review]
Patch part.2 v1.1 Move plugin key/IME handlers to nsCocoaIMEHandler #1
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-16 00:27:31 PST
Created attachment 512729 [details] [diff] [review]
Patch part.3 v1.0 Move dispatching text event code to nsCocoaIMEHandler
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-16 02:54:02 PST
Created attachment 512762 [details] [diff] [review]
Patch part.4 v1.0 Move marked text handling to nsCocoaIMEHandler
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-18 20:57:32 PST
Created attachment 513695 [details] [diff] [review]
Patch part.2 v1.1 Move dispatching text event code to nsCocoaIMEHandler

changed the order of patches.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-18 20:58:49 PST
Created attachment 513696 [details] [diff] [review]
Patch part.3 v1.2 Move marked text handling to nsCocoaIMEHandler
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-18 21:00:51 PST
Created attachment 513697 [details] [diff] [review]
Patch part.4 v1.0 Move NSTextInput implementation to nsCocoaIMEHandler (except insertText)
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-18 21:02:21 PST
Created attachment 513698 [details] [diff] [review]
Patch part.5 v1.0 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-18 21:14:51 PST
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 14 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-18 21:18:59 PST
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 15 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-18 21:23:32 PST
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()
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-23 06:22:34 PST
Created attachment 514476 [details] [diff] [review]
Patch part.5 v1.1 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-23 10:07:24 PST
Created attachment 514525 [details] [diff] [review]
Patch part.5 v1.2 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-23 10:08:09 PST
Created attachment 514526 [details] [diff] [review]
Patch part.5 v1.2 Move low-level keyboard handling code to nsTISInputSource or nsCocoaTextInputHandler
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-02-23 10:11:29 PST
Created attachment 514528 [details] [diff] [review]
Patch part.6 v1.0 Move NSTextInput::insertText and ChildView::processKeyDownEvent to nsCocoaTextInputHandler
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-03 19:43:46 PST
Created attachment 516788 [details] [diff] [review]
Patch part.5 v1.0 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename classes in nsCocoaTextInput.h to modern names

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.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-03 21:13:19 PST
Created attachment 516803 [details] [diff] [review]
Patch part.5 v1.1 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename classes in nsCocoaTextInput.h to modern names

fix a mistake.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-03 21:16:19 PST
Created attachment 516804 [details] [diff] [review]
Patch part.5 v1.1 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename classes in nsCocoaTextInput.h to modern names

oops, sorry for the spam.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-03 21:18:05 PST
Created attachment 516805 [details] [diff] [review]
Patch part.6 v1.3 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-03 22:47:50 PST
Created attachment 516822 [details] [diff] [review]
Patch part.7 v1.1 Move [NSTextInput insertText] and [ChildView processKeyDownEvent] to TextInputHandler
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-03 23:40:52 PST
Created attachment 516829 [details] [diff] [review]
Patch part.8 v1.0 Move plugin text input handling to PluginTextInputHandler
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-04 00:27:43 PST
Created attachment 516832 [details] [diff] [review]
Patch part.8 v1.1 Move plugin text input handling to PluginTextInputHandler
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-08 21:30:17 PST
Created attachment 517976 [details] [diff] [review]
Patch part.6 v1.4 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper
Comment 28 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-08 21:39:12 PST
Created attachment 517980 [details] [diff] [review]
Patch part.7 v1.2 Move [NSTextInput insertText] and [ChildView processKeyDownEvent] to TextInputHandler
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 18:48:12 PST
Created attachment 518610 [details] [diff] [review]
Patch part.2 v1.2 Move text event dispatcher to nsCocoaIMEHandler
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 18:50:45 PST
Created attachment 518611 [details] [diff] [review]
Patch part.3 v1.3 Move marked text handler to nsCocoaIMEHandler
Comment 31 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 18:52:07 PST
Created attachment 518612 [details] [diff] [review]
Patch part.4 v1.1 Move NSTextInput handler to nsCocoaIMEHandler (except insertText)
Comment 32 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 18:56:15 PST
Created attachment 518613 [details] [diff] [review]
Patch part.5 v1.2 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename nsCocoa*Hander to *InputHandler with namespace
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 18:58:39 PST
Created attachment 518614 [details] [diff] [review]
Patch part.6 v1.5 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 19:00:10 PST
Created attachment 518615 [details] [diff] [review]
Patch part.7 v1.3 Move [NSTextInput insertText] and [ChildView processKeyDownEvent] to TextInputHandler
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 19:01:00 PST
Created attachment 518617 [details] [diff] [review]
Patch part.8 v1.2 Move plugin key/IME handling code to PluginTextInputHandler
Comment 36 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-10 21:23:12 PST
Created attachment 518642 [details] [diff] [review]
Patch part.3 v1.4 Move marked text handler to nsCocoaIMEHandler
Comment 37 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-03-30 23:00:41 PDT
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.
Comment 38 Steven Michaud [:smichaud] (Retired) 2011-03-31 08:28:21 PDT
> So, I'd like Steven to review the current review requested patches.

I should be able to review them tomorrow (Friday) or Monday.
Comment 39 Steven Michaud [:smichaud] (Retired) 2011-04-05 14:03:07 PDT
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 40 Steven Michaud [:smichaud] (Retired) 2011-04-05 14:09:26 PDT
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 41 Steven Michaud [:smichaud] (Retired) 2011-04-05 14:55:43 PDT
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 42 Steven Michaud [:smichaud] (Retired) 2011-04-05 14:57:01 PDT
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 43 Steven Michaud [:smichaud] (Retired) 2011-04-05 14:58:18 PDT
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 44 Steven Michaud [:smichaud] (Retired) 2011-04-05 15:56:52 PDT
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 45 Steven Michaud [:smichaud] (Retired) 2011-04-06 08:11:56 PDT
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."
Comment 46 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-15 14:43:59 PDT
Created attachment 526387 [details] [diff] [review]
Patch part.2 v1.3 Move text event dispatcher to nsCocoaIMEHandler (mq)
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-15 14:44:36 PDT
Created attachment 526388 [details] [diff] [review]
Patch part.3 v1.5 Move marked text handler to nsCocoaIMEHandler (mq)
Comment 48 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-15 14:45:17 PDT
Created attachment 526390 [details] [diff] [review]
Patch part.4 v1.2 Move NSTextInput handler to nsCocoaIMEHandler (except insertText) (mq)
Comment 49 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-15 14:46:14 PDT
Created attachment 526391 [details] [diff] [review]
Patch part.5 v1.3 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename nsCocoa*Hander to *InputHandler with namespace (mq)
Comment 50 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-15 15:02:07 PDT
Created attachment 526398 [details] [diff] [review]
Patch part.6 v1.0 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events

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.
Comment 51 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-20 17:27:30 PDT
Steven: ping
Comment 52 Steven Michaud [:smichaud] (Retired) 2011-04-20 17:37:47 PDT
> 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.
Comment 53 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-20 20:06:05 PDT
Okay, don't mind. I have a lot of other work, you too. Thank you for explanation about your schedule.
Comment 54 Steven Michaud [:smichaud] (Retired) 2011-04-26 10:15:45 PDT
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 55 Steven Michaud [:smichaud] (Retired) 2011-04-26 13:38:43 PDT
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.
Comment 56 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-04-29 16:41:20 PDT
(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.
Comment 57 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-05-01 16:52:36 PDT
Created attachment 529396 [details] [diff] [review]
Patch part.5 v1.3 Make TextInputHandlerBase and PluginTextInputHandler classes, and rename nsCocoa*Hander to *InputHandler with namespace (mq)

# updated for latest trunk.
Comment 58 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-05-01 16:53:49 PDT
Created attachment 529397 [details] [diff] [review]
Patch part.6 v1.1 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events
Comment 59 Steven Michaud [:smichaud] (Retired) 2011-05-06 15:48:05 PDT
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.
Comment 61 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 19:29:47 PDT
Created attachment 539089 [details] [diff] [review]
Part.7 Move low level key event handling code from ChildView to TextInputHandler and TISInputSourceWrapper

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.
Comment 62 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 20:14:19 PDT
Created attachment 539102 [details] [diff] [review]
Part.8 Move keydown and insertText implementation to TextInputHandler

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.
Comment 63 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 20:19:29 PDT
Created attachment 539103 [details] [diff] [review]
Part.9 Move plugin key event handling code to PluginTextInputHandler

Moves the plugin key event handlers to PluginTextInputHandler.
Comment 64 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 20:44:10 PDT
Created attachment 539108 [details] [diff] [review]
Part.10 Move keyup and flagsChanged handler to TextInputHnadler

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.
Comment 65 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 20:46:15 PDT
Created attachment 539109 [details] [diff] [review]
Part.11 nsEvent.h should have forward declarations of classes and structs in nsGUIEvent.h

This is needed for next patch.
Comment 66 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 20:54:57 PDT
Created attachment 539110 [details] [diff] [review]
Part.12 Move duplicated static methods in nsChildView.mm and TextInputHandler.mm to nsCocoaUtils
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-13 21:39:32 PDT
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]:
-----------------------------------------------------------------
Comment 68 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 23:21:22 PDT
Created attachment 539131 [details] [diff] [review]
Part.13 Log behavior of TextInputHandler
Comment 69 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-06-13 23:22:32 PDT
part.13 fixes bug 524865 too.
Comment 70 Steven Michaud [:smichaud] (Retired) 2011-07-19 10:21:02 PDT
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 71 Steven Michaud [:smichaud] (Retired) 2011-07-19 15:19:58 PDT
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. ...
Comment 72 Steven Michaud [:smichaud] (Retired) 2011-07-19 15:25:31 PDT
(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().
Comment 73 Steven Michaud [:smichaud] (Retired) 2011-07-19 15:32:13 PDT
> 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().
Comment 74 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-19 17:48:29 PDT
Created attachment 546955 [details] [diff] [review]
Patch part.7 Move low level key event handling code from ChildView to TextInputHandler and TISInputSourceWrapper (mq)

(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.
Comment 75 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-19 18:38:40 PDT
Created attachment 546964 [details] [diff] [review]
Patch part.8 Move keydown and insertText implementation to TextInputHandler

(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).
Comment 76 Steven Michaud [:smichaud] (Retired) 2011-07-19 18:43:50 PDT
(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 :-)
Comment 77 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-19 19:16:32 PDT
(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 78 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-19 19:46:01 PDT
Created attachment 546967 [details] [diff] [review]
Patch part.8 Move keydown and insertText implementation to TextInputHandler
Comment 79 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-19 19:46:52 PDT
Created attachment 546968 [details] [diff] [review]
Patch part.9 Move plugin key event handling code to PluginTextInputHandler
Comment 80 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-19 20:18:01 PDT
Created attachment 546978 [details] [diff] [review]
Patch part.10 Move keyup and flagsChanged handler to TextInputHnadler
Comment 81 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-20 01:51:46 PDT
Created attachment 547008 [details] [diff] [review]
Patch part.12 Move duplicated static methods in nsChildView.mm and TextInputHandler.mm to nsCocoaUtils
Comment 82 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2011-07-20 01:52:36 PDT
Created attachment 547009 [details] [diff] [review]
Patch part.13 Log behavior of TextInputHandler
Comment 83 Steven Michaud [:smichaud] (Retired) 2011-07-20 14:11:42 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.