[Chromoting] The Windows IT2Me host gets any new text items it finds on the clipboard.

remoting/host/clipboard_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/clipboard.h"
 
 #include <windows.h>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 57 matching lines @@
 
   void SetData(UINT uFormat, HANDLE hMem) {
     if (!opened_) {
       NOTREACHED();
       return;
     }
     // The caller must not close the handle that ::SetClipboardData returns.
     ::SetClipboardData(uFormat, hMem);
   }
 
+  // The caller must not free the handle. The caller should lock the handle,
+  // copy the clipboard data, and unlock the handle. All this must be done
+  // before this ScopedClipboard is destroyed.
+  HANDLE GetData(UINT uFormat) {

[dcheng, 2012/05/11 20:28:31]

u_format or just format.

[alexeypa (please no reviews), 2012/05/11 20:42:37]

|uFormat| -> |format|

[simonmorris, 2012/05/11 22:12:58]

Done.

[simonmorris, 2012/05/11 22:12:58]

Done.

+    if (!opened_) {
+      NOTREACHED();
+      return NULL;
+    }
+    return ::GetClipboardData(uFormat);
+  }
+
  private:
   bool opened_;
 };
 
+// A scoper class that wraps a lock on a global memory object.
+template <typename T>
+class ScopedGlobalLock {

[dcheng, 2012/05/11 20:28:31]

Can you use ScopedHGlobalfrom base/win?

[simonmorris, 2012/05/11 22:12:58]

Done.

+ public:
+  ScopedGlobalLock(HGLOBAL hGlobalMem) : hGlobalMem_(hGlobalMem) {

[alexeypa (please no reviews), 2012/05/11 20:42:37]

explicit

[alexeypa (please no reviews), 2012/05/11 20:42:37]

hGlobalMem -> global_mem. Same thing for the data member. I don't think we use
Camel notation elsewhere even if we deal with Win32 APIs.

[simonmorris, 2012/05/11 22:12:58]

Removed code.

[simonmorris, 2012/05/11 22:12:58]

Removed code.

+    pointer_ = ::GlobalLock(hGlobalMem_);

[alexeypa (please no reviews), 2012/05/11 20:42:37]

Validate that the handle is valid. The check should be in the Release build too.
I guess...

[simonmorris, 2012/05/11 22:12:58]

Removed code.

+  }
+
+  ~ScopedGlobalLock() {
+    if (pointer_) {
+      ::GlobalUnlock(hGlobalMem_);
+    }
+  }
+
+  T GetPointer() {

[alexeypa (please no reviews), 2012/05/11 20:42:37]

This is the only place where T is needed. WDYT about making this only method
templated but not the whole class?

[simonmorris, 2012/05/11 22:12:58]

Removed code.

+    return reinterpret_cast<T>(pointer_);
+  }
+
+ private:
+  HGLOBAL hGlobalMem_;

[alexeypa (please no reviews), 2012/05/11 20:42:37]

Make it non-copyable.

[simonmorris, 2012/05/11 22:12:58]

Removed code.

+  LPVOID pointer_;
+};
+
+typedef BOOL (WINAPI AddClipboardFormatListenerFn)(HWND);
+typedef BOOL (WINAPI RemoveClipboardFormatListenerFn)(HWND);
+
 }  // namespace
 
 namespace remoting {
 
 class ClipboardWin : public Clipboard {
  public:
   ClipboardWin();
 
   virtual void Start() OVERRIDE;
   virtual void InjectClipboardEvent(
       const protocol::ClipboardEvent& event) OVERRIDE;
   virtual void Stop() OVERRIDE;
 
  private:
   static LRESULT CALLBACK WndProc(HWND hwmd, UINT msg, WPARAM wParam,
                                   LPARAM lParam);
 
   static bool RegisterWindowClass();
 
+  void OnClipboardUpdate();

[alexeypa (please no reviews), 2012/05/11 20:42:37]

nit: I tend to put static members last. I'm not sure what the style guide says
about it but it feels like the static members  are looked at less often.

[simonmorris, 2012/05/11 22:12:58]

Done.

+
   HWND hwnd_;
 
   DISALLOW_COPY_AND_ASSIGN(ClipboardWin);
 };
 
 ClipboardWin::ClipboardWin() : hwnd_(NULL) {
 }
 
 void ClipboardWin::Start() {
   if (!RegisterWindowClass()) {
     LOG(FATAL) << "Couldn't register clipboard window class.";
     return;
   }
   hwnd_ = ::CreateWindow(kWindowClassName,
                          kWindowName,
                          0, 0, 0, 0, 0,
                          HWND_MESSAGE,
                          NULL,
                          base::GetModuleFromAddress(&WndProc),
                          this);
   if (!hwnd_) {
     LOG(FATAL) << "Couldn't create clipboard window.";
     return;
   }
+
+  AddClipboardFormatListenerFn* add =
+      reinterpret_cast<AddClipboardFormatListenerFn*>(
+          ::GetProcAddress(::GetModuleHandle(L"user32.dll"),

[alexeypa (please no reviews), 2012/05/11 20:42:37]

GetModuleHandle will increment the library reference counter. Not fatal in this
case but generally bad. You can use GetModuleHandleEx to avoid incrementing the
counter.

You can also group two GetProcAddress calls and cache the pointers in the class
because GetProcAddress calls are expensive.

[simonmorris, 2012/05/11 22:12:58]

<http://msdn.microsoft.com/en-us/library/windows/desktop/ms683199(v=vs.85).aspx>
says
GetModuleHandle doesn't increment the reference counter.
 
Done.

[alexeypa (please no reviews), 2012/05/11 23:49:17]

Oops. Yes, indeed. It is actually GetModuleHandleEx who can increment the
counter.

+          "AddClipboardFormatListener"));
+  if (add) {
+    if (!(*add)(hwnd_)) {
+      LOG(WARNING) << "AddClipboardFormatListener() failed: " << GetLastError();
+    }
+  } else {
+    LOG(WARNING) << "AddClipboardFormatListener() couldn't be loaded.";

[alexeypa (please no reviews), 2012/05/11 20:42:37]

This will pollute logs. We should complain in the logs only if the OS version is
less than Vista. 

PS. And if this is the case - we should probably CHECK instead.

[simonmorris, 2012/05/11 22:12:58]

Done (assuming you meant "greater than Vista".)
The user might not have cared about clipboard functionality...

[alexeypa (please no reviews), 2012/05/11 23:49:17]

Yes, I meant that.
It does not matter. It is the same sort of sanity check as AV-ing on a NULL
pointer is. It means that something went terribly wrong and now we don't know
what we are doing.

[simonmorris, 2012/05/12 00:16:56]

I don't agree. It means something went surprisingly wrong, and we know exactly
what we are doing - not listening for items placed on the clipboard. I don't
think some missing functionality is a good reason to crash the entire host.

[alexeypa (please no reviews), 2012/05/15 15:34:59]

Yes, I agree with this part.

I was concerned about that while we know what we are doing we don't know what is
going wrong. I can think of three possibilities: the OS installation is
corrupted; some one is injected into our process, we survived the ice age and
the API is being deprecated. There can be other reasons.

I'm not entirely convinced that this particular "something is very wrong" is
worth of killing the whole process, so if you think it is fine to keep it alive
- I'm OK with it.

+  }
 }
 
 void ClipboardWin::Stop() {
   if (hwnd_) {
+    RemoveClipboardFormatListenerFn* remove =
+        reinterpret_cast<RemoveClipboardFormatListenerFn*>(
+            ::GetProcAddress(::GetModuleHandle(L"user32.dll"),
+                             "RemoveClipboardFormatListener"));
+    if (remove) {
+      (*remove)(hwnd_);

[alexeypa (please no reviews), 2012/05/11 20:42:37]

This should be called only if the Add function was called before.

[simonmorris, 2012/05/11 22:12:58]

Done.

+    }
     ::DestroyWindow(hwnd_);
     hwnd_ = NULL;
   }
 }
 
 void ClipboardWin::InjectClipboardEvent(
     const protocol::ClipboardEvent& event) {
   if (!hwnd_) {
     return;
   }
@@ skipping 22 matching lines @@
       reinterpret_cast<LPWSTR>(::GlobalLock(text_global));
   memcpy(text_global_locked, text.data(), text.size() * sizeof(WCHAR));
   text_global_locked[text.size()] = (WCHAR)0;
   ::GlobalUnlock(text_global);
 
   clipboard.SetData(CF_UNICODETEXT, text_global);
 }
 
 LRESULT CALLBACK ClipboardWin::WndProc(HWND hwnd, UINT msg, WPARAM wParam,
                                        LPARAM lParam) {
+  if (msg == WM_CREATE) {
+    CREATESTRUCT* cs = reinterpret_cast<CREATESTRUCT*>(lParam);

[dcheng, 2012/05/11 20:28:31]

avar or a_var instead of aVar.

[simonmorris, 2012/05/11 22:12:58]

Done.

+    ::SetWindowLongPtr(hwnd,
+                       GWLP_USERDATA,
+                       reinterpret_cast<LONG_PTR>(cs->lpCreateParams));
+    return 0;
+  }
+  ClipboardWin* clipboardWin =

[alexeypa (please no reviews), 2012/05/11 20:42:37]

|clipboard_win|, or, maybe, simply |clipboard|

[simonmorris, 2012/05/11 22:12:58]

Done.

+      reinterpret_cast<ClipboardWin*>(::GetWindowLongPtr(hwnd, GWLP_USERDATA));
+  switch (msg) {
+    case WM_CLIPBOARDUPDATE:
+      clipboardWin->OnClipboardUpdate();
+      return 0;
+  }
   return ::DefWindowProc(hwnd, msg, wParam, lParam);
 }
 
 bool ClipboardWin::RegisterWindowClass() {
   // This method is only called on the desktop thread, so it doesn't matter
   // that the following test is not thread-safe.
   static bool registered = false;
   if (registered) {
     return true;
   }
 
   WNDCLASSEX window_class;
   base::win::InitializeWindowClass(
       kWindowClassName,
       base::win::WrappedWindowProc<WndProc>,
       0, 0, 0, NULL, NULL, NULL, NULL, NULL,
       &window_class);
   if (!::RegisterClassEx(&window_class)) {
     return false;
   }
 
   registered = true;
   return true;
 }
 
+void ClipboardWin::OnClipboardUpdate() {
+  if (!hwnd_) {

[alexeypa (please no reviews), 2012/05/11 20:42:37]

Make it a DCHECK. I don't think there is a way to call it with |hwnd_| being
NULL.

[simonmorris, 2012/05/11 22:12:58]

Done.

+    return;
+  }
+
+  if (::IsClipboardFormatAvailable(CF_UNICODETEXT)) {
+    string16 text;
+    // Add a scope, so that we keep the clipboard open for as short a time as
+    // possible.
+    {
+      ScopedClipboard clipboard;
+      if (!clipboard.Init(hwnd_)) {
+        LOG(WARNING) << "Couldn't open the clipboard." << GetLastError();
+        return;
+      }
+
+      HGLOBAL text_global = clipboard.GetData(CF_UNICODETEXT);
+      if (!text_global) {
+        LOG(WARNING) << "Couldn't get data from the clipboard: "
+                     << GetLastError();
+        return;
+      }
+
+      ScopedGlobalLock<LPWSTR> text_lock(text_global);
+      if (!text_lock.GetPointer()) {
+        LOG(WARNING) << "Couldn't lock clipboard data: " << GetLastError();
+        return;
+      }
+      string16 text_temp(text_lock.GetPointer());

[dcheng, 2012/05/11 20:28:31]

You can just use assign instead of swapping.

[simonmorris, 2012/05/11 22:12:58]

Done.

+      text.swap(text_temp);

[alexeypa (please no reviews), 2012/05/11 20:42:37]

I don't think text.swap buys anything here. text can be assigned directly. I.e.
text.assign(...) should do.

[simonmorris, 2012/05/11 22:12:58]

Done.

+    }
+
+    protocol::ClipboardEvent event;
+    event.set_mime_type(kMimeTypeTextUtf8);
+    event.set_data(UTF16ToUTF8(text));
+
+    // TODO(simonmorris): Send the event to the client.
+  }
+}
+
 scoped_ptr<Clipboard> Clipboard::Create() {
   return scoped_ptr<Clipboard>(new ClipboardWin());
 }
 
 }  // namespace remoting