changelist for estade readability review.

submitted changelist: http://codereview.chromium.org/9154
Review URL: http://codereview.chromium.org/12605

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

3 files changed, 105 insertions, 94 deletions

diff --git a/base/clipboard_unittest.cc b/base/clipboard_unittest.cc
index 5432a58..c752b4c 100644
--- a/base/clipboard_unittest.cc
+++ b/base/clipboard_unittest.cc
@@ -17,13 +17,13 @@ TEST_F(ClipboardTest, ClearTest) {
   Clipboard clipboard;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteText(std::wstring(L"clear me"));
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteText(std::wstring(L"clear me"));
   }
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteHTML(std::wstring(L"<b>broom</b>"), "");
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteHTML(std::wstring(L"<b>broom</b>"), "");
   }
 
   EXPECT_FALSE(clipboard.IsFormatAvailable(
@@ -39,8 +39,8 @@ TEST_F(ClipboardTest, TextTest) {
   std::string ascii_text;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteText(text);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteText(text);
   }
 
   EXPECT_TRUE(clipboard.IsFormatAvailable(
@@ -60,8 +60,8 @@ TEST_F(ClipboardTest, HTMLTest) {
   std::string url("http://www.example.com/"), url_result;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteHTML(markup, url);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteHTML(markup, url);
   }
 
   EXPECT_EQ(true, clipboard.IsFormatAvailable(
@@ -72,7 +72,7 @@ TEST_F(ClipboardTest, HTMLTest) {
   // TODO(playmobil): It's not clear that non windows clipboards need to support
   // this.
   EXPECT_EQ(url, url_result);
-#endif
+#endif  // defined(OS_WIN)
 }
 
 TEST_F(ClipboardTest, TrickyHTMLTest) {
@@ -82,8 +82,8 @@ TEST_F(ClipboardTest, TrickyHTMLTest) {
   std::string url, url_result;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteHTML(markup, url);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteHTML(markup, url);
   }
 
   EXPECT_EQ(true, clipboard.IsFormatAvailable(
@@ -94,7 +94,7 @@ TEST_F(ClipboardTest, TrickyHTMLTest) {
   // TODO(playmobil): It's not clear that non windows clipboards need to support
   // this.
   EXPECT_EQ(url, url_result);
-#endif
+#endif  // defined(OS_WIN)
 }
 
 // TODO(estade): Port the following test (decide what target we use for urls)
@@ -106,8 +106,8 @@ TEST_F(ClipboardTest, BookmarkTest) {
   std::string url("http://www.example.com/"), url_result;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteBookmark(title, url);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteBookmark(title, url);
   }
 
   EXPECT_EQ(true,
@@ -116,7 +116,7 @@ TEST_F(ClipboardTest, BookmarkTest) {
   EXPECT_EQ(title, title_result);
   EXPECT_EQ(url, url_result);
 }
-#endif
+#endif  // defined(OS_WIN)
 
 TEST_F(ClipboardTest, MultiFormatTest) {
   Clipboard clipboard;
@@ -127,9 +127,9 @@ TEST_F(ClipboardTest, MultiFormatTest) {
   std::string ascii_text;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteHTML(markup, url);
-    scw.WriteText(text);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteHTML(markup, url);
+    clipboard_writer.WriteText(text);
   }
 
   EXPECT_EQ(true,
@@ -144,7 +144,7 @@ TEST_F(ClipboardTest, MultiFormatTest) {
   // TODO(playmobil): It's not clear that non windows clipboards need to support
   // this.
   EXPECT_EQ(url, url_result);
-#endif
+#endif  // defined(OS_WIN)
   clipboard.ReadText(&text_result);
   EXPECT_EQ(text, text_result);
   clipboard.ReadAsciiText(&ascii_text);
@@ -159,15 +159,15 @@ TEST_F(ClipboardTest, FileTest) {
   Clipboard clipboard;
 #if defined(OS_WIN)
   std::wstring file = L"C:\\Downloads\\My Downloads\\A Special File.txt";
-#else
+#elif defined(OS_MACOSX)
   // OS X will print a warning message if we stick a non-existant file on the
   // clipboard.
   std::wstring file = L"/usr/bin/make";
-#endif
+#endif  // defined(OS_MACOSX)
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteFile(file);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteFile(file);
   }
 
   std::wstring out_file;
@@ -188,15 +188,15 @@ TEST_F(ClipboardTest, MultipleFilesTest) {
   std::wstring file1 = L"/usr/bin/make";
   std::wstring file2 = L"/usr/bin/man";
   std::wstring file3 = L"/usr/bin/perl";
-#endif
+#endif  // defined(OS_MACOSX)
   std::vector<std::wstring> files;
   files.push_back(file1);
   files.push_back(file2);
   files.push_back(file3);
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteFiles(files);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteFiles(files);
   }
 
   std::vector<std::wstring> out_files;
@@ -218,8 +218,8 @@ TEST_F(ClipboardTest, HyperlinkTest) {
                     L"The Example Company</a>"), html_result;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteHyperlink(title, url);
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteHyperlink(title, url);
   }
 
   EXPECT_EQ(true,
@@ -231,15 +231,14 @@ TEST_F(ClipboardTest, HyperlinkTest) {
   EXPECT_EQ(url, url_result);
   clipboard.ReadHTML(&html_result, &url_result);
   EXPECT_EQ(html, html_result);
-  //XXX EXPECT_FALSE(url_result.is_valid());
 }
 
 TEST_F(ClipboardTest, WebSmartPasteTest) {
   Clipboard clipboard;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteWebSmartPaste();
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteWebSmartPaste();
   }
 
   EXPECT_EQ(true, clipboard.IsFormatAvailable(
@@ -256,11 +255,12 @@ TEST_F(ClipboardTest, BitmapTest) {
   Clipboard clipboard;
 
   {
-    ScopedClipboardWriter scw(&clipboard);
-    scw.WriteBitmapFromPixels(fake_bitmap, gfx::Size(3, 4));
+    ScopedClipboardWriter clipboard_writer(&clipboard);
+    clipboard_writer.WriteBitmapFromPixels(fake_bitmap, gfx::Size(3, 4));
   }
 
   EXPECT_EQ(true, clipboard.IsFormatAvailable(
                       Clipboard::GetBitmapFormatType()));
 }
-#endif
+#endif  // defined(OS_WIN)
+
diff --git a/base/scoped_clipboard_writer.cc b/base/scoped_clipboard_writer.cc
index 5821cd6..3ddb1a9 100644
--- a/base/scoped_clipboard_writer.cc
+++ b/base/scoped_clipboard_writer.cc
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// This file implements the ScopedClipboardWriter class. Documentation on its
+// purpose can be found in base/scoped_clipboard_writer.h. Documentation on the
+// format of the parameters for each clipboard target can be found in
+// base/clipboard.h.
 #include "base/scoped_clipboard_writer.h"
 
 #include "base/clipboard.h"
@@ -22,57 +26,57 @@ void ScopedClipboardWriter::WriteText(const std::wstring& text) {
 
   std::string utf8_text = WideToUTF8(text);
 
-  Clipboard::ObjectMapParams params;
-  params.push_back(Clipboard::ObjectMapParam(utf8_text.begin(),
-                                             utf8_text.end()));
-  objects_[Clipboard::CBF_TEXT] = params;
+  Clipboard::ObjectMapParams parameters;
+  parameters.push_back(Clipboard::ObjectMapParam(utf8_text.begin(),
+                                                 utf8_text.end()));
+  objects_[Clipboard::CBF_TEXT] = parameters;
 }
 
 void ScopedClipboardWriter::WriteHTML(const std::wstring& markup,
-                                      const std::string& src_url) {
+                                      const std::string& source_url) {
   if (markup.empty())
     return;
 
   std::string utf8_markup = WideToUTF8(markup);
 
-  Clipboard::ObjectMapParams params;
-  params.push_back(
+  Clipboard::ObjectMapParams parameters;
+  parameters.push_back(
       Clipboard::ObjectMapParam(utf8_markup.begin(),
                                 utf8_markup.end()));
-  if (!src_url.empty()) {
-    params.push_back(Clipboard::ObjectMapParam(src_url.begin(),
-                                               src_url.end()));
+  if (!source_url.empty()) {
+    parameters.push_back(Clipboard::ObjectMapParam(source_url.begin(),
+                                                   source_url.end()));
   }
 
-  objects_[Clipboard::CBF_HTML] = params;
+  objects_[Clipboard::CBF_HTML] = parameters;
 }
 
-void ScopedClipboardWriter::WriteBookmark(const std::wstring& title,
+void ScopedClipboardWriter::WriteBookmark(const std::wstring& bookmark_title,
                                           const std::string& url) {
-  if (title.empty() || url.empty())
+  if (bookmark_title.empty() || url.empty())
     return;
 
-  std::string utf8_markup = WideToUTF8(title);
+  std::string utf8_markup = WideToUTF8(bookmark_title);
 
-  Clipboard::ObjectMapParams params;
-  params.push_back(Clipboard::ObjectMapParam(utf8_markup.begin(),
-                                             utf8_markup.end()));
-  params.push_back(Clipboard::ObjectMapParam(url.begin(), url.end()));
-  objects_[Clipboard::CBF_BOOKMARK] = params;
+  Clipboard::ObjectMapParams parameters;
+  parameters.push_back(Clipboard::ObjectMapParam(utf8_markup.begin(),
+                                                 utf8_markup.end()));
+  parameters.push_back(Clipboard::ObjectMapParam(url.begin(), url.end()));
+  objects_[Clipboard::CBF_BOOKMARK] = parameters;
 }
 
-void ScopedClipboardWriter::WriteHyperlink(const std::wstring& title,
+void ScopedClipboardWriter::WriteHyperlink(const std::wstring& link_text,
                                            const std::string& url) {
-  if (title.empty() || url.empty())
+  if (link_text.empty() || url.empty())
     return;
 
-  std::string utf8_markup = WideToUTF8(title);
+  std::string utf8_markup = WideToUTF8(link_text);
 
-  Clipboard::ObjectMapParams params;
-  params.push_back(Clipboard::ObjectMapParam(utf8_markup.begin(),
-                                             utf8_markup.end()));
-  params.push_back(Clipboard::ObjectMapParam(url.begin(), url.end()));
-  objects_[Clipboard::CBF_LINK] = params;
+  Clipboard::ObjectMapParams parameters;
+  parameters.push_back(Clipboard::ObjectMapParam(utf8_markup.begin(),
+                                                 utf8_markup.end()));
+  parameters.push_back(Clipboard::ObjectMapParam(url.begin(), url.end()));
+  objects_[Clipboard::CBF_LINK] = parameters;
 }
 
 void ScopedClipboardWriter::WriteFile(const std::wstring& file) {
@@ -85,23 +89,23 @@ void ScopedClipboardWriter::WriteFiles(const std::vector<std::wstring>& files) {
   if (files.empty())
     return;
 
-  Clipboard::ObjectMapParam param;
+  Clipboard::ObjectMapParam parameter;
 
   for (std::vector<std::wstring>::const_iterator iter = files.begin();
        iter != files.end(); ++iter) {
     std::string filename = WideToUTF8(*iter);
     for (std::string::const_iterator filename_iter = filename.begin();
          filename_iter != filename.end(); ++filename_iter) {
-      param.push_back(*filename_iter);
+      parameter.push_back(*filename_iter);
     }
-    param.push_back('\0');
+    parameter.push_back('\0');
   }
 
-  param.push_back('\0');
+  parameter.push_back('\0');
 
-  Clipboard::ObjectMapParams params;
-  params.push_back(param);
-  objects_[Clipboard::CBF_FILES] = params;
+  Clipboard::ObjectMapParams parameters;
+  parameters.push_back(parameter);
+  objects_[Clipboard::CBF_FILES] = parameters;
 }
 
 void ScopedClipboardWriter::WriteWebSmartPaste() {
@@ -111,20 +115,21 @@ void ScopedClipboardWriter::WriteWebSmartPaste() {
 #if defined(OS_WIN)
 void ScopedClipboardWriter::WriteBitmapFromPixels(const void* pixels,
                                                   const gfx::Size& size) {
-  Clipboard::ObjectMapParam param1, param2;
+  Clipboard::ObjectMapParam pixels_parameter, size_parameter;
   const char* pixels_data = reinterpret_cast<const char*>(pixels);
-  size_t pixels_len = 4 * size.width() * size.height();
-  for (size_t i = 0; i < pixels_len; i++)
-    param1.push_back(pixels_data[i]);
+  size_t pixels_length = 4 * size.width() * size.height();
+  for (size_t i = 0; i < pixels_length; i++)
+    pixels_parameter.push_back(pixels_data[i]);
 
   const char* size_data = reinterpret_cast<const char*>(&size);
-  size_t size_len = sizeof(gfx::Size);
-  for (size_t i = 0; i < size_len; i++)
-    param2.push_back(size_data[i]);
-
-  Clipboard::ObjectMapParams params;
-  params.push_back(param1);
-  params.push_back(param2);
-  objects_[Clipboard::CBF_BITMAP] = params;
+  size_t size_length = sizeof(gfx::Size);
+  for (size_t i = 0; i < size_length; i++)
+    size_parameter.push_back(size_data[i]);
+
+  Clipboard::ObjectMapParams parameters;
+  parameters.push_back(pixels_parameter);
+  parameters.push_back(size_parameter);
+  objects_[Clipboard::CBF_BITMAP] = parameters;
 }
 #endif  // defined(OS_WIN)
+
diff --git a/base/scoped_clipboard_writer.h b/base/scoped_clipboard_writer.h
index 932c334..7bfedf4 100644
--- a/base/scoped_clipboard_writer.h
+++ b/base/scoped_clipboard_writer.h
@@ -2,15 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef SCOPED_CLIPBOARD_WRITER_H_
-#define SCOPED_CLIPBOARD_WRITER_H_
+// This file declares the ScopedClipboardWriter class, a wrapper around
+// the Clipboard class which simplifies writing data to the system clipboard.
+// Upon deletion the class atomically writes all data to |clipboard_|,
+// avoiding any potential race condition with other processes that are also
+// writing to the system clipboard.
+#ifndef BASE_SCOPED_CLIPBOARD_WRITER_H_
+#define BASE_SCOPED_CLIPBOARD_WRITER_H_
 
 #include "base/clipboard.h"
 
-#if defined(OS_WIN)
-class SkBitmap;
-#endif
-
 // This class is a wrapper for |Clipboard| that handles packing data
 // into a Clipboard::ObjectMap.
 // NB: You should probably NOT be using this class if you include
@@ -22,19 +23,21 @@ class ScopedClipboardWriter {
 
   ~ScopedClipboardWriter();
 
-  // Adds UNICODE and ASCII text to the clipboard.
+  // Converts |text| to UTF-8 and adds it to the clipboard.
   void WriteText(const std::wstring& text);
 
   // Adds HTML to the clipboard.  The url parameter is optional, but especially
-  // useful if the HTML fragment contains relative links
-  void WriteHTML(const std::wstring& markup, const std::string& src_url);
+  // useful if the HTML fragment contains relative links.
+  void WriteHTML(const std::wstring& markup, const std::string& source_url);
 
-  // Adds a bookmark to the clipboard
-  void WriteBookmark(const std::wstring& title, const std::string& url);
+  // Adds a bookmark to the clipboard.
+  void WriteBookmark(const std::wstring& bookmark_title,
+                     const std::string& url);
 
   // Adds both a bookmark and an HTML hyperlink to the clipboard.  It is a
-  // convenience wrapper around WriteBookmark and WriteHTML.
-  void WriteHyperlink(const std::wstring& title, const std::string& url);
+  // convenience wrapper around WriteBookmark and WriteHTML. |link_text| is
+  // used as the bookmark title.
+  void WriteHyperlink(const std::wstring& link_text, const std::string& url);
 
   // Adds a file or group of files to the clipboard.
   void WriteFile(const std::wstring& file);
@@ -49,9 +52,11 @@ class ScopedClipboardWriter {
   // memcpy the pixels into GDI and the blit the bitmap to the clipboard.
   // Pixel format is assumed to be 32-bit BI_RGB.
   void WriteBitmapFromPixels(const void* pixels, const gfx::Size& size);
-#endif
+#endif  // defined(OS_WIN)
 
  protected:
+  // We accumulate the data passed to the various targets in the |objects_|
+  // vector, and pass it to Clipboard::WriteObjects() during object destruction.
   Clipboard::ObjectMap objects_;
   Clipboard* clipboard_;
 
@@ -60,3 +65,4 @@ class ScopedClipboardWriter {
 };
 
 #endif  // SCOPED_CLIPBOARD_WRITER_H_
+