For MergeUrls(), convert anonymous enum that gives bit values into a typedefed uint32. This allows places wishing to refer to "a bitfield composed of these values" to use an explicit type. (This isn't possible by simply naming the enum as technically the enum doesn't define all of the possible combinations of bits.) This also means the individual named bit constants themselves have the same explicit type.

The drawback to defining constants at their declaration point is that supplying them to a templated function, like what EXPECT_EQ() expands into, triggers an "undefined symbol" error on Mac/Linux (and adding explicit storage for them in the .cc file can cause duplicate symbol errors on Windows).  Here I've worked around that by converting EXPECT_EQ(a, b) to EXPECT_TRUE(b == a).  Another possibility is to define the constants in the .cc file and merely declare them in the header, which works but is more verbose and less readable.

The original motiviation for this change was to find a way to eliminate some cases of passing anonymous-typed values as template arguments (which happens when you use a value from the enum in e.g. EXPECT_EQ()), which is technically illegal in C++03, though we don't warn about it.  Simply naming the enum would have done this, but this would have encouraged readers to actually use the enum name as a type, which for a bitfield is inappropriate for the reason given in the first paragraph.

BUG=92247
TEST=Compiles
Review URL: http://codereview.chromium.org/7756007

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

3 files changed, 27 insertions, 39 deletions

diff --git a/chrome/browser/sync/glue/typed_url_model_associator.cc b/chrome/browser/sync/glue/typed_url_model_associator.cc
index 7bdbb0a..d3bb856 100644
--- a/chrome/browser/sync/glue/typed_url_model_associator.cc
+++ b/chrome/browser/sync/glue/typed_url_model_associator.cc
@@ -127,8 +127,8 @@ bool TypedUrlModelAssociator::AssociateModels(SyncError* error) {
         history::URLRow new_url(*ix);
 
         std::vector<history::VisitInfo> added_visits;
-        int difference = MergeUrls(typed_url, *ix, &visits, &new_url,
-                                   &added_visits);
+        MergeResult difference =
+            MergeUrls(typed_url, *ix, &visits, &new_url, &added_visits);
         if (difference & DIFF_UPDATE_NODE) {
           sync_api::WriteNode write_node(&trans);
           if (!write_node.InitByClientTagLookup(syncable::TYPED_URLS, tag)) {
@@ -439,7 +439,7 @@ bool TypedUrlModelAssociator::WriteToHistoryBackend(
 }
 
 // static
-int TypedUrlModelAssociator::MergeUrls(
+TypedUrlModelAssociator::MergeResult TypedUrlModelAssociator::MergeUrls(
     const sync_pb::TypedUrlSpecifics& node,
     const history::URLRow& url,
     history::VisitVector* visits,
@@ -464,7 +464,7 @@ int TypedUrlModelAssociator::MergeUrls(
 
   // This is a bitfield representing what we'll need to update with the output
   // value.
-  int different = DIFF_NONE;
+  MergeResult different = DIFF_NONE;
 
   // Check if the non-incremented values changed.
   if ((node_title.compare(url.title()) != 0) ||
diff --git a/chrome/browser/sync/glue/typed_url_model_associator.h b/chrome/browser/sync/glue/typed_url_model_associator.h
index b88f6e7..32cddb7 100644
--- a/chrome/browser/sync/glue/typed_url_model_associator.h
+++ b/chrome/browser/sync/glue/typed_url_model_associator.h
@@ -105,41 +105,30 @@ class TypedUrlModelAssociator
                              const history::VisitVector* deleted_visits);
 
   // Bitfield returned from MergeUrls to specify the result of the merge.
-  enum {
-    // No changes were noted.
-    DIFF_NONE           = 0x0000,
-
-    // The local sync node needs to be modified.
-    DIFF_UPDATE_NODE   = 0x0001,
-
-    // The title changed in the local URLRow. DIFF_ROW_CHANGED will also be set
-    // if this is set.
-    DIFF_LOCAL_TITLE_CHANGED  = 0x0002,
-
-    // The local URLRow has changed (typed_count, visit_count, title, etc).
-    DIFF_LOCAL_ROW_CHANGED    = 0x0004,
-
-    // Visits need to be added to the local URLRow.
-    DIFF_LOCAL_VISITS_ADDED   = 0x0008,
-  };
+  typedef uint32 MergeResult;
+  static const MergeResult DIFF_NONE                = 0;
+  static const MergeResult DIFF_UPDATE_NODE         = 1 << 0;
+  static const MergeResult DIFF_LOCAL_TITLE_CHANGED = 1 << 1;
+  static const MergeResult DIFF_LOCAL_ROW_CHANGED   = 1 << 2;
+  static const MergeResult DIFF_LOCAL_VISITS_ADDED  = 1 << 3;
 
   // Merges the URL information in |typed_url| with the URL information from the
   // history database in |url| and |visits|, and returns a bitmask with the
   // results of the merge:
-  // DIFF_NODE_CHANGE - changes have been made to |new_url| and |visits| which
+  // DIFF_UPDATE_NODE - changes have been made to |new_url| and |visits| which
   //   should be persisted to the sync node.
-  // DIFF_TITLE_CHANGED - The title has changed, so the title in |new_url|
+  // DIFF_LOCAL_TITLE_CHANGED - The title has changed, so the title in |new_url|
   //   should be persisted to the history DB.
-  // DIFF_ROW_CHANGED - The history data in |new_url| should be persisted to the
-  //   history DB.
-  // DIFF_VISITS_ADDED - |new_visits| contains a list of visits that should be
-  //   written to the history DB for this URL. Deletions are not written to the
-  //   DB - each client is left to age out visits on their own.
-  static int MergeUrls(const sync_pb::TypedUrlSpecifics& typed_url,
-                       const history::URLRow& url,
-                       history::VisitVector* visits,
-                       history::URLRow* new_url,
-                       std::vector<history::VisitInfo>* new_visits);
+  // DIFF_LOCAL_ROW_CHANGED - The history data in |new_url| should be persisted
+  //   to the history DB.
+  // DIFF_LOCAL_VISITS_ADDED - |new_visits| contains a list of visits that
+  //   should be written to the history DB for this URL. Deletions are not
+  //   written to the DB - each client is left to age out visits on their own.
+  static MergeResult MergeUrls(const sync_pb::TypedUrlSpecifics& typed_url,
+                               const history::URLRow& url,
+                               history::VisitVector* visits,
+                               history::URLRow* new_url,
+                               std::vector<history::VisitInfo>* new_visits);
   static void WriteToSyncNode(const history::URLRow& url,
                               const history::VisitVector& visits,
                               sync_api::WriteNode* node);
diff --git a/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc b/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc
index 9f27385..5637fd8 100644
--- a/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc
+++ b/chrome/browser/sync/glue/typed_url_model_associator_unittest.cc
@@ -66,9 +66,8 @@ TEST_F(TypedUrlModelAssociatorTest, MergeUrls) {
                                                           3, false));
   history::URLRow new_row1(GURL("http://pie.com/"));
   std::vector<history::VisitInfo> new_visits1;
-  EXPECT_EQ(TypedUrlModelAssociator::DIFF_NONE,
-            TypedUrlModelAssociator::MergeUrls(specs1, row1, &visits1,
-                                               &new_row1, &new_visits1));
+  EXPECT_TRUE(TypedUrlModelAssociator::MergeUrls(specs1, row1, &visits1,
+      &new_row1, &new_visits1) == TypedUrlModelAssociator::DIFF_NONE);
 
   history::VisitVector visits2;
   history::URLRow row2(MakeTypedUrlRow("http://pie.com/", "pie",
@@ -81,9 +80,9 @@ TEST_F(TypedUrlModelAssociatorTest, MergeUrls) {
                                             2, 3, true, &expected_visits2));
   history::URLRow new_row2(GURL("http://pie.com/"));
   std::vector<history::VisitInfo> new_visits2;
-  EXPECT_EQ(TypedUrlModelAssociator::DIFF_LOCAL_ROW_CHANGED,
-            TypedUrlModelAssociator::MergeUrls(specs2, row2, &visits2,
-                                               &new_row2, &new_visits2));
+  EXPECT_TRUE(TypedUrlModelAssociator::MergeUrls(specs2, row2, &visits2,
+                                                 &new_row2, &new_visits2) ==
+      TypedUrlModelAssociator::DIFF_LOCAL_ROW_CHANGED);
   EXPECT_TRUE(URLsEqual(new_row2, expected2));
 
   history::VisitVector visits3;