Closed Bug 891316 Opened 7 years ago Closed 7 years ago

Sort out external message handlers

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(8 files, 2 obsolete files)

6.94 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.67 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.66 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.36 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.23 KB, patch
jimm
: review+
Details | Diff | Splinter Review
14.98 KB, patch
jimm
: review+
Details | Diff | Splinter Review
25.31 KB, patch
jimm
: review+
Details | Diff | Splinter Review
8.02 KB, patch
jimm
: review+
Details | Diff | Splinter Review
nsWindow::ProcessMessage() calls external handlers of WindowHook, IMEHandler, MouseScrollHandler and handler for Plugin. This should be moved to a method. Additionally, let's make the result simpler.
Comment on attachment 773164 [details] [diff] [review]
part.1 Make widget::MSGResult struct and use it in nsWindow

This change will help bug 887695. It will need to call external event handlers from ProcessKeyDownMessage() because it will be called directly from nsAppShell.

And also, each external message handler has some different arguments for its out arguments. We should sort out the out arguments to a struct.

This patch maeks widget::MSGResult for it.
Attachment #773164 - Flags: review?(jmathies)
Comment on attachment 773172 [details] [diff] [review]
part.6 Use widget::MSGResult in nsIMEHandler

This patch also changes OnMouseEvent() and OnKeyDownEvent(). They are now handles MSGResult themselves.
Attachment #773172 - Flags: review?(jmathies)
Comment on attachment 773173 [details] [diff] [review]
part.7 Refactor nsIMM32Handler::OnIME*()

This patch changes OnIME*() handler whose caller (nsIMM32Handler::ProcessMessage()) will always return true.

Therefore, these methods always return true. And the old return value is set to aResult.mConsumed.
Attachment #773173 - Flags: review?(jmathies)
Comment on attachment 773174 [details] [diff] [review]
part.8 Refactor other nsIMM32Handler::On*()

This patch changes the remaining On*() methods of nsIMM32Handler.

Although, result of OnInputLangChange() is always ignored, it should return false for consistency behavior with other methods.


OnChar() sometimes consume the WM_CHAR message. Only when it consumes, the other message handler shouldn't handle the message anymore. Otherwise, should be handled. Therefore, this method returns same value as aResult.mConsumed.

OnCharOnPlugin() should be handled by plugin's message handler too. Therefore, it always returns false and doesn't mark as consumed.
Attachment #773174 - Flags: review?(jmathies)
Comment on attachment 773164 [details] [diff] [review]
part.1 Make widget::MSGResult struct and use it in nsWindow

Review of attachment 773164 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +4422,5 @@
>    return true;
>  }
>  
> +bool
> +nsWindow::ProcessMessageWithExternalHandlers(UINT aMessage,

nit, not a fan of this method name, how about ExternalHandlerProcessMessage()?

::: widget/windows/nsWindowDefs.h
@@ +236,5 @@
> +  {
> +  }
> +
> +private:
> +  LRESULT mDummyResult;

Maybe this will be answered when I look at the followup patches, but what is mDummyResult? Maybe add a comment here explaining this? Is there a better name?
Attachment #773166 - Flags: review?(jmathies) → review+
Attachment #773168 - Flags: review?(jmathies) → review+
Attachment #773169 - Flags: review?(jmathies) → review+
Attachment #773170 - Flags: review?(jmathies) → review+
Attachment #773172 - Flags: review?(jmathies) → review+
Attachment #773173 - Flags: review?(jmathies) → review+
Comment on attachment 773174 [details] [diff] [review]
part.8 Refactor other nsIMM32Handler::On*()

Review of attachment 773174 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsIMM32Handler.cpp
@@ +372,5 @@
>   * message handlers
>   ****************************************************************************/
>  
>  bool
>  nsIMM32Handler::OnInputLangChange(nsWindow* aWindow,

Can we get rid of the return result here, doesn't look like we use it, and this method always returns false.
Attachment #773174 - Flags: review?(jmathies) → review+
(In reply to Jim Mathies [:jimm] from comment #16)
> Comment on attachment 773164 [details] [diff] [review]
> part.1 Make widget::MSGResult struct and use it in nsWindow
> 
> Review of attachment 773164 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +4422,5 @@
> >    return true;
> >  }
> >  
> > +bool
> > +nsWindow::ProcessMessageWithExternalHandlers(UINT aMessage,
> 
> nit, not a fan of this method name, how about
> ExternalHandlerProcessMessage()?

Sure.

> 
> ::: widget/windows/nsWindowDefs.h
> @@ +236,5 @@
> > +  {
> > +  }
> > +
> > +private:
> > +  LRESULT mDummyResult;
> 
> Maybe this will be answered when I look at the followup patches, but what is
> mDummyResult? Maybe add a comment here explaining this? Is there a better
> name?

It's necessary when it's used outside wnd proc. Hmm, mResultPlaceholder is better?
(In reply to Jim Mathies [:jimm] from comment #17)
> Comment on attachment 773174 [details] [diff] [review]
> part.8 Refactor other nsIMM32Handler::On*()
> 
> Review of attachment 773174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsIMM32Handler.cpp
> @@ +372,5 @@
> >   * message handlers
> >   ****************************************************************************/
> >  
> >  bool
> >  nsIMM32Handler::OnInputLangChange(nsWindow* aWindow,
> 
> Can we get rid of the return result here, doesn't look like we use it, and
> this method always returns false.

Okay. Thank you for the quick review!
Attachment #773164 - Flags: review?(jmathies) → review+
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> It's necessary when it's used outside wnd proc. Hmm, mResultPlaceholder is
> better?

How about mDefaultResult?
(In reply to Jim Mathies [:jimm] from comment #20)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> > It's necessary when it's used outside wnd proc. Hmm, mResultPlaceholder is
> > better?
> 
> How about mDefaultResult?

Sounds good, thanks!
You need to log in before you can comment on or make changes to this bug.