Closed Bug 519972 Opened 15 years ago Closed 13 years ago

Move NSTextInput implementation to nsCocoaTextInputHandler

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Whiteboard: [approved-patches-landed][not-ready-for-cedar])

Attachments

(13 files, 38 obsolete files)

Patch part.1 v1.0 Remove IME method wrapper
17.25 KB, patch
smichaud
: review+
jaas
: approval2.0+
Details | Diff | Splinter Review
Patch part.2 v1.3 Move text event dispatcher to nsCocoaIMEHandler (mq)
16.93 KB, patch
Details | Diff | Splinter Review
Patch part.3 v1.5 Move marked text handler to nsCocoaIMEHandler (mq)
18.20 KB, patch
Details | Diff | Splinter Review
Patch part.4 v1.2 Move NSTextInput handler to nsCocoaIMEHandler (except insertText) (mq)
15.15 KB, patch
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
Details | Diff | Splinter Review
Patch part.6 v1.1 Make TextInputHandler grabbed by nsChildView and its methods which dispatch events
46.72 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Part.11 nsEvent.h should have forward declarations of classes and structs in nsGUIEvent.h
1.29 KB, patch
roc
: review+
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
Details | Diff | Splinter Review
Patch part.8 Move keydown and insertText implementation to TextInputHandler
38.72 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Patch part.9 Move plugin key event handling code to PluginTextInputHandler
53.21 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Patch part.10 Move keyup and flagsChanged handler to TextInputHnadler
16.99 KB, patch
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
smichaud
: review+
Details | Diff | Splinter Review
Patch part.13 Log behavior of TextInputHandler
62.72 KB, patch
smichaud
: review+
Details | Diff | Splinter Review 
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.
No longer blocks: 519974
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.
Attachment #462310 - Flags: review?(smichaud)
Attachment #462310 - Flags: review?(smichaud) → review+
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.
Attachment #462310 - Flags: approval2.0?
Attachment #462310 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Whiteboard: Please don't close this bug by landing part.1 patch
http://hg.mozilla.org/mozilla-central/rev/a0ea32596abd
Keywords: checkin-needed
Whiteboard: Please don't close this bug by landing part.1 patch → Landed: part.1
I should fix this bug before bug 631165.
Blocks: 631165
Status: NEW → ASSIGNED
changed the order of patches.
Attachment #512678 - Attachment is obsolete: true
Attachment #512729 - Attachment is obsolete: true

        Attachment #512762 -
        Attachment is obsolete: true

        Attachment #513695 -
        Attachment is obsolete: true

        Attachment #513695 -
        Attachment is obsolete: false

        Attachment #513696 -
        Attachment is patch: true

        Attachment #513696 -
        Attachment mime type: application/octet-stream → text/plain

    
    

    
  
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().

        Attachment #513695 -
        Flags: review?(smichaud)

    
    

    
  
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().

        Attachment #513696 -
        Flags: review?(smichaud)

    
    

    
  
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()

        Attachment #513697 -
        Flags: review?(smichaud)
Whiteboard: Landed: part.1 → Landed: part.1 [approved-patches-landed]

    
    

    
  


  
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.

        Attachment #514526 -
        Attachment is obsolete: true

        Attachment #514528 -
        Attachment is obsolete: true

        Attachment #516788 -
        Flags: review?(smichaud)

        Attachment #516805 -
        Attachment description: Patch part.6 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper → Patch part.6 v1.3 Move low level keyboard handling code to TextInputHandler and TISInputSourceWrapper

    
    

    
  


  

        Attachment #513695 -
        Attachment is obsolete: true

        Attachment #518610 -
        Flags: review?(smichaud)

        Attachment #513695 -
        Flags: review?(smichaud)

    
    

    
  


  

        Attachment #513696 -
        Attachment is obsolete: true

        Attachment #518611 -
        Flags: review?(smichaud)

        Attachment #513696 -
        Flags: review?(smichaud)

        Attachment #518610 -
        Attachment description: Patch part.2 Move text event dispatcher to nsCocoaIMEHandler → Patch part.2 v1.2 Move text event dispatcher to nsCocoaIMEHandler

        Attachment #518611 -
        Attachment description: Patch part.3 Move marked text handler to nsCocoaIMEHandler → Patch part.3 v1.3 Move marked text handler to nsCocoaIMEHandler

        Attachment #513697 -
        Attachment is obsolete: true

        Attachment #513697 -
        Flags: review?(smichaud)

    
    

    
  


  

        Attachment #518611 -
        Attachment is obsolete: true

        Attachment #518642 -
        Flags: review?(smichaud)

        Attachment #518611 -
        Flags: review?(smichaud)

    
  
Whiteboard: Landed: part.1 [approved-patches-landed] → Landed: part.1 [approved-patches-landed][not-ready-for-cedar]

    
    

    
  
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");

        Attachment #518610 -
        Flags: review?(smichaud) → review+

    
    

    
  
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."

        Attachment #518612 -
        Flags: review?(smichaud) → review+

    
    

    
  
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.

        Attachment #518612 -
        Flags: review+ → review?(smichaud)

    
    

    
  
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."

        Attachment #518642 -
        Flags: review?(smichaud) → review+

    
    

    
  
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?"

        Attachment #518612 -
        Flags: review?(smichaud) → review+

    
    

    
  
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."

        Attachment #518613 -
        Flags: review?(smichaud) → review+

    
    

    
  


  
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.

        Attachment #518614 -
        Attachment is obsolete: true

        Attachment #518615 -
        Attachment is obsolete: true

        Attachment #518617 -
        Attachment is obsolete: true

        Attachment #526398 -
        Flags: review?(smichaud)

    
    

    
  
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.

    
    

    
  


  

        Attachment #526391 -
        Attachment is obsolete: true

        Attachment #526398 -
        Attachment is obsolete: true

        Attachment #529397 -
        Flags: review?(smichaud)

        Attachment #526398 -
        Flags: review?(smichaud)

    
    

    
  
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.

        Attachment #529397 -
        Flags: review?(smichaud) → review+
Whiteboard: Landed: part.1 [approved-patches-landed][not-ready-for-cedar] → Landed: part.1-6 [approved-patches-landed][not-ready-for-cedar]

    
    

    
  


  
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.

        Attachment #539089 -
        Flags: review?(smichaud)

    
    

    
  


  
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.

        Attachment #539102 -
        Flags: review?(smichaud)

    
    

    
  


  
Moves the plugin key event handlers to PluginTextInputHandler.

        Attachment #539103 -
        Flags: review?(smichaud)

    
    

    
  


  
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.

        Attachment #539108 -
        Flags: review?(smichaud)

    
    

    
  
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]:
-----------------------------------------------------------------

        Attachment #539109 -
        Flags: review?(roc) → review+

    
    

    
  
part.13 fixes bug 524865 too.
Blocks: 524865

    
    

    
  
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.

        Attachment #539089 -
        Flags: review?(smichaud) → review+

    
    

    
  
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. ...

        Attachment #539102 -
        Flags: review?(smichaud) → review-

    
    

    
  
(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.

        Attachment #539089 -
        Attachment is obsolete: true

    
    

    
  


  
(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).

        Attachment #539102 -
        Attachment is obsolete: true

        Attachment #546964 -
        Flags: review?(smichaud)

    
    

    
  
(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.

        Attachment #539103 -
        Attachment is obsolete: true

        Attachment #539103 -
        Flags: review?(smichaud)

    
    

    
  


  

        Attachment #539108 -
        Attachment is obsolete: true

        Attachment #546978 -
        Flags: review?(smichaud)

        Attachment #539108 -
        Flags: review?(smichaud)

    
    

    
  


  

        Attachment #539131 -
        Attachment is obsolete: true

        Attachment #547009 -
        Flags: review?(smichaud)

        Attachment #539131 -
        Flags: review?(smichaud)

        Attachment #546967 -
        Flags: review?(smichaud) → review+

        Attachment #546968 -
        Flags: review?(smichaud) → review+

    
    

    
  
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.

        Attachment #546978 -
        Flags: review?(smichaud) → review+

        Attachment #547008 -
        Flags: review?(smichaud) → review+

        Attachment #547009 -
        Flags: review?(smichaud) → review+
Whiteboard: Landed: part.1-6 [approved-patches-landed][not-ready-for-cedar] → [approved-patches-landed][not-ready-for-cedar]
Target Milestone: --- → mozilla8

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: