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

components/sync/device_info/device_info_sync_bridge_unittest.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/sync/device_info/device_info_sync_bridge.h"
 
 #include <algorithm>
 #include <set>
 #include <utility>
 
@@ skipping 480 matching lines @@
   const std::map<std::string, DeviceInfoSpecifics> expected{
       {specifics1.cache_guid(), specifics1},
       {specifics2.cache_guid(), specifics2}};
   bridge()->GetData({specifics1.cache_guid(), specifics2.cache_guid()},
                     base::Bind(&VerifyDataBatch, expected));
 }
 
 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());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, ApplySyncChangesInMemory) {
   InitializeAndPump();
   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());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, ApplySyncChangesStore) {
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
 
   const DeviceInfoSpecifics specifics = CreateSpecifics(1);
   ModelTypeState state = StateWithEncryption("ekn");
   std::unique_ptr<MetadataChangeList> metadata_changes =
       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();
 
   std::unique_ptr<DeviceInfo> info =
       bridge()->GetDeviceInfo(specifics.cache_guid());
   ASSERT_TRUE(info);
   VerifyEqual(specifics, *info.get());
 
   EXPECT_TRUE(processor()->metadata());
@@ skipping 14 matching lines @@
   EXPECT_EQ(1, change_count());
   // Ensure |last_updated| is about now, plus or minus a little bit.
   const Time last_updated(ProtoTimeToTime(processor()
                                               ->put_multimap()
                                               .begin()
                                               ->second->specifics.device_info()
                                               .last_updated_timestamp()));
   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());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, ClearProviderAndApply) {
   // This will initialize the provider a first time.
   InitializeAndPump();
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
 
   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
   // should not be much overhead.
   EXPECT_EQ(2u, processor()->put_multimap().size());
   EXPECT_EQ(2u, processor()->put_multimap().count(
                     local_device()->GetLocalDeviceInfo()->guid()));
   EXPECT_EQ(0u, processor()->delete_set().size());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, MergeWithData) {
   const DeviceInfoSpecifics unique_local = CreateSpecifics(1);
   DeviceInfoSpecifics conflict_local = CreateSpecifics(2);
   DeviceInfoSpecifics conflict_remote = CreateSpecifics(3);
   const DeviceInfoSpecifics unique_remote = CreateSpecifics(4);
 
   const std::string conflict_guid = "conflict_guid";
   conflict_local.set_cache_guid(conflict_guid);
   conflict_remote.set_cache_guid(conflict_guid);
 
   WriteToStore({unique_local, conflict_local});
   InitializeAndPump();
   EXPECT_EQ(1, change_count());
 
   ModelTypeState state = StateWithEncryption("ekn");
   std::unique_ptr<MetadataChangeList> metadata_changes =
       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.
   EXPECT_EQ(4u, bridge()->GetAllDeviceInfo().size());
   VerifyEqual(unique_local,
               *bridge()->GetDeviceInfo(unique_local.cache_guid()).get());
   VerifyEqual(unique_remote,
               *bridge()->GetDeviceInfo(unique_remote.cache_guid()).get());
   VerifyEqual(conflict_remote, *bridge()->GetDeviceInfo(conflict_guid).get());
 
@@ skipping 11 matching lines @@
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, MergeLocalGuid) {
   // If not recent, then reconcile is going to try to send an updated version to
   // Sync, which makes interpreting change_count() more difficult.
   const DeviceInfoSpecifics specifics =
       CreateSpecifics(kDefaultLocalSuffix, Time::Now());
   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());
   EXPECT_TRUE(processor()->put_multimap().empty());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, MergeLocalGuidBeforeReconcile) {
   InitializeBridge();
 
   // 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());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, ClearProviderAndMerge) {
   // This will initialize the provider a first time.
   InitializeAndPump();
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
 
   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());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, CountActiveDevices) {
   InitializeAndPump();
   EXPECT_EQ(1, bridge()->CountActiveDevices());
 
   // Regardless of the time, these following two ApplySyncChanges(...) calls
   // have the same guid as the local device.
   bridge()->ApplySyncChanges(
@@ skipping 51 matching lines @@
   EXPECT_EQ(2, change_count());
   EXPECT_EQ(2u, processor()->put_multimap().size());
 }
 
 TEST_F(DeviceInfoSyncBridgeTest, DisableSync) {
   InitializeAndPump();
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
   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());
 
   // Should clear out all local data and notify observers.
   bridge()->DisableSync();
   EXPECT_EQ(0u, bridge()->GetAllDeviceInfo().size());
   EXPECT_EQ(3, change_count());
 
   // Reloading from storage shouldn't contain remote data.
   RestartBridge();
   EXPECT_EQ(1u, bridge()->GetAllDeviceInfo().size());
   EXPECT_EQ(4, change_count());
 }
 
 }  // namespace
 
 }  // namespace syncer