[SyncFS] Make ExtensionService usage of SyncWorker asynchronous

[SyncFS] Make ExtensionService usage of SyncWorker asynchronous

As a preparation of thread migration from UI to a worker,
this CL moves the usage of ExtensionService in SyncWorker to UI thread explicitly.

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

[tzik, 2014-04-23 12:58:30]

PTL

[nhiroki, 2014-04-24 01:39:52]

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:456: 
nit: an extra blank line.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:460: void
SyncWorker::QueryAppStatusOnUIThread(
(optional) How about moving this into anon namespace?

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:466: if
(!extension_service)
In this case you don't need to run a callback, right? How about having a comment
for that?

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:478: }
You need to run |callback| here.

[peria, 2014-04-24 02:38:20]

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_helper.cc (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_helper.cc:10: namespace
internal {
anonymous namespace?

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_helper.h (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_helper.h:33: void
RelayToTaskRunner0(base::SequencedTaskRunner* task_runner,
This declaration is not needed.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:439: 
Could you add checking |extension_service_|'s availability?
It may cancel meaningless memory allocations and calling PostTask().

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:462: const
std::vector<std::string>* apps,
(optional) keep its name |app_ids| for consistency

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:509: bool
is_app_enabled = itr->second == APP_STATUS_ENABLED;
nit: a = (b == c);

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:511:
tracker.tracker_kind() == TRACKER_KIND_APP_ROOT;
diito

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.h (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.h:132: enum AppStatus
{
(optional) I recommend to move this enum to extensin_service_interface.h

[tzik, 2014-04-24 06:56:55]

Updated! Please take another look.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_helper.cc (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_helper.cc:10: namespace
internal {
On 2014/04/24 02:38:21, peria wrote:
> anonymous namespace?

Refactored.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/callback_helper.h (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/callback_helper.h:33: void
RelayToTaskRunner0(base::SequencedTaskRunner* task_runner,
On 2014/04/24 02:38:21, peria wrote:
> This declaration is not needed.

Refactored in another CL.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:439: 
On 2014/04/24 02:38:21, peria wrote:
> Could you add checking |extension_service_|'s availability?
> It may cancel meaningless memory allocations and calling PostTask().

To do that, we need to hop to UI thread, which costs higher than saved cost by
the shortcut.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:456: 
On 2014/04/24 01:39:52, nhiroki wrote:
> nit: an extra blank line.

Done.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:460: void
SyncWorker::QueryAppStatusOnUIThread(
On 2014/04/24 01:39:52, nhiroki wrote:
> (optional) How about moving this into anon namespace?

Done.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:462: const
std::vector<std::string>* apps,
On 2014/04/24 02:38:21, peria wrote:
> (optional) keep its name |app_ids| for consistency

Done.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:466: if
(!extension_service)
On 2014/04/24 01:39:52, nhiroki wrote:
> In this case you don't need to run a callback, right? How about having a
comment
> for that?

Ah, I completely forgot about calling the callback.
Yes, it's not needed, but let me call it just to keep consistency.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:478: }
On 2014/04/24 01:39:52, nhiroki wrote:
> You need to run |callback| here.

Done.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:509: bool
is_app_enabled = itr->second == APP_STATUS_ENABLED;
On 2014/04/24 02:38:21, peria wrote:
> nit: a = (b == c);

Done.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:511:
tracker.tracker_kind() == TRACKER_KIND_APP_ROOT;
On 2014/04/24 02:38:21, peria wrote:
> diito

Done.

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.h (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.h:132: enum AppStatus
{
On 2014/04/24 02:38:21, peria wrote:
> (optional) I recommend to move this enum to extensin_service_interface.h

ExtensionRegistry has similar but fine grained, which is not needed for us.
So, I think its owners don't want to put this in.

[nhiroki, 2014-04-24 07:09:06]

lgtm

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_worker.cc (right):

https://codereview.chromium.org/246163005/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_worker.cc:460: void
SyncWorker::QueryAppStatusOnUIThread(
On 2014/04/24 06:56:55, tzik wrote:
> On 2014/04/24 01:39:52, nhiroki wrote:
> > (optional) How about moving this into anon namespace?
> 
> Done.

Hmm... sorry, I missed QueryAppStatusOnUIThread() depends on "enum AppStatus".
The current non-static version is ok, but the previous one is also ok because it
can make the enum and typedef private.

[peria, 2014-04-24 07:38:08]

lgtm

[tzik, 2014-04-24 10:20:16]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-04-24 11:07:50]

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

[commit-bot: I haz the power, 2014-04-24 12:32:36]

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

[commit-bot: I haz the power, 2014-04-24 12:32:36]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[tzik, 2014-04-25 05:26:31]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-04-25 07:37:11]

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

[commit-bot: I haz the power, 2014-04-25 23:13:06]

Change committed as 266277