Persist Directory to disk when the app is backgrounded.

On Android, save the Directory when the app is backgrounded.

Because we don't know when the app will be killed, the current
time before Directory saves can result in lost data. This
change uses the app getting backgrounded as a signal that we
might be killed soon and triggers a Directory::SaveChanges().

BUG=395233
Committed: https://crrev.com/d4470ca3ec0642a68abe141b6bd69529d5814315
Cr-Commit-Position: refs/heads/master@{#297547}

[maxbogue, 2014-09-10 17:31:35]

maxbogue@chromium.org changed reviewers:
+ nyquist@chromium.org, zea@chromium.org

[maxbogue, 2014-09-10 17:31:35]

I got this working. PTAL and let me know if anything could be better.
Specifically, I had no idea where to put FlushDirectoryWrapper. Also, if either
of you have advice regarding how to test this, it would be appreciated.

[nyquist, 2014-09-10 17:43:09]

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:528: void
SyncBackendHostImpl::FlushDirectory() const {
Does this require the sync engine to be initialized?

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:530: if (directory !=
nullptr) {
!= NULL ?

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:797: void FlushDirectory();
Add a comment about what this ends up doing. In what use-case should it be
called?

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.cc:185: if (sync_service_ !=
nullptr) {
Use NULL instead of nullptr.

Also, DCHECK for UI thread?

And thirdly, is it common for this to be null? If not, there is already a DCHECK
in the constructor for this. Since this is not static, I would assume that the
service would always be there?

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.h:63: void
FlushDirectory(JNIEnv* env, jobject obj);
Add a short comment.

[maniscalco, 2014-09-10 17:49:02]

maniscalco@chromium.org changed reviewers:
+ maniscalco@chromium.org

[maniscalco, 2014-09-10 17:49:03]

(drive-by) Glad to see this getting implemented!

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:524: void
FlushDirectoryWrapper(syncer::syncable::Directory* directory) {
What's the motivation for using this free function in the Bind instead of
binding to Directory::SaveChanges?

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:797: void FlushDirectory();
Not const like the other FlushDirectory methods?

[maxbogue, 2014-09-10 20:11:02]

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:524: void
FlushDirectoryWrapper(syncer::syncable::Directory* directory) {
On 2014/09/10 17:49:03, maniscalco wrote:
> What's the motivation for using this free function in the Bind instead of
> binding to Directory::SaveChanges?

Directory::SaveChanges returns a bool, so it cannot be used as a Closure. Added
a comment saying so.

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:528: void
SyncBackendHostImpl::FlushDirectory() const {
On 2014/09/10 17:43:09, nyquist wrote:
> Does this require the sync engine to be initialized?

I kind of assumed that directory being NULL meant sync was uninitialized,
meaning this function can be called but does nothing if it's not initialized.
Nicholas, is that correct?

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:530: if (directory !=
nullptr) {
On 2014/09/10 17:43:09, nyquist wrote:
> != NULL ?

Done.

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:797: void FlushDirectory();
On 2014/09/10 17:49:03, maniscalco wrote:
> Not const like the other FlushDirectory methods?

Done.

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.h:797: void FlushDirectory();
On 2014/09/10 17:43:09, nyquist wrote:
> Add a comment about what this ends up doing. In what use-case should it be
> called?

Done.

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.cc:185: if (sync_service_ !=
nullptr) {
On 2014/09/10 17:43:09, nyquist wrote:
> Use NULL instead of nullptr.
> 
> Also, DCHECK for UI thread?
> 
> And thirdly, is it common for this to be null? If not, there is already a
DCHECK
> in the constructor for this. Since this is not static, I would assume that the
> service would always be there?

Added the DCHECK. I've removed the null check entirely; I think I only added it
trying to fix the crash that the other null check prevented.

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service_android.h:63: void
FlushDirectory(JNIEnv* env, jobject obj);
On 2014/09/10 17:43:09, nyquist wrote:
> Add a short comment.

Done.

[maniscalco, 2014-09-10 20:19:25]

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:524: void
FlushDirectoryWrapper(syncer::syncable::Directory* directory) {
On 2014/09/10 20:11:01, maxbogue wrote:
> On 2014/09/10 17:49:03, maniscalco wrote:
> > What's the motivation for using this free function in the Bind instead of
> > binding to Directory::SaveChanges?
> 
> Directory::SaveChanges returns a bool, so it cannot be used as a Closure.
Added
> a comment saying so.

Ah, check out IgnoreResult:

https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=167

That should let you forego the extra free function.

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:532:
base::Bind(&FlushDirectoryWrapper, directory));
As we talked about in person over lunch, it seems like the tricky part is
ensuring that when the task eventually executes in the sync thread, the
directory is still around.  It seems like we could end up calling SaveChanges on
a deleted Directory.  Maybe zea@ has some suggestions here?

[maxbogue, 2014-09-12 16:45:38]

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:524: void
FlushDirectoryWrapper(syncer::syncable::Directory* directory) {
On 2014/09/10 20:19:25, maniscalco wrote:
> On 2014/09/10 20:11:01, maxbogue wrote:
> > On 2014/09/10 17:49:03, maniscalco wrote:
> > > What's the motivation for using this free function in the Bind instead of
> > > binding to Directory::SaveChanges?
> > 
> > Directory::SaveChanges returns a bool, so it cannot be used as a Closure.
> Added
> > a comment saying so.
> 
> Ah, check out IgnoreResult:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=167
> 
> That should let you forego the extra free function.

Done.

https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_impl.cc:532:
base::Bind(&FlushDirectoryWrapper, directory));
On 2014/09/10 20:19:25, maniscalco wrote:
> As we talked about in person over lunch, it seems like the tricky part is
> ensuring that when the task eventually executes in the sync thread, the
> directory is still around.  It seems like we could end up calling SaveChanges
on
> a deleted Directory.  Maybe zea@ has some suggestions here?

Routed through SyncBackendHostCore to try to solve this problem.

[maniscalco, 2014-09-12 17:31:44]

On 2014/09/12 16:45:38, maxbogue wrote:
>
https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
> File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):
> 
>
https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
> chrome/browser/sync/glue/sync_backend_host_impl.cc:524: void
> FlushDirectoryWrapper(syncer::syncable::Directory* directory) {
> On 2014/09/10 20:19:25, maniscalco wrote:
> > On 2014/09/10 20:11:01, maxbogue wrote:
> > > On 2014/09/10 17:49:03, maniscalco wrote:
> > > > What's the motivation for using this free function in the Bind instead
of
> > > > binding to Directory::SaveChanges?
> > > 
> > > Directory::SaveChanges returns a bool, so it cannot be used as a Closure.
> > Added
> > > a comment saying so.
> > 
> > Ah, check out IgnoreResult:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=167
> > 
> > That should let you forego the extra free function.
> 
> Done.
> 
>
https://codereview.chromium.org/551843003/diff/20001/chrome/browser/sync/glue...
> chrome/browser/sync/glue/sync_backend_host_impl.cc:532:
> base::Bind(&FlushDirectoryWrapper, directory));
> On 2014/09/10 20:19:25, maniscalco wrote:
> > As we talked about in person over lunch, it seems like the tricky part is
> > ensuring that when the task eventually executes in the sync thread, the
> > directory is still around.  It seems like we could end up calling
SaveChanges
> on
> > a deleted Directory.  Maybe zea@ has some suggestions here?
> 
> Routed through SyncBackendHostCore to try to solve this problem.

Can you kick off a round of try bots (git cl try)?

[maniscalco, 2014-09-12 17:42:19]

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.cc:667:
syncer::syncable::Directory* directory =
It would be a good idea to add a DCHECK to ensure this is called on the correct
thread (similar to what's done in other methods like DoRetryConfiguraiton):

DCHECK_EQ(base::MessageLoop::current(), sync_loop_);

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.h:216: // Called from the UI
thread to save the directory to disk.
This comment should say "called on the sync" thread, right?

[maxbogue, 2014-09-12 18:48:14]

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.cc:667:
syncer::syncable::Directory* directory =
On 2014/09/12 17:42:19, maniscalco wrote:
> It would be a good idea to add a DCHECK to ensure this is called on the
correct
> thread (similar to what's done in other methods like DoRetryConfiguraiton):
> 
> DCHECK_EQ(base::MessageLoop::current(), sync_loop_);

Done.

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.h:216: // Called from the UI
thread to save the directory to disk.
On 2014/09/12 17:42:19, maniscalco wrote:
> This comment should say "called on the sync" thread, right?

Removed any mention of thread from this comment. All the Do* functions are
called via SyncBackendHost on the sync thread, as explained in a comment higher
up.

[maniscalco, 2014-09-12 20:04:26]

On 2014/09/12 18:48:14, maxbogue wrote:
>
https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
> File chrome/browser/sync/glue/sync_backend_host_core.cc (right):
> 
>
https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
> chrome/browser/sync/glue/sync_backend_host_core.cc:667:
> syncer::syncable::Directory* directory =
> On 2014/09/12 17:42:19, maniscalco wrote:
> > It would be a good idea to add a DCHECK to ensure this is called on the
> correct
> > thread (similar to what's done in other methods like DoRetryConfiguraiton):
> > 
> > DCHECK_EQ(base::MessageLoop::current(), sync_loop_);
> 
> Done.
> 
>
https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
> File chrome/browser/sync/glue/sync_backend_host_core.h (right):
> 
>
https://codereview.chromium.org/551843003/diff/60001/chrome/browser/sync/glue...
> chrome/browser/sync/glue/sync_backend_host_core.h:216: // Called from the UI
> thread to save the directory to disk.
> On 2014/09/12 17:42:19, maniscalco wrote:
> > This comment should say "called on the sync" thread, right?
> 
> Removed any mention of thread from this comment. All the Do* functions are
> called via SyncBackendHost on the sync thread, as explained in a comment
higher
> up.

Nice.  This LGTM, but, of course, please wait for LGTMs from zea@ and nyquist@.

[nyquist, 2014-09-15 07:19:41]

i don't know enough about the details about the threading models after the PSS
calls into the backend host, but the rest lgtm. The host and host core threading
looks reasonable to me though, if that counts for anything.

[Nicolas Zea, 2014-09-15 22:39:00]

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.cc:668:
syncer::syncable::Directory* directory =
What will this do if the SBH isn't initialized yet? Should we let it crash or
just do nothing?

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2773: if (backend_ != NULL) {
nit: no curly braces for single line conditional (to match the rest of file)

[maxbogue, 2014-09-16 15:34:31]

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.cc:668:
syncer::syncable::Directory* directory =
On 2014/09/15 22:38:59, Nicolas Zea wrote:
> What will this do if the SBH isn't initialized yet? Should we let it crash or
> just do nothing?

Which part will currently crash? How would the call even get to this point if
things aren't initialized?

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:2773: if (backend_ != NULL) {
On 2014/09/15 22:38:59, Nicolas Zea wrote:
> nit: no curly braces for single line conditional (to match the rest of file)

Done. This should be against the Chrome style guide though.

[Nicolas Zea, 2014-09-18 16:52:18]

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.cc:668:
syncer::syncable::Directory* directory =
On 2014/09/16 15:34:31, maxbogue wrote:
> On 2014/09/15 22:38:59, Nicolas Zea wrote:
> > What will this do if the SBH isn't initialized yet? Should we let it crash
or
> > just do nothing?
> 
> Which part will currently crash? How would the call even get to this point if
> things aren't initialized?

The SyncManager is only created when DoInitialize is called. It's possible for
the core to exist without DoInitialize having been called, and therefore the
SyncManager having been created.

As far as I can tell nowhere higher up in the callstack is it verified that the
backend has been initialized right?

[maxbogue, 2014-09-22 17:23:25]

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/80001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_host_core.cc:668:
syncer::syncable::Directory* directory =
On 2014/09/18 16:52:17, Nicolas Zea wrote:
> On 2014/09/16 15:34:31, maxbogue wrote:
> > On 2014/09/15 22:38:59, Nicolas Zea wrote:
> > > What will this do if the SBH isn't initialized yet? Should we let it crash
> or
> > > just do nothing?
> > 
> > Which part will currently crash? How would the call even get to this point
if
> > things aren't initialized?
> 
> The SyncManager is only created when DoInitialize is called. It's possible for
> the core to exist without DoInitialize having been called, and therefore the
> SyncManager having been created.
> 
> As far as I can tell nowhere higher up in the callstack is it verified that
the
> backend has been initialized right?

Fixed this in PSS.

[Nicolas Zea, 2014-09-22 17:27:46]

Mostly LG. Were you going to add tests?

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_core.cc:670: if (directory != NULL) {
The PSS conditions should protect directory from being null right? I'd get rid
of this conditional and let it crash if not, which would let us know that
something else isn't being taken into account.

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:523: void
SyncBackendHostImpl::FlushDirectory() const {
DCHECK initialized()?

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:2772: // Make sure backend_ isn't
NULL and that the manager exists.
nit: clarify in comment how that relates to backend_initialized_

[pval...(no longer on Chromium), 2014-09-23 20:11:11]

pvalenzuela@chromium.org changed reviewers:
+ pvalenzuela@chromium.org

[pval...(no longer on Chromium), 2014-09-23 20:11:12]

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_core.cc:666: void
SyncBackendHostCore::DoFlushDirectory() {
Max/Nicolas: Here is an idea for testing this forced-flush feature:

1) Add a callback param that populates a value in ProfileSyncServiceAndroid
(let's call it wasFlushDirectorySuccessful) and call it with the return value of
directory->SaveChanges().

2) Add a JNI method to ProfileSyncServiceAndroid that returns
wasFlushDirectorySuccessful.

3) In a Java test, we can trigger the PAUSED state change (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...),
and then wait for wasFlushDirectorySuccessful to be true.

Thoughts on this? I wish we could actually verify that the data was saved in the
data store, but this seems almost as good.

[maxbogue, 2014-09-23 22:56:46]

On 2014/09/23 20:11:12, pvalenzuela wrote:
>
https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
> File chrome/browser/sync/glue/sync_backend_host_core.cc (right):
> 
>
https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
> chrome/browser/sync/glue/sync_backend_host_core.cc:666: void
> SyncBackendHostCore::DoFlushDirectory() {
> Max/Nicolas: Here is an idea for testing this forced-flush feature:
> 
> 1) Add a callback param that populates a value in ProfileSyncServiceAndroid
> (let's call it wasFlushDirectorySuccessful) and call it with the return value
of
> directory->SaveChanges().
> 
> 2) Add a JNI method to ProfileSyncServiceAndroid that returns
> wasFlushDirectorySuccessful.
> 
> 3) In a Java test, we can trigger the PAUSED state change (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java/...),
> and then wait for wasFlushDirectorySuccessful to be true.
> 
> Thoughts on this? I wish we could actually verify that the data was saved in
the
> data store, but this seems almost as good.

Sounds fine to me. What do you think Nicolas?

[nyquist, 2014-09-24 16:34:06]

Would this be a member that would be cleared when flush is called again? Or
would you pass in a callback when you call flush?

[Nicolas Zea, 2014-09-25 18:16:48]

FYI, I just noticed the the backup/rollback tests are already checking the db
last modified time (via syncer::CheckSyncDbLastModifiedTime). Might be useful
for testing.

[maxbogue, 2014-09-30 00:26:33]

PTAL. I worked with Patrick to add a minimal test with a TODO for future work,
as well as consolidated on the existing SyncBackendHostCore::SaveChanges()
function instead of adding DoFlushDirectory().

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/551843003/diff/120001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:523: void
SyncBackendHostImpl::FlushDirectory() const {
On 2014/09/22 17:27:46, Nicolas Zea wrote:
> DCHECK initialized()?

Done.

[pval...(no longer on Chromium), 2014-09-30 17:55:21]

lgtm

Thanks for adding the test, Max. This will give us a really good test case in
the future.

[Nicolas Zea, 2014-09-30 21:20:22]

LGTM with a couple nits

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_core.cc:25: #include
"sync/internal_api/public/user_share.h"
Is this needed?

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_core.h:242: // Tell the syncapi to
persist its state by writing to disk.
nit: syncapi -> sync manager

[maxbogue, 2014-09-30 21:59:34]

Comments addressed. Thanks for the reviews!

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_core.cc:25: #include
"sync/internal_api/public/user_share.h"
On 2014/09/30 21:20:22, Nicolas Zea wrote:
> Is this needed?

Nope; good catch.

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/551843003/diff/140001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_core.h:242: // Tell the syncapi to
persist its state by writing to disk.
On 2014/09/30 21:20:22, Nicolas Zea wrote:
> nit: syncapi -> sync manager

Done.

[maxbogue, 2014-09-30 21:59:41]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2014-09-30 22:01:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/551843003/160001

[commit-bot: I haz the power, 2014-09-30 23:38:19]

Committed patchset #9 (id:160001) as 0dc698045295c6474d38d0a63264d6431a94093f

[commit-bot: I haz the power, 2014-09-30 23:39:01]

Patchset 9 (id:??) landed as
https://crrev.com/d4470ca3ec0642a68abe141b6bd69529d5814315
Cr-Commit-Position: refs/heads/master@{#297547}