wifi_sync: allow WifiCredentialSyncableServiceFactory to ignore LoginState

wifi_sync: allow WifiCredentialSyncableServiceFactory to ignore LoginState

WifiCredentialSyncableServiceFactory normally uses
chromeos::LoginState to associate a SyncableService with a Shill
profile. For this to work, LoginState must have been set properly,
before someone tries to use the Factory to create a SyncableService.

The precondition is met in production mode. However, in sync
integration tests, it is difficult to arrange for LoginState to be
configured properly. The problem is that the sync integration test
framework creates multiple SyncableServices, without providing the
test case the ability to change configuration between the construction
of the SyncableServices.

Acccomodate the way sync integration test work, by allowing the
WifiCredentialSyncableServiceFactory to ignore LoginState. When
LoginState is ignored, the factory will, instead, use the
BrowserContext to associate the new SyncableService with a Shill
profile. This works for sync integration tests, because each
SyncableService created by the test framework has a separate
BrowserContext.

BUG=chromium:431435
TEST=components_unittests --gtest_filter="Wifi*"

Committed: https://crrev.com/73bbc93c565dac4de1054a0664217989d1761979
Cr-Commit-Position: refs/heads/master@{#314981}

[mukesh agrawal, 2015-01-27 02:03:45]

This CL is needed by https://codereview.chromium.org/843483004/

[mukesh agrawal, 2015-01-27 02:19:18]

quiche@chromium.org changed reviewers:
+ erikwright@chromium.org, pavely@chromium.org, stevenjb@chromium.org

[mukesh agrawal, 2015-01-27 02:19:18]

quiche@chromium.org changed required reviewers:
+ erikwright@chromium.org, stevenjb@chromium.org

[stevenjb, 2015-01-28 01:04:58]

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:38: void
WifiCredentialSyncableServiceFactory::IgnoreLoginStateForTest() {
Rather than having a method to set this, would it be straightforward to have a
BuildWifiConfigDelegateForTest method instead?

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:64:
content::BrowserContext* context) const {
This extra layer just adds confusion IMHO, I would just fold this into the
method above.

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:86: std::string
WifiCredentialSyncableServiceFactory::GetShillProfile(
This doesn't return a Profile as the name suggests, but rather an identifier, we
should rename this to reflect that.

[mukesh agrawal, 2015-01-28 19:39:30]

oops. i folded the wrong methods together. will revise.

[mukesh agrawal, 2015-01-28 20:07:24]

Thanks, stevenjb. PTAL.

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:38: void
WifiCredentialSyncableServiceFactory::IgnoreLoginStateForTest() {
On 2015/01/28 01:04:58, stevenjb wrote:
> Rather than having a method to set this, would it be straightforward to have a
> BuildWifiConfigDelegateForTest method instead?

Unfortunately, we don't have control over construction of the
WifiCredentialSyncableService instances. They're constructed by the sync
integration test framework, using ProfileSyncComponentsFactoryImpl.

FWIW, here's my best guess at the call-stack (after collapsing callbacks as if
they completed synchronously):

SyncTest::SetupSync() {  // called from each wifi_sync integration test
  for (int i = 0; i < num_clients_; ++i) {
    ProfileSyncServiceHarness* client = GetClient(i);
    client->ProfileSyncServiceHarness::SetupSync() {
      ProfileSyncService::OnUserChoseDatatypes() {
        ProfileSyncService::ChangePreferredDataTypes() {
          ProfileSyncService::ReconfigureDataTypeManager() {
            ProfileSyncService::ConfigureDataTypeManager() {
              DataTypeManagerImpl::Configure() {
                DataTypeManagerImpl::ConfigureImpl() {
                  DataTypeManagerImpl::Restart() {
                    SyncBackendHostImpl::ConfigureDataTypes() {
                      SyncBackendHostImpl::RequestConfigureSyncer() {
                        SyncBackendHostCore::DoConfigureSyncer() {
                          SyncManagerImpl::ConfigureSyncer() {
                            SyncSchedulerImpl::ScheduleConfiguration() {
                              SyncBackendHostCore::DoFinishConfigureDataTypes()
{
                               
SyncBackendHostImpl::FinishConfigureDataTypesOnFrontendLoop() {
                                  DataTypeManagerImpl::DownloadReady() {
                                    DataTypeManagerImpl::StartNextAssociation()
{
                                     
ModelAssociationManager::StartAssociationAsync() {
                                        UIDataTypeController::StartAssociating()
{
                                          UIDataTypeController::Associate() {
                                            SharedChangeProcessor::Connect() {
                                             
ProfileSyncComponentsFactoryImpl::GetSyncableServiceForType() {
                                               
wifi_sync::WifiCredentialSyncableServiceFactory::
                                                    GetForBrowserContext();

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:64:
content::BrowserContext* context) const {
On 2015/01/28 01:04:58, stevenjb wrote:
> This extra layer just adds confusion IMHO, I would just fold this into the
> method above.

Done.

https://codereview.chromium.org/876833002/diff/1/components/wifi_sync/wifi_cr...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:86: std::string
WifiCredentialSyncableServiceFactory::GetShillProfile(
On 2015/01/28 01:04:57, stevenjb wrote:
> This doesn't return a Profile as the name suggests, but rather an identifier,
we
> should rename this to reflect that.

How's this? (I consider GetShillProfileId, but that might imply
"/profile/user-hash", instead of just "user-hash".)

[stevenjb, 2015-01-28 21:12:20]

OK, that's fine, mostly looks OK with a couple small changes.

https://codereview.chromium.org/876833002/diff/40001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/876833002/diff/40001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:64: std::string
GetUserHash(content::BrowserContext* context) const;
This could be file local if you pass it the 'use login state' member.

https://codereview.chromium.org/876833002/diff/40001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:69: bool
use_login_state_chrome_os_;
This should also be #if OS_CHROMEOS. Also, slight preference for something like
ignore_login_state_for_test_ so that the default is false.

[mukesh agrawal, 2015-01-28 22:26:27]

Thanks again, stevenjb. PTAL.

https://codereview.chromium.org/876833002/diff/40001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/876833002/diff/40001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:64: std::string
GetUserHash(content::BrowserContext* context) const;
On 2015/01/28 21:12:20, stevenjb wrote:
> This could be file local if you pass it the 'use login state' member.

Done.

https://codereview.chromium.org/876833002/diff/40001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:69: bool
use_login_state_chrome_os_;
On 2015/01/28 21:12:20, stevenjb wrote:
> This should also be #if OS_CHROMEOS. Also, slight preference for something
like
> ignore_login_state_for_test_ so that the default is false.

Done.

https://codereview.chromium.org/876833002/diff/60001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/876833002/diff/60001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:40: void
set_ignore_login_state_for_test(bool new_value) {
- After renaming the member variable, the previous method was
CamelCaseVariable(). Changed to a mutator, to conform to style.

- I've omitted a comment for this method, since the comment would simply restate
the (one-line) implementation.

https://codereview.chromium.org/876833002/diff/60001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:63: #if
defined(OS_CHROMEOS)
While this #if block could be collapsed into the previous one, I thought it
would be better to maintain a separation between methods and variables.

https://codereview.chromium.org/876833002/diff/60001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:68: bool
ignore_login_state_for_test_ = false;
Since this field is only present on ChromeOS, I'm using a non-static class
member initializer to set it. Alternatively, I can place an ifdef inside the
ctor body (setting this field outside the initializer list), or have the ctor be
platform-specific.

FWIW, non-static class member initializers are an approved C++11 feature, per
https://chromium-cpp.appspot.com/

[stevenjb, 2015-01-28 22:47:24]

lgtm

https://codereview.chromium.org/876833002/diff/80001/components/wifi_sync/wif...
File components/wifi_sync/wifi_credential_syncable_service_factory.h (right):

https://codereview.chromium.org/876833002/diff/80001/components/wifi_sync/wif...
components/wifi_sync/wifi_credential_syncable_service_factory.h:66: bool
ignore_login_state_for_test_ = false;
nice.

[erikwright (departed), 2015-01-29 16:01:35]

Where are the syncable services actually instantiated?

https://codereview.chromium.org/876833002/diff/80001/components/wifi_sync/DEPS
File components/wifi_sync/DEPS (right):

https://codereview.chromium.org/876833002/diff/80001/components/wifi_sync/DEP...
components/wifi_sync/DEPS:8: "+content/public/browser/browser_context.h",
Don't go to the file-level. content/public/browser.

[mukesh agrawal, 2015-01-29 18:56:45]

On 2015/01/29 16:01:35, erikwright wrote:
> Where are the syncable services actually instantiated?

In normal operation, and in the sync integration tests, SyncableSerices are
instantiated from ProfileSyncComponentsFactoryImpl. (See below for my
understanding of the call-stack in the integration test case. Note that this
call-stack was constructed via code inspection. The stack includes callbacks, as
if they completed synchronously.)

In wifi_credential_syncable_service_unittest.cc, the
WifiCredentialSyncableServices are instantiated directly (using operator new).

SyncTest::SetupSync() {  // called from each wifi_sync integration test
  for (int i = 0; i < num_clients_; ++i) {
    ProfileSyncServiceHarness* client = GetClient(i);
    client->ProfileSyncServiceHarness::SetupSync() {
      ProfileSyncService::OnUserChoseDatatypes() {
        ProfileSyncService::ChangePreferredDataTypes() {
          ProfileSyncService::ReconfigureDataTypeManager() {
            ProfileSyncService::ConfigureDataTypeManager() {
              DataTypeManagerImpl::Configure() {
                DataTypeManagerImpl::ConfigureImpl() {
                  DataTypeManagerImpl::Restart() {
                    SyncBackendHostImpl::ConfigureDataTypes() {
                      SyncBackendHostImpl::RequestConfigureSyncer() {
                        SyncBackendHostCore::DoConfigureSyncer() {
                          SyncManagerImpl::ConfigureSyncer() {
                            SyncSchedulerImpl::ScheduleConfiguration() {
                              SyncBackendHostCore::DoFinishConfigureDataTypes()
{
                               
SyncBackendHostImpl::FinishConfigureDataTypesOnFrontendLoop() {
                                  DataTypeManagerImpl::DownloadReady() {
                                    DataTypeManagerImpl::StartNextAssociation()
{
                                     
ModelAssociationManager::StartAssociationAsync() {
                                        UIDataTypeController::StartAssociating()
{
                                          UIDataTypeController::Associate() {
                                            SharedChangeProcessor::Connect() {
                                             
ProfileSyncComponentsFactoryImpl::GetSyncableServiceForType() {
                                               
wifi_sync::WifiCredentialSyncableServiceFactory::
                                                    GetForBrowserContext();

[mukesh agrawal, 2015-01-29 18:57:03]

erikwright: Thanks and PTAL.

https://codereview.chromium.org/876833002/diff/80001/components/wifi_sync/DEPS
File components/wifi_sync/DEPS (right):

https://codereview.chromium.org/876833002/diff/80001/components/wifi_sync/DEP...
components/wifi_sync/DEPS:8: "+content/public/browser/browser_context.h",
On 2015/01/29 16:01:35, erikwright wrote:
> Don't go to the file-level. content/public/browser.

Done.

[erikwright (departed), 2015-01-29 19:02:31]

I think it would be better if the dependencies of the code under test could be
properly satisfied in the test environment.

If you have already reached out to experts on sync and sync integration tests,
and they have agreed that there is no practical and clean way to resolve this,
LGTM.

[mukesh agrawal, 2015-02-04 19:46:48]

On 2015/01/29 19:02:31, erikwright wrote:
> I think it would be better if the dependencies of the code under test could be
> properly satisfied in the test environment.
> 
> If you have already reached out to experts on sync and sync integration tests,
> and they have agreed that there is no practical and clean way to resolve this,
> LGTM.

Agreed that it would be better to satisfy the dependencies in another way. I
checked with the sync team, though, and there's no better way today. So
committing as-is.

[mukesh agrawal, 2015-02-04 19:46:53]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2015-02-04 19:48:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/876833002/100001

[commit-bot: I haz the power, 2015-02-04 19:54:41]

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

[commit-bot: I haz the power, 2015-02-04 19:54:43]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mukesh agrawal, 2015-02-04 22:55:41]

quiche@chromium.org changed reviewers:
+ creis@chromium.org

[mukesh agrawal, 2015-02-04 22:55:41]

quiche@chromium.org changed required reviewers:
+ creis@chromium.org

[mukesh agrawal, 2015-02-04 22:57:32]

creis@, can you review/approve the DEPS change? (I'm adding a dependency from my
new component, on content/public/browser, so that my component can access
BrowserContext.)

[Charlie Reis, 2015-02-05 01:07:57]

Happy to rubber stamp, but I don't understand why this needs the
BrowserContext's path, or why it's safe (e.g., from a privacy perspective).

https://codereview.chromium.org/876833002/diff/100001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/876833002/diff/100001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:37: return
context->GetPath().BaseName().value();
It's not clear to me why this is a good hash, or what it will be used for.  Can
you add some comments explaining why it's ok to use the BrowserContext's file
path for this?

[mukesh agrawal, 2015-02-06 02:40:32]

On 2015/02/05 01:07:57, Charlie Reis wrote:
> Happy to rubber stamp, but I don't understand why this needs the
> BrowserContext's path, or why it's safe (e.g., from a privacy perspective).
> 
>
https://codereview.chromium.org/876833002/diff/100001/components/wifi_sync/wi...
> File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):
> 
>
https://codereview.chromium.org/876833002/diff/100001/components/wifi_sync/wi...
> components/wifi_sync/wifi_credential_syncable_service_factory.cc:37: return
> context->GetPath().BaseName().value();
> It's not clear to me why this is a good hash, or what it will be used for. 
Can
> you add some comments explaining why it's ok to use the BrowserContext's file
> path for this?

creis@: Thanks and PTAL.

[mukesh agrawal, 2015-02-06 02:40:55]

https://codereview.chromium.org/876833002/diff/100001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/876833002/diff/100001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:37: return
context->GetPath().BaseName().value();
On 2015/02/05 01:07:57, Charlie Reis wrote:
> It's not clear to me why this is a good hash, or what it will be used for. 
Can
> you add some comments explaining why it's ok to use the BrowserContext's file
> path for this?

Done (I think).

I couldn't find a documentation of appropriate uses of GetPath in the
BrowserContext header, but I've tried to express a reasonable constraint on the
use here. Let me know if I missed.

https://codereview.chromium.org/876833002/diff/120001/components/wifi_sync/wi...
File components/wifi_sync/wifi_credential_syncable_service_factory.cc (right):

https://codereview.chromium.org/876833002/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:31: // exposed
to any untrusted code (e.g., via web APIs).
I believe this (newly added) comment captures how the hash will be used, and
also circumscribes inappropriate uses in the future.

https://codereview.chromium.org/876833002/diff/120001/components/wifi_sync/wi...
components/wifi_sync/wifi_credential_syncable_service_factory.cc:44: // paths.
This (newly added) comment should explain why this a "good" hash. (It's good in
the sense that it's a value that the sync tests and this factory can share.)

WRT why it's okay, I assume you mean okay from a privacy perspective. For that,
see the comment at line 28.

[Charlie Reis, 2015-02-06 04:47:53]

Thanks for clarifying.  Seems that it's just for tests, so that's fine.  DEPS
LGTM.

[mukesh agrawal, 2015-02-06 04:53:39]

The CQ bit was checked by quiche@chromium.org

[commit-bot: I haz the power, 2015-02-06 04:54:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/876833002/120001

[commit-bot: I haz the power, 2015-02-06 05:33:09]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-06 05:34:53]

Patchset 7 (id:??) landed as
https://crrev.com/73bbc93c565dac4de1054a0664217989d1761979
Cr-Commit-Position: refs/heads/master@{#314981}