Clean up some confusing naming of ClipboardLock. It isn't a lock, and the way it was written previous made it seem like it was supposed to be some sort of lock. It was just to manage acquiring the clipboard (which involves some Windows clipboard lock).

Review URL: http://codereview.chromium.org/9745

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@5122 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 38 insertions, 44 deletions

diff --git a/base/clipboard_win.cc b/base/clipboard_win.cc
index d8581cc..affb520 100644
--- a/base/clipboard_win.cc
+++ b/base/clipboard_win.cc
@@ -16,29 +16,27 @@
 
 namespace {
 
-// A small object to ensure we close the clipboard after opening it.
-class ClipboardLock {
+// A scoper to manage acquiring and automatically releasing the clipboard.
+class ScopedClipboard {
  public:
-  ClipboardLock() : we_own_the_lock_(false) { }
+  ScopedClipboard() : opened_(false) { }
 
-  ~ClipboardLock() {
-    if (we_own_the_lock_)
+  ~ScopedClipboard() {
+    if (opened_)
       Release();
   }
 
   bool Acquire(HWND owner) {
-    // We shouldn't be calling this if we already own the clipboard lock.
-    DCHECK(!we_own_the_lock_);
+    const int kMaxAttemptsToOpenClipboard = 5;
 
-    // We already have the lock.  We don't want to stomp on the other use.
-    if (we_own_the_lock_)
+    if (opened_) {
+      NOTREACHED();
       return false;
+    }
 
-    const int kMaxAttemptsToOpenClipboard = 5;
-
-    // Attempt to acquire the clipboard lock.  This may fail if another process
-    // currently holds the lock.  We're willing to try a few times in the hopes
-    // of acquiring it.
+    // Attempt to open the clipboard, which will acquire the Windows clipboard
+    // lock.  This may fail if another process currently holds this lock.
+    // We're willing to try a few times in the hopes of acquiring it.
     //
     // This turns out to be an issue when using remote desktop because the
     // rdpclip.exe process likes to read what we've written to the clipboard and
@@ -48,18 +46,17 @@ class ClipboardLock {
     //
     // In fact, we believe we'll only spin this loop over remote desktop.  In
     // normal situations, the user is initiating clipboard operations and there
-    // shouldn't be lock contention.
+    // shouldn't be contention.
 
     for (int attempts = 0; attempts < kMaxAttemptsToOpenClipboard; ++attempts) {
+      // If we didn't manage to open the clipboard, sleep a bit and be hopeful.
+      if (attempts != 0)
+        ::Sleep(5);
+
       if (::OpenClipboard(owner)) {
-        we_own_the_lock_ = true;
-        return we_own_the_lock_;
+        opened_ = true;
+        return true;
       }
-
-      // Having failed, we yield our timeslice to other processes. ::Yield seems
-      // to be insufficient here, so we sleep for 5 ms.
-      if (attempts < (kMaxAttemptsToOpenClipboard - 1))
-        ::Sleep(5);
     }
 
     // We failed to acquire the clipboard.
@@ -67,19 +64,16 @@ class ClipboardLock {
   }
 
   void Release() {
-    // We should only be calling this if we already own the clipboard lock.
-    DCHECK(we_own_the_lock_);
-
-    // We we don't have the lock, there is nothing to release.
-    if (!we_own_the_lock_)
-      return;
-
-    ::CloseClipboard();
-    we_own_the_lock_ = false;
+    if (opened_) {
+      ::CloseClipboard();
+      opened_ = false;
+    } else {
+      NOTREACHED();
+    }
   }
 
  private:
-  bool we_own_the_lock_;
+  bool opened_;
 };
 
 LRESULT CALLBACK ClipboardOwnerWndProc(HWND hwnd,
@@ -154,8 +148,8 @@ void Clipboard::WriteObjects(const ObjectMap& objects) {
 }
 
 void Clipboard::WriteObjects(const ObjectMap& objects, ProcessHandle process) {
-  ClipboardLock lock;
-  if (!lock.Acquire(clipboard_owner_))
+  ScopedClipboard clipboard;
+  if (!clipboard.Acquire(clipboard_owner_))
     return;
 
   ::EmptyClipboard();
@@ -390,8 +384,8 @@ void Clipboard::ReadText(std::wstring* result) const {
   result->clear();
 
   // Acquire the clipboard.
-  ClipboardLock lock;
-  if (!lock.Acquire(clipboard_owner_))
+  ScopedClipboard clipboard;
+  if (!clipboard.Acquire(clipboard_owner_))
     return;
 
   HANDLE data = ::GetClipboardData(CF_UNICODETEXT);
@@ -411,8 +405,8 @@ void Clipboard::ReadAsciiText(std::string* result) const {
   result->clear();
 
   // Acquire the clipboard.
-  ClipboardLock lock;
-  if (!lock.Acquire(clipboard_owner_))
+  ScopedClipboard clipboard;
+  if (!clipboard.Acquire(clipboard_owner_))
     return;
 
   HANDLE data = ::GetClipboardData(CF_TEXT);
@@ -431,8 +425,8 @@ void Clipboard::ReadHTML(std::wstring* markup, std::string* src_url) const {
     src_url->clear();
 
   // Acquire the clipboard.
-  ClipboardLock lock;
-  if (!lock.Acquire(clipboard_owner_))
+  ScopedClipboard clipboard;
+  if (!clipboard.Acquire(clipboard_owner_))
     return;
 
   HANDLE data = ::GetClipboardData(GetHtmlFormatType());
@@ -453,8 +447,8 @@ void Clipboard::ReadBookmark(std::wstring* title, std::string* url) const {
     url->clear();
 
   // Acquire the clipboard.
-  ClipboardLock lock;
-  if (!lock.Acquire(clipboard_owner_))
+  ScopedClipboard clipboard;
+  if (!clipboard.Acquire(clipboard_owner_))
     return;
 
   HANDLE data = ::GetClipboardData(GetUrlWFormatType());
@@ -492,8 +486,8 @@ void Clipboard::ReadFiles(std::vector<std::wstring>* files) const {
 
   files->clear();
 
-  ClipboardLock lock;
-  if (!lock.Acquire(clipboard_owner_))
+  ScopedClipboard clipboard;
+  if (!clipboard.Acquire(clipboard_owner_))
     return;
 
   HDROP drop = static_cast<HDROP>(::GetClipboardData(CF_HDROP));