Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10966003:
Add LocalFileChangeTracker database to record non-synced dirty files (Closed)
|
Created:
8 years, 3 months ago by nhiroki Modified:
8 years, 2 months ago CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch, darin-cc_chromium.org, calvinlo Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd LocalFileChangeTracker database to record non-synced dirty files
BUG=148886
TEST=content_unittests --gtest_filter=\*LocalFileChangeTrackerTest\*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158791
Patch Set 1 #
Total comments: 16
Patch Set 2 : Add serialize and deserialize methods #Patch Set 3 : Fix #
Total comments: 12
Patch Set 4 : Reflect comments #
Total comments: 8
Patch Set 5 : Reflect comments #
Total comments: 6
Patch Set 6 : Clean up #Patch Set 7 : Normalize expected file path #
Messages
Total messages: 18 (0 generated)
Can you review this? Thanks!
http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:229: << from_here.ToString() << " with error: " << status.ToString(); (Not directly related to this cl but would be nice to have these common methods somewhere else...) http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:252: INIT_STATUS_UNKNOWN_ERROR, INIT_STATUS_MAX); Adding UMA is great, but can we not add these in the initial commits? http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:256: void LocalFileChangeTracker::TrackerDB::MarkDirty(const FileSystemURL& url) { Can we return success/fail here? http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:265: leveldb::Status status = db_->Put(leveldb::WriteOptions(), url.spec(), kMark); I'm afraid Current spec() impl does not maintain all information FileSystemURL has. We'd probably need a safe serialization / deserialization method. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:274: void LocalFileChangeTracker::TrackerDB::ClearDirty(const FileSystemURL& url) { Can we return success/fail here?
http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:203: if (Repair(path)) can be just return Repair(path); http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:215: if (leveldb::RepairDB(db_path, leveldb::Options()).ok() || We might lose some database entry on this method. Can we perform some consistency check with DB to FileSystem? Or, could you just leave some TODO comment to do it later. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:42: base::MessageLoopProxy::current())); Could you move them into SetUp()?
Thanks for reviewing! http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:203: if (Repair(path)) On 2012/09/20 05:24:50, tzik wrote: > can be just return Repair(path); Done. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:215: if (leveldb::RepairDB(db_path, leveldb::Options()).ok() || On 2012/09/20 05:24:50, tzik wrote: > We might lose some database entry on this method. > Can we perform some consistency check with DB to FileSystem? > > Or, could you just leave some TODO comment to do it later. For now, added TODO comments. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:229: << from_here.ToString() << " with error: " << status.ToString(); On 2012/09/20 05:09:11, kinuko wrote: > (Not directly related to this cl but would be nice to have these common methods > somewhere else...) Added TODO comments. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:252: INIT_STATUS_UNKNOWN_ERROR, INIT_STATUS_MAX); On 2012/09/20 05:09:11, kinuko wrote: > Adding UMA is great, but can we not add these in the initial commits? Okay, removed it. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:256: void LocalFileChangeTracker::TrackerDB::MarkDirty(const FileSystemURL& url) { On 2012/09/20 05:09:11, kinuko wrote: > Can we return success/fail here? Done. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:265: leveldb::Status status = db_->Put(leveldb::WriteOptions(), url.spec(), kMark); On 2012/09/20 05:09:11, kinuko wrote: > I'm afraid Current spec() impl does not maintain all information FileSystemURL > has. We'd probably need a safe serialization / deserialization method. Added serialize/deserialize methods. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker.cc:274: void LocalFileChangeTracker::TrackerDB::ClearDirty(const FileSystemURL& url) { On 2012/09/20 05:09:11, kinuko wrote: > Can we return success/fail here? Done. http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/1/webkit/fileapi/syncable/local_... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:42: base::MessageLoopProxy::current())); On 2012/09/20 05:24:50, tzik wrote: > Could you move them into SetUp()? Done.
Sorry, I found my mistake in patchset2. Now I have no time, so I'll fix it later. Thanks
On 2012/09/20 09:43:42, nhiroki wrote: > Sorry, I found my mistake in patchset2. > Now I have no time, so I'll fix it later. > > Thanks Fixed. Can you take another look? Thanks
Looks good, overall http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:241: db_disabled_ = true; Could you reset db_ to NULL here? http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:248: db_disabled_ = true; ditto http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:259: db_disabled_ = true; ditto http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:266: db_disabled_ = true; ditto http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:42: void SetUp() { nit: please add "virtual" and "OVERRIDE" http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:150: Could you test to ensure we can use the database correctly? That is, - create new change tracker, - store something, - drop the tracker, - create another tracker, - ensure stored something exists
http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:241: db_disabled_ = true; On 2012/09/21 04:16:32, tzik wrote: > Could you reset db_ to NULL here? Done. http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:248: db_disabled_ = true; On 2012/09/21 04:16:32, tzik wrote: > ditto Done. http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:259: db_disabled_ = true; On 2012/09/21 04:16:32, tzik wrote: > ditto Done. http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:266: db_disabled_ = true; On 2012/09/21 04:16:32, tzik wrote: > ditto Done. http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:42: void SetUp() { On 2012/09/21 04:16:32, tzik wrote: > nit: please add "virtual" and "OVERRIDE" Done. http://codereview.chromium.org/10966003/diff/3002/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:150: On 2012/09/21 04:16:32, tzik wrote: > Could you test to ensure we can use the database correctly? > That is, > - create new change tracker, > - store something, > - drop the tracker, > - create another tracker, > - ensure stored something exists I think a method to get dirty urls from DB is useful for writing such unittest. I mean to add the method and the test in the next CL.
I see. LGTM
http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:175: return FileSystemURL(GURL(url)); I prefer making this method like: bool DeserializeExternalFileSystemURL(const std::string& serialized_url, FileSystemURL* url) and return url->is_valid() http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:219: // cloud-backed file system. nit: cloud-backed -> syncable http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:260: if (!Init(REPAIR_ON_CORRUPTION)) { Not sure if this could get called before initialization is done. Maybe we could just DCHECK(db_.get()) ? http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:144: FileSystemURL url = FileSystemURL(GURL(kExternalFileSystemURL)); Maybe if this file system gets mature we might add the official public type to handle this. It could be a separate bug but how about: - adding a separate syncable_file_system_util and having following methods: * RegisterSyncableFileSystem(origin, service_name /* == mount_name in external fs internally */) * CreateSyncableFileSystemURL(origin, service_name) * SerializableSyncableFileSystemURL(url) * DeserializeSyncableFileSystemURL(serialized_url, url)
Thank you for reviewing. Can you take another look? Thanks! http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:175: return FileSystemURL(GURL(url)); On 2012/09/21 08:12:32, kinuko wrote: > I prefer making this method like: > > bool DeserializeExternalFileSystemURL(const std::string& serialized_url, > FileSystemURL* url) > > and return url->is_valid() Done. http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:219: // cloud-backed file system. On 2012/09/21 08:12:32, kinuko wrote: > nit: cloud-backed -> syncable Done. http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker.cc:260: if (!Init(REPAIR_ON_CORRUPTION)) { On 2012/09/21 08:12:32, kinuko wrote: > Not sure if this could get called before initialization is done. Maybe we could > just DCHECK(db_.get()) ? I understand. Yes, we can just do DCHECK(). http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/1007/webkit/fileapi/syncable/loc... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:144: FileSystemURL url = FileSystemURL(GURL(kExternalFileSystemURL)); On 2012/09/21 08:12:32, kinuko wrote: > Maybe if this file system gets mature we might add the official public type to > handle this. It could be a separate bug but how about: > - adding a separate syncable_file_system_util and having following methods: > * RegisterSyncableFileSystem(origin, service_name /* == mount_name in external > fs internally */) > * CreateSyncableFileSystemURL(origin, service_name) > * SerializableSyncableFileSystemURL(url) > * DeserializeSyncableFileSystemURL(serialized_url, url) Okay, I filed it (http://crbug.com/151876), and I'll make a separate patch.
lgtm http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... webkit/fileapi/syncable/local_file_change_tracker.cc:52: base::Time last_reported_time_; Not used? http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... webkit/fileapi/syncable/local_file_change_tracker.cc:156: MarkDirtyOnDatabase(url); We need to propagate the error code up to UI thread (so that eventually ). Can you file a bug about that and add a TODO here? http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:146: FileSystemURL url = FileSystemURL(GURL(kExternalFileSystemURL)); this could be const too?
Still LGTM
Thanks for your comments! http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... File webkit/fileapi/syncable/local_file_change_tracker.cc (right): http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... webkit/fileapi/syncable/local_file_change_tracker.cc:52: base::Time last_reported_time_; On 2012/09/25 04:25:17, kinuko wrote: > Not used? Done. http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... webkit/fileapi/syncable/local_file_change_tracker.cc:156: MarkDirtyOnDatabase(url); On 2012/09/25 04:25:17, kinuko wrote: > We need to propagate the error code up to UI thread (so that eventually ). Can > you file a bug about that and add a TODO here? Done. http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... File webkit/fileapi/syncable/local_file_change_tracker_unittest.cc (right): http://codereview.chromium.org/10966003/diff/16001/webkit/fileapi/syncable/lo... webkit/fileapi/syncable/local_file_change_tracker_unittest.cc:146: FileSystemURL url = FileSystemURL(GURL(kExternalFileSystemURL)); On 2012/09/25 04:25:17, kinuko wrote: > this could be const too? Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/10966003/19001
Retried try job too often for step(s) content_unittests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/10966003/20007
Change committed as 158791 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
