[sync] Clean up path generation for the local sync database.

[sync] Clean up path generation for the local sync database.

A lot of the logic was built into Sync although it was inherent to Chrome
to pick and build the location for its profiles. With this CL the logic is
offloaded to the PathService and injected into Sync.

Also this CL cleans up a duplication of the logic for picking the path
between the SyncPrefs and SyncServiceBase classes.

BUG=694464
TEST=Manual verify the right location is used.

Review-Url: https://codereview.chromium.org/2710623003
Cr-Commit-Position: refs/heads/master@{#453932}
Committed: https://chromium.googlesource.com/chromium/src/+/8b8b5e3692189301795e9fb21fec7f371f19b7f0

[pastarmovj, 2017-02-21 14:58:30]

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

[commit-bot: I haz the power, 2017-02-21 14:59:02]

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

[pastarmovj, 2017-02-21 15:07:01]

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

[pastarmovj, 2017-02-21 15:07:01]

Hi Nicolas,

as mentioned in the code review request for
https://codereview.chromium.org/2709693002/ this is the proper solution which I
intend to land right after the stop-gap (which is the merge friendly fix for the
immediate issue on the bug listed.

Thanks,
Julian

[commit-bot: I haz the power, 2017-02-21 16:25:25]

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

[commit-bot: I haz the power, 2017-02-21 16:25:26]

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

[Nicolas Zea, 2017-02-21 23:58:40]

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_factory.cc:178: SYSLOG(WARNING) <<
"Local sync can not get the roaming profile folder.";
Out of curiosity why SYSLOG instead of LOG?

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_factory.cc:179:
UMA_HISTOGRAM_BOOLEAN("Sync.Local.RoamingProfileUnavailable", false);
It seems like this should probably be part of a larger histogram that just
captures roaming profile startup result, with this specific scenario being one
of several possible startup errors recorded.

As it is, it's not clear to me how useful this count would be with no point of
reference for the success.

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_factory.cc:180:
local_sync_backend_enabled = false;
Should we just return a nullptr here?

https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
File components/sync/driver/sync_service_base.h (right):

https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
components/sync/driver/sync_service_base.h:43: const base::FilePath&
local_sync_directory,
I wonder if this would be better plumbed in via the SyncClient, rather than as a
construction param. SyncClient is useful for abstracting away dependencies on
other areas of the code, which seems to fit the local sync logic.

[pastarmovj, 2017-02-22 14:23:14]

Thanks for the great suggestions! I think it became way more cleaner now. :)

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_factory.cc:178: SYSLOG(WARNING) <<
"Local sync can not get the roaming profile folder.";
On 2017/02/21 23:58:40, Nicolas Zea wrote:
> Out of curiosity why SYSLOG instead of LOG?

I think this is an actionable error and the admin needs to know about it. The
reason I decided to surface it because initially I had a CHECK around the Get
and it started crashing in the wild (see
https://bugs.chromium.org/p/chromium/issues/detail?id=687882 ). It turned out
that on some (virus infested) machines trying to get the roaming user directory
can fail. I decided that this is not a reason to crash Chrome but only to
disable local sync. However it is important info that can help the admin
diagnose and fix the issue. And they don't have to run with --enable-logging
which might not be possible on a user machine.

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_factory.cc:179:
UMA_HISTOGRAM_BOOLEAN("Sync.Local.RoamingProfileUnavailable", false);
On 2017/02/21 23:58:40, Nicolas Zea wrote:
> It seems like this should probably be part of a larger histogram that just
> captures roaming profile startup result, with this specific scenario being one
> of several possible startup errors recorded.
> 
> As it is, it's not clear to me how useful this count would be with no point of
> reference for the success.

Sorry this one got broken when I was moving the code around. Of course it was
meant to measure success vs. failure.

https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_factory.cc:180:
local_sync_backend_enabled = false;
On 2017/02/21 23:58:40, Nicolas Zea wrote:
> Should we just return a nullptr here?

Great idea! I forgot a service doesn't have to be created.

https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
File components/sync/driver/sync_service_base.h (right):

https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
components/sync/driver/sync_service_base.h:43: const base::FilePath&
local_sync_directory,
On 2017/02/21 23:58:40, Nicolas Zea wrote:
> I wonder if this would be better plumbed in via the SyncClient, rather than as
a
> construction param. SyncClient is useful for abstracting away dependencies on
> other areas of the code, which seems to fit the local sync logic.

Another great idea! :) 

I think this clean up some more the code altogether.

[pastarmovj, 2017-02-22 14:37:00]

On 2017/02/22 14:23:14, pastarmovj wrote:
> Thanks for the great suggestions! I think it became way more cleaner now. :)
> 
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> File chrome/browser/sync/profile_sync_service_factory.cc (right):
> 
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_factory.cc:178: SYSLOG(WARNING) <<
> "Local sync can not get the roaming profile folder.";
> On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > Out of curiosity why SYSLOG instead of LOG?
> 
> I think this is an actionable error and the admin needs to know about it. The
> reason I decided to surface it because initially I had a CHECK around the Get
> and it started crashing in the wild (see
> https://bugs.chromium.org/p/chromium/issues/detail?id=687882 ). It turned out
> that on some (virus infested) machines trying to get the roaming user
directory
> can fail. I decided that this is not a reason to crash Chrome but only to
> disable local sync. However it is important info that can help the admin
> diagnose and fix the issue. And they don't have to run with --enable-logging
> which might not be possible on a user machine.
> 
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_factory.cc:179:
> UMA_HISTOGRAM_BOOLEAN("Sync.Local.RoamingProfileUnavailable", false);
> On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > It seems like this should probably be part of a larger histogram that just
> > captures roaming profile startup result, with this specific scenario being
one
> > of several possible startup errors recorded.
> > 
> > As it is, it's not clear to me how useful this count would be with no point
of
> > reference for the success.
> 
> Sorry this one got broken when I was moving the code around. Of course it was
> meant to measure success vs. failure.
> 
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> chrome/browser/sync/profile_sync_service_factory.cc:180:
> local_sync_backend_enabled = false;
> On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > Should we just return a nullptr here?
> 
> Great idea! I forgot a service doesn't have to be created.
> 
>
https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
> File components/sync/driver/sync_service_base.h (right):
> 
>
https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
> components/sync/driver/sync_service_base.h:43: const base::FilePath&
> local_sync_directory,
> On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > I wonder if this would be better plumbed in via the SyncClient, rather than
as
> a
> > construction param. SyncClient is useful for abstracting away dependencies
on
> > other areas of the code, which seems to fit the local sync logic.
> 
> Another great idea! :) 
> 
> I think this clean up some more the code altogether.

Working on this CL also helped me grasp all the refactoring changes that were
happening in parallel to my initial work on local sync. :)

[pastarmovj, 2017-02-27 09:55:49]

On 2017/02/22 14:37:00, pastarmovj wrote:
> On 2017/02/22 14:23:14, pastarmovj wrote:
> > Thanks for the great suggestions! I think it became way more cleaner now. :)
> > 
> >
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> > File chrome/browser/sync/profile_sync_service_factory.cc (right):
> > 
> >
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> > chrome/browser/sync/profile_sync_service_factory.cc:178: SYSLOG(WARNING) <<
> > "Local sync can not get the roaming profile folder.";
> > On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > > Out of curiosity why SYSLOG instead of LOG?
> > 
> > I think this is an actionable error and the admin needs to know about it.
The
> > reason I decided to surface it because initially I had a CHECK around the
Get
> > and it started crashing in the wild (see
> > https://bugs.chromium.org/p/chromium/issues/detail?id=687882 ). It turned
out
> > that on some (virus infested) machines trying to get the roaming user
> directory
> > can fail. I decided that this is not a reason to crash Chrome but only to
> > disable local sync. However it is important info that can help the admin
> > diagnose and fix the issue. And they don't have to run with --enable-logging
> > which might not be possible on a user machine.
> > 
> >
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> > chrome/browser/sync/profile_sync_service_factory.cc:179:
> > UMA_HISTOGRAM_BOOLEAN("Sync.Local.RoamingProfileUnavailable", false);
> > On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > > It seems like this should probably be part of a larger histogram that just
> > > captures roaming profile startup result, with this specific scenario being
> one
> > > of several possible startup errors recorded.
> > > 
> > > As it is, it's not clear to me how useful this count would be with no
point
> of
> > > reference for the success.
> > 
> > Sorry this one got broken when I was moving the code around. Of course it
was
> > meant to measure success vs. failure.
> > 
> >
>
https://codereview.chromium.org/2710623003/diff/1/chrome/browser/sync/profile...
> > chrome/browser/sync/profile_sync_service_factory.cc:180:
> > local_sync_backend_enabled = false;
> > On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > > Should we just return a nullptr here?
> > 
> > Great idea! I forgot a service doesn't have to be created.
> > 
> >
>
https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
> > File components/sync/driver/sync_service_base.h (right):
> > 
> >
>
https://codereview.chromium.org/2710623003/diff/1/components/sync/driver/sync...
> > components/sync/driver/sync_service_base.h:43: const base::FilePath&
> > local_sync_directory,
> > On 2017/02/21 23:58:40, Nicolas Zea wrote:
> > > I wonder if this would be better plumbed in via the SyncClient, rather
than
> as
> > a
> > > construction param. SyncClient is useful for abstracting away dependencies
> on
> > > other areas of the code, which seems to fit the local sync logic.
> > 
> > Another great idea! :) 
> > 
> > I think this clean up some more the code altogether.
> 
> Working on this CL also helped me grasp all the refactoring changes that were
> happening in parallel to my initial work on local sync. :)

Gentle ping? :)

[Nicolas Zea, 2017-02-27 19:21:26]

LGTM, thanks for cleaning that up!

https://codereview.chromium.org/2710623003/diff/20001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2710623003/diff/20001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:265: // one should be considered
roamed.
nit: This paragraph is kind of repeating the same statement twice. Maybe
rephrase to be something like: "TODO(pastarmovj): crbug.com/674928 Decide if
XXX. For now the code assumes all profiles are created in the same order on all
machines."

https://codereview.chromium.org/2710623003/diff/20001/components/sync/driver/...
File components/sync/driver/sync_client.h (right):

https://codereview.chromium.org/2710623003/diff/20001/components/sync/driver/...
components/sync/driver/sync_client.h:71: // It is only used when sync is running
against its local backend.
nit: "its local backend" -> "a local backend"

[pastarmovj, 2017-02-28 17:22:34]

https://codereview.chromium.org/2710623003/diff/20001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2710623003/diff/20001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:265: // one should be considered
roamed.
On 2017/02/27 19:21:26, Nicolas Zea wrote:
> nit: This paragraph is kind of repeating the same statement twice. Maybe
> rephrase to be something like: "TODO(pastarmovj): crbug.com/674928 Decide if
> XXX. For now the code assumes all profiles are created in the same order on
all
> machines."

Done.

https://codereview.chromium.org/2710623003/diff/20001/components/sync/driver/...
File components/sync/driver/sync_client.h (right):

https://codereview.chromium.org/2710623003/diff/20001/components/sync/driver/...
components/sync/driver/sync_client.h:71: // It is only used when sync is running
against its local backend.
On 2017/02/27 19:21:26, Nicolas Zea wrote:
> nit: "its local backend" -> "a local backend"

Done.

[pastarmovj, 2017-02-28 17:22:40]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2017-02-28 17:22:40]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2710623003/#ps40001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-02-28 17:23:18]

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, 2017-02-28 17:33:01]

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

[commit-bot: I haz the power, 2017-02-28 17:33:02]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[pastarmovj, 2017-03-01 13:50:36]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2017-03-01 13:50:37]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2710623003/#ps60001
(title: "Fix non-windows bots.")

[commit-bot: I haz the power, 2017-03-01 13:50:47]

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, 2017-03-01 14:45:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488376236663740,
"parent_rev": "e8f5e97deacc862b85406302b05c8351f2b38468", "commit_rev":
"8b8b5e3692189301795e9fb21fec7f371f19b7f0"}

[commit-bot: I haz the power, 2017-03-01 14:46:12]

Description was changed from

==========
[sync] Clean up path generation for the local sync database.

A lot of the logic was built into Sync although it was inherent to Chrome
to pick and build the location for its profiles. With this CL the logic is
offloaded to the PathService and injected into Sync.

Also this CL cleans up a duplication of the logic for picking the path
between the SyncPrefs and SyncServiceBase classes.

BUG=694464
TEST=Manual verify the right location is used.
==========

to

==========
[sync] Clean up path generation for the local sync database.

A lot of the logic was built into Sync although it was inherent to Chrome
to pick and build the location for its profiles. With this CL the logic is
offloaded to the PathService and injected into Sync.

Also this CL cleans up a duplication of the logic for picking the path
between the SyncPrefs and SyncServiceBase classes.

BUG=694464
TEST=Manual verify the right location is used.

Review-Url: https://codereview.chromium.org/2710623003
Cr-Commit-Position: refs/heads/master@{#453932}
Committed:
https://chromium.googlesource.com/chromium/src/+/8b8b5e3692189301795e9fb21fec...
==========

[commit-bot: I haz the power, 2017-03-01 14:46:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8b8b5e3692189301795e9fb21fec...