[Sync] Implements the loopback sync server.

[Sync] Implements the loopback sync server.

The LoopbackServer implements a local sync server that keeps track of the
synced data. It is not meant for concurent operation with multiple clients
but rather serves as a persistent storage for the user profile e.g. for the
purpose of using Chrome with roaming profiles on Windows.

The implementation is based on the FakeSyncServer used for testing the
sync client.

BUG=650258
TEST=components_unittests:LoopbackServerTest.*

Committed: https://crrev.com/b71840a8a2693a61d215fa29986f3a7d47315777
Cr-Commit-Position: refs/heads/master@{#426761}

[pastarmovj, 2016-09-26 12:47:04]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 12:47:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 12:55:22]

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

[commit-bot: I haz the power, 2016-09-26 12:55:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pastarmovj, 2016-09-26 13:09:17]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 13:09:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pastarmovj, 2016-09-26 14:03:10]

Description was changed from

==========
[Sync] Implements the loopback sync server.

The LoopbackServer implements a local sync server that keeps track of the
synced data. It is not meant for concurent operation with multiple clients
but rather serves as a persistent storage for the user profile e.g. for the
purpose of using Chrome with roaming profiles on Windows.

The implementation is based on the FakeSyncServer used for testing the
sync client.

BUG=TBD
TEST=TBD
==========

to

==========
[Sync] Implements the loopback sync server.

The LoopbackServer implements a local sync server that keeps track of the
synced data. It is not meant for concurent operation with multiple clients
but rather serves as a persistent storage for the user profile e.g. for the
purpose of using Chrome with roaming profiles on Windows.

The implementation is based on the FakeSyncServer used for testing the
sync client.

BUG=650258
TEST=components_unittests:LoopbackServerTest.*
==========

[pastarmovj, 2016-09-26 14:04:39]

pastarmovj@chromium.org changed reviewers:
+ pavely@chromium.org

[pastarmovj, 2016-09-26 14:04:40]

Hi Pavel,
Please review this CL. It implements the LoopbackServer. It is branched off the
FakeServer and hopefully will be able to replace it eventually. 
Thanks!
Julian

[commit-bot: I haz the power, 2016-09-26 14:54:26]

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

[commit-bot: I haz the power, 2016-09-26 14:54:27]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pastarmovj, 2016-09-30 07:22:15]

gentle ping? :)

[maxbogue, 2016-09-30 19:58:33]

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

[maxbogue, 2016-09-30 19:58:34]

Hey Julian, I have some concerns with this :(

- Would it be possible to do this by changing FakeServer and then moving it?
That is, never branching it. We really would prefer to not ever have two
versions of this code.
- I suppose it's not that large, but we would prefer to not compile this in
situations where it's not needed. Could it have its own source_set in BUILD.gn
that's only included for windows and tests? Binary size is a concern, especially
on Android.
- It feels weird putting it in engine_impl/net, though maybe there isn't a
better place? Whatever pieces you'll need to use outside of sync/engine(_impl)
should live in engine/net though.
- I'd prefer the *Entity classes either have Loopback added to their name
(PermanentLoopbackEntity, etc) or you maintain a namespace around it all. It'd
be a bit confusing to have something like BookmarkEntity in the syncer namespace
that's only used for this.

[skym, 2016-10-03 15:31:14]

On 2016/09/30 19:58:34, maxbogue wrote:
> Hey Julian, I have some concerns with this :(
> 
> - Would it be possible to do this by changing FakeServer and then moving it?
> That is, never branching it. We really would prefer to not ever have two
> versions of this code.
> - I suppose it's not that large, but we would prefer to not compile this in
> situations where it's not needed. Could it have its own source_set in BUILD.gn
> that's only included for windows and tests? Binary size is a concern,
especially
> on Android.
> - It feels weird putting it in engine_impl/net, though maybe there isn't a
> better place? Whatever pieces you'll need to use outside of sync/engine(_impl)
> should live in engine/net though.
> - I'd prefer the *Entity classes either have Loopback added to their name
> (PermanentLoopbackEntity, etc) or you maintain a namespace around it all. It'd
> be a bit confusing to have something like BookmarkEntity in the syncer
namespace
> that's only used for this.

While I agree with Max that a non-general BookmarkEntity in the syncer namespace
would be bad, I disagree about appending Lookback. I worry about using the
Loopback term for most of these classes. While looping back is how you plan to
use them, it's not at all what the individual classes need to do/accomplish
themselves. Especially if they're also used in tests. For example, looking at
LoopbackServer, it's core new logic is about [de]serialization to a file. It
knows nothing about how network requests arrived at it, and nothing about the
type of folder it's saving the file to.

[pastarmovj, 2016-10-04 15:08:50]

On 2016/10/03 15:31:14, skym wrote:
> On 2016/09/30 19:58:34, maxbogue wrote:
> > Hey Julian, I have some concerns with this :(
> > 
> > - Would it be possible to do this by changing FakeServer and then moving it?
> > That is, never branching it. We really would prefer to not ever have two
> > versions of this code.
> > - I suppose it's not that large, but we would prefer to not compile this in
> > situations where it's not needed. Could it have its own source_set in
BUILD.gn
> > that's only included for windows and tests? Binary size is a concern,
> especially
> > on Android.
> > - It feels weird putting it in engine_impl/net, though maybe there isn't a
> > better place? Whatever pieces you'll need to use outside of
sync/engine(_impl)
> > should live in engine/net though.
> > - I'd prefer the *Entity classes either have Loopback added to their name
> > (PermanentLoopbackEntity, etc) or you maintain a namespace around it all.
It'd
> > be a bit confusing to have something like BookmarkEntity in the syncer
> namespace
> > that's only used for this.
> 
> While I agree with Max that a non-general BookmarkEntity in the syncer
namespace
> would be bad, I disagree about appending Lookback. I worry about using the
> Loopback term for most of these classes. While looping back is how you plan to
> use them, it's not at all what the individual classes need to do/accomplish
> themselves. Especially if they're also used in tests. For example, looking at
> LoopbackServer, it's core new logic is about [de]serialization to a file. It
> knows nothing about how network requests arrived at it, and nothing about the
> type of folder it's saving the file to.

Thanks for the great comments to both of you!

I will certainly follow up on pulling the code out of the global sync target and
making it optional for the platforms where it should be available (win initially
and quite probably ChromeOS).

About where those files should live in the first place I will totally follow
your advice on a better place. I placed them in engine_impl/net because I
followed the location of the code that inspired my work.

I agree also about better naming of the entity classes. What about adding the
"Persistent" qualifier (I prefer it a little over Permanent but can agree to
both)? E.g. PersistentBookmarkEntity etc.

Lastly I am all on board to making this class the only "fake" server avaiable in
the codebase however for the sake of not exploding the complexity of the changes
I think that a two stage process where we first come up with a separate impl and
gradually shift tests to it will make the work split better. If you are
concerned I will conveniently forget to do this once my job is accomplished rest
assured it is already captured in a bug that carries my name -
https://bugs.chromium.org/p/chromium/issues/detail?id=651415 :)

[maxbogue, 2016-10-04 21:29:45]

On 2016/10/04 15:08:50, pastarmovj wrote:
> Thanks for the great comments to both of you!
> 
> I will certainly follow up on pulling the code out of the global sync target
and
> making it optional for the platforms where it should be available (win
initially
> and quite probably ChromeOS).

Would you mind doing this part in this CL?

> About where those files should live in the first place I will totally follow
> your advice on a better place. I placed them in engine_impl/net because I
> followed the location of the code that inspired my work.

I discussed with the team and I believe
//components/sync/engine_impl/loopback_server would be best. Pieces that are
needed outside of engine(_impl) should live in
//components/sync/engine/loopback_server. (The _impl folders are how we're doing
"package private" things.) In case you didn't know about it,
https://cs.chromium.org/chromium/src/tools/git/mass-rename.py makes moving a lot
of files around very, very easy.

> I agree also about better naming of the entity classes. What about adding the
> "Persistent" qualifier (I prefer it a little over Permanent but can agree to
> both)? E.g. PersistentBookmarkEntity etc.

I would lean toward Persistent or Persisted, yeah.
 
> Lastly I am all on board to making this class the only "fake" server avaiable
in
> the codebase however for the sake of not exploding the complexity of the
changes
> I think that a two stage process where we first come up with a separate impl
and
> gradually shift tests to it will make the work split better. If you are
> concerned I will conveniently forget to do this once my job is accomplished
rest
> assured it is already captured in a bug that carries my name -
> https://bugs.chromium.org/p/chromium/issues/detail?id=651415 :)

Ok, thanks! Such cleanup tasks have a nasty habit of never getting done, so it's
reassuring that you at least have a bug tracking it. I'd prefer it be done from
the start but if you don't think that's feasible this is ok.

[pastarmovj, 2016-10-05 12:21:45]

Thanks for all the comments guys! 

I moved all new classes in engine_impl/loopback_server. I don't think any of
those will become public interface and will have to move outside of the impl
path. Unless needed for replacing the FakeServer in tests.

[maxbogue, 2016-10-05 16:57:08]

maxbogue@chromium.org changed reviewers:
- maxbogue@chromium.org

[pavely, 2016-10-06 23:48:46]

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_connection_manager.cc
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_connection_manager.cc:8:
#include "components/sync/syncable/directory.h"
I don't think you need directory.h here.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_connection_manager.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_connection_manager.h:19:
class LoopbackConnectionManager : public ServerConnectionManager {
In today's code we don't have a way to inject server connection manager.
Specific implementation of it is instantiated in SyncManagerImpl::Init. Fake
server injects itself through NetworkResources, which is a bit more work.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server.cc (right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.cc:499: if
(entity.second->IsDeleted())
This "if" statement is essentially a switch on the type of loopback server
entity. 
Is it possible to remove it and have each implementation of LoopbackServerEntity
return/populate appropriate value?

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.cc:523: switch
(entity.type()) {
I would refactor this switch into a factory function that takes entity.entity()
and returns unique_ptr<LoopbackServerEntity>. You can add this factory function
as a static method of LoopbackServerEntity. This will minimize number of places
to modify if we ever decide to add another type of entity.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server.h (right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.h:106: void
SerializeState(sync_pb::LoopbackServerProto* proto) const;
Right now you are moving in the direction of persisting the whole state every
time you trigger SaveStateToFile. Depending on how often this happens it could
add up to noticeable IO. If you use some database instead of flat file (leveldb
or sqlite) and write only modified entities this could save some traffic.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server_entity.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_entity.h:68:
std::string id_;
Why did you remove const? Are you planning to modify id or model_type of
existing entities?

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server_unittest.cc
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:37:
static SyncProtocolError CallGetProtocolErrorFromResponse(
You don't use this function right now, Do you have plans for it later?

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:53:
TestDirectorySetterUpper dir_maker_;
Why do you need directory and message loop here?

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:159:
CallPostAndProcessHeaders(lcm_.get(), NULL, get_updates_msg, &response));
I think you meant second_user.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:161:
ASSERT_TRUE(response.has_get_updates());
Sometimes you use ASSERT instead of EXPECT. Is it intentional?

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:175: //
Not quite well formed but enough to fool the server.
This block of code is repeated in every test that involves commit. I'd refactor
it into helper function.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h:27: //
only for
Could you join this line with the next one. The same in comment to next
function.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h:44:
static std::unique_ptr<LoopbackServerEntity> CreateFromEntity(
Could you add comment in which scenario this factory method should be used.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/persistent_permanent_entity.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/persistent_permanent_entity.h:31: //
globally
Could you reformat line wraps in comments.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/protoco...
File components/sync/protocol/loopback_server.proto (right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/protoco...
components/sync/protocol/loopback_server.proto:5: // Sync protocol for
communication between sync client and server.
I don't think this comment applies here. I think this is protocol for persisting
loopback server state to disk.

[pastarmovj, 2016-10-13 13:49:43]

Patchset #7 (id:120001) has been deleted

[pastarmovj, 2016-10-13 14:13:40]

Thanks for the great comments!

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_connection_manager.cc
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_connection_manager.cc:8:
#include "components/sync/syncable/directory.h"
On 2016/10/06 23:48:45, pavely wrote:
> I don't think you need directory.h here.

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_connection_manager.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_connection_manager.h:19:
class LoopbackConnectionManager : public ServerConnectionManager {
On 2016/10/06 23:48:45, pavely wrote:
> In today's code we don't have a way to inject server connection manager.
> Specific implementation of it is instantiated in SyncManagerImpl::Init. Fake
> server injects itself through NetworkResources, which is a bit more work.

I was planning on conditionally creating one or the other in the Init function.
See my proof of concept here https://codereview.chromium.org/2106743002/ . Of
course I am going to follow your wisdom on this front, I just wanted to show you
what my solution was.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server.cc (right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.cc:499: if
(entity.second->IsDeleted())
On 2016/10/06 23:48:45, pavely wrote:
> This "if" statement is essentially a switch on the type of loopback server
> entity. 
> Is it possible to remove it and have each implementation of
LoopbackServerEntity
> return/populate appropriate value?

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.cc:523: switch
(entity.type()) {
On 2016/10/06 23:48:45, pavely wrote:
> I would refactor this switch into a factory function that takes
entity.entity()
> and returns unique_ptr<LoopbackServerEntity>. You can add this factory
function
> as a static method of LoopbackServerEntity. This will minimize number of
places
> to modify if we ever decide to add another type of entity.

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server.h (right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.h:106: void
SerializeState(sync_pb::LoopbackServerProto* proto) const;
On 2016/10/06 23:48:45, pavely wrote:
> Right now you are moving in the direction of persisting the whole state every
> time you trigger SaveStateToFile. Depending on how often this happens it could
> add up to noticeable IO. If you use some database instead of flat file
(leveldb
> or sqlite) and write only modified entities this could save some traffic.

I agree. I would rather keep this part simple in this CL and elaborate in future
CLs. My goal being to build it top-down having something solid that works even
if it is slow/memory consuming and then replace the parts that make it slow.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server_entity.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_entity.h:68:
std::string id_;
On 2016/10/06 23:48:45, pavely wrote:
> Why did you remove const? Are you planning to modify id or model_type of
> existing entities?

Nope. I needed this for manual testing but then I forgot to revert it. Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/loopback_server_unittest.cc
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:37:
static SyncProtocolError CallGetProtocolErrorFromResponse(
On 2016/10/06 23:48:45, pavely wrote:
> You don't use this function right now, Do you have plans for it later?

I was planning to but for not I don't need it so removed.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:53:
TestDirectorySetterUpper dir_maker_;
On 2016/10/06 23:48:45, pavely wrote:
> Why do you need directory and message loop here?

Leftovers :) Removed.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:159:
CallPostAndProcessHeaders(lcm_.get(), NULL, get_updates_msg, &response));
On 2016/10/06 23:48:45, pavely wrote:
> I think you meant second_user.

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:161:
ASSERT_TRUE(response.has_get_updates());
On 2016/10/06 23:48:45, pavely wrote:
> Sometimes you use ASSERT instead of EXPECT. Is it intentional?

Yes. The EXPECT below will crash if this is false. ASSERT will cause the test to
end in a controlled way instead.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server_unittest.cc:175: //
Not quite well formed but enough to fool the server.
On 2016/10/06 23:48:45, pavely wrote:
> This block of code is repeated in every test that involves commit. I'd
refactor
> it into helper function.

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h:27: //
only for
On 2016/10/06 23:48:46, pavely wrote:
> Could you join this line with the next one. The same in comment to next
> function.

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h:44:
static std::unique_ptr<LoopbackServerEntity> CreateFromEntity(
On 2016/10/06 23:48:45, pavely wrote:
> Could you add comment in which scenario this factory method should be used.

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
File components/sync/engine_impl/loopback_server/persistent_permanent_entity.h
(right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/engine_...
components/sync/engine_impl/loopback_server/persistent_permanent_entity.h:31: //
globally
On 2016/10/06 23:48:46, pavely wrote:
> Could you reformat line wraps in comments.

Done.

https://codereview.chromium.org/2360703002/diff/80001/components/sync/protoco...
File components/sync/protocol/loopback_server.proto (right):

https://codereview.chromium.org/2360703002/diff/80001/components/sync/protoco...
components/sync/protocol/loopback_server.proto:5: // Sync protocol for
communication between sync client and server.
On 2016/10/06 23:48:46, pavely wrote:
> I don't think this comment applies here. I think this is protocol for
persisting
> loopback server state to disk.

Done.

Also cleaned up some commented out fields.

[pastarmovj, 2016-10-19 09:46:31]

ping? :)

[pavely, 2016-10-19 18:21:30]

lgtm

https://codereview.chromium.org/2360703002/diff/100001/components/sync/engine...
File components/sync/engine_impl/loopback_server/loopback_server_entity.cc
(right):

https://codereview.chromium.org/2360703002/diff/100001/components/sync/engine...
components/sync/engine_impl/loopback_server/loopback_server_entity.cc:67:
default:
You can handle UNKNOWN case with CHECK(false) and omit "default:" completely.
This way when you forget to handle new enum value here compiler will complain
about it.

[pastarmovj, 2016-10-20 16:23:22]

pastarmovj@chromium.org changed reviewers:
+ asanka@chromium.org

[pastarmovj, 2016-10-20 16:23:22]

Adding asanka for DEPS of +net approval.
Sync already uses net only this is a new directory which required its own DEPS
file.

[asanka, 2016-10-20 21:50:48]

On 2016/10/20 at 16:23:22, pastarmovj wrote:
> Adding asanka for DEPS of +net approval.
> Sync already uses net only this is a new directory which required its own DEPS
file.

The net dependency lgtm.

[pastarmovj, 2016-10-21 09:04:43]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-10-21 09:04:43]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2360703002/#ps160001
(title: "Address comment.")

[commit-bot: I haz the power, 2016-10-21 09:04:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 10:14:01]

Description was changed from

==========
[Sync] Implements the loopback sync server.

The LoopbackServer implements a local sync server that keeps track of the
synced data. It is not meant for concurent operation with multiple clients
but rather serves as a persistent storage for the user profile e.g. for the
purpose of using Chrome with roaming profiles on Windows.

The implementation is based on the FakeSyncServer used for testing the
sync client.

BUG=650258
TEST=components_unittests:LoopbackServerTest.*
==========

to

==========
[Sync] Implements the loopback sync server.

The LoopbackServer implements a local sync server that keeps track of the
synced data. It is not meant for concurent operation with multiple clients
but rather serves as a persistent storage for the user profile e.g. for the
purpose of using Chrome with roaming profiles on Windows.

The implementation is based on the FakeSyncServer used for testing the
sync client.

BUG=650258
TEST=components_unittests:LoopbackServerTest.*
==========

[commit-bot: I haz the power, 2016-10-21 10:14:02]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-10-21 13:28:30]

Description was changed from

==========
[Sync] Implements the loopback sync server.

The LoopbackServer implements a local sync server that keeps track of the
synced data. It is not meant for concurent operation with multiple clients
but rather serves as a persistent storage for the user profile e.g. for the
purpose of using Chrome with roaming profiles on Windows.

The implementation is based on the FakeSyncServer used for testing the
sync client.

BUG=650258
TEST=components_unittests:LoopbackServerTest.*
==========

to

==========
[Sync] Implements the loopback sync server.

The LoopbackServer implements a local sync server that keeps track of the
synced data. It is not meant for concurent operation with multiple clients
but rather serves as a persistent storage for the user profile e.g. for the
purpose of using Chrome with roaming profiles on Windows.

The implementation is based on the FakeSyncServer used for testing the
sync client.

BUG=650258
TEST=components_unittests:LoopbackServerTest.*

Committed: https://crrev.com/b71840a8a2693a61d215fa29986f3a7d47315777
Cr-Commit-Position: refs/heads/master@{#426761}
==========

[commit-bot: I haz the power, 2016-10-21 13:28:32]

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