[Sync] Remove ModelError::IsSet() in favor of base::Optional.

components/sync/device_info/device_info_sync_bridge_unittest.cc

Index: components/sync/device_info/device_info_sync_bridge_unittest.cc
diff --git a/components/sync/device_info/device_info_sync_bridge_unittest.cc b/components/sync/device_info/device_info_sync_bridge_unittest.cc
index 75f94aab7be0c095accc01e4c361e9b5335feb3f..e0f484d30ad115741765348e385c26c4c1de04f4 100644
--- a/components/sync/device_info/device_info_sync_bridge_unittest.cc
+++ b/components/sync/device_info/device_info_sync_bridge_unittest.cc
@@ -498,9 +498,9 @@ TEST_F(DeviceInfoSyncBridgeTest, GetAllData) {
 TEST_F(DeviceInfoSyncBridgeTest, ApplySyncChangesEmpty) {
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
-  const ModelError error = bridge()->ApplySyncChanges(
-      bridge()->CreateMetadataChangeList(), EntityChangeList());
-  EXPECT_FALSE(error.IsSet());
+  auto error = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
+                                          EntityChangeList());
+  EXPECT_FALSE(error);
   EXPECT_EQ(1, change_count());
 }
 
@@ -509,21 +509,21 @@ TEST_F(DeviceInfoSyncBridgeTest, ApplySyncChangesInMemory) {
   EXPECT_EQ(1, change_count());
 
   const DeviceInfoSpecifics specifics = CreateSpecifics(1);
-  const ModelError error_on_add = bridge()->ApplySyncChanges(
+  auto error_on_add = bridge()->ApplySyncChanges(
       bridge()->CreateMetadataChangeList(), EntityAddList({specifics}));
 
-  EXPECT_FALSE(error_on_add.IsSet());
+  EXPECT_FALSE(error_on_add);
   std::unique_ptr<DeviceInfo> info =
       bridge()->GetDeviceInfo(specifics.cache_guid());
   ASSERT_TRUE(info);
   VerifyEqual(specifics, *info.get());
   EXPECT_EQ(2, change_count());
 
-  const ModelError error_on_delete = bridge()->ApplySyncChanges(
+  auto error_on_delete = bridge()->ApplySyncChanges(
       bridge()->CreateMetadataChangeList(),
       {EntityChange::CreateDelete(specifics.cache_guid())});
 
-  EXPECT_FALSE(error_on_delete.IsSet());
+  EXPECT_FALSE(error_on_delete);
   EXPECT_FALSE(bridge()->GetDeviceInfo(specifics.cache_guid()));
   EXPECT_EQ(3, change_count());
 }
@@ -538,9 +538,9 @@ TEST_F(DeviceInfoSyncBridgeTest, ApplySyncChangesStore) {
       bridge()->CreateMetadataChangeList();
   metadata_changes->UpdateModelTypeState(state);
 
-  const ModelError error = bridge()->ApplySyncChanges(
-      std::move(metadata_changes), EntityAddList({specifics}));
-  EXPECT_FALSE(error.IsSet());
+  auto error = bridge()->ApplySyncChanges(std::move(metadata_changes),
+                                          EntityAddList({specifics}));
+  EXPECT_FALSE(error);
   EXPECT_EQ(2, change_count());
 
   RestartBridge();
@@ -575,25 +575,24 @@ TEST_F(DeviceInfoSyncBridgeTest, ApplySyncChangesWithLocalGuid) {
   EXPECT_LT(Time::Now() - TimeDelta::FromMinutes(1), last_updated);
   EXPECT_GT(Time::Now() + TimeDelta::FromMinutes(1), last_updated);
 
-  const ModelError error_on_add = bridge()->ApplySyncChanges(
+  auto error_on_add = bridge()->ApplySyncChanges(
       bridge()->CreateMetadataChangeList(), EntityAddList({specifics}));
-  EXPECT_FALSE(error_on_add.IsSet());
+  EXPECT_FALSE(error_on_add);
   EXPECT_EQ(1, change_count());
 
-  const ModelError error_on_delete = bridge()->ApplySyncChanges(
+  auto error_on_delete = bridge()->ApplySyncChanges(
       bridge()->CreateMetadataChangeList(),
       {EntityChange::CreateDelete(specifics.cache_guid())});
-  EXPECT_FALSE(error_on_delete.IsSet());
+  EXPECT_FALSE(error_on_delete);
   EXPECT_EQ(1, change_count());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, ApplyDeleteNonexistent) {
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
-  const ModelError error =
-      bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
-                                 {EntityChange::CreateDelete("guid")});
-  EXPECT_FALSE(error.IsSet());
+  auto error = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
+                                          {EntityChange::CreateDelete("guid")});
+  EXPECT_FALSE(error);
   EXPECT_EQ(1, change_count());
 }
 
@@ -605,23 +604,24 @@ TEST_F(DeviceInfoSyncBridgeTest, ClearProviderAndApply) {
   const DeviceInfoSpecifics specifics = CreateSpecifics(1, Time::Now());
 
   local_device()->Clear();
-  ModelError error = bridge()->ApplySyncChanges(
-      bridge()->CreateMetadataChangeList(), EntityAddList({specifics}));
-  EXPECT_FALSE(error.IsSet());
+  auto error1 = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
+                                           EntityAddList({specifics}));
+  EXPECT_FALSE(error1);
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
 
   local_device()->Initialize(CreateModel(kDefaultLocalSuffix));
-  error = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
-                                     EntityAddList({specifics}));
+  auto error2 = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
+                                           EntityAddList({specifics}));
+  EXPECT_FALSE(error2);
   EXPECT_EQ(2u, bridge()->GetAllDeviceInfo().size());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, MergeEmpty) {
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
-  const ModelError error = bridge()->MergeSyncData(
-      bridge()->CreateMetadataChangeList(), EntityDataMap());
-  EXPECT_FALSE(error.IsSet());
+  auto error = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),

[skym, 2017/01/10 16:24:42]

Does it make sense to now do things like:

EXPECT_FALSE(bridge()->Mergexxxxxxxxxxxxxxx);

Or is the variable name 'error' still that useful?

[maxbogue, 2017/01/10 17:53:50]

Generally if things are returning a boolean, false means failure. Given that,
I'm going to leave things this way so it's clear the check is that there isn't
an error.

+                                       EntityDataMap());
+  EXPECT_FALSE(error);
   EXPECT_EQ(1, change_count());
   // TODO(skym): Stop sending local twice. The first of the two puts will
   // probably happen before the processor is tracking metadata yet, and so there
@@ -651,10 +651,10 @@ TEST_F(DeviceInfoSyncBridgeTest, MergeWithData) {
       bridge()->CreateMetadataChangeList();
   metadata_changes->UpdateModelTypeState(state);
 
-  const ModelError error = bridge()->MergeSyncData(
+  auto error = bridge()->MergeSyncData(
       std::move(metadata_changes),
       InlineEntityDataMap({conflict_remote, unique_remote}));
-  EXPECT_FALSE(error.IsSet());
+  EXPECT_FALSE(error);
   EXPECT_EQ(2, change_count());
 
   // The remote should beat the local in conflict.
@@ -686,9 +686,9 @@ TEST_F(DeviceInfoSyncBridgeTest, MergeLocalGuid) {
   WriteToStore({specifics});
   InitializeAndPump();
 
-  const ModelError error = bridge()->MergeSyncData(
-      bridge()->CreateMetadataChangeList(), InlineEntityDataMap({specifics}));
-  EXPECT_FALSE(error.IsSet());
+  auto error = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
+                                       InlineEntityDataMap({specifics}));
+  EXPECT_FALSE(error);
   EXPECT_EQ(0, change_count());
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
   EXPECT_TRUE(processor()->delete_set().empty());
@@ -701,10 +701,10 @@ TEST_F(DeviceInfoSyncBridgeTest, MergeLocalGuidBeforeReconcile) {
   // The message loop is never pumped, which means local data/metadata is never
   // loaded, and thus reconcile is never called. The bridge should ignore this
   // EntityData because its cache guid is the same the local device's.
-  const ModelError error = bridge()->MergeSyncData(
+  auto error = bridge()->MergeSyncData(
       bridge()->CreateMetadataChangeList(),
       InlineEntityDataMap({CreateSpecifics(kDefaultLocalSuffix)}));
-  EXPECT_FALSE(error.IsSet());
+  EXPECT_FALSE(error);
   EXPECT_EQ(0, change_count());
   EXPECT_EQ(0u, bridge()->GetAllDeviceInfo().size());
 }
@@ -717,13 +717,15 @@ TEST_F(DeviceInfoSyncBridgeTest, ClearProviderAndMerge) {
   const DeviceInfoSpecifics specifics = CreateSpecifics(1, Time::Now());
 
   local_device()->Clear();
-  ModelError error = bridge()->MergeSyncData(
-      bridge()->CreateMetadataChangeList(), InlineEntityDataMap({specifics}));
-  EXPECT_FALSE(error.IsSet());
+  auto error1 = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
+                                        InlineEntityDataMap({specifics}));
+  EXPECT_FALSE(error1);
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
+
   local_device()->Initialize(CreateModel(kDefaultLocalSuffix));
-  error = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
-                                  InlineEntityDataMap({specifics}));
+  auto error2 = bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
+                                        InlineEntityDataMap({specifics}));
+  EXPECT_FALSE(error2);

[skym, 2017/01/10 16:24:42]

Good catch.

[maxbogue, 2017/01/10 17:53:50]

Acknowledged.

   EXPECT_EQ(2u, bridge()->GetAllDeviceInfo().size());
 }
 
@@ -795,10 +797,10 @@ TEST_F(DeviceInfoSyncBridgeTest, DisableSync) {
   EXPECT_EQ(1, change_count());
 
   const DeviceInfoSpecifics specifics = CreateSpecifics(1);
-  const ModelError error = bridge()->ApplySyncChanges(
-      bridge()->CreateMetadataChangeList(), EntityAddList({specifics}));
+  auto error = bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
+                                          EntityAddList({specifics}));
 
-  EXPECT_FALSE(error.IsSet());
+  EXPECT_FALSE(error);
   EXPECT_EQ(2u, bridge()->GetAllDeviceInfo().size());
   EXPECT_EQ(2, change_count());