SyncFS: Fix LocalSyncOperationResolverTest to cover all test cases

SyncFS: Fix LocalSyncOperationResolverTest to cover all test cases

To cover all test cases, this CL removes hard-coded parameters and adds inputs.


BUG=none
TEST=unit_tests --gtest_filter=\*LocalSyncOperationResolver\

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198773

[nhiroki, 2013-05-02 12:46:51]

PTAL, thanks!

[kinuko, 2013-05-07 04:26:10]

https://codereview.chromium.org/14851005/diff/4001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc
(right):

https://codereview.chromium.org/14851005/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc:19:
FileChange remote_file_change;
Maybe this could be scoped_ptr<FileChange> remote_file_change and can be NULL
when no remote change's there?

Similarly we may pass a ptr to remote_file_change to
LocalSyncOperationResolver::Resolve only when has_remote_change is true? (As we
do for drive_metadata)

https://codereview.chromium.org/14851005/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc:97:
for (ExpectedTypes::size_type i = 0; i < expected_types.size(); ++i)
nit: please put { } for multi-line body (here and in all tests)

[nhiroki, 2013-05-07 11:13:17]

Thanks, updated.

https://codereview.chromium.org/14851005/diff/4001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc
(right):

https://codereview.chromium.org/14851005/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc:19:
FileChange remote_file_change;
On 2013/05/07 04:26:10, kinuko wrote:
> Maybe this could be scoped_ptr<FileChange> remote_file_change and can be NULL
> when no remote change's there?
> 
> Similarly we may pass a ptr to remote_file_change to
> LocalSyncOperationResolver::Resolve only when has_remote_change is true? (As
we
> do for drive_metadata)

Done.

https://codereview.chromium.org/14851005/diff/4001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc:97:
for (ExpectedTypes::size_type i = 0; i < expected_types.size(); ++i)
On 2013/05/07 04:26:10, kinuko wrote:
> nit: please put { } for multi-line body (here and in all tests)

Done.

[kinuko, 2013-05-07 11:20:16]

looks good, some more nits

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_sync_operation_resolver.h (right):

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.h:33: static
LocalSyncOperationType Resolve(
Can you comment about parameters?

// |remote_file_change| is non-null when we have remote change for the file, ...

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.h:35: FileChange*
remote_file_change,
Can this be const pointer?

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.h:36:
DriveMetadata* drive_metadata);
ditto

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc
(right):

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc:72:
return vector.Pass();
Is this vector<Input> to ScopedVector<Input> change related to other changes you
wanted to make? (Just asking because the simple std::vector<Input> looked
fine/enough)

[kinuko, 2013-05-07 11:25:25]

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc
(right):

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver_unittest.cc:72:
return vector.Pass();
On 2013/05/07 11:20:16, kinuko wrote:
> Is this vector<Input> to ScopedVector<Input> change related to other changes
you
> wanted to make? (Just asking because the simple std::vector<Input> looked
> fine/enough)

Ah ok, I think I get it... scoped_ptr is not copyable.

[nhiroki, 2013-05-07 12:20:25]

PTAL

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_sync_operation_resolver.h (right):

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.h:33: static
LocalSyncOperationType Resolve(
On 2013/05/07 11:20:16, kinuko wrote:
> Can you comment about parameters?
> 
> // |remote_file_change| is non-null when we have remote change for the file,
...

Done.

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.h:35: FileChange*
remote_file_change,
On 2013/05/07 11:20:16, kinuko wrote:
> Can this be const pointer?

Done.

https://codereview.chromium.org/14851005/diff/18001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.h:36:
DriveMetadata* drive_metadata);
On 2013/05/07 11:20:16, kinuko wrote:
> ditto

Done.

[kinuko, 2013-05-07 12:29:54]

lgtm

[kinuko, 2013-05-07 12:30:05]

https://codereview.chromium.org/14851005/diff/21001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_sync_operation_resolver.cc (right):

https://codereview.chromium.org/14851005/diff/21001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.cc:17: if
(remote_file_change != NULL && remote_file_change->IsTypeUnknown())
nit: maybe just

if (remote_file_change && remote_file_change->IsTypeUnknown())

https://codereview.chromium.org/14851005/diff/21001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.cc:72: if
(remote_file_change == NULL) {
ditto.  if (!remote_file_change)

[nhiroki, 2013-05-07 12:41:33]

Thanks!

https://codereview.chromium.org/14851005/diff/21001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local_sync_operation_resolver.cc (right):

https://codereview.chromium.org/14851005/diff/21001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.cc:17: if
(remote_file_change != NULL && remote_file_change->IsTypeUnknown())
On 2013/05/07 12:30:05, kinuko wrote:
> nit: maybe just
> 
> if (remote_file_change && remote_file_change->IsTypeUnknown())

Done.

https://codereview.chromium.org/14851005/diff/21001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local_sync_operation_resolver.cc:72: if
(remote_file_change == NULL) {
On 2013/05/07 12:30:05, kinuko wrote:
> ditto.  if (!remote_file_change)

Done.

[commit-bot: I haz the power, 2013-05-07 12:41:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/14851005/18002

[commit-bot: I haz the power, 2013-05-07 12:48:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-07 13:13:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/14851005/18002

[commit-bot: I haz the power, 2013-05-07 18:57:03]

Change committed as 198773