SyncFS: Introduce RemoteSyncOperationResolver for directory operation support

SyncFS: Introduce RemoteSyncOperationResolver for directory operation support

To support directory operations in SyncFileSystem, this patch introduces
RemoteSyncOperationResolver to determine remote operation type.


BUG=231852
TEST=unit_tests, manual tests

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

[nhiroki, 2013-04-23 11:57:53]

Hi super-syncers!
Can you review this? Thanks!

[kinuko, 2013-04-24 02:32:32]

some high-level comments

https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1569: bool
missing_local_file) {
It looks this doesn't need entire ProcessRemoteChangeParam but only need
local_file_type and is_conflicting flag.

I think this method could be separated out from this class and can have its own
unittest.
And then we can verify this logic solely not by inline-comments but in a full
matrix test?

https://codereview.chromium.org/13986011/diff/5001/webkit/fileapi/syncable/fi...
File webkit/fileapi/syncable/file_change.h (right):

https://codereview.chromium.org/13986011/diff/5001/webkit/fileapi/syncable/fi...
webkit/fileapi/syncable/file_change.h:38: bool IsDeleteDirectory() const {
return IsDelete() && IsDirectory(); }
These methods are good for DCHECKs but I'm worried using them in a conditional
check as it makes easier to miss possible patterns

[kinuko, 2013-04-24 02:42:29]

https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1569: bool
missing_local_file) {
On 2013/04/24 02:32:32, kinuko wrote:
> It looks this doesn't need entire ProcessRemoteChangeParam but only need
> local_file_type and is_conflicting flag.
> 
> I think this method could be separated out from this class and can have its
own
> unittest.

If this takes too much your time it'd be still nicer to add a unittest for this
method.

> And then we can verify this logic solely not by inline-comments but in a full
> matrix test?

[tzik, 2013-04-24 03:39:07]

lgtm

[nhiroki, 2013-04-24 13:29:05]

Thanks for your review.

This patchset#2 factors out ResolveSyncOperationType function to
RemoteSyncOperationResolver class, and adds a matrix test for it.

Please take another look. Thanks!

https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/13986011/diff/5001/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1569: bool
missing_local_file) {
On 2013/04/24 02:32:32, kinuko wrote:
> It looks this doesn't need entire ProcessRemoteChangeParam but only need
> local_file_type and is_conflicting flag.
> 
> I think this method could be separated out from this class and can have its
own
> unittest.
> And then we can verify this logic solely not by inline-comments but in a full
> matrix test?

Done.

https://codereview.chromium.org/13986011/diff/5001/webkit/fileapi/syncable/fi...
File webkit/fileapi/syncable/file_change.h (right):

https://codereview.chromium.org/13986011/diff/5001/webkit/fileapi/syncable/fi...
webkit/fileapi/syncable/file_change.h:38: bool IsDeleteDirectory() const {
return IsDelete() && IsDirectory(); }
On 2013/04/24 02:32:32, kinuko wrote:
> These methods are good for DCHECKs but I'm worried using them in a conditional
> check as it makes easier to miss possible patterns

hmm... I think this seems straightforward and useful for building conditional
branches. Anyway I'll find a way to implement them simlply without these helper
functions.

[kinuko, 2013-04-24 14:07:59]

Phew... I seem to have made you to produce a lot more code.
Replying to the high-level comment first, review comments will come next.

https://codereview.chromium.org/13986011/diff/5001/webkit/fileapi/syncable/fi...
File webkit/fileapi/syncable/file_change.h (right):

https://codereview.chromium.org/13986011/diff/5001/webkit/fileapi/syncable/fi...
webkit/fileapi/syncable/file_change.h:38: bool IsDeleteDirectory() const {
return IsDelete() && IsDirectory(); }
On 2013/04/24 13:29:05, nhiroki wrote:
> On 2013/04/24 02:32:32, kinuko wrote:
> > These methods are good for DCHECKs but I'm worried using them in a
conditional
> > check as it makes easier to miss possible patterns
> 
> hmm... I think this seems straightforward and useful for building conditional
> branches. Anyway I'll find a way to implement them simlply without these
helper
> functions.

If it's really useful that's fine of course, I meant I usually prefer
switch/case because we can be warned when we have missing cases and it's easier
to add new cases.  In this case we can also assume we won't add any more
combinations (so that these 4 convenient methods will remain complete), but in
general.

[kinuko, 2013-04-24 14:54:50]

Looking good, thanks for adding all these tests. I think we can probably reduce
the code slightly more. (Please feel free to just ping me when any of my
comments don't make sense. Thx!)

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right):

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:16: if
(remote_file_change.IsAddOrUpdate()) {
nit: can we use switch/case instead?

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:49:
local_changes.list().back().IsDelete())
You can add back() method to FileChangeList (we already have front())

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:56: return
REMOTE_SYNC_OPERATION_CONFLICT;
Does it make sense to change line 55-56 to:

DCHECK(local_changes.list().back().IsAddOrUpdate());
DCHECK(local_changes.list().back().IsFile());
return ...

Could other patterns come? Or you wrote this as if-clause because of testing,
i.e. to compare the return value with OPERATION_FAIL?  (I guess the latter is
yes)

Ok... how about having sanity checks like following in Resolve() (where we seem
to be able to have DCHECKs):

DCHECK(local_file_type == SYNC_FILE_TYPE_UNKNOWN ||
       local_changes.empty() ||
       local_file_type == local_changes.back().file_type())
  << local_file_type << " / " << local_changes.DebugString();

Probably we can extract the check part in a static method like:

bool IsValidCombination(local_changes, local_file_type);

and have DCHECK in Resolve() and return REMOTE_SYNC_OPERATION_FAIL at the
beginning of each sub-methods. Then the remaining switch/case body could become
slightly more simple?

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.h (right):

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.h:15: enum
RemoteSyncOperationType {
If this is a nested enum in a class, I think the enum name could be a lot
shorter (like Operation, OperationType or ResolvedOperation), or you can move
this enum out of the class.

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver_unittest.cc
(right):

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver_unittest.cc:26:
FileChangeList CreateNoneChange() {
nit: None -> Empty?

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver_unittest.cc:91:
void VerifyResolveForAddOrUpdateFile(
Are these Verify* helper methods in order to access private methods from this
class?

Can we use FRIEND_TEST_ALL_PREFIXES() and remove these methods?

Also matrix test cases usually make it harder to find where the test's failing,
it'll be more helpful to add some logging like

for (... i = 0; i < expected_types.size(); ++i) {
  EXPECT_EQ(...) << "Case " << i;

[nhiroki, 2013-04-25 06:54:43]

Thanks! Updated.

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right):

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:16: if
(remote_file_change.IsAddOrUpdate()) {
On 2013/04/24 14:54:50, kinuko wrote:
> nit: can we use switch/case instead?

Done.

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:49:
local_changes.list().back().IsDelete())
On 2013/04/24 14:54:50, kinuko wrote:
> You can add back() method to FileChangeList (we already have front())

Done.

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:56: return
REMOTE_SYNC_OPERATION_CONFLICT;
On 2013/04/24 14:54:50, kinuko wrote:
> Does it make sense to change line 55-56 to:
> 
> DCHECK(local_changes.list().back().IsAddOrUpdate());
> DCHECK(local_changes.list().back().IsFile());
> return ...
> 
> Could other patterns come? Or you wrote this as if-clause because of testing,
> i.e. to compare the return value with OPERATION_FAIL?  (I guess the latter is
> yes)
> 
> Ok... how about having sanity checks like following in Resolve() (where we
seem
> to be able to have DCHECKs):
> 
> DCHECK(local_file_type == SYNC_FILE_TYPE_UNKNOWN ||
>        local_changes.empty() ||
>        local_file_type == local_changes.back().file_type())
>   << local_file_type << " / " << local_changes.DebugString();
> 
> Probably we can extract the check part in a static method like:
> 
> bool IsValidCombination(local_changes, local_file_type);
> 
> and have DCHECK in Resolve() and return REMOTE_SYNC_OPERATION_FAIL at the
> beginning of each sub-methods. Then the remaining switch/case body could
become
> slightly more simple?

Your idea looks good. It enables us to remove helper functions of FileChange.
Added IsValidCombination().

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.h (right):

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.h:15: enum
RemoteSyncOperationType {
On 2013/04/24 14:54:50, kinuko wrote:
> If this is a nested enum in a class, I think the enum name could be a lot
> shorter (like Operation, OperationType or ResolvedOperation), or you can move
> this enum out of the class.

Done.

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver_unittest.cc
(right):

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver_unittest.cc:26:
FileChangeList CreateNoneChange() {
On 2013/04/24 14:54:50, kinuko wrote:
> nit: None -> Empty?

Done.

https://codereview.chromium.org/13986011/diff/14001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver_unittest.cc:91:
void VerifyResolveForAddOrUpdateFile(
On 2013/04/24 14:54:50, kinuko wrote:
> Are these Verify* helper methods in order to access private methods from this
> class?
> 
> Can we use FRIEND_TEST_ALL_PREFIXES() and remove these methods?
> 
> Also matrix test cases usually make it harder to find where the test's
failing,
> it'll be more helpful to add some logging like
> 
> for (... i = 0; i < expected_types.size(); ++i) {
>   EXPECT_EQ(...) << "Case " << i;

Done.

[kinuko, 2013-04-25 08:34:10]

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right):

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:15: if
(local_file_type == SYNC_FILE_TYPE_UNKNOWN ||
I think this one can be probably more improved. Looks like we have really many
common non-valid cases.

I'd write like following (have tried this in my local env):

bool IsValidCombination(const FileChangeList& local_changes,
                        SyncFileType local_file_type,
                        bool is_conflicting) {
  if (local_changes.empty()) {
    // We never leave directory or non-existing entry in a conflicting state.
    return !(is_conflicting && local_file_type != SYNC_FILE_TYPE_FILE);
  }
  switch (local_file_type) {
    case SYNC_FILE_TYPE_UNKNOWN:
      return local_changes.back().IsDelete();
    case SYNC_FILE_TYPE_FILE:
    case SYNC_FILE_TYPE_DIRECTORY:
      return !local_changes.back().IsDelete() &&
             local_changes.back().file_type() == local_file_type;
  }
  return false;
}

Then we should be able to get rid of most of if conditionals in the sub-methods.
 And if it makes some tests fail (it actually does), the test expectations could
be wrong.

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:29:
DCHECK(IsValidCombination(local_changes, local_file_type));
Hmm, maybe this one's a bit too verbose... sorry could you remove this line? (We
can DCHECK to see if Resolve's return value is not FAIL)

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:90: return
REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
Can we add a comment:
// Currently we always prioritize directories over files.

[nhiroki, 2013-04-25 10:29:27]

Updated, thanks!

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right):

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:15: if
(local_file_type == SYNC_FILE_TYPE_UNKNOWN ||
On 2013/04/25 08:34:10, kinuko wrote:
> I think this one can be probably more improved. Looks like we have really many
> common non-valid cases.
> 
> I'd write like following (have tried this in my local env):
> 
> bool IsValidCombination(const FileChangeList& local_changes,
>                         SyncFileType local_file_type,
>                         bool is_conflicting) {
>   if (local_changes.empty()) {
>     // We never leave directory or non-existing entry in a conflicting state.
>     return !(is_conflicting && local_file_type != SYNC_FILE_TYPE_FILE);
>   }
>   switch (local_file_type) {
>     case SYNC_FILE_TYPE_UNKNOWN:
>       return local_changes.back().IsDelete();
>     case SYNC_FILE_TYPE_FILE:
>     case SYNC_FILE_TYPE_DIRECTORY:
>       return !local_changes.back().IsDelete() &&
>              local_changes.back().file_type() == local_file_type;
>   }
>   return false;
> }
> 
> Then we should be able to get rid of most of if conditionals in the
sub-methods.
>  And if it makes some tests fail (it actually does), the test expectations
could
> be wrong.

Done! I found some wrong expectations in the design doc. I fixed the doc and
reflected it in this patch.

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:29:
DCHECK(IsValidCombination(local_changes, local_file_type));
On 2013/04/25 08:34:10, kinuko wrote:
> Hmm, maybe this one's a bit too verbose... sorry could you remove this line?
(We
> can DCHECK to see if Resolve's return value is not FAIL)

Done.

https://codereview.chromium.org/13986011/diff/24001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:90: return
REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL;
On 2013/04/25 08:34:10, kinuko wrote:
> Can we add a comment:
> // Currently we always prioritize directories over files.

Done.

[kinuko, 2013-04-25 14:07:54]

Thanks, a few more comments.

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right):

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:17: return
!(is_conflicting && local_file_type != SYNC_FILE_TYPE_FILE);
Can you add a comment:

// We never leave directory or non-existing entry in conflicting state, so
// if there're no local changes it shouldn't be in conflicting state or
// should be a file.

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:112: return
REMOTE_SYNC_OPERATION_FAIL;
It could be valid when we have local changes-- what if there was a file and in
conflicting state, and then it's removed and new directory is created.
So returning REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL seems correct

(After updating IsValidCombination the comment looks stale, could you remove the
comment?)

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:153: return
REMOTE_SYNC_OPERATION_FAIL;
ditto.  We just need to clear the conflicting flag in this case?

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:170: return
REMOTE_SYNC_OPERATION_NONE;
DELETE_METADATA in either case?

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:196: return
REMOTE_SYNC_OPERATION_FAIL;
ditto.  RESOLVE_TO_LOCAL?

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:237: return
REMOTE_SYNC_OPERATION_FAIL;
ditto.  RESOLVE_TO_LOCAL?

[nhiroki, 2013-04-26 06:04:00]

PTAL

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/remote_sync_operation_resolver.cc (right):

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:17: return
!(is_conflicting && local_file_type != SYNC_FILE_TYPE_FILE);
On 2013/04/25 14:07:54, kinuko wrote:
> Can you add a comment:
> 
> // We never leave directory or non-existing entry in conflicting state, so
> // if there're no local changes it shouldn't be in conflicting state or
> // should be a file.

Done.

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:112: return
REMOTE_SYNC_OPERATION_FAIL;
On 2013/04/25 14:07:54, kinuko wrote:
> It could be valid when we have local changes-- what if there was a file and in
> conflicting state, and then it's removed and new directory is created.
> So returning REMOTE_SYNC_OPERATION_RESOLVE_TO_LOCAL seems correct
> 
> (After updating IsValidCombination the comment looks stale, could you remove
the
> comment?)

Oh... yes, it's correct. Sorry for wrong fixing. Done.

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:153: return
REMOTE_SYNC_OPERATION_FAIL;
On 2013/04/25 14:07:54, kinuko wrote:
> ditto.  We just need to clear the conflicting flag in this case?

It'd be ok to return RESOLVE_TO_LOCAL to clear the flag. Done.

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:170: return
REMOTE_SYNC_OPERATION_NONE;
On 2013/04/25 14:07:54, kinuko wrote:
> DELETE_METADATA in either case?

Done.

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:196: return
REMOTE_SYNC_OPERATION_FAIL;
On 2013/04/25 14:07:54, kinuko wrote:
> ditto.  RESOLVE_TO_LOCAL?

Done.

https://codereview.chromium.org/13986011/diff/4004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/remote_sync_operation_resolver.cc:237: return
REMOTE_SYNC_OPERATION_FAIL;
On 2013/04/25 14:07:54, kinuko wrote:
> ditto.  RESOLVE_TO_LOCAL?

Done.

[kinuko, 2013-04-26 06:12:07]

lgtm

Fix later if we find new problems :)

[nhiroki, 2013-04-26 06:16:52]

+jam@

Hi jam, could you review for chrome/chrome_tests_unit.gypi? Thanks.

[jam, 2013-04-26 15:43:27]

On 2013/04/26 06:16:52, nhiroki wrote:
> +jam@
> 
> Hi jam, could you review for chrome/chrome_tests_unit.gypi? Thanks.

please see chrome/OWNERS (i'm too overloaded to do reviews there, so i'm not in
the reviewer section)

[nhiroki, 2013-04-29 15:51:22]

On 2013/04/26 15:43:27, jam wrote:
> On 2013/04/26 06:16:52, nhiroki wrote:
> > +jam@
> > 
> > Hi jam, could you review for chrome/chrome_tests_unit.gypi? Thanks.
> 
> please see chrome/OWNERS (i'm too overloaded to do reviews there, so i'm not
in
> the reviewer section)

jam: I'm sorry, I was misunderstanding the description in the OWNERS file.

[nhiroki, 2013-04-29 15:58:32]

+thestig@

Hi Lei, could you review chrome/chrome_tests_gypi? Thanks!

[Lei Zhang, 2013-04-29 19:44:07]

lgtm

[commit-bot: I haz the power, 2013-04-29 19:44:19]

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

[commit-bot: I haz the power, 2013-04-29 22:46:42]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-04-30 01:49:20]

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

[commit-bot: I haz the power, 2013-04-30 03:17:28]

Change committed as 197245