[Sync] Inject startup dependencies into StartupController.

[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was no longer recorded as anything but true.

BUG=600754
Committed: https://crrev.com/01eeca4d034210d0aa851531257e15208dda7a67
Cr-Commit-Position: refs/heads/master@{#385202}

[maxbogue, 2016-04-04 18:11:58]

Description was changed from

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.
==========

to

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was never recorded as anything but true.
==========

[maxbogue, 2016-04-04 18:22:58]

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

[maxbogue, 2016-04-04 18:22:59]

Nicolas, PTAL. This is a cleanup CL I had left over from my StartupController
simplification work.

+Alexei for histograms.xml; are we even allowed to remove histograms?

[Alexei Svitkine (slow), 2016-04-04 19:10:06]

https://codereview.chromium.org/1858673002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1858673002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:52212: -<histogram
name="Sync.RefreshTokenAvailable" enum="BooleanSuccess">
Mark this as <obsolete> instead - since there's still data logged for this from
old versions.

[Nicolas Zea, 2016-04-04 20:44:17]

LGTM

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
File components/sync_driver/startup_controller.cc (right):

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
components/sync_driver/startup_controller.cc:117: if (!can_start_.Run()) {
consistency nit: remove braces

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
File components/sync_driver/startup_controller.h (right):

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
components/sync_driver/startup_controller.h:88: base::Callback<bool()>
can_start_;
comment what this is for

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
File components/sync_driver/startup_controller_unittest.cc (right):

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
components/sync_driver/startup_controller_unittest.cc:46: controller_.reset();
is it necessary to have these here? These will automatically get destroyed when
after the test runs anyways right?

[maxbogue, 2016-04-04 22:56:55]

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
File components/sync_driver/startup_controller.cc (right):

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
components/sync_driver/startup_controller.cc:117: if (!can_start_.Run()) {
On 2016/04/04 20:44:17, Nicolas Zea wrote:
> consistency nit: remove braces

Done.

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
File components/sync_driver/startup_controller.h (right):

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
components/sync_driver/startup_controller.h:88: base::Callback<bool()>
can_start_;
On 2016/04/04 20:44:17, Nicolas Zea wrote:
> comment what this is for

Done.

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
File components/sync_driver/startup_controller_unittest.cc (right):

https://codereview.chromium.org/1858673002/diff/1/components/sync_driver/star...
components/sync_driver/startup_controller_unittest.cc:46: controller_.reset();
On 2016/04/04 20:44:17, Nicolas Zea wrote:
> is it necessary to have these here? These will automatically get destroyed
when
> after the test runs anyways right?

Good catch! They must have been explicitly called before to ensure some ordering
wrt the token service Shutdown() call. Shouldn't be needed now; I've removed the
whole method and made |controller_| the last member variable so it still gets
destroyed first.

https://codereview.chromium.org/1858673002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1858673002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:52212: -<histogram
name="Sync.RefreshTokenAvailable" enum="BooleanSuccess">
On 2016/04/04 19:10:06, Alexei Svitkine wrote:
> Mark this as <obsolete> instead - since there's still data logged for this
from
> old versions.

Done.

[Alexei Svitkine (slow), 2016-04-05 15:00:36]

lgtm for histograms.xml

Probably worth having a BUG= for this since the change doesn't look trivial.

[maxbogue, 2016-04-05 17:08:02]

Description was changed from

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was never recorded as anything but true.
==========

to

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was no longer recorded as anything but true.

BUG=600754
==========

[maxbogue, 2016-04-05 17:08:14]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-04-05 17:08:15]

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

[commit-bot: I haz the power, 2016-04-05 17:08:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1858673002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1858673002/20001

[commit-bot: I haz the power, 2016-04-05 17:14:00]

Description was changed from

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was no longer recorded as anything but true.

BUG=600754
==========

to

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was no longer recorded as anything but true.

BUG=600754
==========

[commit-bot: I haz the power, 2016-04-05 17:14:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-05 17:15:23]

Description was changed from

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was no longer recorded as anything but true.

BUG=600754
==========

to

==========
[Sync] Inject startup dependencies into StartupController.

This forces StartupController to adhere to CanSyncStart instead of them
coincidentally sharing conditions. It also removes external dependencies
from StartupController and makes it cleaner to test.

This change also removes the Sync.RefreshTokenAvailable histogram,
which was no longer recorded as anything but true.

BUG=600754

Committed: https://crrev.com/01eeca4d034210d0aa851531257e15208dda7a67
Cr-Commit-Position: refs/heads/master@{#385202}
==========

[commit-bot: I haz the power, 2016-04-05 17:15:24]

Patchset 2 (id:??) landed as
https://crrev.com/01eeca4d034210d0aa851531257e15208dda7a67
Cr-Commit-Position: refs/heads/master@{#385202}