[SyncFS] Construct and destruct sync_worker in worker_task_runner

[SyncFS] Construct and destruct instances live in worker_task_runner.
- SyncWorker: Construct / Destruct
- SyncWorker::Observer: Destruct


BUG=347425
TEST=./unit_tests --gtest_filter="SyncEngineTest.*:DriveBackendSyncTest.*"

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

[peria, 2014-05-26 05:09:51]

PTL

[peria, 2014-05-26 05:11:22]

FYI:
Following CL is a test in multi-threaded environment.
https://codereview.chromium.org/296893002/

[nhiroki, 2014-05-26 05:22:43]

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:254:
net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
I haven't read sourcecode closely, but I guess these observers should be added
after a worker is created.

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:110: delete
sync_worker.release();
How about using scoped_ptr::reset()?

[nhiroki, 2014-05-26 05:31:32]

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:191:
run_loop.Run();
Hmm... this can block the UI thread?

[peria, 2014-05-26 05:32:35]

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:254:
net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
On 2014/05/26 05:22:44, nhiroki wrote:
> I haven't read sourcecode closely, but I guess these observers should be added
> after a worker is created.

Done.
Yes, those observers communicate with SyncWorker through SyncEngine!

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/288193002/diff/160001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:110: delete
sync_worker.release();
On 2014/05/26 05:22:44, nhiroki wrote:
> How about using scoped_ptr::reset()?

Done.

[tzik, 2014-05-26 05:41:29]

https://codereview.chromium.org/288193002/diff/180001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:211:
worker_task_runner_->PostTask(
worker_task_runner_->DeleteSoon(); should work.

https://codereview.chromium.org/288193002/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:258:
base::Passed(sync_engine_context.Pass()),
s/sync_engine_context.Pass()/&sync_engine_context/?

[peria, 2014-05-26 06:08:10]

Will remove RunLoop from SyncEngine::CreateForBrowserContext.

https://codereview.chromium.org/288193002/diff/180001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:211:
worker_task_runner_->PostTask(
On 2014/05/26 05:41:30, tzik wrote:
> worker_task_runner_->DeleteSoon(); should work.

Done.

https://codereview.chromium.org/288193002/diff/180001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:258:
base::Passed(sync_engine_context.Pass()),
On 2014/05/26 05:41:30, tzik wrote:
> s/sync_engine_context.Pass()/&sync_engine_context/?

Done.

[tzik, 2014-05-26 06:50:20]

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:85:

could you revert these space only change?

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:11: #include
"base/run_loop.h"
please drop this

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:259:
sync_worker_->AsWeakPtr()));
Unretained?

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc
(right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:121: 
ditto

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.h (right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.h:53: public
base::SupportsWeakPtr<SyncWorker> {
please revert this.

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.h:65: virtual
~Observer() {}
Not need to public?

[peria, 2014-05-26 07:55:09]

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc
(right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc:85:

On 2014/05/26 06:50:21, tzik wrote:
> could you revert these space only change?

Done.

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:11: #include
"base/run_loop.h"
On 2014/05/26 06:50:21, tzik wrote:
> please drop this

Done.

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:259:
sync_worker_->AsWeakPtr()));
On 2014/05/26 06:50:21, tzik wrote:
> Unretained?

Done.

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc
(right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc:121: 
On 2014/05/26 06:50:21, tzik wrote:
> ditto

Done.

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_worker.h (right):

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.h:53: public
base::SupportsWeakPtr<SyncWorker> {
On 2014/05/26 06:50:21, tzik wrote:
> please revert this.

Done.

https://codereview.chromium.org/288193002/diff/220001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_worker.h:65: virtual
~Observer() {}
On 2014/05/26 06:50:21, tzik wrote:
> Not need to public?

Done.

[nhiroki, 2014-05-26 08:06:03]

Looks good overall.

https://codereview.chromium.org/288193002/diff/300001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/300001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:243: sync_worker_ =
SyncWorker::CreateOnWorker(
How about renaming this or calling the ctor directly?

https://codereview.chromium.org/288193002/diff/300001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:258:
base::Unretained(sync_worker_.get())));
You may want to move this to line 249 for readability.

[peria, 2014-05-26 08:14:40]

https://codereview.chromium.org/288193002/diff/300001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine.cc (right):

https://codereview.chromium.org/288193002/diff/300001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:243: sync_worker_ =
SyncWorker::CreateOnWorker(
On 2014/05/26 08:06:03, nhiroki wrote:
> How about renaming this or calling the ctor directly?

Done.

https://codereview.chromium.org/288193002/diff/300001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine.cc:258:
base::Unretained(sync_worker_.get())));
On 2014/05/26 08:06:03, nhiroki wrote:
> You may want to move this to line 249 for readability.

Done.

[nhiroki, 2014-05-26 08:30:43]

lgtm

[tzik, 2014-05-26 08:38:47]

lgtm

[peria, 2014-05-26 08:41:58]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-05-26 08:42:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/peria@chromium.org/288193002/310003

[commit-bot: I haz the power, 2014-05-26 20:17:08]

Change committed as 272876