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

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

This is more idiomatic and cleaner.

BUG=673883
Review-Url: https://codereview.chromium.org/2623723002
Cr-Commit-Position: refs/heads/master@{#442661}
Committed: https://chromium.googlesource.com/chromium/src/+/2956e8ee01c51ffab5e2749e8b670855c8d11d99

[maxbogue, 2017-01-09 23:11:56]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 23:12:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2017-01-09 23:18:33]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 23:18:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2017-01-09 23:26:16]

maxbogue@chromium.org changed reviewers:
+ skym@chromium.org

[maxbogue, 2017-01-09 23:26:16]

Sky, PTAL! ModelError part 3: using base::Optional. I really like this change,
thanks for suggesting it!

[commit-bot: I haz the power, 2017-01-09 23:42:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-09 23:42:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[maxbogue, 2017-01-10 00:40:38]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 00:41:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 02:43:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 02:43:44]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

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

lgtm

https://codereview.chromium.org/2623723002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.mm (right):

https://codereview.chromium.org/2623723002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.mm:223: auto error =
reading_list_store_->MergeSyncData(std::move(metadata_changes),
Why are these error variables being stored? They're never used. Should they be
checked?

https://codereview.chromium.org/2623723002/diff/40001/components/sync/device_...
File components/sync/device_info/device_info_sync_bridge_unittest.cc (right):

https://codereview.chromium.org/2623723002/diff/40001/components/sync/device_...
components/sync/device_info/device_info_sync_bridge_unittest.cc:622: auto error
= bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
Does it make sense to now do things like:

EXPECT_FALSE(bridge()->Mergexxxxxxxxxxxxxxx);

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

https://codereview.chromium.org/2623723002/diff/40001/components/sync/device_...
components/sync/device_info/device_info_sync_bridge_unittest.cc:728:
EXPECT_FALSE(error2);
Good catch.

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

https://codereview.chromium.org/2623723002/diff/40001/components/reading_list...
File components/reading_list/ios/reading_list_store_unittest.mm (right):

https://codereview.chromium.org/2623723002/diff/40001/components/reading_list...
components/reading_list/ios/reading_list_store_unittest.mm:223: auto error =
reading_list_store_->MergeSyncData(std::move(metadata_changes),
On 2017/01/10 16:24:41, skym wrote:
> Why are these error variables being stored? They're never used. Should they be
> checked?

I don't know; I'll ask Olivier about it.

https://codereview.chromium.org/2623723002/diff/40001/components/sync/device_...
File components/sync/device_info/device_info_sync_bridge_unittest.cc (right):

https://codereview.chromium.org/2623723002/diff/40001/components/sync/device_...
components/sync/device_info/device_info_sync_bridge_unittest.cc:622: auto error
= bridge()->MergeSyncData(bridge()->CreateMetadataChangeList(),
On 2017/01/10 16:24:42, skym wrote:
> Does it make sense to now do things like:
> 
> EXPECT_FALSE(bridge()->Mergexxxxxxxxxxxxxxx);
> 
> Or is the variable name 'error' still that useful?

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.

https://codereview.chromium.org/2623723002/diff/40001/components/sync/device_...
components/sync/device_info/device_info_sync_bridge_unittest.cc:728:
EXPECT_FALSE(error2);
On 2017/01/10 16:24:42, skym wrote:
> Good catch.

Acknowledged.

[maxbogue, 2017-01-10 17:55:48]

maxbogue@chromium.org changed reviewers:
+ mathp@chromium.org, olivierrobin@chromium.org

[maxbogue, 2017-01-10 17:55:49]

+mathp for components/autofill/
+olivierrobin for components/reading_list/

Olivier, the error variables in reading_list_store_unittest.mm are never used; I
suspect that should be changed?

[Olivier, 2017-01-10 18:00:35]

On 2017/01/10 17:55:49, maxbogue wrote:
> +mathp for components/autofill/
> +olivierrobin for components/reading_list/
> 
> Olivier, the error variables in reading_list_store_unittest.mm are never used;
I
> suspect that should be changed?

Yes, I will add some asserts. They are missing.
Thanks

LGTM for the other changes.

[Mathieu, 2017-01-10 19:10:47]

autofill lgtm

[maxbogue, 2017-01-10 19:38:43]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2017-01-10 19:39:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 19:45:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484077123581830,
"parent_rev": "dac25810b3491c5fade87e92a66a285c5ca51d87", "commit_rev":
"2956e8ee01c51ffab5e2749e8b670855c8d11d99"}

[commit-bot: I haz the power, 2017-01-10 19:45:48]

Description was changed from

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

This is more idiomatic and cleaner.

BUG=673883
==========

to

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

This is more idiomatic and cleaner.

BUG=673883

Review-Url: https://codereview.chromium.org/2623723002
Cr-Commit-Position: refs/heads/master@{#442661}
Committed:
https://chromium.googlesource.com/chromium/src/+/2956e8ee01c51ffab5e2749e8b67...
==========

[commit-bot: I haz the power, 2017-01-10 19:45:50]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2956e8ee01c51ffab5e2749e8b67...