Remove Profile dependency from TiclInvalidationService

Remove Profile dependency from TiclInvalidationService

This CL introduces a TicSettingsProvider abstraction that allows
TiclInvalidationService to retrieve configuration settings without having
to directly observe a Profile's PrefStore.

The CL completes the work on removing the Profile dependency from
TiclInvalidationService.

BUG=358696
TEST=New and updated unit tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266853

[bartfab (slow), 2014-04-24 15:14:55]

Hi Daniel,

Could you take a look? This CL completes the work on removing the Profile
dependency.

[dcheng, 2014-04-24 18:26:54]

LGTM with nits.

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File chrome/browser/invalidation/invalidation_service_factory.cc (right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/invalidation_service_factory.cc:63: if
(profile->IsManaged()) {
Nit: Not sure why this is needed in this CL. Is this a bug that we overlooked
previously? If it's unrelated, I'd prefer to see it moved to a separate patch
(I'm happy to LGTM+CQ that).

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h
(right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h:30:
Profile* profile_;
Nit: Profile* const.

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File chrome/browser/invalidation/ticl_invalidation_service_unittest.cc (right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/ticl_invalidation_service_unittest.cc:55:
FakeTiclInvalidationServiceSettingsProvider::
These class names are really long!
Technically, Ticl and Invalidation are redundant--ticl stands for Tango
Invalidation Client Library. Might be worth shortening these names in the
future.

[bartfab (slow), 2014-04-24 19:14:48]

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File chrome/browser/invalidation/invalidation_service_factory.cc (right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/invalidation_service_factory.cc:63: if
(profile->IsManaged()) {
On 2014/04/24 18:26:54, dcheng wrote:
> Nit: Not sure why this is needed in this CL. Is this a bug that we overlooked
> previously? If it's unrelated, I'd prefer to see it moved to a separate patch
> (I'm happy to LGTM+CQ that).

This was previously handled by TiclInvalidationService::IsReadyToStart()
returning false for managed profiles, which led to yet another Profile
dependency. I moved this change to a separate CL now.

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h
(right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h:30:
Profile* profile_;
On 2014/04/24 18:26:54, dcheng wrote:
> Nit: Profile* const.

This gets fed to gcm::GCMProfileService::GetGCMEnabledState(), which requires a
non-const pointer.

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File chrome/browser/invalidation/ticl_invalidation_service_unittest.cc (right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/ticl_invalidation_service_unittest.cc:55:
FakeTiclInvalidationServiceSettingsProvider::
On 2014/04/24 18:26:54, dcheng wrote:
> These class names are really long!
> Technically, Ticl and Invalidation are redundant--ticl stands for Tango
> Invalidation Client Library. Might be worth shortening these names in the
> future.

Done.

[dcheng, 2014-04-24 21:02:18]

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h
(right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h:30:
Profile* profile_;
On 2014/04/24 19:14:48, bartfab wrote:
> On 2014/04/24 18:26:54, dcheng wrote:
> > Nit: Profile* const.
> 
> This gets fed to gcm::GCMProfileService::GetGCMEnabledState(), which requires
a
> non-const pointer.

T* const, not const T*. The former simply means that the pointer value is
immutable for the lifetime of this class, while the latter indicates that you
can't call non-const methods on it. A T* const can still be passed to a function
that takes a T*.

[bartfab (slow), 2014-04-25 08:51:27]

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
File
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h
(right):

https://codereview.chromium.org/255443005/diff/1/chrome/browser/invalidation/...
chrome/browser/invalidation/ticl_invalidation_service_profile_settings_provider.h:30:
Profile* profile_;
On 2014/04/24 21:02:18, dcheng wrote:
> On 2014/04/24 19:14:48, bartfab wrote:
> > On 2014/04/24 18:26:54, dcheng wrote:
> > > Nit: Profile* const.
> > 
> > This gets fed to gcm::GCMProfileService::GetGCMEnabledState(), which
requires
> a
> > non-const pointer.
> 
> T* const, not const T*. The former simply means that the pointer value is
> immutable for the lifetime of this class, while the latter indicates that you
> can't call non-const methods on it. A T* const can still be passed to a
function
> that takes a T*.

Done.

[bartfab (slow), 2014-04-29 08:03:14]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-29 08:03:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/255443005/30001

[commit-bot: I haz the power, 2014-04-29 08:13:29]

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

[commit-bot: I haz the power, 2014-04-29 08:13:29]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[bartfab (slow), 2014-04-29 08:21:24]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-29 08:22:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/255443005/50001

[commit-bot: I haz the power, 2014-04-29 09:49:33]

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

[commit-bot: I haz the power, 2014-04-29 09:49:33]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[bartfab (slow), 2014-04-29 10:01:03]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-29 10:01:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/255443005/50001

[commit-bot: I haz the power, 2014-04-29 11:52:49]

Change committed as 266853