[Sync] Relax EnumSet semantics for some functions

Clean up some call sites to take advantage of relaxed
semantics.

Also add comment clarifying usage of EnumSet::Iterator.

BUG=
TEST=


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

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

4 files changed, 29 insertions, 12 deletions

diff --git a/chrome/browser/sync/engine/get_commit_ids_command.cc b/chrome/browser/sync/engine/get_commit_ids_command.cc
index aa90ccc..2fbd9ed 100644
--- a/chrome/browser/sync/engine/get_commit_ids_command.cc
+++ b/chrome/browser/sync/engine/get_commit_ids_command.cc
@@ -97,8 +97,7 @@ bool IsEntryReadyForCommit(syncable::ModelEnumSet encrypted_types,
   // We special case the nigori node because even though it is considered an
   // "encrypted type", not all nigori node changes require valid encryption
   // (ex: sync_tabs).
-  if (syncable::IsRealDataType(type) &&
-      (type != syncable::NIGORI) &&
+  if ((type != syncable::NIGORI) &&
       encrypted_types.Has(type) &&
       (passphrase_missing ||
        syncable::EntryNeedsEncryption(encrypted_types, entry))) {
@@ -112,7 +111,7 @@ bool IsEntryReadyForCommit(syncable::ModelEnumSet encrypted_types,
   }
 
   // Look at the throttled types.
-  if (syncable::IsRealDataType(type) && throttled_types.Has(type))
+  if (throttled_types.Has(type))
     return false;
 
   return true;
diff --git a/chrome/browser/sync/engine/nigori_util.cc b/chrome/browser/sync/engine/nigori_util.cc
index df9bd4d..9b089ec 100644
--- a/chrome/browser/sync/engine/nigori_util.cc
+++ b/chrome/browser/sync/engine/nigori_util.cc
@@ -71,8 +71,6 @@ bool EntryNeedsEncryption(ModelEnumSet encrypted_types,
 bool SpecificsNeedsEncryption(ModelEnumSet encrypted_types,
                               const sync_pb::EntitySpecifics& specifics) {
   const ModelType type = GetModelTypeFromSpecifics(specifics);
-  if (!syncable::IsRealDataType(type))
-    return false;
   if (type == PASSWORDS || type == NIGORI)
     return false;  // These types have their own encryption schemes.
   if (!encrypted_types.Has(type))
diff --git a/chrome/browser/sync/util/enum_set.h b/chrome/browser/sync/util/enum_set.h
index d127122..51d3afe 100644
--- a/chrome/browser/sync/util/enum_set.h
+++ b/chrome/browser/sync/util/enum_set.h
@@ -65,8 +65,16 @@ class EnumSet {
   //   Process(it.Get());
   // }
   //
-  // There are no guarantees as to what will happen if you modify an
-  // EnumSet while traversing it with an iterator.
+  // The iterator must not be outlived by the set.  In particular, the
+  // following is an error:
+  //
+  // EnumSet<...> SomeFn() { ... }
+  //
+  // /* ERROR */
+  // for (EnumSet<...>::Iterator it = SomeFun().First(); ...
+  //
+  // Also, there are no guarantees as to what will happen if you
+  // modify an EnumSet while traversing it with an iterator.
   class Iterator {
    public:
     // A default-constructed iterator can't do anything except check
@@ -148,7 +156,7 @@ class EnumSet {
   // self-mutating versions of Union, Intersection, and Difference
   // (defined below).
 
-  // Adds the given value to our set.
+  // Adds the given value (which must be in range) to our set.
   void Put(E value) {
     enums_.set(ToIndex(value));
   }
@@ -165,9 +173,11 @@ class EnumSet {
     enums_ &= other.enums_;
   }
 
-  // Removes the given value from our set.
+  // If the given value is in range, removes it from our set.
   void Remove(E value) {
-    enums_.reset(ToIndex(value));
+    if (InRange(value)) {
+      enums_.reset(ToIndex(value));
+    }
   }
 
   // Removes all values in the given set from our set.
@@ -180,9 +190,10 @@ class EnumSet {
     enums_.reset();
   }
 
-  // Returns true iff the given value is a member of our set.
+  // Returns true iff the given value is in range and a member of our
+  // set.
   bool Has(E value) const {
-    return enums_.test(ToIndex(value));
+    return InRange(value) && enums_.test(ToIndex(value));
   }
 
   // Returns true iff the given set is a subset of our set.
@@ -221,6 +232,10 @@ class EnumSet {
 
   explicit EnumSet(EnumBitSet enums) : enums_(enums) {}
 
+  static bool InRange(E value) {
+    return (value >= MinEnumValue) && (value <= MaxEnumValue);
+  }
+
   // Converts a value to/from an index into |enums_|.
 
   static size_t ToIndex(E value) {
diff --git a/chrome/browser/sync/util/enum_set_unittest.cc b/chrome/browser/sync/util/enum_set_unittest.cc
index 5e40dd0..6a8eecc 100644
--- a/chrome/browser/sync/util/enum_set_unittest.cc
+++ b/chrome/browser/sync/util/enum_set_unittest.cc
@@ -18,6 +18,7 @@ enum TestEnum {
   TEST_3,
   TEST_4,
   TEST_MAX = TEST_4,
+  TEST_5
 };
 
 typedef EnumSet<TestEnum, TEST_MIN, TEST_MAX> TestEnumSet;
@@ -108,11 +109,13 @@ TEST_F(EnumSetTest, RetainAll) {
 
 TEST_F(EnumSetTest, Remove) {
   TestEnumSet enums(TEST_3, TEST_4);
+  enums.Remove(TEST_0);
   enums.Remove(TEST_2);
   EXPECT_TRUE(enums.Equals(TestEnumSet(TEST_3, TEST_4)));
   enums.Remove(TEST_3);
   EXPECT_TRUE(enums.Equals(TestEnumSet(TEST_4)));
   enums.Remove(TEST_4);
+  enums.Remove(TEST_5);
   EXPECT_TRUE(enums.Empty());
 }
 
@@ -130,10 +133,12 @@ TEST_F(EnumSetTest, Clear) {
 
 TEST_F(EnumSetTest, Has) {
   const TestEnumSet enums(TEST_3, TEST_4);
+  EXPECT_FALSE(enums.Has(TEST_0));
   EXPECT_FALSE(enums.Has(TEST_1));
   EXPECT_FALSE(enums.Has(TEST_2));
   EXPECT_TRUE(enums.Has(TEST_3));
   EXPECT_TRUE(enums.Has(TEST_4));
+  EXPECT_FALSE(enums.Has(TEST_5));
 }
 
 TEST_F(EnumSetTest, HasAll) {