Cache force-installed apps/extensions in device-local accounts

Cache force-installed apps/extensions in device-local accounts

This CL adds the DeviceLocalAccountExternalPolicyLoader, a replacement for
the ExternalPolicyLoader that caches force-installed apps/extensions,
allowing them to be installed offline. The caches for individual accounts
are managed by the DeviceLocalAccountPolicyService which makes sure to
remove obsolete cache directories.

BUG=287802
TEST=Full coverage with new browser and unit tests
NOTRY=true

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

[bartfab (slow), 2013-10-16 20:01:22]

Hi Matt,

Please review c/b/chromeos/extensions/* and c/b/extensions/*.

Hi Joao,

Please review c/b/chromeos/policy/* and c/b/policy/*

Hi Daniel,

Please review chromeos/chromeos_paths.*

[Daniel Erat, 2013-10-16 20:07:28]

LGTM for chromeos/chromeos_paths.*

[Matt Perry, 2013-10-16 21:19:29]

I'm not very familiar with the ExternalPolicyLoader code, but this seems OK at a
high level.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/external_provider_impl.cc:358: if (user &&
policy::IsDeviceLocalAccountUser(user->email(), NULL)) {
I don't know what a device-local account is, but are you sure you want to
replace the ExternalPolicyLoader rather than adding them both to the
provider_list?

[bartfab (slow), 2013-10-17 09:16:36]

On 2013/10/16 21:19:29, Matt Perry wrote:
> I'm not very familiar with the ExternalPolicyLoader code, but this seems OK at
a
> high level.

I will need an lgtm from an owner. If you are not very familiar with the code,
could you recommend another reviewer?

> 
>
https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
> File chrome/browser/extensions/external_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
> chrome/browser/extensions/external_provider_impl.cc:358: if (user &&
> policy::IsDeviceLocalAccountUser(user->email(), NULL)) {
> I don't know what a device-local account is, but are you sure you want to
> replace the ExternalPolicyLoader rather than adding them both to the
> provider_list?

[bartfab (slow), 2013-10-17 09:16:45]

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/external_provider_impl.cc:358: if (user &&
policy::IsDeviceLocalAccountUser(user->email(), NULL)) {
On 2013/10/16 21:19:29, Matt Perry wrote:
> I don't know what a device-local account is, but are you sure you want to
> replace the ExternalPolicyLoader rather than adding them both to the
> provider_list?

A device-local account is the mechanism shared by all Chrome OS account types
that do not have a GAIA login. Public sessions, kiosk-app mode and retail mode
are three examples of this. And yes, I want to replace the ExternalPolicyLoader.
The regular ExternalPolicyLoader parses the list of force-installed extensions
and provides this list to Chrome, instructing it to download the extensions on
the fly. The DeviceLocalAccountExternalPolicyLoader parses the exact same list
of force-installed extensions (hence, the two are mutually exclusive) but caches
the extensions locally before providing a list to Chrome.

[Joao da Silva, 2013-10-17 14:57:54]

First pass at this monster CL. I think the overall structure is sound and this
is mostly good to go.

Nice test coverage btw.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:22:
char kExtensionInstallForcelist[] = "forcelist";
const char

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:24:
}
// namespace

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:52:
false));
Suggestion: just by looking at this it's not clear what true and false mean in
this context. How about:

const bool check_for_updates = true;
const bool wait_for_init_flag_file = false;

and using that as the arguments?

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:99:
policy_handler(kExtensionInstallForcelist);
Daniel has recently refactored this class and the pref argument isn't needed
anymore

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:104:
const base::DictionaryValue* dict = NULL;  // Not owned.
Same comment re // not owned comment

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.h
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.h:68:
policy::CloudPolicyStore* store_;  // Not owned.
I find this comment pattern quite low value and just adding noise. If it were
owned it'd be in a scoped_ptr.

If it were owned and not in a scoped_ptr (because it's posted to a DeleteSoon on
another thread, or something) then it would warrant an ownership comment.

wdyt?

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:106:
content::TestBrowserThreadBundle thread_bundle_;
This probably won't be necessary if you just have a loop and pass its proxy as
the background_task_runner

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:108:
base::FilePath cache_dir_;
#include

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:146:
content::RenderProcessHost::SetRunRendererInProcess(true);
Why?

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:146:
content::RenderProcessHost::SetRunRendererInProcess(true);
Why?

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:307:
}
nice test

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:642:
base::Bind(DoesInstallFailureReferToId, kGoodExtensionID));
Why does the extension fail to install?

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

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:22:
#include "chrome/browser/chromeos/settings/cros_settings.h"
already in .h

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:101:
callback);
See below about avoiding BrowserThread and the UI thread explicitly

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:119:
base::Bind(callback, account_id_to_delete));
Same here, see below

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:142:
task_runner));
Why is the core in a scoped_ptr now?

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:340:
weak_factory_.GetWeakPtr())));
Instead of posting this callback to UI, why don't you use PostTaskAndReply? It
achieves the same & avoids having to use BrowserThread here, which I've been
trying to avoid (even though this code won't move into the component)

See also PostTaskAndReplyWithResult in task_runner_utils.h for the callback with
the string.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:437:
account_ids,
|account_ids| only seem to be used here, and then
DeleteOrphanedExtensionCaches() goes over them and builds a set with the encoded
names. Why not just build the encoded names set from here?

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:440:
weak_factory_.GetWeakPtr())));
PostTaskAndReply?

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:445: 
remove newline

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:27: class
CrosSettings;
already #included

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:51: const
std::string& user_id() const { return user_id_; }
Document these two, it's not clear when each should be used. Or alternatively,
document these fields in DeviceLocalAccount and document here that these fields
refer to those others.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:155: void
OnObsoleteExensionCacheShutdown(const std::string& account_id);
OnObsoleteExtensionCacheShutdown (missing "t")

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:183: // are
not trusted yet).
remove )

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:196:
std::set<std::string> busy_extension_cache_directories_;
I think you got the logic right but I'm wary of having the set and 3 bools; it's
quite a bit of state to track, and very easy to get wrong in a subsequent
extension/refactoring to this class.

It'd be extra nice if this could be simplified, but I'm not stopping this change
because of this :-)

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_test_helper.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_test_helper.cc:66:
std::vector<RetrievePolicyCallback> callbacks;
Another nice catch :-)

Another option here: callbacks.clear()

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/external_provider_impl.cc:357: const chromeos::User*
user = chromeos::UserManager::Get()->GetActiveUser();
Use GetUserByProfile() instead.

When a new user is added to the session he doesn't automatically become the
active user.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/policy/configu...
File chrome/browser/policy/configuration_policy_handler_list.cc (left):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/policy/configu...
chrome/browser/policy/configuration_policy_handler_list.cc:148: Value::TYPE_LIST
},
Nice catch

[Matt Perry, 2013-10-17 18:34:20]

I think the original author of the loader code left, so I'll just lgtm.

[bartfab (slow), 2013-10-18 12:58:39]

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:22:
char kExtensionInstallForcelist[] = "forcelist";
On 2013/10/17 14:57:54, Joao da Silva wrote:
> const char

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:24:
}
On 2013/10/17 14:57:54, Joao da Silva wrote:
> // namespace

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:24:
}
On 2013/10/17 14:57:54, Joao da Silva wrote:
> // namespace

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:52:
false));
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Suggestion: just by looking at this it's not clear what true and false mean in
> this context. How about:
> 
> const bool check_for_updates = true;
> const bool wait_for_init_flag_file = false;
> 
> and using that as the arguments?

Done, although I went with a documentation pattern that seems to be more
commonly used in Chrome:

true /* always_check_updates */,
false /* wait_for_cache_initialization */

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:99:
policy_handler(kExtensionInstallForcelist);
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Daniel has recently refactored this class and the pref argument isn't needed
> anymore

At least for now, the class still takes a pref name. I will adjust once Daniel's
CL lands.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:104:
const base::DictionaryValue* dict = NULL;  // Not owned.
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Same comment re // not owned comment

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.h
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.h:68:
policy::CloudPolicyStore* store_;  // Not owned.
On 2013/10/17 14:57:54, Joao da Silva wrote:
> I find this comment pattern quite low value and just adding noise. If it were
> owned it'd be in a scoped_ptr.
> 
> If it were owned and not in a scoped_ptr (because it's posted to a DeleteSoon
on
> another thread, or something) then it would warrant an ownership comment.
> 
> wdyt?

A matter of opinion - some reviewers explicitly ask for this, others discourage
it. Removed.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:106:
content::TestBrowserThreadBundle thread_bundle_;
On 2013/10/17 14:57:54, Joao da Silva wrote:
> This probably won't be necessary if you just have a loop and pass its proxy as
> the background_task_runner

DeviceLocalAccountExternalPolicyLoader inherits from extensions::ExternalLoader
and that class CHECK()s whether it is running on the correct browser thread.
Hence, a thread bundle is required.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:108:
base::FilePath cache_dir_;
On 2013/10/17 14:57:54, Joao da Silva wrote:
> #include

Already included by device_local_account_external_policy_loader.h.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader_unittest.cc:146:
content::RenderProcessHost::SetRunRendererInProcess(true);
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Why?

Without this, the extension system would try to unpack CRX files in a sandboxed
child process but the unit test is not allowed to launch such a processs.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account_browsertest.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_browsertest.cc:642:
base::Bind(DoesInstallFailureReferToId, kGoodExtensionID));
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Why does the extension fail to install?

Extensions are not allowed in device-local accounts. I added a clarifying
comment.

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

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:22:
#include "chrome/browser/chromeos/settings/cros_settings.h"
On 2013/10/17 14:57:54, Joao da Silva wrote:
> already in .h

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:101:
callback);
On 2013/10/17 14:57:54, Joao da Silva wrote:
> See below about avoiding BrowserThread and the UI thread explicitly

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:119:
base::Bind(callback, account_id_to_delete));
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Same here, see below

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:142:
task_runner));
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Why is the core in a scoped_ptr now?

That was a leftover from some earlier iteration of the construction sequence.
Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:340:
weak_factory_.GetWeakPtr())));
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Instead of posting this callback to UI, why don't you use PostTaskAndReply? It
> achieves the same & avoids having to use BrowserThread here, which I've been
> trying to avoid (even though this code won't move into the component)
> 
> See also PostTaskAndReplyWithResult in task_runner_utils.h for the callback
with
> the string.

Done. No need for PostTaskAndReplyWithResult() either as I can just bind
|account_id| to the completion callback straight away.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:437:
account_ids,
On 2013/10/17 14:57:54, Joao da Silva wrote:
> |account_ids| only seem to be used here, and then
> DeleteOrphanedExtensionCaches() goes over them and builds a set with the
encoded
> names. Why not just build the encoded names set from here?

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:440:
weak_factory_.GetWeakPtr())));
On 2013/10/17 14:57:54, Joao da Silva wrote:
> PostTaskAndReply?

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.cc:445: 
On 2013/10/17 14:57:54, Joao da Silva wrote:
> remove newline

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
File chrome/browser/chromeos/policy/device_local_account_policy_service.h
(right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:27: class
CrosSettings;
On 2013/10/17 14:57:54, Joao da Silva wrote:
> already #included

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:51: const
std::string& user_id() const { return user_id_; }
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Document these two, it's not clear when each should be used. Or alternatively,
> document these fields in DeviceLocalAccount and document here that these
fields
> refer to those others.

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:155: void
OnObsoleteExensionCacheShutdown(const std::string& account_id);
On 2013/10/17 14:57:54, Joao da Silva wrote:
> OnObsoleteExtensionCacheShutdown (missing "t")

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:183: // are
not trusted yet).
On 2013/10/17 14:57:54, Joao da Silva wrote:
> remove )

Done.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/polic...
chrome/browser/chromeos/policy/device_local_account_policy_service.h:196:
std::set<std::string> busy_extension_cache_directories_;
On 2013/10/17 14:57:54, Joao da Silva wrote:
> I think you got the logic right but I'm wary of having the set and 3 bools;
it's
> quite a bit of state to track, and very easy to get wrong in a subsequent
> extension/refactoring to this class.
> 
> It'd be extra nice if this could be simplified, but I'm not stopping this
change
> because of this :-)

I introduced an enum that combines the two bools into a single state.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_test_helper.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_test_helper.cc:66:
std::vector<RetrievePolicyCallback> callbacks;
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Another nice catch :-)
> 
> Another option here: callbacks.clear()

I prefer creating a new object as it conveys best what this is - a brand new
list, starting out blank.

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/external_provider_impl.cc (right):

https://codereview.chromium.org/27548004/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/external_provider_impl.cc:357: const chromeos::User*
user = chromeos::UserManager::Get()->GetActiveUser();
On 2013/10/17 14:57:54, Joao da Silva wrote:
> Use GetUserByProfile() instead.
> 
> When a new user is added to the session he doesn't automatically become the
> active user.

Done.

[Joao da Silva, 2013-10-18 18:12:12]

lgtm

https://codereview.chromium.org/27548004/diff/94001/chrome/browser/chromeos/e...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc
(right):

https://codereview.chromium.org/27548004/diff/94001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:52:
false /* wait_for_cache_initialization */));
two spaces before // or /*

[bartfab (slow), 2013-10-21 11:44:04]

https://codereview.chromium.org/27548004/diff/94001/chrome/browser/chromeos/e...
File
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc
(right):

https://codereview.chromium.org/27548004/diff/94001/chrome/browser/chromeos/e...
chrome/browser/chromeos/extensions/device_local_account_external_policy_loader.cc:52:
false /* wait_for_cache_initialization */));
On 2013/10/18 18:12:12, Joao da Silva wrote:
> two spaces before // or /*

Done.

[commit-bot: I haz the power, 2013-10-21 11:48:52]

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

[commit-bot: I haz the power, 2013-10-21 12:32:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-10-21 12:50:01]

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

[commit-bot: I haz the power, 2013-10-21 13:07:07]

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

[bartfab (slow), 2013-10-21 16:07:48]

Hi Joao,

Could you take a quick look at the changes in patch set 6?

[bartfab (slow), 2013-10-21 16:08:14]

... and by patch set 6, I mean patch set 7.

[Joao da Silva, 2013-10-21 16:41:58]

Still looks good, go ahead :-)

If you find the time later I think it'd be a plus to revisit this code and
perhaps simplify it a bit. I'm thinking of getting rid of some of the state
(waiting_for_cros_settings, orphaned_cache_state, etc) and streamlining the
initialization order.

[commit-bot: I haz the power, 2013-10-21 16:55:00]

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

[commit-bot: I haz the power, 2013-10-21 20:10:26]

Change committed as 229896

[commit-bot: I haz the power, 2013-10-21 20:53:40]

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

[commit-bot: I haz the power, 2013-10-21 21:08:55]

Change committed as 229912