891316 - Sort out external message handlers

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Make widget::MsgResult for the result of external message handlers

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Make a method for calling all external message handlers

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=16c03fb40623

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Make widget::MSGResult struct and use it in nsWindow

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 Use widget::MSGResult in widget::WindowHook

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.3 Use widget::MSGResult in widget::MouseScrollHandler

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.4 Use widget::MSGResult at nsWindow::ProcessMessageForPlugin

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.5 Use widget::MSGResult in widget::IMEHandler

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.6 Use widget::MSGResult in nsIMEHandler

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.7 Refactor nsIMM32Handler::OnIME*()

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.8 Refactor other nsIMM32Handler::On*()

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 16

[Jim Mathies [:jimm]]

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?

Comment 17

[Jim Mathies [:jimm]]

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.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 20

[Jim Mathies [:jimm]]

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

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/012a553952ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/f606c524f2fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1e77493ee6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e443a45b66c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ddda634900
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae37b6b30321
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf0e61abaae
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bac560565ec

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/012a553952ae
https://hg.mozilla.org/mozilla-central/rev/f606c524f2fc
https://hg.mozilla.org/mozilla-central/rev/7f1e77493ee6
https://hg.mozilla.org/mozilla-central/rev/8e443a45b66c
https://hg.mozilla.org/mozilla-central/rev/d9ddda634900
https://hg.mozilla.org/mozilla-central/rev/ae37b6b30321
https://hg.mozilla.org/mozilla-central/rev/2bf0e61abaae
https://hg.mozilla.org/mozilla-central/rev/2bac560565ec