Precache policy-for-extensions for device-local accounts.

Precache policy-for-extensions for device-local accounts.

This change makes the component cloud policy code start downloading
policies for extensions as soon as the device-local account is loaded.
That happens at the earliest during enrollment time.

TBR=satorux@chromium.org
BUG=224596
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278939

[Joao da Silva, 2014-06-20 15:57:24]

And that's the last one. PTAL, thanks!

[bartfab (slow), 2014-06-20 17:03:46]

lgtm

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc
(right):

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc:24: :
store_(store), schema_registry_(schema_registry) {
Style guide nit: Each initialization on its own line.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc:26: //
This is easy: just add a component for the app id.
Nit: s/just/Just/

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc:73:
LOG(ERROR)
Nit: Remove.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_extension_tracker.h
(right):

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.h:16: //
Helper class that keeps all the extensions that a device local account uses
Nit: s/device local/device-local/

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.h:25:
~DeviceLocalAccountExtensionTracker();
Nit: virtual.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.h:28:
virtual void OnStoreLoaded(CloudPolicyStore* store) OVERRIDE;
Nit: #include "base/compiler_specific.h"

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:167:
LOG(ERROR) << "--- connecting broker for " << user_id_;
Nit: Remove.

https://codereview.chromium.org/337053005/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/component_cloud_policy_service.cc
(right):

https://codereview.chromium.org/337053005/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/component_cloud_policy_service.cc:403:
LOG(ERROR) << "-- updating external policy for: " << ns.component_id;
Nit: Remove.

[Joao da Silva, 2014-06-20 22:28:20]

This needed some more work in the end.

One issue is that the ComponentCloudPolicyService expects the SchemaMap to have
valid Schemas, which is not true anymore (while caching before the session
starts). So there is a small time window when an extension gets loaded where we
may serve policies from the Backend that have not been filtered by the right
Schema yet; the changes in the CCPS handle that.

Another issue is the legacy code for LBS, which allows extensions without a
Schema to get any policies. This code will be removed eventually; this CL only
makes an exception for LBS explicitly now.

Some tests expected to work without a Schema and have been fixed.

Other tests expected a cloud policy refresh when an extension is installed and
have been fixed too.

On the bright side, the whole thing works quite well now without making any
spurious requests to the server :-)

I'll land over the weekend; please have a look when you can.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc
(right):

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc:24: :
store_(store), schema_registry_(schema_registry) {
On 2014/06/20 17:03:46, bartfab wrote:
> Style guide nit: Each initialization on its own line.

Done.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc:26: //
This is easy: just add a component for the app id.
On 2014/06/20 17:03:46, bartfab wrote:
> Nit: s/just/Just/

Done.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.cc:73:
LOG(ERROR)
On 2014/06/20 17:03:45, bartfab wrote:
> Nit: Remove.

Doh, done.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_extension_tracker.h
(right):

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.h:16: //
Helper class that keeps all the extensions that a device local account uses
On 2014/06/20 17:03:46, bartfab wrote:
> Nit: s/device local/device-local/

Done.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.h:25:
~DeviceLocalAccountExtensionTracker();
On 2014/06/20 17:03:46, bartfab wrote:
> Nit: virtual.

Done.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_extension_tracker.h:28:
virtual void OnStoreLoaded(CloudPolicyStore* store) OVERRIDE;
On 2014/06/20 17:03:46, bartfab wrote:
> Nit: #include "base/compiler_specific.h"

Done.

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/337053005/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:167:
LOG(ERROR) << "--- connecting broker for " << user_id_;
On 2014/06/20 17:03:46, bartfab wrote:
> Nit: Remove.

Done ._.

https://codereview.chromium.org/337053005/diff/1/components/policy/core/commo...
File components/policy/core/common/cloud/component_cloud_policy_service.cc
(right):

https://codereview.chromium.org/337053005/diff/1/components/policy/core/commo...
components/policy/core/common/cloud/component_cloud_policy_service.cc:403:
LOG(ERROR) << "-- updating external policy for: " << ns.component_id;
On 2014/06/20 17:03:46, bartfab wrote:
> Nit: Remove.

Done

https://codereview.chromium.org/337053005/diff/20001/chromeos/dbus/session_ma...
File chromeos/dbus/session_manager_client.cc (left):

https://codereview.chromium.org/337053005/diff/20001/chromeos/dbus/session_ma...
chromeos/dbus/session_manager_client.cc:586: }
This was introduced in https://chromiumcodereview.appspot.com/14761012 before
the more recent work to persist user policy across restarts. It is now
interfering with the new behavior on the desktop and should be removed.

[Joao da Silva, 2014-06-20 22:28:52]

+satorux for the stub session_manager_client.cc changes

[Joao da Silva, 2014-06-20 23:06:00]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-20 23:08:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/337053005/20001

[Joao da Silva, 2014-06-21 00:10:00]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-21 00:11:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/337053005/40001

[commit-bot: I haz the power, 2014-06-21 05:55:02]

Change committed as 278939

[bartfab (slow), 2014-06-23 09:53:38]

lgtm

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:148:
PolicyNamespace(POLICY_DOMAIN_CHROME, ""),
Nit: #include "components/policy/core/common/policy_namespace.h"

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:418:
const em::PolicyFetchRequest* public_account =
Nit: #include "policy/proto/device_management_backend.pb.h"

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:425:
const em::PolicyFetchRequest* tmp = public_account;
Nit: Use std::swap() from <algorithm>

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
File components/policy/core/common/forwarding_policy_provider_unittest.cc
(right):

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/forwarding_policy_provider_unittest.cc:51:
std::string error;
Nit: #include <string>

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
File components/policy/core/common/schema_registry.cc (right):

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/schema_registry.cc:208: // POLICY_DOMAIN_CHROME is
skipped to avoid spurious updated when a new
Nit: s/updated/updates/

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/schema_registry.cc:209: // Profile is created. If
the ForwardingSchemaRegistry is meant to be used
Nit: s/meant to be//

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
File components/policy/core/common/schema_registry_unittest.cc (right):

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/schema_registry_unittest.cc:290:
forwarding.SetReady(POLICY_DOMAIN_EXTENSIONS);
Why is this conditional for |registry| above but unconditional for |forwarding|
here?

[Joao da Silva, 2014-07-16 11:47:52]

Bartosz, I've addressed your late comments at
https://codereview.chromium.org/392233003. See replies inline here.

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_local_account_policy_service.cc
(right):

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:148:
PolicyNamespace(POLICY_DOMAIN_CHROME, ""),
On 2014/06/23 09:53:37, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_namespace.h"

Done.

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:418:
const em::PolicyFetchRequest* public_account =
On 2014/06/23 09:53:37, bartfab wrote:
> Nit: #include "policy/proto/device_management_backend.pb.h"

Done.

https://codereview.chromium.org/337053005/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:425:
const em::PolicyFetchRequest* tmp = public_account;
On 2014/06/23 09:53:37, bartfab wrote:
> Nit: Use std::swap() from <algorithm>

Done.

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
File components/policy/core/common/forwarding_policy_provider_unittest.cc
(right):

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/forwarding_policy_provider_unittest.cc:51:
std::string error;
On 2014/06/23 09:53:37, bartfab wrote:
> Nit: #include <string>

Done.

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
File components/policy/core/common/schema_registry.cc (right):

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/schema_registry.cc:208: // POLICY_DOMAIN_CHROME is
skipped to avoid spurious updated when a new
On 2014/06/23 09:53:38, bartfab wrote:
> Nit: s/updated/updates/

Done.

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/schema_registry.cc:209: // Profile is created. If
the ForwardingSchemaRegistry is meant to be used
On 2014/06/23 09:53:38, bartfab wrote:
> Nit: s/meant to be//

Done.

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
File components/policy/core/common/schema_registry_unittest.cc (right):

https://codereview.chromium.org/337053005/diff/40001/components/policy/core/c...
components/policy/core/common/schema_registry_unittest.cc:290:
forwarding.SetReady(POLICY_DOMAIN_EXTENSIONS);
Good catch, it's actually not needed above.

POLICY_DOMAIN_EXTENSIONS is always ready on platforms where ENABLE_EXTENSIONS is
not defined, so that it doesn't block the initialization of the registry.

So setting it as ready on those platforms again is effectively a nop, and the
#ifdef is just adding confusion. Removed.