[Sync] Adds a new switch for enabling the new local server backend.

Adds a new switch for enabling the new local server backend.

Actually two flags which play in tandem:

--enable-local-sync-backend and --local-sync-backend-dir

The second is respected only if the first one is specified.
Eventually those will be controlled by policy but the flags will allow
easier testing and potentially unlock the feature for advanced users.

For now sign-in is needed to kick the sync logic on but the data
is only stored locally when this flag is specified.

BUG=651411
TEST=unit_tests/manual.

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

[pastarmovj, 2016-10-21 13:10:25]

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

[pastarmovj, 2016-10-21 13:10:25]

Hi Pavel,

here is the next (much smaller) part of the puzzle.
Please let me know if you'd rather have the review load distributed over other
members of the sync team (or simply reassign the CL yourself). I put you as a
reviewer because you were on the design meetings and know what is supposed to
happen eventually :)

Cheers,
Julian

[pastarmovj, 2016-10-21 13:30:44]

Description was changed from

==========
Adds a new switch for enabling the new local server backend.

Actually two flags which play in tandem:

--enable-local-sync-backend and --local-sync-backend-dir

The second is respected only if the first one is specified.
Eventually those will be controlled by policy but the flags will allow
easier testing and potentially unlock the feature for advanced users.

For now sign-in is needed to kick the sync logic on but the data
is only stored locally when this flag is specified.

BUG=651411
TEST=unit_tests/manual.
==========

to

==========
Adds a new switch for enabling the new local server backend.

Actually two flags which play in tandem:

--enable-local-sync-backend and --local-sync-backend-dir

The second is respected only if the first one is specified.
Eventually those will be controlled by policy but the flags will allow
easier testing and potentially unlock the feature for advanced users.

For now sign-in is needed to kick the sync logic on but the data
is only stored locally when this flag is specified.

BUG=651411
TEST=unit_tests/manual.
==========

[pastarmovj, 2016-10-27 09:43:00]

On 2016/10/21 13:10:25, pastarmovj wrote:
> Hi Pavel,
> 
> here is the next (much smaller) part of the puzzle.
> Please let me know if you'd rather have the review load distributed over other
> members of the sync team (or simply reassign the CL yourself). I put you as a
> reviewer because you were on the design meetings and know what is supposed to
> happen eventually :)
> 
> Cheers,
> Julian

ping? :)

[pavely, 2016-10-27 23:53:39]

lgtm

https://codereview.chromium.org/2442453002/diff/60001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2442453002/diff/60001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:528:
local_sync_backend_folder.Append(base_directory_.BaseName());
I guess base_directory_.BaseName() returns "Default" or "Profile 0" or etc. What
if the same user has their profile in different directories on different
computers?

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

https://codereview.chromium.org/2442453002/diff/60001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.cc:526:
CHECK(base::CreateDirectory(filename.DirName()));
Failure to create directory is the same as failure to write contents to file. In
both cases what's on disk doesn't match what the code thinks should be there. I
think you should treat them similarly, either both CHECK or both "return
false;".

https://codereview.chromium.org/2442453002/diff/60001/components/sync/engine_...
File components/sync/engine_impl/sync_manager_impl.cc (right):

https://codereview.chromium.org/2442453002/diff/60001/components/sync/engine_...
components/sync/engine_impl/sync_manager_impl.cc:14: #include
"base/command_line.h"
I don't think you use CommandLine in this file.

[pastarmovj, 2016-11-02 13:39:25]

https://codereview.chromium.org/2442453002/diff/60001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2442453002/diff/60001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:528:
local_sync_backend_folder.Append(base_directory_.BaseName());
On 2016/10/27 23:53:39, pavely wrote:
> I guess base_directory_.BaseName() returns "Default" or "Profile 0" or etc.
What
> if the same user has their profile in different directories on different
> computers?

Yes this is a question we are still debating internally - if only the Default
profile or all profiles should get this ability. Meanwhile I added a comment to
indicate this issue here in the code.

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

https://codereview.chromium.org/2442453002/diff/60001/components/sync/engine_...
components/sync/engine_impl/loopback_server/loopback_server.cc:526:
CHECK(base::CreateDirectory(filename.DirName()));
On 2016/10/27 23:53:39, pavely wrote:
> Failure to create directory is the same as failure to write contents to file.
In
> both cases what's on disk doesn't match what the code thinks should be there.
I
> think you should treat them similarly, either both CHECK or both "return
> false;".

Agree and it should not crash for this reason imho - rather log and continue
working in local mode.

https://codereview.chromium.org/2442453002/diff/60001/components/sync/engine_...
File components/sync/engine_impl/sync_manager_impl.cc (right):

https://codereview.chromium.org/2442453002/diff/60001/components/sync/engine_...
components/sync/engine_impl/sync_manager_impl.cc:14: #include
"base/command_line.h"
On 2016/10/27 23:53:39, pavely wrote:
> I don't think you use CommandLine in this file.

Done.

[pastarmovj, 2016-11-02 13:39:37]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-11-02 13:39:37]

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/2442453002/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-11-02 13:40:00]

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-11-02 14:12:25]

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

[commit-bot: I haz the power, 2016-11-02 14:12:25]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pastarmovj, 2016-11-02 14:21:18]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-11-02 14:21:18]

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/2442453002/#ps100001
(title: "Fix InitArgs contructor.")

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

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-11-02 14:58:12]

Description was changed from

==========
Adds a new switch for enabling the new local server backend.

Actually two flags which play in tandem:

--enable-local-sync-backend and --local-sync-backend-dir

The second is respected only if the first one is specified.
Eventually those will be controlled by policy but the flags will allow
easier testing and potentially unlock the feature for advanced users.

For now sign-in is needed to kick the sync logic on but the data
is only stored locally when this flag is specified.

BUG=651411
TEST=unit_tests/manual.
==========

to

==========
Adds a new switch for enabling the new local server backend.

Actually two flags which play in tandem:

--enable-local-sync-backend and --local-sync-backend-dir

The second is respected only if the first one is specified.
Eventually those will be controlled by policy but the flags will allow
easier testing and potentially unlock the feature for advanced users.

For now sign-in is needed to kick the sync logic on but the data
is only stored locally when this flag is specified.

BUG=651411
TEST=unit_tests/manual.
==========

[commit-bot: I haz the power, 2016-11-02 14:58:13]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-02 15:01:09]

Description was changed from

==========
Adds a new switch for enabling the new local server backend.

Actually two flags which play in tandem:

--enable-local-sync-backend and --local-sync-backend-dir

The second is respected only if the first one is specified.
Eventually those will be controlled by policy but the flags will allow
easier testing and potentially unlock the feature for advanced users.

For now sign-in is needed to kick the sync logic on but the data
is only stored locally when this flag is specified.

BUG=651411
TEST=unit_tests/manual.
==========

to

==========
Adds a new switch for enabling the new local server backend.

Actually two flags which play in tandem:

--enable-local-sync-backend and --local-sync-backend-dir

The second is respected only if the first one is specified.
Eventually those will be controlled by policy but the flags will allow
easier testing and potentially unlock the feature for advanced users.

For now sign-in is needed to kick the sync logic on but the data
is only stored locally when this flag is specified.

BUG=651411
TEST=unit_tests/manual.

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

[commit-bot: I haz the power, 2016-11-02 15:01:10]

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

[please use gerrit instead, 2016-11-02 15:45:00]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2470153002/ by rouslan@chromium.org.

The reason for reverting is: Speculative revert to fix "sizes" on "Linux x64"
bot.

Failed build:
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/27974

Error message:
FAILED linux-release-64/sizes/chrome-si/initializers: actual 8, expected 7,
better lower

Most likely due to using:

    const base::FilePath::StringType kLoopbackServerBackendFilename

instead of

    static const base::FilePath::CharType kLoopbackServerBackendFilename[]

as on line 153 in profile_sync_service.cc..

[pastarmovj, 2016-11-02 15:54:11]

On 2016/11/02 15:45:00, rouslan wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> <crxref style="background-image:
url("chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png");">https://codereview.chromium.org/2470153002/</crxref>
by mailto:rouslan@chromium.org.
> 
> The reason for reverting is: Speculative revert to fix "sizes" on "Linux x64"
> bot.
> 
> Failed build:
> https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/27974
> 
> Error message:
> FAILED linux-release-64/sizes/chrome-si/initializers: actual 8, expected 7,
> better lower
> 
> Most likely due to using:
> 
>     const base::FilePath::StringType kLoopbackServerBackendFilename
> 
> instead of
> 
>     static const base::FilePath::CharType kLoopbackServerBackendFilename[]
> 
> as on line 153 in profile_sync_service.cc..

Indeed. Fixed CL is underway but it would have been just as simple to commit the
fix instead of the revert...

[Avi (use Gerrit), 2016-11-02 17:02:15]

On 2016/11/02 15:54:11, pastarmovj wrote:
> On 2016/11/02 15:45:00, rouslan wrote:
> > A revert of this CL (patchset #6 id:100001) has been created in
> > <crxref style="background-image:
>
url("chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png");">https://codereview.chromium.org/2470153002/</crxref>
> by mailto:rouslan@chromium.org.
> > 
> > The reason for reverting is: Speculative revert to fix "sizes" on "Linux
x64"
> > bot.
> > 
> > Failed build:
> > https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/27974
> > 
> > Error message:
> > FAILED linux-release-64/sizes/chrome-si/initializers: actual 8, expected 7,
> > better lower
> > 
> > Most likely due to using:
> > 
> >     const base::FilePath::StringType kLoopbackServerBackendFilename
> > 
> > instead of
> > 
> >     static const base::FilePath::CharType kLoopbackServerBackendFilename[]
> > 
> > as on line 153 in profile_sync_service.cc..
> 
> Indeed. Fixed CL is underway but it would have been just as simple to commit
the
> fix instead of the revert...

It's nothing personal, but the rule is to revert first (see
https://www.chromium.org/developers/tree-sheriffs ).