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

remoting/host/clipboard_win.cc

Index: remoting/host/clipboard_win.cc
diff --git a/remoting/host/clipboard_win.cc b/remoting/host/clipboard_win.cc
index d812d2f86ba71531f61e339cf41a2de64abf771c..0a4c8d24b4733a9ad08155cb69139992913f5db6 100644
--- a/remoting/host/clipboard_win.cc
+++ b/remoting/host/clipboard_win.cc
@@ -75,10 +75,47 @@ class ScopedClipboard {
     ::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 {
@@ -98,6 +135,8 @@ class ClipboardWin : public Clipboard {
 
   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);
@@ -122,10 +161,29 @@ void ClipboardWin::Start() {
     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;
   }
@@ -168,6 +226,20 @@ void ClipboardWin::InjectClipboardEvent(
 
 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);
 }
 
@@ -193,6 +265,46 @@ bool ClipboardWin::RegisterWindowClass() {
   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());
 }