[SyncFS] Add MetadataDatabase::UpdateTracker

[SyncFS] Add MetadataDatabase::UpdateTracker

BUG=

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

[tzik, 2013-08-27 03:19:08]

PTL

[kinuko, 2013-08-27 07:47:05]

Sending out minor comments for now, I'll take another look later

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:736: // Local
file is deleted.
nit: the term 'local' is confusing here. Do you assume updated_details contain
local file change?

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:197: // Update
|synced_details| of the tracker to |updated_details|.
nit: to -> with

[kinuko, 2013-08-27 13:20:19]

Some more comments.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1034: bool
ignore_name_conflicted_trackers) {
nit: I'd name this bool parameter similar to the method name
RemoveTrackerIgnoringSiblings.

Or.. for simplicity could these three methods be combined into one with enum
params, say,

RemoveTracker(int tracker_id, RemoveTrackerMode mode, WriteBatch* batch);

enum RemoveTrackerMode {
  MARK_DIRTY_CONFLICTS_BY_PATH,
  IGNORE_CONFLICTS_BY_PATH,
};

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1266: return
false;
What does this case mean..?

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1297:
DCHECK(!new_title.empty());
We don't check this in UpdateTracker.  Is new_title guaranteed to be a non-empty
string? (Does it always refer to a local file name?)

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1311: //
Inactivate existing active tracker.
Why does the new one always win here?

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1313:
new_siblings->Inactivate(obstacle);
Do we need to check if the obstacle is KIND_REGULAR?

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1315:
TrackerSet* chain = &trackers_by_file_id_[obstacle->file_id()];
chain... ?

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1323:
new_siblings->Insert(tracker);
duplicated with line 1327?

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.proto
(right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.proto:92:
optional bool priority_low = 10;
Not used in this CL yet?

This name usually does not imply what the comment says.  If this flag is used
solely for the commented purpose should we rename the flag?  Or if it could be
used just to indicate it should be processed at later stages should the comment
be updated..?

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:925:
file.metadata.mutable_details()->set_title("renamed file");;
nit: I'm still often confused by this 'metadata' which actually means unsynced
server-side metadata... for us in the future could we have a bit verbose
comment, e.g.

// file.metadata.details() holds updated metadata which is going to be
// used to update file.tracker.synced_details() later.

[tzik, 2013-08-30 05:51:57]

Updated!

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:736: // Local
file is deleted.
On 2013/08/27 07:47:05, kinuko wrote:
> nit: the term 'local' is confusing here. Do you assume updated_details contain
> local file change? 

Yes, the updated_details should contain latest local file state.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1034: bool
ignore_name_conflicted_trackers) {
On 2013/08/27 13:20:19, kinuko wrote:
> nit: I'd name this bool parameter similar to the method name
> RemoveTrackerIgnoringSiblings.
> 
> Or.. for simplicity could these three methods be combined into one with enum
> params, say,
> 
> RemoveTracker(int tracker_id, RemoveTrackerMode mode, WriteBatch* batch);
> 
> enum RemoveTrackerMode {
>   MARK_DIRTY_CONFLICTS_BY_PATH,
>   IGNORE_CONFLICTS_BY_PATH,
> };

Done.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1266: return
false;
On 2013/08/27 13:20:19, kinuko wrote:
> What does this case mean..?

This means the trackers is out of any app-root tree, e.g. a direct child of the
sync-root which is not a app-root.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1297:
DCHECK(!new_title.empty());
On 2013/08/27 13:20:19, kinuko wrote:
> We don't check this in UpdateTracker.  Is new_title guaranteed to be a
non-empty
> string? (Does it always refer to a local file name?)

New title should not be empty unless the file is deleted locally.
And every title update is handled here. So, I think it's enough to check it
here.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1311: //
Inactivate existing active tracker.
On 2013/08/27 13:20:19, kinuko wrote:
> Why does the new one always win here?

The file is already updated locally here by the sync engine.
Deciding winner should be performed elsewhere in it.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1313:
new_siblings->Inactivate(obstacle);
On 2013/08/27 13:20:19, kinuko wrote:
> Do we need to check if the obstacle is KIND_REGULAR?

Done.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1315:
TrackerSet* chain = &trackers_by_file_id_[obstacle->file_id()];
On 2013/08/27 13:20:19, kinuko wrote:
> chain... ?

Done.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1323:
new_siblings->Insert(tracker);
On 2013/08/27 13:20:19, kinuko wrote:
> duplicated with line 1327?

Done.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:197: // Update
|synced_details| of the tracker to |updated_details|.
On 2013/08/27 07:47:05, kinuko wrote:
> nit: to -> with

Done.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.proto
(right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.proto:92:
optional bool priority_low = 10;
On 2013/08/27 13:20:19, kinuko wrote:
> Not used in this CL yet?
> 
> This name usually does not imply what the comment says.  If this flag is used
> solely for the commented purpose should we rename the flag?  Or if it could be
> used just to indicate it should be processed at later stages should the
comment
> be updated..?

Reverted.
It's just a merge failure in my local repo.

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:925:
file.metadata.mutable_details()->set_title("renamed file");;
On 2013/08/27 13:20:19, kinuko wrote:
> nit: I'm still often confused by this 'metadata' which actually means unsynced
> server-side metadata... for us in the future could we have a bit verbose
> comment, e.g.
> 
> // file.metadata.details() holds updated metadata which is going to be
> // used to update file.tracker.synced_details() later.

Done.
I added some comment to the struct declaration.

[kinuko, 2013-08-30 06:03:46]

ok, lgtm

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/23288007/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1297:
DCHECK(!new_title.empty());
On 2013/08/30 05:51:57, tzik wrote:
> On 2013/08/27 13:20:19, kinuko wrote:
> > We don't check this in UpdateTracker.  Is new_title guaranteed to be a
> non-empty
> > string? (Does it always refer to a local file name?)
> 
> New title should not be empty unless the file is deleted locally.
> And every title update is handled here. So, I think it's enough to check it
> here.

Ok, I assume title cannot be assigned to an empty string.

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1036: bool
remove_tracker_ignoring_siblings) {
nit: well... I think just ignore_siblings is fine

[nhiroki, 2013-08-30 06:16:26]

lgtm

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:198: // Update
|synced_details| of the tracker with |updated_details|.
nit: s/Update/Updates/

[tzik, 2013-08-30 07:11:28]

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1036: bool
remove_tracker_ignoring_siblings) {
On 2013/08/30 06:03:46, kinuko wrote:
> nit: well... I think just ignore_siblings is fine

Done.

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.h (right):

https://codereview.chromium.org/23288007/diff/29001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.h:198: // Update
|synced_details| of the tracker with |updated_details|.
On 2013/08/30 06:16:27, nhiroki wrote:
> nit: s/Update/Updates/

Done.

[commit-bot: I haz the power, 2013-08-30 07:12:05]

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

[commit-bot: I haz the power, 2013-08-30 12:39:13]

Change committed as 220565