Sort out external message handlers

RESOLVED FIXED in mozilla25

Status

()

Core
Widget: Win32
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks: 1 bug)

Trunk
mozilla25
x86_64
Windows 8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 2 obsolete attachments)

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.
Created attachment 772647 [details] [diff] [review]
part.1 Make widget::MsgResult for the result of external message handlers
Created attachment 772648 [details] [diff] [review]
part.2 Make a method for calling all external message handlers
Created attachment 773164 [details] [diff] [review]
part.1 Make widget::MSGResult struct and use it in nsWindow
Attachment #772647 - Attachment is obsolete: true
Attachment #772648 - Attachment is obsolete: true
Created attachment 773166 [details] [diff] [review]
part.2 Use widget::MSGResult in widget::WindowHook
Created attachment 773168 [details] [diff] [review]
part.3 Use widget::MSGResult in widget::MouseScrollHandler
Created attachment 773169 [details] [diff] [review]
part.4 Use widget::MSGResult at nsWindow::ProcessMessageForPlugin
Created attachment 773170 [details] [diff] [review]
part.5 Use widget::MSGResult in widget::IMEHandler
Created attachment 773172 [details] [diff] [review]
part.6 Use widget::MSGResult in nsIMEHandler
Created attachment 773173 [details] [diff] [review]
part.7 Refactor nsIMM32Handler::OnIME*()
Created attachment 773174 [details] [diff] [review]
part.8 Refactor other nsIMM32Handler::On*()
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)
Attachment #773166 - Flags: review?(jmathies)
Attachment #773168 - Flags: review?(jmathies)
Attachment #773169 - Flags: review?(jmathies)
Attachment #773170 - 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 16

5 years ago
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?

Updated

5 years ago
Attachment #773166 - Flags: review?(jmathies) → review+

Updated

5 years ago
Attachment #773168 - Flags: review?(jmathies) → review+

Updated

5 years ago
Attachment #773169 - Flags: review?(jmathies) → review+

Updated

5 years ago
Attachment #773170 - Flags: review?(jmathies) → review+

Updated

5 years ago
Attachment #773172 - Flags: review?(jmathies) → review+

Updated

5 years ago
Attachment #773173 - Flags: review?(jmathies) → review+

Comment 17

5 years ago
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!

Updated

5 years ago
Attachment #773164 - Flags: review?(jmathies) → review+

Comment 20

5 years ago
(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.