[SyncFS] Wire up MetadataDatabaseIndex to MetadataDatabase

[SyncFS] Wire up MetadataDatabaseIndex to MetadataDatabase

Use MetadataDatabaseIndex as the separate index management class in MetadataDatabase.

BUG=344769
NOTRY=true
TEST=unit_tests --gtest_filter='DriveBackendSyncTest.*:MetadataDatabaseTest.*'

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

[tzik, 2014-03-05 04:13:32]

I think this is ready for review.
Please take a look.

[nhiroki, 2014-03-05 04:23:30]

https://codereview.chromium.org/177233005/diff/250001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/177233005/diff/250001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:475:
metadata_database_->index_.get();
metadata_database_ -> metadata_database_2

Can you run tests again after fixing this?

[tzik, 2014-03-05 04:36:05]

https://codereview.chromium.org/177233005/diff/250001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/177233005/diff/250001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:475:
metadata_database_->index_.get();
On 2014/03/05 04:23:31, nhiroki wrote:
> metadata_database_ -> metadata_database_2
> 
> Can you run tests again after fixing this?

Thanks! Done!
Tests passed successfully after this fix.

[nhiroki, 2014-03-05 06:21:35]

LGTM

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:425:
base::hash_set<std::string> affected_file_ids;
Can you move |affected_file_ids| to line 435 for readability.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:464: ++itr) {
super-nit: Can you move this "++itr" to line 463?

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:578: void
RemoveTracker(int64 tracker_id,
nit: RemoveTracker -> RemoveFileTracker

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1131:
child_file_ids.end());
indent-nit

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:484:
SCOPED_TRACE("Expect equivalent file_by_id_ contents.");
s/file_by_id_/metadata_by_id_/

[tzik, 2014-03-05 07:05:54]

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:425:
base::hash_set<std::string> affected_file_ids;
On 2014/03/05 06:21:35, nhiroki wrote:
> Can you move |affected_file_ids| to line 435 for readability.

Done.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:464: ++itr) {
On 2014/03/05 06:21:35, nhiroki wrote:
> super-nit: Can you move this "++itr" to line 463?

Done.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:578: void
RemoveTracker(int64 tracker_id,
On 2014/03/05 06:21:35, nhiroki wrote:
> nit: RemoveTracker -> RemoveFileTracker

Done.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1131:
child_file_ids.end());
On 2014/03/05 06:21:35, nhiroki wrote:
> indent-nit

Done.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:484:
SCOPED_TRACE("Expect equivalent file_by_id_ contents.");
On 2014/03/05 06:21:35, nhiroki wrote:
> s/file_by_id_/metadata_by_id_/

Done.

[kinuko, 2014-03-05 07:41:32]

lgtm (not looked really thoroughly though)

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/drive_backend_util.h (right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_util.h:72:
scoped_ptr<T> Clone(const T* obj) {
nit: so far it's only used for FileTracker. Can this be an inline method named
like CloneTracker for now? (Not feeling that this should be a very common
utility for scoped_ptr)

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1217: //
Check if any other tracker exists has the same parent, title and
exists has -> "has", or "exists and has" ?

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1306:
LOG(ERROR) << "HERE";
?

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1658: int64
app_root_tracker_id = index_->GetAppRootTracker(tracker.app_id());
nit: not related to this CL, but it might have been more readable if methods
that return tracker id has suffix 'TrackerID' (vs. 'Tracker' for methods return
tracker)

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:480:
metadata_database_2->service_metadata_.get());
on_memory, reloaded?

[tzik, 2014-03-05 08:23:41]

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/drive_backend_util.h (right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_util.h:72:
scoped_ptr<T> Clone(const T* obj) {
On 2014/03/05 07:41:32, kinuko wrote:
> nit: so far it's only used for FileTracker. Can this be an inline method named
> like CloneTracker for now? (Not feeling that this should be a very common
> utility for scoped_ptr)

Done.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_util.h:72:
scoped_ptr<T> Clone(const T* obj) {
On 2014/03/05 07:41:32, kinuko wrote:
> nit: so far it's only used for FileTracker. Can this be an inline method named
> like CloneTracker for now? (Not feeling that this should be a very common
> utility for scoped_ptr)

Done.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1306:
LOG(ERROR) << "HERE";
On 2014/03/05 07:41:32, kinuko wrote:
> ?

Awww. It's a remnant of my local debugging.
Removed.

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc
(right):

https://codereview.chromium.org/177233005/diff/270001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/metadata_database_unittest.cc:480:
metadata_database_2->service_metadata_.get());
On 2014/03/05 07:41:32, kinuko wrote:
> on_memory, reloaded?

I didn't move service_metadata_ to MetadataDatabaseIndex, it is still held by
MetadataDatabase which are |metadata_database_| and |metadata_database_2|.

[tzik, 2014-03-05 11:34:27]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-03-05 11:34:55]

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

[commit-bot: I haz the power, 2014-03-05 11:39:25]

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

[commit-bot: I haz the power, 2014-03-05 11:39:26]

Try jobs failed on following builders: mac_chromium_rel

[tzik, 2014-03-05 12:48:58]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-03-05 12:49:15]

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

[commit-bot: I haz the power, 2014-03-05 12:53:40]

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

[commit-bot: I haz the power, 2014-03-05 12:53:40]

Try jobs failed on following builders: mac_chromium_rel

[tzik, 2014-03-05 13:53:47]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-03-05 13:54:03]

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

[commit-bot: I haz the power, 2014-03-05 14:21:22]

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

[commit-bot: I haz the power, 2014-03-05 14:21:23]

Try jobs failed on following builders: mac_chromium_compile_dbg

[tzik, 2014-03-06 04:00:38]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-03-06 04:01:56]

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

[commit-bot: I haz the power, 2014-03-06 04:39:26]

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

[commit-bot: I haz the power, 2014-03-06 04:39:26]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[tzik, 2014-03-06 04:58:59]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-03-06 04:59:19]

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

[commit-bot: I haz the power, 2014-03-06 14:12:41]

Change committed as 255340