SyncFS: Reorder initialization sequence of SyncFileSystemService

SyncFS: Reorder initialization sequence of SyncFileSystemService

Moves initialization part for SyncFileSystemService from SyncFileSystem's API
handling layer (sync_file_system_api.{cc,h}) to SyncFileSystemBackend so that
non-chrome API can start SyncFileSystemService.

This is a preparation patch for supporting window.resolveLocalFileSystemURL()
on SyncFileSystem. See the bug issue for more details.


BUG=177137
TEST=unit_tests
TEST=browser_tests --gtest_filter=SyncFileSystemApiTest.*

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

[nhiroki, 2013-08-23 08:51:34]

Hi, can you take a look? Thanks!

[kinuko, 2013-08-23 10:02:02]

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:30:
skip_initialize_syncfs_service_for_testing_(false) {
This is called on UI thread.. is it correct? Can we have an assertion?

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:202:
SyncFileSystemServiceFactory::GetForProfile(profile_);
I'm not 100% sure at this point profile_ is always valid. We must be called on
IO thread via IPC, which must have been terminated before profile shutdown
happens, but here we're going back to UI thread... are we really safe?  Should
we listen to PROFILE_DESTROYED notifications on UI thread?  Or can we not to
start initialization of SyncFileSystemService when it's constructed but lazily
do so in InitializeForApp, construct it when SyncFileSystemBackend is created
(when we're sure profile_ is valid) and let it notify the backend when its
Shutdown() method is called for profile destruction?

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_status_code.cc (right):

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_status_code.cc:152: SyncStatusCode status)
{
SyncStatusCodeToPlatformFileError ??

[nhiroki, 2013-08-28 05:37:42]

Update! Can you take another look?

And I wrote a rough document about the initialization sequence of
SyncFileSystemService. Please see for more details.

https://docs.google.com/a/google.com/document/d/1Kkg486ewnXNJeDehyso995cxmOJX...
(internal only)

Thanks!

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:30:
skip_initialize_syncfs_service_for_testing_(false) {
On 2013/08/23 10:02:03, kinuko wrote:
> This is called on UI thread.. is it correct? Can we have an assertion?

Added an assertion.

I found some tests using CannedSyncableFileSystem call this on a thread other
than the UI thread, but these don't take care of Profile lifetime since
initialization routines will be skipped by |skip_initialize_...| flag and I
think it could ignore the assertion, so I introduced another creating method for
testing (i.e. CreateForTesting()).

Or, should I fix the CannedSyncableFileSystem to run this ctor on the UI thread?

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:202:
SyncFileSystemServiceFactory::GetForProfile(profile_);
On 2013/08/23 10:02:03, kinuko wrote:
> I'm not 100% sure at this point profile_ is always valid. We must be called on
> IO thread via IPC, which must have been terminated before profile shutdown
> happens, but here we're going back to UI thread... are we really safe?  Should
> we listen to PROFILE_DESTROYED notifications on UI thread?  Or can we not to
> start initialization of SyncFileSystemService when it's constructed but lazily
> do so in InitializeForApp, construct it when SyncFileSystemBackend is created
> (when we're sure profile_ is valid) and let it notify the backend when its
> Shutdown() method is called for profile destruction?

Right. |profile_| could be invalidated before using here.

I adopted the former way (observing PROFILE_DESTROYED event) since it looks
simpler and the latter way might make
SyncFileSystemService-SyncFileSystemBackend relationship more tight (I'd like to
keep them simply...).

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_status_code.cc (right):

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_status_code.cc:152: SyncStatusCode status)
{
On 2013/08/23 10:02:03, kinuko wrote:
> SyncStatusCodeToPlatformFileError ??

Whoa...!! Fixed, thanks!

[kinuko, 2013-08-28 06:51:28]

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:30:
skip_initialize_syncfs_service_for_testing_(false) {
On 2013/08/28 05:37:42, nhiroki wrote:
> On 2013/08/23 10:02:03, kinuko wrote:
> > This is called on UI thread.. is it correct? Can we have an assertion?
> 
> Added an assertion.
> 
> I found some tests using CannedSyncableFileSystem call this on a thread other
> than the UI thread, but these don't take care of Profile lifetime since
> initialization routines will be skipped by |skip_initialize_...| flag and I
> think it could ignore the assertion, so I introduced another creating method
for
> testing (i.e. CreateForTesting()).
> 
> Or, should I fix the CannedSyncableFileSystem to run this ctor on the UI
thread?

Which test fails?  In most cases I expect we could fix it by explicitly making
current message_loop UI_TYPE.  (replacing it with MessageLoopForUI or
initializing message_loop_(TYPE_UI)).

https://codereview.chromium.org/22810002/diff/45001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.h (right):

https://codereview.chromium.org/22810002/diff/45001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.h:71: class
ProfileHolder : public content::NotificationObserver {
Is it possible to just make SyncFSBackend implement observer?  It could hold
registrar as scoped_ptr and releases it on UI thread.

[kinuko, 2013-08-28 06:55:45]

On Wed, Aug 28, 2013 at 3:51 PM, <kinuko@chromium.org> wrote:

>
> https://codereview.chromium.**org/22810002/diff/35001/**
>
chrome/browser/sync_file_**system/local/sync_file_system_**backend.cc<https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_system/local/sync_file_system_backend.cc>
> File chrome/browser/sync_file_**system/local/sync_file_system_**backend.cc
> (right):
>
> https://codereview.chromium.**org/22810002/diff/35001/**
> chrome/browser/sync_file_**system/local/sync_file_system_**
>
backend.cc#newcode30<https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_system/local/sync_file_system_backend.cc#newcode30>
> chrome/browser/sync_file_**system/local/sync_file_system_**backend.cc:30:
> skip_initialize_syncfs_**service_for_testing_(false) {
> On 2013/08/28 05:37:42, nhiroki wrote:
>
>> On 2013/08/23 10:02:03, kinuko wrote:
>> > This is called on UI thread.. is it correct? Can we have an
>>
> assertion?
>
>  Added an assertion.
>>
>
>  I found some tests using CannedSyncableFileSystem call this on a
>>
> thread other
>
>> than the UI thread, but these don't take care of Profile lifetime
>>
> since
>
>> initialization routines will be skipped by |skip_initialize_...| flag
>>
> and I
>
>> think it could ignore the assertion, so I introduced another creating
>>
> method for
>
>> testing (i.e. CreateForTesting()).
>>
>
>  Or, should I fix the CannedSyncableFileSystem to run this ctor on the
>>
> UI thread?
>
> Which test fails?  In most cases I expect we could fix it by explicitly
> making current message_loop UI_TYPE.  (replacing it with
> MessageLoopForUI or initializing message_loop_(TYPE_UI)).
>
> https://codereview.chromium.**org/22810002/diff/45001/**
>
chrome/browser/sync_file_**system/local/sync_file_system_**backend.h<https://codereview.chromium.org/22810002/diff/45001/chrome/browser/sync_file_system/local/sync_file_system_backend.h>
> File chrome/browser/sync_file_**system/local/sync_file_system_**backend.h
> (right):
>
> https://codereview.chromium.**org/22810002/diff/45001/**
> chrome/browser/sync_file_**system/local/sync_file_system_**
>
backend.h#newcode71<https://codereview.chromium.org/22810002/diff/45001/chrome/browser/sync_file_system/local/sync_file_system_backend.h#newcode71>
> chrome/browser/sync_file_**system/local/sync_file_system_**backend.h:71:
> class ProfileHolder : public content::NotificationObserver {
> Is it possible to just make SyncFSBackend implement observer?  It could
> hold registrar as scoped_ptr and releases it on UI thread.
>

Just found some notes about this in the doc... (would you want to allow me
to comment on the doc?)

https://codereview.chromium.**org/22810002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kinuko, 2013-08-28 07:00:19]

On Wed, Aug 28, 2013 at 3:55 PM, Kinuko Yasuda <kinuko@chromium.org> wrote:

> On Wed, Aug 28, 2013 at 3:51 PM, <kinuko@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/22810002/diff/35001/**
>>
chrome/browser/sync_file_**system/local/sync_file_system_**backend.cc<https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_system/local/sync_file_system_backend.cc>
>> File chrome/browser/sync_file_**system/local/sync_file_system_**
>> backend.cc
>> (right):
>>
>> https://codereview.chromium.**org/22810002/diff/35001/**
>> chrome/browser/sync_file_**system/local/sync_file_system_**
>>
backend.cc#newcode30<https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_system/local/sync_file_system_backend.cc#newcode30>
>> chrome/browser/sync_file_**system/local/sync_file_system_**backend.cc:30:
>> skip_initialize_syncfs_**service_for_testing_(false) {
>> On 2013/08/28 05:37:42, nhiroki wrote:
>>
>>> On 2013/08/23 10:02:03, kinuko wrote:
>>> > This is called on UI thread.. is it correct? Can we have an
>>>
>> assertion?
>>
>>  Added an assertion.
>>>
>>
>>  I found some tests using CannedSyncableFileSystem call this on a
>>>
>> thread other
>>
>>> than the UI thread, but these don't take care of Profile lifetime
>>>
>> since
>>
>>> initialization routines will be skipped by |skip_initialize_...| flag
>>>
>> and I
>>
>>> think it could ignore the assertion, so I introduced another creating
>>>
>> method for
>>
>>> testing (i.e. CreateForTesting()).
>>>
>>
>>  Or, should I fix the CannedSyncableFileSystem to run this ctor on the
>>>
>> UI thread?
>>
>> Which test fails?  In most cases I expect we could fix it by explicitly
>> making current message_loop UI_TYPE.  (replacing it with
>> MessageLoopForUI or initializing message_loop_(TYPE_UI)).
>>
>> https://codereview.chromium.**org/22810002/diff/45001/**
>>
chrome/browser/sync_file_**system/local/sync_file_system_**backend.h<https://codereview.chromium.org/22810002/diff/45001/chrome/browser/sync_file_system/local/sync_file_system_backend.h>
>> File chrome/browser/sync_file_**system/local/sync_file_system_**backend.h
>> (right):
>>
>> https://codereview.chromium.**org/22810002/diff/45001/**
>> chrome/browser/sync_file_**system/local/sync_file_system_**
>>
backend.h#newcode71<https://codereview.chromium.org/22810002/diff/45001/chrome/browser/sync_file_system/local/sync_file_system_backend.h#newcode71>
>> chrome/browser/sync_file_**system/local/sync_file_system_**backend.h:71:
>> class ProfileHolder : public content::NotificationObserver {
>> Is it possible to just make SyncFSBackend implement observer?  It could
>> hold registrar as scoped_ptr and releases it on UI thread.
>>
>
> Just found some notes about this in the doc... (would you want to allow me
> to comment on the doc?)
>

Ok, never mind about this.  ProfileHolder looks reasonable to me


> 
https://codereview.chromium.**org/22810002/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nhiroki, 2013-08-28 07:07:17]

On 2013/08/28 06:55:45, kinuko wrote:
> Just found some notes about this in the doc... (would you want to allow me
> to comment on the doc?)

Oops... sorry, added a permission to comment.

[nhiroki, 2013-08-28 07:46:46]

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:30:
skip_initialize_syncfs_service_for_testing_(false) {
On 2013/08/28 06:51:28, kinuko wrote:
> On 2013/08/28 05:37:42, nhiroki wrote:
> > On 2013/08/23 10:02:03, kinuko wrote:
> > > This is called on UI thread.. is it correct? Can we have an assertion?
> > 
> > Added an assertion.
> > 
> > I found some tests using CannedSyncableFileSystem call this on a thread
other
> > than the UI thread, but these don't take care of Profile lifetime since
> > initialization routines will be skipped by |skip_initialize_...| flag and I
> > think it could ignore the assertion, so I introduced another creating method
> for
> > testing (i.e. CreateForTesting()).
> > 
> > Or, should I fix the CannedSyncableFileSystem to run this ctor on the UI
> thread?
> 
> Which test fails?  In most cases I expect we could fix it by explicitly making
> current message_loop UI_TYPE.  (replacing it with MessageLoopForUI or
> initializing message_loop_(TYPE_UI)).

AFAIK, the following tests failed:

- LocalFileChangeTrackerTest
- LocalFileSyncContextTest
- SyncableFileSystemTest
- SyncableFileSystemUtilTest
- SyncableFileOperationRunnerTest

[kinuko, 2013-08-28 08:23:57]

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:30:
skip_initialize_syncfs_service_for_testing_(false) {
On 2013/08/28 07:46:46, nhiroki wrote:
> On 2013/08/28 06:51:28, kinuko wrote:
> > On 2013/08/28 05:37:42, nhiroki wrote:
> > > On 2013/08/23 10:02:03, kinuko wrote:
> > > > This is called on UI thread.. is it correct? Can we have an assertion?
> > > 
> > > Added an assertion.
> > > 
> > > I found some tests using CannedSyncableFileSystem call this on a thread
> other
> > > than the UI thread, but these don't take care of Profile lifetime since
> > > initialization routines will be skipped by |skip_initialize_...| flag and
I
> > > think it could ignore the assertion, so I introduced another creating
method
> > for
> > > testing (i.e. CreateForTesting()).

Sorry I've overlooked this part.  I think it's ok to add a separate static
creation method (and avoid some checks for testing) but I'd like to avoid having
multiple ctors.  Can we avoid it?

Btw commonly seen pattern for safer thread dcheck is:
DCHECK(!BrowserThread::IsMessageLoopValid(UI) || BrowserThread::CurrentlyOn(UI))

> > > Or, should I fix the CannedSyncableFileSystem to run this ctor on the UI
> > thread?
> > 
> > Which test fails?  In most cases I expect we could fix it by explicitly
making
> > current message_loop UI_TYPE.  (replacing it with MessageLoopForUI or
> > initializing message_loop_(TYPE_UI)).
> 
> AFAIK, the following tests failed:
> 
> - LocalFileChangeTrackerTest
> - LocalFileSyncContextTest
> - SyncableFileSystemTest
> - SyncableFileSystemUtilTest
> - SyncableFileOperationRunnerTest

[nhiroki, 2013-08-28 10:51:33]

Updated! PTAL.

On 2013/08/28 08:23:57, kinuko wrote:
>
https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
> File chrome/browser/sync_file_system/local/sync_file_system_backend.cc
(right):
> 
>
https://codereview.chromium.org/22810002/diff/35001/chrome/browser/sync_file_...
> chrome/browser/sync_file_system/local/sync_file_system_backend.cc:30:
> skip_initialize_syncfs_service_for_testing_(false) {
> On 2013/08/28 07:46:46, nhiroki wrote:
> > On 2013/08/28 06:51:28, kinuko wrote:
> > > On 2013/08/28 05:37:42, nhiroki wrote:
> > > > On 2013/08/23 10:02:03, kinuko wrote:
> > > > > This is called on UI thread.. is it correct? Can we have an assertion?
> > > > 
> > > > Added an assertion.
> > > > 
> > > > I found some tests using CannedSyncableFileSystem call this on a thread
> > other
> > > > than the UI thread, but these don't take care of Profile lifetime since
> > > > initialization routines will be skipped by |skip_initialize_...| flag
and
> I
> > > > think it could ignore the assertion, so I introduced another creating
> method
> > > for
> > > > testing (i.e. CreateForTesting()).
> 
> Sorry I've overlooked this part.  I think it's ok to add a separate static
> creation method (and avoid some checks for testing) but I'd like to avoid
having
> multiple ctors.  Can we avoid it?

Removed one of them.

> Btw commonly seen pattern for safer thread dcheck is:
> DCHECK(!BrowserThread::IsMessageLoopValid(UI) ||
BrowserThread::CurrentlyOn(UI))

[kinuko, 2013-08-28 11:24:44]

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/extensions...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/extensions...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:112:
GetSyncFileSystemService(profile());
nit: Can you comment that this is to do the necessary extension-related
initialization?

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:44:
content::NotificationService::AllSources());
content::Source<Profile>(profile_) ?

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:53: case
chrome::NOTIFICATION_PROFILE_DESTROYED: {
nit: maybe DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type) and no
DCHECK?

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:54: Profile*
profile = content::Source<Profile>(source).ptr();
nit: probably you don't need store it to a temp variable.

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:56: profile_ =
NULL;
I think here this can remove itself from the registrar.

registrar_.RemoveAll();

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:81: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
Also check profile_holder_ is not null?

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:132:
base::Unretained(this), origin_url, type, mode, callback);
Why we use Unretained here (and else)?

[nhiroki, 2013-08-29 04:31:34]

Thank you for reviewing. Updated.

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/extensions...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/extensions...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:112:
GetSyncFileSystemService(profile());
On 2013/08/28 11:24:45, kinuko wrote:
> nit: Can you comment that this is to do the necessary extension-related
> initialization?

Done.

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:44:
content::NotificationService::AllSources());
On 2013/08/28 11:24:45, kinuko wrote:
> content::Source<Profile>(profile_) ?

Done.

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:53: case
chrome::NOTIFICATION_PROFILE_DESTROYED: {
On 2013/08/28 11:24:45, kinuko wrote:
> nit: maybe DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type) and no
> DCHECK?

Replaced with DCHECK_EQ(...).

Sorry, I couldn't get what "no DCHECK" means. You mean I can remove line 51...?

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:54: Profile*
profile = content::Source<Profile>(source).ptr();
On 2013/08/28 11:24:45, kinuko wrote:
> nit: probably you don't need store it to a temp variable.

Done.

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:56: profile_ =
NULL;
On 2013/08/28 11:24:45, kinuko wrote:
> I think here this can remove itself from the registrar.
> 
> registrar_.RemoveAll();

Done.

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:81: if
(!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2013/08/28 11:24:45, kinuko wrote:
> Also check profile_holder_ is not null?

Done.

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:132:
base::Unretained(this), origin_url, type, mode, callback);
On 2013/08/28 11:24:45, kinuko wrote:
> Why we use Unretained here (and else)?

Hmm... yes, we have to manage lifetime of SyncFileSystemBackend appropriately. I
changed here to increment the ref-count of FileSystemContext owning the backend.

[kinuko, 2013-08-29 08:28:21]

lgtm

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/65001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:53: case
chrome::NOTIFICATION_PROFILE_DESTROYED: {
On 2013/08/29 04:31:34, nhiroki wrote:
> On 2013/08/28 11:24:45, kinuko wrote:
> > nit: maybe DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type) and no
> > DCHECK?
> 
> Replaced with DCHECK_EQ(...).
> 
> Sorry, I couldn't get what "no DCHECK" means. You mean I can remove line
51...? 

d'oh, I think I wanted to say no switch/case.

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:112: //
Initializes observer of sync events in SyncFileSystem.
Can you elaborate a little more:
// SyncFileSystem initialization is done in OpenFileSystem below, but we call
GetSyncFileSystemService
// here too to initialize sync event observer for extensions API.

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:77: if
(profile_holder_ && !BrowserThread::CurrentlyOn(BrowserThread::UI)) {
CurrentlyOn -> CalledOnUIThread ?
(We may leak in tests otherwise)

[nhiroki, 2013-08-29 08:43:34]

Thanks!

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc
(right):

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/api/sync_file_system/sync_file_system_api.cc:112: //
Initializes observer of sync events in SyncFileSystem.
On 2013/08/29 08:28:21, kinuko wrote:
> Can you elaborate a little more:
> // SyncFileSystem initialization is done in OpenFileSystem below, but we call
> GetSyncFileSystemService
> // here too to initialize sync event observer for extensions API.

Done.

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/local/sync_file_system_backend.cc (right):

https://codereview.chromium.org/22810002/diff/80001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:77: if
(profile_holder_ && !BrowserThread::CurrentlyOn(BrowserThread::UI)) {
On 2013/08/29 08:28:21, kinuko wrote:
> CurrentlyOn -> CalledOnUIThread ?
> (We may leak in tests otherwise)

Done.

[tzik, 2013-08-30 04:47:34]

lgtm

[commit-bot: I haz the power, 2013-08-30 05:47:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/22810002/94001

[commit-bot: I haz the power, 2013-08-30 05:47:46]

Failed to apply patch for
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/sync_file_system/local/sync_file_system_backend.cc
  Hunk #4 FAILED at 116.
  Hunk #5 succeeded at 145 (offset -6 lines).
  Hunk #6 succeeded at 166 (offset -6 lines).
  Hunk #7 succeeded at 176 (offset -6 lines).
  Hunk #8 succeeded at 199 (offset -6 lines).
  Hunk #9 succeeded at 216 (offset -6 lines).
  1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/sync_file_system/local/sync_file_system_backend.cc.rej

Patch:       chrome/browser/sync_file_system/local/sync_file_system_backend.cc
Index: chrome/browser/sync_file_system/local/sync_file_system_backend.cc
diff --git a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
index
ac426c9bca6f8f8b0f8c2052ebe7e0fefcc1992c..ef1f46bb5bc1f72a7ef8f5e9da8f9af0312ffd49
100644
--- a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
+++ b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
@@ -5,10 +5,15 @@
 #include "chrome/browser/sync_file_system/local/sync_file_system_backend.h"
 
 #include "base/logging.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/sync_file_system/local/local_file_change_tracker.h"
 #include "chrome/browser/sync_file_system/local/local_file_sync_context.h"
 #include
"chrome/browser/sync_file_system/local/syncable_file_system_operation.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service_factory.h"
 #include "chrome/browser/sync_file_system/syncable_file_system_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "webkit/browser/fileapi/file_system_context.h"
 #include "webkit/browser/fileapi/file_system_file_stream_reader.h"
 #include "webkit/browser/fileapi/file_system_operation_impl.h"
@@ -16,17 +21,71 @@
 #include "webkit/browser/fileapi/sandbox_quota_observer.h"
 #include "webkit/common/fileapi/file_system_util.h"
 
+using content::BrowserThread;
+
 namespace sync_file_system {
 
-SyncFileSystemBackend::SyncFileSystemBackend()
-    : delegate_(NULL) {
+namespace {
+
+bool CalledOnUIThread() {
+  // Ensure that these methods are called on the UI thread, except for
unittests
+  // where a UI thread might not have been created.
+  return BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         !BrowserThread::IsMessageLoopValid(BrowserThread::UI);
+}
+
+}  // namespace
+
+SyncFileSystemBackend::ProfileHolder::ProfileHolder(Profile* profile)
+    : profile_(profile) {
+  DCHECK(CalledOnUIThread());
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_PROFILE_DESTROYED,
+                 content::Source<Profile>(profile_));
+}
+
+void SyncFileSystemBackend::ProfileHolder::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(CalledOnUIThread());
+  DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
+  DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
+  profile_ = NULL;
+  registrar_.RemoveAll();
+}
+
+Profile* SyncFileSystemBackend::ProfileHolder::GetProfile() {
+  DCHECK(CalledOnUIThread());
+  return profile_;
+}
+
+SyncFileSystemBackend::SyncFileSystemBackend(Profile* profile)
+    : context_(NULL),
+      skip_initialize_syncfs_service_for_testing_(false) {
+  DCHECK(CalledOnUIThread());
+  if (profile)
+    profile_holder_.reset(new ProfileHolder(profile));
 }
 
 SyncFileSystemBackend::~SyncFileSystemBackend() {
   if (change_tracker_) {
-    delegate_->file_task_runner()->DeleteSoon(
+    GetDelegate()->file_task_runner()->DeleteSoon(
         FROM_HERE, change_tracker_.release());
   }
+
+  if (profile_holder_ && !CalledOnUIThread()) {
+    BrowserThread::DeleteSoon(
+        BrowserThread::UI, FROM_HERE, profile_holder_.release());
+  }
+}
+
+// static
+SyncFileSystemBackend* SyncFileSystemBackend::CreateForTesting() {
+  DCHECK(CalledOnUIThread());
+  SyncFileSystemBackend* backend = new SyncFileSystemBackend(NULL);
+  backend->skip_initialize_syncfs_service_for_testing_ = true;
+  return backend;
 }
 
 bool SyncFileSystemBackend::CanHandleType(
@@ -37,17 +96,18 @@ bool SyncFileSystemBackend::CanHandleType(
 
 void SyncFileSystemBackend::Initialize(fileapi::FileSystemContext* context) {
   DCHECK(context);
-  DCHECK(!delegate_);
-  delegate_ = context->sandbox_delegate();
+  DCHECK(!context_);
+  context_ = context;
 
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
-  delegate_->AddFileUpdateObserver(
+      delegate->quota_observer(),
+      delegate->file_task_runner());
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncableForInternalSync,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
+      delegate->quota_observer(),
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::OpenFileSystem(
@@ -56,21 +116,29 @@ void SyncFileSystemBackend::OpenFileSystem(
     fileapi::OpenFileSystemMode mode,
     const OpenFileSystemCallback& callback) {
   DCHECK(CanHandleType(type));
-  DCHECK(delegate_);
-  delegate_->OpenFileSystem(origin_url, type, mode, callback,
-                            GetSyncableFileSystemRootURI(origin_url));
+
+  if (skip_initialize_syncfs_service_for_testing_) {
+    GetDelegate()->OpenFileSystem(origin_url, type, mode, callback,
+                                  GetSyncableFileSystemRootURI(origin_url));
+    return;
+  }
+
+  // It is safe to pass Unretained(this) since |context_| owns it.
+  SyncStatusCallback initialize_callback =
+      base::Bind(&SyncFileSystemBackend::DidInitializeSyncFileSystemService,
+                 base::Unretained(this), make_scoped_refptr(context_),
+                 origin_url, type, mode, callback);
+  InitializeSyncFileSystemService(origin_url, initialize_callback);
 }
 
 fileapi::FileSystemFileUtil* SyncFileSystemBackend::GetFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->sync_file_util();
+  return GetDelegate()->sync_file_util();
 }
 
 fileapi::AsyncFileUtil* SyncFileSystemBackend::GetAsyncFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->file_util();
+  return GetDelegate()->file_util();
 }
 
 fileapi::CopyOrMoveFileValidatorFactory*
@@ -91,9 +159,8 @@ SyncFileSystemBackend::CreateFileSystemOperation(
   DCHECK(context);
   DCHECK(error_code);
 
-  DCHECK(delegate_);
   scoped_ptr<fileapi::FileSystemOperationContext> operation_context =
-      delegate_->CreateFileSystemOperationContext(url, context, error_code);
+      GetDelegate()->CreateFileSystemOperationContext(url, context,
error_code);
   if (!operation_context)
     return NULL;
 
@@ -113,8 +180,7 @@ SyncFileSystemBackend::CreateFileStreamReader(
     const base::Time& expected_modification_time,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamReader(
+  return GetDelegate()->CreateFileStreamReader(
       url, offset, expected_modification_time, context);
 }
 
@@ -124,13 +190,12 @@ SyncFileSystemBackend::CreateFileStreamWriter(
     int64 offset,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamWriter(
+  return GetDelegate()->CreateFileStreamWriter(
       url, offset, context, fileapi::kFileSystemTypeSyncableForInternalSync);
 }
 
 fileapi::FileSystemQuotaUtil* SyncFileSystemBackend::GetQuotaUtil() {
-  return delegate_;
+  return GetDelegate();
 }
 
 // static
@@ -148,15 +213,15 @@ void SyncFileSystemBackend::SetLocalFileChangeTracker(
   DCHECK(tracker);
   change_tracker_ = tracker.Pass();
 
-  DCHECK(delegate_);
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
-  delegate_->AddFileChangeObserver(
+      delegate->file_task_runner());
+  delegate->AddFileChangeObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::set_sync_context(
@@ -165,4 +230,66 @@ void SyncFileSystemBackend::set_sync_context(
   sync_context_ = sync_context;
 }
 
+fileapi::SandboxFileSystemBackendDelegate*
+SyncFileSystemBackend::GetDelegate() const {
+  DCHECK(context_);
+  DCHECK(context_->sandbox_delegate());
+  return context_->sandbox_delegate();
+}
+
+void SyncFileSystemBackend::InitializeSyncFileSystemService(
+    const GURL& origin_url,
+    const SyncStatusCallback& callback) {
+  // Repost to switch from IO thread to UI thread.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    // It is safe to pass Unretained(this) (see comments in OpenFileSystem()).
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&SyncFileSystemBackend::InitializeSyncFileSystemService,
+                   base::Unretained(this), origin_url, callback));
+    return;
+  }
+
+  if (!profile_holder_->GetProfile()) {
+    // Profile was destroyed.
+    callback.Run(SYNC_FILE_ERROR_FAILED);
+    return;
+  }
+
+  SyncFileSystemService* service = SyncFileSystemServiceFactory::GetForProfile(
+          profile_holder_->GetProfile());
+  DC…
(message too large)

[commit-bot: I haz the power, 2013-08-30 05:54:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/22810002/107001

[commit-bot: I haz the power, 2013-08-30 05:55:04]

Failed to apply patch for
chrome/browser/sync_file_system/local/sync_file_system_backend.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/sync_file_system/local/sync_file_system_backend.cc
  Hunk #4 FAILED at 116.
  Hunk #5 succeeded at 145 (offset -6 lines).
  Hunk #6 succeeded at 166 (offset -6 lines).
  Hunk #7 succeeded at 176 (offset -6 lines).
  Hunk #8 succeeded at 199 (offset -6 lines).
  Hunk #9 succeeded at 216 (offset -6 lines).
  1 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/sync_file_system/local/sync_file_system_backend.cc.rej

Patch:       chrome/browser/sync_file_system/local/sync_file_system_backend.cc
Index: chrome/browser/sync_file_system/local/sync_file_system_backend.cc
diff --git a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
index
ac426c9bca6f8f8b0f8c2052ebe7e0fefcc1992c..ef1f46bb5bc1f72a7ef8f5e9da8f9af0312ffd49
100644
--- a/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
+++ b/chrome/browser/sync_file_system/local/sync_file_system_backend.cc
@@ -5,10 +5,15 @@
 #include "chrome/browser/sync_file_system/local/sync_file_system_backend.h"
 
 #include "base/logging.h"
+#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/sync_file_system/local/local_file_change_tracker.h"
 #include "chrome/browser/sync_file_system/local/local_file_sync_context.h"
 #include
"chrome/browser/sync_file_system/local/syncable_file_system_operation.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service.h"
+#include "chrome/browser/sync_file_system/sync_file_system_service_factory.h"
 #include "chrome/browser/sync_file_system/syncable_file_system_util.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "webkit/browser/fileapi/file_system_context.h"
 #include "webkit/browser/fileapi/file_system_file_stream_reader.h"
 #include "webkit/browser/fileapi/file_system_operation_impl.h"
@@ -16,17 +21,71 @@
 #include "webkit/browser/fileapi/sandbox_quota_observer.h"
 #include "webkit/common/fileapi/file_system_util.h"
 
+using content::BrowserThread;
+
 namespace sync_file_system {
 
-SyncFileSystemBackend::SyncFileSystemBackend()
-    : delegate_(NULL) {
+namespace {
+
+bool CalledOnUIThread() {
+  // Ensure that these methods are called on the UI thread, except for
unittests
+  // where a UI thread might not have been created.
+  return BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         !BrowserThread::IsMessageLoopValid(BrowserThread::UI);
+}
+
+}  // namespace
+
+SyncFileSystemBackend::ProfileHolder::ProfileHolder(Profile* profile)
+    : profile_(profile) {
+  DCHECK(CalledOnUIThread());
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_PROFILE_DESTROYED,
+                 content::Source<Profile>(profile_));
+}
+
+void SyncFileSystemBackend::ProfileHolder::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(CalledOnUIThread());
+  DCHECK_EQ(chrome::NOTIFICATION_PROFILE_DESTROYED, type);
+  DCHECK_EQ(profile_, content::Source<Profile>(source).ptr());
+  profile_ = NULL;
+  registrar_.RemoveAll();
+}
+
+Profile* SyncFileSystemBackend::ProfileHolder::GetProfile() {
+  DCHECK(CalledOnUIThread());
+  return profile_;
+}
+
+SyncFileSystemBackend::SyncFileSystemBackend(Profile* profile)
+    : context_(NULL),
+      skip_initialize_syncfs_service_for_testing_(false) {
+  DCHECK(CalledOnUIThread());
+  if (profile)
+    profile_holder_.reset(new ProfileHolder(profile));
 }
 
 SyncFileSystemBackend::~SyncFileSystemBackend() {
   if (change_tracker_) {
-    delegate_->file_task_runner()->DeleteSoon(
+    GetDelegate()->file_task_runner()->DeleteSoon(
         FROM_HERE, change_tracker_.release());
   }
+
+  if (profile_holder_ && !CalledOnUIThread()) {
+    BrowserThread::DeleteSoon(
+        BrowserThread::UI, FROM_HERE, profile_holder_.release());
+  }
+}
+
+// static
+SyncFileSystemBackend* SyncFileSystemBackend::CreateForTesting() {
+  DCHECK(CalledOnUIThread());
+  SyncFileSystemBackend* backend = new SyncFileSystemBackend(NULL);
+  backend->skip_initialize_syncfs_service_for_testing_ = true;
+  return backend;
 }
 
 bool SyncFileSystemBackend::CanHandleType(
@@ -37,17 +96,18 @@ bool SyncFileSystemBackend::CanHandleType(
 
 void SyncFileSystemBackend::Initialize(fileapi::FileSystemContext* context) {
   DCHECK(context);
-  DCHECK(!delegate_);
-  delegate_ = context->sandbox_delegate();
+  DCHECK(!context_);
+  context_ = context;
 
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
-  delegate_->AddFileUpdateObserver(
+      delegate->quota_observer(),
+      delegate->file_task_runner());
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncableForInternalSync,
-      delegate_->quota_observer(),
-      delegate_->file_task_runner());
+      delegate->quota_observer(),
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::OpenFileSystem(
@@ -56,21 +116,29 @@ void SyncFileSystemBackend::OpenFileSystem(
     fileapi::OpenFileSystemMode mode,
     const OpenFileSystemCallback& callback) {
   DCHECK(CanHandleType(type));
-  DCHECK(delegate_);
-  delegate_->OpenFileSystem(origin_url, type, mode, callback,
-                            GetSyncableFileSystemRootURI(origin_url));
+
+  if (skip_initialize_syncfs_service_for_testing_) {
+    GetDelegate()->OpenFileSystem(origin_url, type, mode, callback,
+                                  GetSyncableFileSystemRootURI(origin_url));
+    return;
+  }
+
+  // It is safe to pass Unretained(this) since |context_| owns it.
+  SyncStatusCallback initialize_callback =
+      base::Bind(&SyncFileSystemBackend::DidInitializeSyncFileSystemService,
+                 base::Unretained(this), make_scoped_refptr(context_),
+                 origin_url, type, mode, callback);
+  InitializeSyncFileSystemService(origin_url, initialize_callback);
 }
 
 fileapi::FileSystemFileUtil* SyncFileSystemBackend::GetFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->sync_file_util();
+  return GetDelegate()->sync_file_util();
 }
 
 fileapi::AsyncFileUtil* SyncFileSystemBackend::GetAsyncFileUtil(
     fileapi::FileSystemType type) {
-  DCHECK(delegate_);
-  return delegate_->file_util();
+  return GetDelegate()->file_util();
 }
 
 fileapi::CopyOrMoveFileValidatorFactory*
@@ -91,9 +159,8 @@ SyncFileSystemBackend::CreateFileSystemOperation(
   DCHECK(context);
   DCHECK(error_code);
 
-  DCHECK(delegate_);
   scoped_ptr<fileapi::FileSystemOperationContext> operation_context =
-      delegate_->CreateFileSystemOperationContext(url, context, error_code);
+      GetDelegate()->CreateFileSystemOperationContext(url, context,
error_code);
   if (!operation_context)
     return NULL;
 
@@ -113,8 +180,7 @@ SyncFileSystemBackend::CreateFileStreamReader(
     const base::Time& expected_modification_time,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamReader(
+  return GetDelegate()->CreateFileStreamReader(
       url, offset, expected_modification_time, context);
 }
 
@@ -124,13 +190,12 @@ SyncFileSystemBackend::CreateFileStreamWriter(
     int64 offset,
     fileapi::FileSystemContext* context) const {
   DCHECK(CanHandleType(url.type()));
-  DCHECK(delegate_);
-  return delegate_->CreateFileStreamWriter(
+  return GetDelegate()->CreateFileStreamWriter(
       url, offset, context, fileapi::kFileSystemTypeSyncableForInternalSync);
 }
 
 fileapi::FileSystemQuotaUtil* SyncFileSystemBackend::GetQuotaUtil() {
-  return delegate_;
+  return GetDelegate();
 }
 
 // static
@@ -148,15 +213,15 @@ void SyncFileSystemBackend::SetLocalFileChangeTracker(
   DCHECK(tracker);
   change_tracker_ = tracker.Pass();
 
-  DCHECK(delegate_);
-  delegate_->AddFileUpdateObserver(
+  fileapi::SandboxFileSystemBackendDelegate* delegate = GetDelegate();
+  delegate->AddFileUpdateObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
-  delegate_->AddFileChangeObserver(
+      delegate->file_task_runner());
+  delegate->AddFileChangeObserver(
       fileapi::kFileSystemTypeSyncable,
       change_tracker_.get(),
-      delegate_->file_task_runner());
+      delegate->file_task_runner());
 }
 
 void SyncFileSystemBackend::set_sync_context(
@@ -165,4 +230,66 @@ void SyncFileSystemBackend::set_sync_context(
   sync_context_ = sync_context;
 }
 
+fileapi::SandboxFileSystemBackendDelegate*
+SyncFileSystemBackend::GetDelegate() const {
+  DCHECK(context_);
+  DCHECK(context_->sandbox_delegate());
+  return context_->sandbox_delegate();
+}
+
+void SyncFileSystemBackend::InitializeSyncFileSystemService(
+    const GURL& origin_url,
+    const SyncStatusCallback& callback) {
+  // Repost to switch from IO thread to UI thread.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    // It is safe to pass Unretained(this) (see comments in OpenFileSystem()).
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&SyncFileSystemBackend::InitializeSyncFileSystemService,
+                   base::Unretained(this), origin_url, callback));
+    return;
+  }
+
+  if (!profile_holder_->GetProfile()) {
+    // Profile was destroyed.
+    callback.Run(SYNC_FILE_ERROR_FAILED);
+    return;
+  }
+
+  SyncFileSystemService* service = SyncFileSystemServiceFactory::GetForProfile(
+          profile_holder_->GetProfile());
+  DC…
(message too large)

[commit-bot: I haz the power, 2013-08-30 06:10:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/22810002/110001

[commit-bot: I haz the power, 2013-08-30 11:20:42]

Change committed as 220552