Replace PeekMessage for TSF awareness

base/message_pump_win.cc

Index: base/message_pump_win.cc
diff --git a/base/message_pump_win.cc b/base/message_pump_win.cc
index 2b2a10e07534afcf7c7dd8de14567bd4c794d5cd..2f027b564dab301798aed4ee24300e501f860a5f 100644
--- a/base/message_pump_win.cc
+++ b/base/message_pump_win.cc
@@ -4,14 +4,92 @@
 
 #include "base/message_pump_win.h"
 
+#include <imm.h>
 #include <math.h>
+#include <msctf.h>
+#include <Windows.h>
 
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/process_util.h"
+#include "base/win/metro.h"
+#include "base/win/scoped_comptr.h"
 #include "base/win/wrapped_window_proc.h"
 
+namespace base {
+
+static const wchar_t kWndClass[] = L"Chrome_MessagePumpWindow";
+
+// Message sent to get an additional time slice for pumping (processing) another
+// task (a series of such messages creates a continuous task pump).
+static const int kMsgHaveWork = WM_USER + 1;
+
+class TextServicesBridge {

[rvargas (doing something else), 2012/08/18 02:42:07]

In any case, this doesn't belong to this file. It should be on a dedicated file.

[yoichio, 2012/08/20 10:16:48]

Done.

+ public:
+  TfClientId client_id;
+  bool is_initialized;
+  base::win::ScopedComPtr<ITfThreadMgr> thread_mgr;
+  base::win::ScopedComPtr<ITfMessagePump> message_pump;
+  base::win::ScopedComPtr<ITfKeystrokeMgr> keystroke_mgr;

[jar (doing other things), 2012/08/16 18:21:40]

nit: members come after methods.  
Members should have a postfix "_" since they are in a class.  Members should all
be private.

[yoichio, 2012/08/20 10:16:48]

Done.

+  TextServicesBridge()
+      : client_id(TF_CLIENTID_NULL), is_initialized(true) {

[jar (doing other things), 2012/08/16 18:21:40]

nit: one initializer per line (once you had to wrap).

[rvargas (doing something else), 2012/08/18 02:42:07]

There is no point in having is_initialized in the initialization list,
especially setting it to true.

[yoichio, 2012/08/20 10:16:48]

Done.

+    is_initialized &= SUCCEEDED(thread_mgr.CreateInstance(CLSID_TF_ThreadMgr));

[rvargas (doing something else), 2012/08/18 02:42:07]

There is no guarantee so far that a MessagePump can use com.

[yoichio, 2012/08/20 10:16:48]

So I added ScopedComInitialiser.

+    if (!thread_mgr)

[rvargas (doing something else), 2012/08/18 02:42:07]

This is a case of checking two different signals for fn success. You should use
is_initialized (the actual return value from the call) at this point.

[yoichio, 2012/08/20 10:16:48]

Done.

[yoichio, 2012/08/20 10:16:48]

Done.

+      return;
+
+    is_initialized &= SUCCEEDED(message_pump.QueryFrom(thread_mgr));
+    is_initialized &= SUCCEEDED(keystroke_mgr.QueryFrom(thread_mgr));
+    // When activate succeeded, |client_id| is set valid value.
+    is_initialized &= SUCCEEDED(thread_mgr->Activate(&client_id));
+  }
+  ~TextServicesBridge() {
+    if (thread_mgr && client_id != TF_CLIENTID_NULL)

[rvargas (doing something else), 2012/08/18 02:42:07]

if (is_initialized_)

[yoichio, 2012/08/20 10:16:48]

Done.

+      thread_mgr->Deactivate();
+  }
+
+  bool ForwardKeyMessageForTSF(const MSG& msg) {

[rvargas (doing something else), 2012/08/18 02:42:07]

This method requires a description.

[yoichio, 2012/08/20 10:16:48]

Done.

+    if (!is_initialized)
+      return false;
+
+    if(msg.message == WM_KEYDOWN) {
+      BOOL eaten = FALSE;
+      HRESULT hr = keystroke_mgr->TestKeyDown(msg.wParam, msg.lParam, &eaten);
+      if (FAILED(hr) && !eaten)
+        return false;
+      eaten = FALSE;
+      hr = keystroke_mgr->KeyDown(msg.wParam, msg.lParam, &eaten);
+      return (SUCCEEDED(hr) && eaten);
+    }
+
+    if(msg.message == WM_KEYUP) {
+      BOOL eaten = FALSE;
+      HRESULT hr = keystroke_mgr->TestKeyUp(msg.wParam, msg.lParam, &eaten);
+      if (FAILED(hr) && !eaten)
+        return false;
+      eaten = FALSE;
+      hr = keystroke_mgr->KeyUp(msg.wParam, msg.lParam, &eaten);
+      return (SUCCEEDED(hr) && eaten);
+    }
+
+    return false;
+  }
+
+  bool PeekMessageForTSF(MSG* msg, HWND hwnd, UINT wMsgFilterMin,
+                         UINT wMsgFilterMax, UINT wRemoveMsg) {

[jar (doing other things), 2012/08/16 18:21:40]

nit: one arg per line.

[yoichio, 2012/08/20 10:16:48]

Done.

+    if (!is_initialized)

[rvargas (doing something else), 2012/08/18 02:42:07]

braces

[yoichio, 2012/08/20 10:16:48]

Done.

+      // FallBack.
+      return !!::PeekMessage(msg, hwnd, wMsgFilterMin, wMsgFilterMax,

[rvargas (doing something else), 2012/08/18 02:42:07]

If this method is replacing a windows API it should have the same signature
(including the return type)

[yoichio, 2012/08/20 10:16:48]

Done.

+                             wRemoveMsg);
+
+    BOOL result = FALSE;
+    if(FAILED(message_pump->PeekMessage(msg, hwnd, wMsgFilterMin, wMsgFilterMax,
+                                        wRemoveMsg, &result)))

[jar (doing other things), 2012/08/16 18:21:40]

nit: use curly braces since the "if" line took two lines.

[yoichio, 2012/08/20 10:16:48]

Done.

+      result = FALSE;
+    return !!result;
+  }
+};

[jar (doing other things), 2012/08/16 18:21:40]

nit: please add  DISALLOW_COPY_AND_ASSIGN(...);

Also, it appears that the associated header file in this CL is missing this from
all the classes.  Please add it, or add a comment explaining why it can't be
done (usually, because of use in an STL container... but I don't think that
happens to these classes).

[yoichio, 2012/08/20 10:16:48]

Done.

+
 namespace {
 
 enum MessageLoopProblems {
@@ -23,14 +101,6 @@ enum MessageLoopProblems {
 
 }  // namespace
 
-namespace base {
-
-static const wchar_t kWndClass[] = L"Chrome_MessagePumpWindow";
-
-// Message sent to get an additional time slice for pumping (processing) another
-// task (a series of such messages creates a continuous task pump).
-static const int kMsgHaveWork = WM_USER + 1;
-
 //-----------------------------------------------------------------------------
 // MessagePumpWin public:
 
@@ -97,9 +167,14 @@ int MessagePumpWin::GetCurrentDelay() const {
 
 MessagePumpForUI::MessagePumpForUI() : instance_(NULL) {
   InitMessageWnd();
+
+  if (base::win::IsTsfAwareRequired())
+    text_services_bridge_.reset(new TextServicesBridge);

[jar (doing other things), 2012/08/16 18:21:40]

Once you set the text_service_bridge_ to have a pointer, I think you should stop
calling IsTsfAwareRequired() and start just testing text_service_bridge_.get()
for non-NULLness.

[yoichio, 2012/08/20 10:16:48]

Done.

 }
 
 MessagePumpForUI::~MessagePumpForUI() {
+  if (base::win::IsTsfAwareRequired())
+    text_services_bridge_.reset(NULL);

[jar (doing other things), 2012/08/16 18:21:40]

Why don't you count on scoped ptr handling this delete? Is the order critical?? 
...and if so, adding a comment would be helpful.

Also, even if the order is critical (delete before lines 178), why not just do
the reset() without the test?  

I'm assuming TSF awareness doesn't change during the lifetime of the message
loop.

[yoichio, 2012/08/20 10:16:48]

Oops! I overlooked scoped_ptr's basic function.

   DestroyWindow(message_hwnd_);
   UnregisterClass(kWndClass, instance_);
 }
@@ -182,7 +257,7 @@ void MessagePumpForUI::PumpOutPendingPaintMessages() {
   int peek_count;
   for (peek_count = 0; peek_count < kMaxPeekCount; ++peek_count) {
     MSG msg;
-    if (!PeekMessage(&msg, NULL, 0, 0, PM_REMOVE | PM_QS_PAINT))
+    if (!ProcessPeekMessage(&msg, NULL, 0, 0, PM_REMOVE | PM_QS_PAINT))

[jar (doing other things), 2012/08/16 18:21:40]

Why are you asking for help from TSF in this case?

This is *ONLY* supposed to handle PAINT messages, and not handle generic other
messages.  Is there another queue that must be service here, and is hidden of
fis TSF land?

[yoichio, 2012/08/20 10:16:48]

I don't know what kind of message is peeked from ITfMessagePump::PeekMessage and
not so I just replaced all.

       break;
     ProcessMessageHelper(msg);
     if (state_->should_quit)  // Handle WM_QUIT.
@@ -304,7 +379,8 @@ void MessagePumpForUI::WaitForWork() {
     MSG msg = {0};
     DWORD queue_status = GetQueueStatus(QS_MOUSE);
     if (HIWORD(queue_status) & QS_MOUSE &&
-       !PeekMessage(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST, PM_NOREMOVE)) {
+        !ProcessPeekMessage(&msg, NULL, WM_MOUSEFIRST, WM_MOUSELAST,
+                            PM_NOREMOVE)) {
       WaitMessage();
     }
     return;
@@ -361,7 +437,7 @@ bool MessagePumpForUI::ProcessNextWindowsMessage() {
     sent_messages_in_queue = true;
 
   MSG msg;
-  if (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
+  if (ProcessPeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
     return ProcessMessageHelper(msg);
 
   return sent_messages_in_queue;
@@ -387,12 +463,20 @@ bool MessagePumpForUI::ProcessMessageHelper(const MSG& msg) {
 
   WillProcessMessage(msg);
 
-  if (state_->dispatcher) {
-    if (!state_->dispatcher->Dispatch(msg))
-      state_->should_quit = true;
-  } else {
-    TranslateMessage(&msg);
-    DispatchMessage(&msg);
+  bool is_key_dispatched_by_tsf = false;
+  if (base::win::IsTsfAwareRequired()) {

[jar (doing other things), 2012/08/16 18:21:40]

Please change to:
if (text_services_bridge_.get())

[yoichio, 2012/08/20 10:16:48]

Done.

+    is_key_dispatched_by_tsf =
+        text_services_bridge_->ForwardKeyMessageForTSF(msg);

[rvargas (doing something else), 2012/08/18 02:42:07]

PreProcessMessage(msg) ?

+  }
+
+  if (!is_key_dispatched_by_tsf) {
+    if (state_->dispatcher) {
+      if (!state_->dispatcher->Dispatch(msg))
+        state_->should_quit = true;
+    } else {
+      TranslateMessage(&msg);
+      DispatchMessage(&msg);
+    }
   }
 
   DidProcessMessage(msg);
@@ -416,10 +500,11 @@ bool MessagePumpForUI::ProcessPumpReplacementMessage() {
   // This is to ensure that these messages are peeked out by the OS modal loop.
   if (MessageLoop::current()->os_modal_loop()) {
     // We only peek out WM_PAINT and WM_TIMER here for reasons mentioned above.
-    have_message = PeekMessage(&msg, NULL, WM_PAINT, WM_PAINT, PM_REMOVE) ||
-                   PeekMessage(&msg, NULL, WM_TIMER, WM_TIMER, PM_REMOVE);
+    have_message =
+        ProcessPeekMessage(&msg, NULL, WM_PAINT, WM_PAINT, PM_REMOVE) ||
+        ProcessPeekMessage(&msg, NULL, WM_TIMER, WM_TIMER, PM_REMOVE);
   } else {
-    have_message = (0 != PeekMessage(&msg, NULL, 0, 0, PM_REMOVE));
+    have_message = ProcessPeekMessage(&msg, NULL, 0, 0, PM_REMOVE);
   }
 
   DCHECK(!have_message || kMsgHaveWork != msg.message ||
@@ -441,6 +526,17 @@ bool MessagePumpForUI::ProcessPumpReplacementMessage() {
   return ProcessMessageHelper(msg);
 }
 
+bool MessagePumpForUI::ProcessPeekMessage(
+    MSG* pMsg, HWND hwnd, UINT wMsgFilterMin, UINT wMsgFilterMax,

[jar (doing other things), 2012/08/16 18:21:40]

nit: one arg per line, wrap at paren is preferred.

[yoichio, 2012/08/20 10:16:48]

Done.

+    UINT wRemoveMsg)
+{
+  if (base::win::IsTsfAwareRequired())

[jar (doing other things), 2012/08/16 18:21:40]

Please change to:
if (text_services_bridge_.get())

[rvargas (doing something else), 2012/08/18 02:42:07]

braces

[yoichio, 2012/08/20 10:16:48]

Done.

[yoichio, 2012/08/20 10:16:48]

Done.

+    return text_services_bridge_->PeekMessageForTSF(
+        pMsg, hwnd, wMsgFilterMin, wMsgFilterMax, wRemoveMsg);
+
+  return !!::PeekMessage(pMsg, hwnd, wMsgFilterMin, wMsgFilterMax, wRemoveMsg);
+}
+
 //-----------------------------------------------------------------------------
 // MessagePumpForIO public: