Regression for importer. Updated the code to reflect the coding standards of the chrome team.

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

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

3 files changed, 221 insertions, 228 deletions

diff --git a/chrome/browser/importer/toolbar_importer.cc b/chrome/browser/importer/toolbar_importer.cc
index 80d5a7f..350b2edb 100644
--- a/chrome/browser/importer/toolbar_importer.cc
+++ b/chrome/browser/importer/toolbar_importer.cc
@@ -5,27 +5,28 @@
 #include "chrome/browser/importer/toolbar_importer.h"
 
 #include <limits>
+
 #include "base/string_util.h"
 #include "base/rand_util.h"
 #include "base/registry.h"
 #include "chrome/browser/first_run.h"
 #include "chrome/common/l10n_util.h"
 #include "chrome/common/libxml_utils.h"
-#include "net/base/data_url.h"
 #include "net/base/cookie_monster.h"
+#include "net/base/data_url.h"
 
 #include "generated_resources.h"
 
 //
 // ToolbarImporterUtils
 //
-static const std::string kGoogleDomainUrl = "http://.google.com/";
+static const char* kGoogleDomainUrl = "http://.google.com/";
 static const wchar_t kSplitStringToken = L';';
-static const std::string kGoogleDomainSecureCookieId = "SID=";
+static const char* kGoogleDomainSecureCookieId = "SID=";
 
 bool ToolbarImporterUtils::IsGoogleGAIACookieInstalled() {
   URLRequestContext* context = Profile::GetDefaultRequestContext();
-  net::CookieMonster* store= context->cookie_store();
+  net::CookieMonster* store = context->cookie_store();
   GURL url(kGoogleDomainUrl);
   net::CookieMonster::CookieOptions options;
   options.set_include_httponly();  // The SID cookie might be httponly.
@@ -37,7 +38,7 @@ bool ToolbarImporterUtils::IsGoogleGAIACookieInstalled() {
        ++current) {
     size_t position = (*current).find(kGoogleDomainSecureCookieId);
     if (0 == position)
-        return true;
+      return true;
   }
   return false;
 }
@@ -45,38 +46,44 @@ bool ToolbarImporterUtils::IsGoogleGAIACookieInstalled() {
 //
 // Toolbar5Importer
 //
-const std::string Toolbar5Importer::kXmlApiReplyXmlTag = "xml_api_reply";
-const std::string Toolbar5Importer::kBookmarksXmlTag = "bookmarks";
-const std::string Toolbar5Importer::kBookmarkXmlTag = "bookmark";
-const std::string Toolbar5Importer::kTitleXmlTag = "title";
-const std::string Toolbar5Importer::kUrlXmlTag = "url";
-const std::string Toolbar5Importer::kTimestampXmlTag = "timestamp";
-const std::string Toolbar5Importer::kLabelsXmlTag = "labels";
-const std::string Toolbar5Importer::kLabelsXmlCloseTag = "/labels";
-const std::string Toolbar5Importer::kLabelXmlTag = "label";
-const std::string Toolbar5Importer::kAttributesXmlTag = "attributes";
-
-const std::string Toolbar5Importer::kRandomNumberToken = "{random_number}";
-const std::string Toolbar5Importer::kAuthorizationToken = "{auth_token}";
-const std::string Toolbar5Importer::kAuthorizationTokenPrefix = "/*";
-const std::string Toolbar5Importer::kAuthorizationTokenSuffix = "*/";
-const std::string Toolbar5Importer::kMaxNumToken = "{max_num}";
-const std::string Toolbar5Importer::kMaxTimestampToken = "{max_timestamp}";
-
-const std::string Toolbar5Importer::kT5AuthorizationTokenUrl =
+const char Toolbar5Importer::kXmlApiReplyXmlTag[] = "xml_api_reply";
+const char Toolbar5Importer::kBookmarksXmlTag[] = "bookmarks";
+const char Toolbar5Importer::kBookmarkXmlTag[] = "bookmark";
+const char Toolbar5Importer::kTitleXmlTag[] = "title";
+const char Toolbar5Importer::kUrlXmlTag[] = "url";
+const char Toolbar5Importer::kTimestampXmlTag[] = "timestamp";
+const char Toolbar5Importer::kLabelsXmlTag[] = "labels";
+const char Toolbar5Importer::kLabelsXmlCloseTag[] = "/labels";
+const char Toolbar5Importer::kLabelXmlTag[] = "label";
+const char Toolbar5Importer::kAttributesXmlTag[] = "attributes";
+
+const char Toolbar5Importer::kRandomNumberToken[] = "{random_number}";
+const char Toolbar5Importer::kAuthorizationToken[] = "{auth_token}";
+const char Toolbar5Importer::kAuthorizationTokenPrefix[] = "/*";
+const char Toolbar5Importer::kAuthorizationTokenSuffix[] = "*/";
+const char Toolbar5Importer::kMaxNumToken[] = "{max_num}";
+const char Toolbar5Importer::kMaxTimestampToken[] = "{max_timestamp}";
+
+const char Toolbar5Importer::kT5AuthorizationTokenUrl[] =
     "http://www.google.com/notebook/token?zx={random_number}";
-const std::string Toolbar5Importer::kT5FrontEndUrlTemplate =
-    "http://www.google.com/notebook/toolbar?cmd=list&tok={auth_token}& "
+const char Toolbar5Importer::kT5FrontEndUrlTemplate[] =
+    "http://www.google.com/notebook/toolbar?cmd=list&tok={auth_token}&"
     "num={max_num}&min={max_timestamp}&all=0&zx={random_number}";
 
 // Importer methods.
-Toolbar5Importer::Toolbar5Importer() : writer_(NULL),
-                                       state_(NOT_USED),
-                                       items_to_import_(NONE),
-                                       token_fetcher_(NULL),
-                                       data_fetcher_(NULL) {
+
+// The constructor should set the initial state to NOT_USED.
+Toolbar5Importer::Toolbar5Importer()
+    : writer_(NULL),
+      state_(NOT_USED),
+      items_to_import_(NONE),
+      token_fetcher_(NULL),
+      data_fetcher_(NULL) {
 }
 
+// The destructor insures that the fetchers are currently not being used, as
+// their thread-safe implementation requires that they are cancelled from the
+// thread in which they were constructed.
 Toolbar5Importer::~Toolbar5Importer() {
   DCHECK(!token_fetcher_);
   DCHECK(!data_fetcher_);
@@ -100,7 +107,7 @@ void Toolbar5Importer::StartImport(ProfileInfo profile_info,
   ContinueImport();
 }
 
-// The public cancel method servers two functions, as a callback from the UI
+// The public cancel method serves two functions, as a callback from the UI
 // as well as an internal callback in case of cancel.  An internal callback
 // is required since the URLFetcher must be destroyed from the thread it was
 // created.
@@ -135,8 +142,9 @@ void Toolbar5Importer::OnURLFetchComplete(
   }
 
   if (200 != response_code) {  // HTTP/Ok
-    // Display to the user an error dialog and cancel the import
-    EndImportBookmarks(false);
+    // Cancelling here will update the UI and bypass the rest of bookmark
+    // import.
+    EndImportBookmarks();
     return;
   }
 
@@ -145,11 +153,11 @@ void Toolbar5Importer::OnURLFetchComplete(
       GetBookmarkDataFromServer(data);
       break;
     case GET_BOOKMARKS:
-      GetBookmarsFromServerDataResponse(data);
+      GetBookmarksFromServerDataResponse(data);
       break;
     default:
       NOTREACHED() << "Invalid state.";
-      EndImportBookmarks(false);
+      EndImportBookmarks();
       break;
   }
 }
@@ -162,19 +170,26 @@ void Toolbar5Importer::ContinueImport() {
   // of its item before its task finishes and re-enters this method.
   if (NONE == items_to_import_) {
     EndImport();
+    return;
   }
   if ((items_to_import_ & FAVORITES) && !cancelled()) {
     items_to_import_ &= ~FAVORITES;
     BeginImportBookmarks();
+    return;
   }
   // TODO(brg): Import history, autocomplete, other toolbar information
-  // for 2.0
+  // in a future release.
+
+  // This code should not be reached, but gracefully handles the possibility
+  // that StartImport was called with unsupported items_to_import.
+  if (!cancelled())
+    EndImport();
 }
 
 void Toolbar5Importer::EndImport() {
   if (state_ != DONE) {
     state_ = DONE;
-    // By spec the fetcher's must be destroyed within the same
+    // By spec the fetchers must be destroyed within the same
     // thread they are created.  The importer is destroyed in the ui_thread
     // so when we complete in the file_thread we destroy them first.
     if (NULL != token_fetcher_) {
@@ -196,20 +211,20 @@ void Toolbar5Importer::BeginImportBookmarks() {
   GetAuthenticationFromServer();
 }
 
-void Toolbar5Importer::EndImportBookmarks(bool success) {
+void Toolbar5Importer::EndImportBookmarks() {
   NotifyItemEnded(FAVORITES);
   ContinueImport();
 }
 
 
-// Notebook FE connection managers.
+// Notebook front-end connection manager implementation follows.
 void Toolbar5Importer::GetAuthenticationFromServer() {
   if (cancelled()) {
     EndImport();
     return;
   }
 
-  // Authentication is a token string retreived from the authentication server
+  // Authentication is a token string retrieved from the authentication server
   // To access it we call the url below with a random number replacing the
   // value in the string.
   state_ = GET_AUTHORIZATION_TOKEN;
@@ -221,9 +236,9 @@ void Toolbar5Importer::GetAuthenticationFromServer() {
   // Retrieve authorization token from the network.
   std::string url_string(kT5AuthorizationTokenUrl);
   url_string.replace(url_string.find(kRandomNumberToken),
-                     kRandomNumberToken.size(),
+                     arraysize(kRandomNumberToken) - 1,
                      random_string);
-  GURL  url(url_string);
+  GURL url(url_string);
 
   token_fetcher_ = new  URLFetcher(url, URLFetcher::GET, this);
   token_fetcher_->set_request_context(Profile::GetDefaultRequestContext());
@@ -241,7 +256,7 @@ void Toolbar5Importer::GetBookmarkDataFromServer(const std::string& response) {
   // Parse and verify the authorization token from the response.
   std::string token;
   if (!ParseAuthenticationTokenResponse(response, &token)) {
-    EndImportBookmarks(false);
+    EndImportBookmarks();
     return;
   }
 
@@ -251,19 +266,19 @@ void Toolbar5Importer::GetBookmarkDataFromServer(const std::string& response) {
   int random = base::RandInt(0, std::numeric_limits<int>::max());
   std::string random_string = UintToString(random);
   conn_string.replace(conn_string.find(kRandomNumberToken),
-                      kRandomNumberToken.size(),
+                      arraysize(kRandomNumberToken) - 1,
                       random_string);
   conn_string.replace(conn_string.find(kAuthorizationToken),
-                      kAuthorizationToken.size(),
+                      arraysize(kAuthorizationToken) - 1,
                       token);
-  GURL  url(conn_string);
+  GURL url(conn_string);
 
   data_fetcher_ = new URLFetcher(url, URLFetcher::GET, this);
   data_fetcher_->set_request_context(Profile::GetDefaultRequestContext());
   data_fetcher_->Start();
 }
 
-void Toolbar5Importer::GetBookmarsFromServerDataResponse(
+void Toolbar5Importer::GetBookmarksFromServerDataResponse(
     const std::string& response) {
   if (cancelled()) {
     EndImport();
@@ -272,15 +287,14 @@ void Toolbar5Importer::GetBookmarsFromServerDataResponse(
 
   state_ = PARSE_BOOKMARKS;
 
-  bool retval = false;
   XmlReader reader;
   if (reader.Load(response) && !cancelled()) {
     // Construct Bookmarks
     std::vector<ProfileWriter::BookmarkEntry> bookmarks;
-    retval = ParseBookmarksFromReader(&reader, &bookmarks);
-    if (retval) AddBookMarksToChrome(bookmarks);
+    if (ParseBookmarksFromReader(&reader, &bookmarks))
+      AddBookmarksToChrome(bookmarks);
   }
-  EndImportBookmarks(retval);
+  EndImportBookmarks();
 }
 
 bool Toolbar5Importer::ParseAuthenticationTokenResponse(
@@ -292,12 +306,12 @@ bool Toolbar5Importer::ParseAuthenticationTokenResponse(
   size_t position = token->find(kAuthorizationTokenPrefix);
   if (0 != position)
     return false;
-  token->replace(position, kAuthorizationTokenPrefix.size(), "");
+  token->replace(position, arraysize(kAuthorizationTokenPrefix) - 1, "");
 
   position = token->find(kAuthorizationTokenSuffix);
-  if (token->size() != (position + kAuthorizationTokenSuffix.size()))
+  if (token->size() != (position + (arraysize(kAuthorizationTokenSuffix) - 1)))
     return false;
-  token->replace(position, kAuthorizationTokenSuffix.size(), "");
+  token->replace(position, arraysize(kAuthorizationTokenSuffix) - 1, "");
 
   return true;
 }
@@ -326,11 +340,11 @@ bool Toolbar5Importer::ParseBookmarksFromReader(
   while (LocateNextTagWithStopByName(reader, kBookmarkXmlTag,
                                      kBookmarksXmlTag)) {
     ProfileWriter::BookmarkEntry bookmark_entry;
-    std::vector<BOOKMARK_FOLDER> folders;
+    std::vector<BookmarkFolderType> folders;
     if (ExtractBookmarkInformation(reader, &bookmark_entry, &folders)) {
       // For each folder we create a new bookmark entry.  Duplicates will
-      // be detected whence we attempt to creaete the bookmark in the profile.
-      for (std::vector<BOOKMARK_FOLDER>::iterator folder = folders.begin();
+      // be detected when we attempt to create the bookmark in the profile.
+      for (std::vector<BookmarkFolderType>::iterator folder = folders.begin();
           folder != folders.end();
           ++folder) {
         bookmark_entry.path = *folder;
@@ -388,7 +402,7 @@ bool Toolbar5Importer::LocateNextTagWithStopByName(XmlReader* reader,
 bool Toolbar5Importer::ExtractBookmarkInformation(
     XmlReader* reader,
     ProfileWriter::BookmarkEntry* bookmark_entry,
-    std::vector<BOOKMARK_FOLDER>* bookmark_folders) {
+    std::vector<BookmarkFolderType>* bookmark_folders) {
   DCHECK(reader);
   DCHECK(bookmark_entry);
   DCHECK(bookmark_folders);
@@ -509,7 +523,7 @@ bool Toolbar5Importer::ExtractTimeFromXmlReader(
 
 bool Toolbar5Importer::ExtractFoldersFromXmlReader(
     XmlReader* reader,
-    std::vector<BOOKMARK_FOLDER>* bookmark_folders) {
+    std::vector<BookmarkFolderType>* bookmark_folders) {
   DCHECK(reader);
   DCHECK(bookmark_folders);
 
@@ -566,7 +580,7 @@ bool Toolbar5Importer::ExtractFoldersFromXmlReader(
 }
 
 // Bookmark creation
-void  Toolbar5Importer::AddBookMarksToChrome(
+void  Toolbar5Importer::AddBookmarksToChrome(
     const std::vector<ProfileWriter::BookmarkEntry>& bookmarks) {
   if (!bookmarks.empty() && !cancelled()) {
     int options = ProfileWriter::ADD_IF_UNIQUE |
diff --git a/chrome/browser/importer/toolbar_importer.h b/chrome/browser/importer/toolbar_importer.h
index eb2e5f8..0a1c601 100644
--- a/chrome/browser/importer/toolbar_importer.h
+++ b/chrome/browser/importer/toolbar_importer.h
@@ -4,14 +4,6 @@
 
 // The functionality provided here allows the user to import their bookmarks
 // (favorites) from Google Toolbar.
-//
-// Currently the only configuration information we need is to check whether or
-// not the user currently has their GAIA cookie.  This is done by the functions
-// exposed through the ToolbarImportUtils namespace.
-//
-// Toolbar5Importer is a class which exposes the functionality needed to
-// communicate with the Google Toolbar v5 front-end, negotiate the download of
-// Toolbar bookmarks, parse them, and install them on the client.
 
 #ifndef CHROME_BROWSER_IMPORTER_TOOLBAR_IMPORTER_H__
 #define CHROME_BROWSER_IMPORTER_TOOLBAR_IMPORTER_H__
@@ -24,17 +16,28 @@
 
 class XmlReader;
 
+// Currently the only configuration information we need is to check whether or
+// not the user currently has their GAIA cookie.  This is done by the function
+// exposed through the ToolbarImportUtils namespace.
 namespace ToolbarImporterUtils {
 bool IsGoogleGAIACookieInstalled();
 }  // namespace ToolbarImporterUtils
 
-class Toolbar5Importer : public URLFetcher::Delegate,
-                         public Importer {
+// Toolbar5Importer is a class which exposes the functionality needed to
+// communicate with the Google Toolbar v5 front-end, negotiate the download of
+// Toolbar bookmarks, parse them, and install them on the client.
+// Toolbar5Importer should not have StartImport called more than once.  Futher
+// if StartImport is called, then the class must not be destroyed until it
+// has either completed or Toolbar5Importer->Cancel() has been called.
+class Toolbar5Importer : public URLFetcher::Delegate, public Importer {
  public:
   Toolbar5Importer();
   virtual ~Toolbar5Importer();
 
-  // Importer view calls this method to being the process.
+  // Importer view calls this method to begin the process.  The items parameter
+  // should only either be NONE or FAVORITES, since as of right now these are
+  // the only items this importer supports.  This method provides implementation
+  // of Importer::StartImport.
   virtual void StartImport(ProfileInfo profile_info,
                            uint16 items,
                            ProfileWriter* writer,
@@ -47,7 +50,7 @@ class Toolbar5Importer : public URLFetcher::Delegate,
   virtual void Cancel();
 
   // URLFetcher::Delegate method called back from the URLFetcher object.
-  void OnURLFetchComplete(const URLFetcher* source,
+  virtual void OnURLFetchComplete(const URLFetcher* source,
                           const GURL& url,
                           const URLRequestStatus& status,
                           int response_code,
@@ -57,8 +60,8 @@ class Toolbar5Importer : public URLFetcher::Delegate,
  private:
   FRIEND_TEST(Toolbar5ImporterTest, BookmarkParse);
 
-  // Internal state
-  enum INTERNAL_STATE {
+  // Internal states of the toolbar importer.
+  enum InternalStateEnum {
     NOT_USED = -1,
     INITIALIZED,
     GET_AUTHORIZATION_TOKEN,
@@ -67,50 +70,56 @@ class Toolbar5Importer : public URLFetcher::Delegate,
     DONE
   };
 
-  typedef std::vector<std::wstring> BOOKMARK_FOLDER;
-
-  // URLs for connecting to the toolbar front end
-  static const std::string kT5AuthorizationTokenUrl;
-  static const std::string kT5FrontEndUrlTemplate;
-
-  // Token replacement tags
-  static const std::string kRandomNumberToken;
-  static const std::string kAuthorizationToken;
-  static const std::string kAuthorizationTokenPrefix;
-  static const std::string kAuthorizationTokenSuffix;
-  static const std::string kMaxNumToken;
-  static const std::string kMaxTimestampToken;
-
-  // XML tag names
-  static const std::string kXmlApiReplyXmlTag;
-  static const std::string kBookmarksXmlTag;
-  static const std::string kBookmarkXmlTag;
-  static const std::string kTitleXmlTag;
-  static const std::string kUrlXmlTag;
-  static const std::string kTimestampXmlTag;
-  static const std::string kLabelsXmlTag;
-  static const std::string kLabelsXmlCloseTag;
-  static const std::string kLabelXmlTag;
-  static const std::string kAttributesXmlTag;
-
-  // Flow control
+  typedef std::vector<std::wstring> BookmarkFolderType;
+
+  // URLs for connecting to the toolbar front end are defined below.
+  static const char kT5AuthorizationTokenUrl[];
+  static const char kT5FrontEndUrlTemplate[];
+
+  // Token replacement tags are defined below.
+  static const char kRandomNumberToken[];
+  static const char kAuthorizationToken[];
+  static const char kAuthorizationTokenPrefix[];
+  static const char kAuthorizationTokenSuffix[];
+  static const char kMaxNumToken[];
+  static const char kMaxTimestampToken[];
+
+  // XML tag names are defined below.
+  static const char kXmlApiReplyXmlTag[];
+  static const char kBookmarksXmlTag[];
+  static const char kBookmarkXmlTag[];
+  static const char kTitleXmlTag[];
+  static const char kUrlXmlTag[];
+  static const char kTimestampXmlTag[];
+  static const char kLabelsXmlTag[];
+  static const char kLabelsXmlCloseTag[];
+  static const char kLabelXmlTag[];
+  static const char kAttributesXmlTag[];
+
+  // Flow control for asynchronous import is controlled by the methods below.
+  // ContinueImport is called back by each import action taken.  BeginXXX
+  // and EndXXX are responsible for updating the state of the asynchronous
+  // import.  EndImport is responsible for state cleanup and notifying the
+  // caller that import has completed.
   void ContinueImport();
   void EndImport();
   void BeginImportBookmarks();
-  void EndImportBookmarks(bool success);
+  void EndImportBookmarks();
 
-  // Network I/O
+  // Network I/O is done by the methods below.  These three methods are called
+  // in the order provided.  The last two are called back with the HTML
+  // response provided by the Toolbar server.
   void GetAuthenticationFromServer();
   void GetBookmarkDataFromServer(const std::string& response);
-  void GetBookmarsFromServerDataResponse(const std::string& response);
+  void GetBookmarksFromServerDataResponse(const std::string& response);
 
-  // XML Parsing
+  // XML Parsing is implemented with the methods below.
   bool ParseAuthenticationTokenResponse(const std::string& response,
                                         std::string* token);
 
   static bool ParseBookmarksFromReader(
       XmlReader* reader,
-      std::vector< ProfileWriter::BookmarkEntry >* bookmarks);
+      std::vector<ProfileWriter::BookmarkEntry>* bookmarks);
 
   static bool LocateNextOpenTag(XmlReader* reader);
   static bool LocateNextTagByName(XmlReader* reader, const std::string& tag);
@@ -122,7 +131,7 @@ class Toolbar5Importer : public URLFetcher::Delegate,
   static bool ExtractBookmarkInformation(
       XmlReader* reader,
       ProfileWriter::BookmarkEntry* bookmark_entry,
-      std::vector<BOOKMARK_FOLDER>* bookmark_folders);
+      std::vector<BookmarkFolderType>* bookmark_folders);
   static bool ExtractNamedValueFromXmlReader(XmlReader* reader,
                                              const std::string& name,
                                              std::string* buffer);
@@ -134,24 +143,25 @@ class Toolbar5Importer : public URLFetcher::Delegate,
                                        ProfileWriter::BookmarkEntry* entry);
   static bool ExtractFoldersFromXmlReader(
       XmlReader* reader,
-      std::vector<BOOKMARK_FOLDER>* bookmark_folders);
+      std::vector<BookmarkFolderType>* bookmark_folders);
 
-  // Bookmark creation
-  void AddBookMarksToChrome(
+  // Bookmark creation is done by the method below.
+  void AddBookmarksToChrome(
       const std::vector<ProfileWriter::BookmarkEntry>& bookmarks);
 
-  // Hosts the writer used in this importer.
+  // The writer used in this importer is stored in writer_.
   ProfileWriter* writer_;
 
-  // Internal state
-  INTERNAL_STATE state_;
+  // Internal state is stored in state_.
+  InternalStateEnum state_;
 
-  // Bitmask of Importer::ImportItem
+  // Bitmask of Importer::ImportItem is stored in items_to_import_.
   uint16  items_to_import_;
 
   // The fetchers need to be available to cancel the network call on user cancel
-  URLFetcher * token_fetcher_;
-  URLFetcher * data_fetcher_;
+  // hence they are stored as member variables.
+  URLFetcher* token_fetcher_;
+  URLFetcher* data_fetcher_;
 
   DISALLOW_COPY_AND_ASSIGN(Toolbar5Importer);
 };
diff --git a/chrome/browser/importer/toolbar_importer_unittest.cc b/chrome/browser/importer/toolbar_importer_unittest.cc
index 8c291d0..4201fbf 100644
--- a/chrome/browser/importer/toolbar_importer_unittest.cc
+++ b/chrome/browser/importer/toolbar_importer_unittest.cc
@@ -13,18 +13,33 @@
 
 
 namespace toolbar_importer_unittest {
-static const std::wstring kTitle = L"MyTitle";
-static const std::wstring kUrl = L"http://www.google.com/";
-static const std::wstring kTimestamp = L"1153328691085181";
-static const std::wstring kFolder = L"Google";
-static const std::wstring kFolder2 = L"Homepage";
-static const std::wstring kFolderArray[3] = {L"Google", L"Search", L"Page"};
-
-static const std::wstring kOtherTitle = L"MyOtherTitle";
-static const std::wstring kOtherUrl = L"http://www.google.com/mail";
-static const std::wstring kOtherFolder = L"Mail";
-
-static const std::string kGoodBookMark =
+static const wchar_t* kTitle = L"MyTitle";
+static const wchar_t* kUrl = L"http://www.google.com/";
+static const wchar_t* kTimestamp = L"1153328691085181";
+static const wchar_t* kFolder = L"Google";
+static const wchar_t* kFolder2 = L"Homepage";
+static const wchar_t* kFolderArray[3] = {L"Google", L"Search", L"Page"};
+
+static const wchar_t* kOtherTitle = L"MyOtherTitle";
+static const wchar_t* kOtherUrl = L"http://www.google.com/mail";
+static const wchar_t* kOtherFolder = L"Mail";
+
+// Since the following is very dense to read I enumerate the test cases here.
+// 1. Correct bookmark structure with one label.
+// 2. Correct bookmark structure with no labels.
+// 3. Correct bookmark structure with two labels.
+// 4. Correct bookmark structure with a folder->label translation by toolbar.
+// 5. Correct bookmark structure with no favicon.
+// 6. Two correct bookmarks.
+// The following are error cases by removing sections from the xml:
+// 7. Empty string passed as xml.
+// 8. No <bookmarks> section in the xml.
+// 9. No <bookmark> section below the <bookmarks> section.
+// 10. No <title> in a <bookmark> section.
+// 11. No <url> in a <bookmark> section.
+// 12. No <timestamp> in a <bookmark> section.
+// 13. No <labels> in a <bookmark> section.
+static const char* kGoodBookmark =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -45,7 +60,7 @@ static const std::string kGoodBookMark =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kGoodBookMarkNoLabel =
+static const char* kGoodBookmarkNoLabel =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -66,7 +81,7 @@ static const std::string kGoodBookMarkNoLabel =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kGoodBookMarkTwoLabels =
+static const char* kGoodBookmarkTwoLabels =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -87,7 +102,7 @@ static const std::string kGoodBookMarkTwoLabels =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kGoodBookMarkFolderLabel =
+static const char* kGoodBookmarkFolderLabel =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -108,7 +123,7 @@ static const std::string kGoodBookMarkFolderLabel =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kGoodBookMarkNoFavicon =
+static const char* kGoodBookmarkNoFavicon =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -126,7 +141,7 @@ static const std::string kGoodBookMarkNoFavicon =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kGoodBookMark2Items =
+static const char* kGoodBookmark2Items =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -168,8 +183,8 @@ static const std::string kGoodBookMark2Items =
     "</value> </attribute> </attributes> "
     "</bookmark>"
     "</bookmarks>";
-static const std::string kEmptyString = "";
-static const std::string kBadBookMarkNoBookmarks =
+static const char* kEmptyString = "";
+static const char* kBadBookmarkNoBookmarks =
     " <bookmark> "
     "<title>MyTitle</title> "
     "<url>http://www.google.com/</url> "
@@ -189,7 +204,7 @@ static const std::string kBadBookMarkNoBookmarks =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kBadBookMarkNoBookmark =
+static const char* kBadBookmarkNoBookmark =
     " <bookmarks>"
     "<title>MyTitle</title> "
     "<url>http://www.google.com/</url> "
@@ -209,7 +224,7 @@ static const std::string kBadBookMarkNoBookmark =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kBadBookMarkNoTitle =
+static const char* kBadBookmarkNoTitle =
     " <bookmarks>"
     " <bookmark> "
     "<url>http://www.google.com/</url> "
@@ -229,7 +244,7 @@ static const std::string kBadBookMarkNoTitle =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kBadBookMarkNoUrl =
+static const char* kBadBookmarkNoUrl =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -249,7 +264,7 @@ static const std::string kBadBookMarkNoUrl =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kBadBookMarkNoTimestamp =
+static const char* kBadBookmarkNoTimestamp =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -269,7 +284,7 @@ static const std::string kBadBookMarkNoTimestamp =
     "<attribute> <name>section_name</name> <value>My section 0 "
     "</value> </attribute> </attributes> "
     "</bookmark> </bookmarks>";
-static const std::string kBadBookMarkNoLabels =
+static const char* kBadBookmarkNoLabels =
     " <bookmarks>"
     " <bookmark> "
     "<title>MyTitle</title> "
@@ -291,41 +306,21 @@ static const std::string kBadBookMarkNoLabels =
     "</bookmark> </bookmarks>";
 }  // namespace toolbar_importer_unittest
 
-// Since the above is very dense to read I enumerate the test cases here.
-// 1. Correct bookmark structure with one label
-// 2. "" with no labels
-// 3. "" with two labels
-// 4. "" with a folder->label translation done by toolbar
-// 5. "" with no favicon
-// 6. Two correct bookmarks.
-// The following are error cases by removing sections:
-// 7. Empty string
-// 8. No <bookmarks>
-// 9. No <bookmark>
-// 10. No <title>
-// 11. No <url>
-// 12. No <timestamp>
-// 13. No <labels>
+// The parsing tests for Toolbar5Importer use the string above.  For a
+// description of all the tests run please see the comments immediately before
+// the string constants above.
 TEST(Toolbar5ImporterTest, BookmarkParse) {
-//  bool ParseBookmarksFromReader(
-//    XmlReader* reader,
-//    std::vector<ProfileWriter::BookmarkEntry>* bookmarks,
-//    std::vector< history::ImportedFavIconUsage >* favicons);
-
   XmlReader reader;
   std::vector<ProfileWriter::BookmarkEntry> bookmarks;
 
   const GURL url(toolbar_importer_unittest::kUrl);
   const GURL other_url(toolbar_importer_unittest::kOtherUrl);
 
-  // Test case 1 - Basic bookmark, single lables
+  // Test case 1 is parsing a basic bookmark with a single label.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookMark));
-  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookmark));
+  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // verificaiton
   EXPECT_EQ(bookmarks.size(), 1);
   EXPECT_FALSE(bookmarks[0].in_toolbar);
   EXPECT_EQ(toolbar_importer_unittest::kTitle, bookmarks[0].title);
@@ -333,28 +328,22 @@ TEST(Toolbar5ImporterTest, BookmarkParse) {
   EXPECT_EQ(2, bookmarks[0].path.size());
   EXPECT_EQ(toolbar_importer_unittest::kFolder, bookmarks[0].path[1]);
 
-  // Test case 2 - No labels
+  // Test case 2 is parsing a single bookmark with no label.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookMarkNoLabel));
-  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookmarkNoLabel));
+  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // verificaiton
   EXPECT_EQ(bookmarks.size(), 1);
   EXPECT_FALSE(bookmarks[0].in_toolbar);
   EXPECT_EQ(toolbar_importer_unittest::kTitle, bookmarks[0].title);
   EXPECT_EQ(bookmarks[0].url, url);
   EXPECT_EQ(1, bookmarks[0].path.size());
 
-  // Test case 3 - Two labels
+  // Test case 3 is parsing a single bookmark with two labels.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookMarkTwoLabels));
-  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookmarkTwoLabels));
+  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // verificaiton
   EXPECT_EQ(2, bookmarks.size());
   EXPECT_FALSE(bookmarks[0].in_toolbar);
   EXPECT_FALSE(bookmarks[1].in_toolbar);
@@ -365,14 +354,12 @@ TEST(Toolbar5ImporterTest, BookmarkParse) {
   EXPECT_EQ(toolbar_importer_unittest::kFolder, bookmarks[0].path[1]);
   EXPECT_EQ(toolbar_importer_unittest::kFolder2, bookmarks[1].path[1]);
 
-  // Test case 4 - Label with a colon (file name translation).
+  // Test case 4 is parsing a single bookmark which has a label with a colon,
+  // this test file name translation between Toolbar and Chrome.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookMarkFolderLabel));
-  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookmarkFolderLabel));
+  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // verificaiton
   EXPECT_EQ(bookmarks.size(), 1);
   EXPECT_FALSE(bookmarks[0].in_toolbar);
   EXPECT_EQ(toolbar_importer_unittest::kTitle, bookmarks[0].title);
@@ -385,14 +372,11 @@ TEST(Toolbar5ImporterTest, BookmarkParse) {
   EXPECT_TRUE(toolbar_importer_unittest::kFolderArray[2] ==
       bookmarks[0].path[3]);
 
-  // Test case 5 - No favicon
+  // Test case 5 is parsing a single bookmark without a favicon URL.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookMarkNoFavicon));
-  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookmarkNoFavicon));
+  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // verificaiton
   EXPECT_EQ(1, bookmarks.size());
   EXPECT_FALSE(bookmarks[0].in_toolbar);
   EXPECT_EQ(toolbar_importer_unittest::kTitle, bookmarks[0].title);
@@ -400,14 +384,11 @@ TEST(Toolbar5ImporterTest, BookmarkParse) {
   EXPECT_EQ(2, bookmarks[0].path.size());
   EXPECT_EQ(toolbar_importer_unittest::kFolder, bookmarks[0].path[1]);
 
-  // Test case 6 - Two bookmarks
+  // Test case 6 is parsing two bookmarks.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookMark2Items));
-  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kGoodBookmark2Items));
+  EXPECT_TRUE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // verificaiton
   EXPECT_EQ(2, bookmarks.size());
   EXPECT_FALSE(bookmarks[0].in_toolbar);
   EXPECT_FALSE(bookmarks[1].in_toolbar);
@@ -420,52 +401,40 @@ TEST(Toolbar5ImporterTest, BookmarkParse) {
   EXPECT_EQ(2, bookmarks[0].path.size());
   EXPECT_EQ(toolbar_importer_unittest::kOtherFolder, bookmarks[1].path[1]);
 
-  // Test case 7 - Empty string
+  // Test case 7 is parsing an empty string for bookmarks.
   bookmarks.clear();
 
   EXPECT_FALSE(reader.Load(toolbar_importer_unittest::kEmptyString));
 
-  // Test case 8 - No <bookmarks> section.
+  // Test case 8 is testing the error when no <bookmarks> section is present.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookMarkNoBookmarks));
-  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookmarkNoBookmarks));
+  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // Test case 9 - No <bookmark> section
+  // Test case 9 tests when no <bookmark> section is present.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookMarkNoBookmark));
-  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookmarkNoBookmark));
+  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
 
-  // Test case 10 - No title.
+  // Test case 10 tests when a bookmark has no <title> section.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookMarkNoTitle));
-  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookmarkNoTitle));
+  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
 
-  // Test case 11 - No URL.
+  // Test case 11 tests when a bookmark has no <url> section.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookMarkNoUrl));
-  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookmarkNoUrl));
+  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // Test case 12 - No timestamp.
+  // Test case 12 tests when a bookmark has no <timestamp> section.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookMarkNoTimestamp));
-  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookmarkNoTimestamp));
+  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 
-  // Test case 13
+  // Test case 13 tests when a bookmark has no <labels> section.
   bookmarks.clear();
-  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookMarkNoLabels));
-  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(
-    &reader,
-    &bookmarks));
+  EXPECT_TRUE(reader.Load(toolbar_importer_unittest::kBadBookmarkNoLabels));
+  EXPECT_FALSE(Toolbar5Importer::ParseBookmarksFromReader(&reader, &bookmarks));
 }