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.

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

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

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

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

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