Add CloudExternalDataPolicyObserverChromeOS

Add CloudExternalDataPolicyObserver

This CL adds the CloudExternalDataPolicyObserver. This helper can be used
to observe a policy referencing external data for all users on a device,
covering both device-local accounts (whose policy is available and may
change anytime) and regular accounts (whose policy is available and may
change only when the user is logged in).

The helper emits notifications when an external data reference is set,
cleared or an external data fetch completes successfully.

The helper will be used to implement a policy for setting the avatar image
and a policy for setting the wallpaper.

The CL also adds the first policy referencing external data. The policy is
not wired up yet but having it defined unblocks server-side work and makes
testing easier. The policy will be implemented in a follow-up CL coming
soon.

BUG=152957, 152959, 220418
TEST=Full unit test coverage
TBR=askvitkine@chromium.org

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

[bartfab (slow), 2013-11-26 14:39:34]

Hi Joao,

Could you take a look at this CL?

Hi Paweł,

Could you take a look at chrome/test/base/testing_profile.*?

[Paweł Hajdan Jr., 2013-11-26 22:55:51]

chrome/test LGTM with comments

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:640: #endif
nit:  // defined(ENABLE...)

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
chrome/test/base/testing_profile.h:109: void
SetPolicyService(scoped_ptr<policy::PolicyService> policy_service);
nit: Please add a trivial comment to keep the style of surrounding code.

[bartfab (slow), 2013-11-27 14:31:24]

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:640: #endif
On 2013/11/26 22:55:52, Paweł Hajdan Jr. wrote:
> nit:  // defined(ENABLE...)

Done.

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.h (right):

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
chrome/test/base/testing_profile.h:109: void
SetPolicyService(scoped_ptr<policy::PolicyService> policy_service);
On 2013/11/26 22:55:52, Paweł Hajdan Jr. wrote:
> nit: Please add a trivial comment to keep the style of surrounding code.

Done.

[Joao da Silva, 2013-11-27 14:33:59]

Seems reasonable though a bit heavyweight.

https://codereview.chromium.org/88423002/diff/20001/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/88423002/diff/20001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:5730: 'name': 'Avatar',
I find the name a bit too generic, maybe 'UserAvatarImage'?

https://codereview.chromium.org/88423002/diff/20001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:5731: 'type': 'external',
You'll probably have to update & roll grit to support this.

Can you build target policy_templates with this policy in the templates?

https://codereview.chromium.org/88423002/diff/20001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:5750: This policy allows you to
configure the avatar image representing the user on the login screen. The policy
is set by specifying the URL from which <ph name="PRODUCT_OS_NAME">$2<ex>Google
Chrome OS</ex></ph> can download the avatar image and a cryptographic hash used
to verify the integrity of the download. The image must be in JPEG format, its
size must not exceed 512kB. The URL must be accessible without any
authentication.
The hash is also used to determine if the picture has been updated.

https://codereview.chromium.org/88423002/diff/20001/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:5758: "type": "string",
JSON doesn't allow trailing commas

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc
(right):

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:146:
}
device_local_account_entries_.clear()?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:168:
logged_in_user_observers_.end()) {
ContainsKey() is clearer

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:184:
logged_in_user_observers_.end()) {
ContainsKey

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:220:
map_entry = *entry->DeepCopy();
This leaks map_entry.value and map_entry.external_data_fetcher.

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:234:
base::Unretained(this)))) {
Is it safe to pass an unretained this? Can't the callback come after this is
gone?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:250:
if (device_local_accounts.find(it->first) == device_local_accounts.end()) {
!ContainsKey

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:253:
delete it->second.external_data_fetcher;
This is repeated in 3 places, and maybe a 4th is also needed. Maybe add a
Release/Wipe method to Entry?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h
(right):

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:6:
#define
CHROME_BROWSER_CHROMEOS_POLICY_CLOUD_EXTERNAL_DATA_POLICY_OBSERVER_CHROMEOS_H_
The _chromeos part of the filename is a bit redundant, since this lives in
chrome/browser/chromeos; up to you.

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:50:
virtual void Shutdown();
Who is going to own this? When is this going to be called?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:64:
scoped_ptr<std::string> data) = 0;
WDYT of putting these in a Delegate interface instead? It would allow tracking
more than one policy.

Also, if more than one policy is observed then passing the policy name to these
callbacks could be handy.

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:95:
typedef std::map<std::string, linked_ptr<PolicyServiceObserver> >
I've been asked to use maps of linked_ptrs before; personally I prefer
STLDeleteValues in dtors due to the lower overhead. Up to you.

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc
(right):

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:96:
typedef std::pair<std::string, linked_ptr<std::string> > FetchedCall;
Could be pair<string, string>, and you could use swap() to avoid unnecessary
copying.

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:116:
std::vector<std::string> set_calls_;
private:?

DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:173:
std::make_pair(user_id, make_linked_ptr(data.release())));
If you change FetchedCall to pair<string, string> then this could be

  fetched_calls_.push_back(make_pair(user_id, std::string()));
  fetched_calls_.back().second.swap(*data);

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:380:
profile_.reset(builder.Build().release());
profile_ = builder.Build();

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:413:
profile_.reset(builder.Build().release());
profile_ = builder.Build()

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:574:
EXPECT_FALSE(url_fetcher_factory_.GetFetcherByID(0));
1?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:685:
EXPECT_FALSE(url_fetcher_factory_.GetFetcherByID(1));
2?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:809:
EXPECT_FALSE(url_fetcher_factory_.GetFetcherByID(0));
1?

https://codereview.chromium.org/88423002/diff/20001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:1002:
}
Could we have a test verifying that, when available, fetch() uses cached data
and doesn't trigger a new network request? And that the observer gets that data?

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/88423002/diff/20001/chrome/test/base/testing_...
chrome/test/base/testing_profile.cc:253:
policy_service_(policy_service.release()) {
I think .Pass() is more common

[bartfab (slow), 2013-11-27 20:10:15]

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/app/policy/...
File chrome/app/policy/policy_templates.json (right):

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/app/policy/...
chrome/app/policy/policy_templates.json:5730: 'name': 'Avatar',
On 2013/11/27 14:33:59, Joao da Silva wrote:
> I find the name a bit too generic, maybe 'UserAvatarImage'?

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/app/policy/...
chrome/app/policy/policy_templates.json:5731: 'type': 'external',
On 2013/11/27 14:33:59, Joao da Silva wrote:
> You'll probably have to update & roll grit to support this.
> 
> Can you build target policy_templates with this policy in the templates?

Done: https://codereview.chromium.org/92213002

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/app/policy/...
chrome/app/policy/policy_templates.json:5750: This policy allows you to
configure the avatar image representing the user on the login screen. The policy
is set by specifying the URL from which <ph name="PRODUCT_OS_NAME">$2<ex>Google
Chrome OS</ex></ph> can download the avatar image and a cryptographic hash used
to verify the integrity of the download. The image must be in JPEG format, its
size must not exceed 512kB. The URL must be accessible without any
authentication.
On 2013/11/27 14:33:59, Joao da Silva wrote:
> The hash is also used to determine if the picture has been updated.

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/app/policy/...
chrome/app/policy/policy_templates.json:5758: "type": "string",
On 2013/11/27 14:33:59, Joao da Silva wrote:
> JSON doesn't allow trailing commas

Trailing comma is best comma.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:146:
}
On 2013/11/27 14:33:59, Joao da Silva wrote:
> device_local_account_entries_.clear()?

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:168:
logged_in_user_observers_.end()) {
On 2013/11/27 14:33:59, Joao da Silva wrote:
> ContainsKey() is clearer

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:184:
logged_in_user_observers_.end()) {
On 2013/11/27 14:33:59, Joao da Silva wrote:
> ContainsKey

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:220:
map_entry = *entry->DeepCopy();
On 2013/11/27 14:33:59, Joao da Silva wrote:
> This leaks map_entry.value and map_entry.external_data_fetcher.

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:234:
base::Unretained(this)))) {
On 2013/11/27 14:33:59, Joao da Silva wrote:
> Is it safe to pass an unretained this? Can't the callback come after this is
> gone?

Nice catch. I copy & pasted this snippet from somewhere and did not think about
it much. Now that you point that out... I wonder whether the place I copied this
from is buggy.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:250:
if (device_local_accounts.find(it->first) == device_local_accounts.end()) {
On 2013/11/27 14:33:59, Joao da Silva wrote:
> !ContainsKey

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.cc:253:
delete it->second.external_data_fetcher;
On 2013/11/27 14:33:59, Joao da Silva wrote:
> This is repeated in 3 places, and maybe a 4th is also needed. Maybe add a
> Release/Wipe method to Entry?

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h
(right):

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:6:
#define
CHROME_BROWSER_CHROMEOS_POLICY_CLOUD_EXTERNAL_DATA_POLICY_OBSERVER_CHROMEOS_H_
On 2013/11/27 14:33:59, Joao da Silva wrote:
> The _chromeos part of the filename is a bit redundant, since this lives in
> chrome/browser/chromeos; up to you.

I modeled this after things like device_cloud_policy_store_chromeos, which also
live in chromeos. But I do like the idea of dropping this part - long file names
are very unwieldy.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:50:
virtual void Shutdown();
On 2013/11/27 14:33:59, Joao da Silva wrote:
> Who is going to own this? When is this going to be called?

No longer relevant - there is a Delegate interface now and no Shutdown() method
is needed anymore.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:64:
scoped_ptr<std::string> data) = 0;
On 2013/11/27 14:33:59, Joao da Silva wrote:
> WDYT of putting these in a Delegate interface instead? It would allow tracking
> more than one policy.
> 
> Also, if more than one policy is observed then passing the policy name to
these
> callbacks could be handy.

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos.h:95:
typedef std::map<std::string, linked_ptr<PolicyServiceObserver> >
On 2013/11/27 14:33:59, Joao da Silva wrote:
> I've been asked to use maps of linked_ptrs before; personally I prefer
> STLDeleteValues in dtors due to the lower overhead. Up to you.

I prefer linked_ptrs: The overhead is negligible here (one linked_ptr per
logged-in user, so effectively 0-1 linked_ptrs in most cases, 3 at worst) and
the linked_ptrs make it more difficult to leak memory.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
File
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:96:
typedef std::pair<std::string, linked_ptr<std::string> > FetchedCall;
On 2013/11/27 14:33:59, Joao da Silva wrote:
> Could be pair<string, string>, and you could use swap() to avoid unnecessary
> copying.

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:116:
std::vector<std::string> set_calls_;
On 2013/11/27 14:33:59, Joao da Silva wrote:
> private:?
> 
> DISALLOW_COPY_AND_ASSIGN?

The class is gone altogether now.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:173:
std::make_pair(user_id, make_linked_ptr(data.release())));
On 2013/11/27 14:33:59, Joao da Silva wrote:
> If you change FetchedCall to pair<string, string> then this could be
> 
>   fetched_calls_.push_back(make_pair(user_id, std::string()));
>   fetched_calls_.back().second.swap(*data);

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:380:
profile_.reset(builder.Build().release());
On 2013/11/27 14:33:59, Joao da Silva wrote:
> profile_ = builder.Build();

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:413:
profile_.reset(builder.Build().release());
On 2013/11/27 14:33:59, Joao da Silva wrote:
> profile_ = builder.Build()

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:574:
EXPECT_FALSE(url_fetcher_factory_.GetFetcherByID(0));
On 2013/11/27 14:33:59, Joao da Silva wrote:
> 1?

Fetch 0 started but was never completed. This verifies that the fetch got
canceled in the meantime.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:685:
EXPECT_FALSE(url_fetcher_factory_.GetFetcherByID(1));
On 2013/11/27 14:33:59, Joao da Silva wrote:
> 2?

Done.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:809:
EXPECT_FALSE(url_fetcher_factory_.GetFetcherByID(0));
On 2013/11/27 14:33:59, Joao da Silva wrote:
> 1?

Fetch 0 started but was never completed. This verifies that the fetch got
canceled in the meantime.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer_chromeos_unittest.cc:1002:
}
On 2013/11/27 14:33:59, Joao da Silva wrote:
> Could we have a test verifying that, when available, fetch() uses cached data
> and doesn't trigger a new network request? And that the observer gets that
data?

The caching is not part of this class. It is handled by
CloudExternalDataManagerBase, which is not under test here.
CloudExternalDataManagerBase does have unit tests that do verify it uses caching
correctly of course.

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/test/base/t...
File chrome/test/base/testing_profile.cc (right):

https://chromiumcodereview.appspot.com/88423002/diff/20001/chrome/test/base/t...
chrome/test/base/testing_profile.cc:253:
policy_service_(policy_service.release()) {
On 2013/11/27 14:33:59, Joao da Silva wrote:
> I think .Pass() is more common

13 lines above, release() is used :). So considering the rule of staying
consistent with surrounding code, I tend toward release().

[Joao da Silva, 2013-11-28 08:40:17]

lgtm

Impressive test coverage!

https://chromiumcodereview.appspot.com/88423002/diff/60001/chrome/browser/chr...
File chrome/browser/chromeos/policy/cloud_external_data_policy_observer.h
(right):

https://chromiumcodereview.appspot.com/88423002/diff/60001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer.h:58:
scoped_ptr<std::string> data) = 0;
Suggestion: have a default impl that does nothing, so that a delegate can
override only methods of interest.

[bartfab (slow), 2013-11-28 12:58:11]

https://chromiumcodereview.appspot.com/88423002/diff/60001/chrome/browser/chr...
File chrome/browser/chromeos/policy/cloud_external_data_policy_observer.h
(right):

https://chromiumcodereview.appspot.com/88423002/diff/60001/chrome/browser/chr...
chrome/browser/chromeos/policy/cloud_external_data_policy_observer.h:58:
scoped_ptr<std::string> data) = 0;
On 2013/11/28 08:40:19, Joao da Silva wrote:
> Suggestion: have a default impl that does nothing, so that a delegate can
> override only methods of interest.

Done.

[commit-bot: I haz the power, 2013-11-28 12:58:23]

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

[commit-bot: I haz the power, 2013-11-28 16:56:44]

Change committed as 237806