PART2: Make SignedSettings use proper Value types instead of string all around the place.

Make SignedSettings use proper Value types instead of string all around the place.

Extra: Add ListValue support to the bunch and Userlist handling through Retrieve/StorePropertyOp.

BUG=chromium-os:14054
TEST=unit_tests:*Signed*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111141

[pastarmovj, 2011-10-04 16:41:12]

The second part of the refactoring is opened for reviewing too.

[Denis Lagno, 2011-10-04 17:01:50]

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:38: void Set(const std::string& path,
const base::Value& in_value);
AFAIU base::Values are intended to be created via Create functions, so users
normally operate with pointers to Value.  Thus why take reference instead of
pointer?

[pastarmovj, 2011-10-05 08:42:31]

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:38: void Set(const std::string& path,
const base::Value& in_value);
On 2011/10/04 17:01:50, Denis Lagno wrote:
> AFAIU base::Values are intended to be created via Create functions, so users
> normally operate with pointers to Value.  Thus why take reference instead of
> pointer?

This is true because the usual pref stores take ownership of those values and
take care to destroy them. In signed settings however the values are not stored
internally so I decided to go for an interface that makes sure the caller takes
the responsibility for the lifetime of the Value object itself to avoid memory
leaks. I think this way is much safer and straightforward for the users than
using a pointer and taking ownership of it.

[Denis Lagno, 2011-10-05 17:50:10]

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:38: void Set(const std::string& path,
const base::Value& in_value);
So you use const reference as a signal that ownership is not transferred.  I am
not sure if this style is acknowledged in  chromium codebase.  For instance
Value::Equals uses pointers as arguments and still ownership is not transferred.

[Chris Masone, 2011-10-06 16:13:05]

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:651: } else if (prop ==
kAccountsPrefUsers) {
so, this essentially means that WhitelistOp is obsolete, right?  Can we get a
comment to that effect on the WhitelistOp class declaration?

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_unittest.cc:507:
TEST_F(SignedSettingsTest, RetrieveProperty) {
Could you put in a new test to validate the whitelist getting/setting you added?

[Mattias Nissler (ping if slow), 2011-10-07 11:02:57]

Some comments.

Also, I'm leaning towards voting for Value* + ownership semantics, since from
what I've seen this seems to be easier for client code.

http://codereview.chromium.org/8091002/diff/4001/base/values.cc
File base/values.cc (left):

http://codereview.chromium.org/8091002/diff/4001/base/values.cc#oldcode888
base/values.cc:888: }
This is obsolete.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:522:
IPC::Message* reply_message) {
drive-by nit: indentation?

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:535:
chromeos::CrosSettings::Get()->Set(setting_path, *value);
Either the code is wrong or the comment is wrong. I guess it's the comment.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:88: scoped_ptr<base::Value>
new_value(new base::ListValue());
Allocating an object and throwing it away immediately if there's an existing
one? Seems weird.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:99: scoped_ptr<base::Value>
new_value(new base::ListValue());
ditto.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_provider.h (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_provider.h:23: // Note that this takes
ownership of |in_value|.
comment is wrong.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:206: static const char*
kVeritas[];
This should be unused, so remove it.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:617:
allow->set_allow_new_users(allow_value);
else NOTREACHED() (here and below?)

Actually, it might be quite helpful to write a small helpers for the common
boolean and string cases.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:758: return
(base::Value::CreateBooleanValue(
don't need outer parentheses.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc
(right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc:24:
ref_map_.Set("some_stuff", new base::StringValue("a=35;code=64"));
You should rather use Value::CreateStringValue() (same in the lines below).

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc:41:
//std::map<std::string, std::string> ref_map_;
remove.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_unittest.cc:85: // Speicalize the
template for base::Value obects because these compare
typos!

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_unittest.cc:650:
string_fake_value.c_str() + string_fake_value.length());
seems like a helper for this would be nice now that it's longer and occurring
already 4 times.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:78:
migration_values_[path] = value;
this may leak btw. if called twice for the same value. Consider using
PrefValueMap.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:221: }
unrelated?

[pastarmovj, 2011-10-13 11:25:06]

Addressed comments and rebased on the clean up version of PART1.

Again please keep in mind that there are at least 4 more parts in this series in
case you wonder why something is not in here.

http://codereview.chromium.org/8091002/diff/4001/base/values.cc
File base/values.cc (left):

http://codereview.chromium.org/8091002/diff/4001/base/values.cc#oldcode888
base/values.cc:888: }
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> This is obsolete.

Yes, rebasing artifact. Removed.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/automation/te...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:522:
IPC::Message* reply_message) {
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> drive-by nit: indentation?

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/automation/te...
chrome/browser/automation/testing_automation_provider_chromeos.cc:535:
chromeos::CrosSettings::Get()->Set(setting_path, *value);
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> Either the code is wrong or the comment is wrong. I guess it's the comment.

Comment is wrong the object is not owned by the provider therefore we only give
a ref.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:88: scoped_ptr<base::Value>
new_value(new base::ListValue());
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> Allocating an object and throwing it away immediately if there's an existing
> one? Seems weird.

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.cc:99: scoped_ptr<base::Value>
new_value(new base::ListValue());
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> ditto.

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings.h:38: void Set(const std::string& path,
const base::Value& in_value);
On 2011/10/05 17:50:10, Denis Lagno wrote:
> So you use const reference as a signal that ownership is not transferred.  I
am
> not sure if this style is acknowledged in  chromium codebase.  For instance
> Value::Equals uses pointers as arguments and still ownership is not
transferred.

Actually my interface exactly mimics the interface of the PrefService class
which was my aim from the very beginning so I am not an exception rather a
confirmation for the rule :)

Excerpt from pref_Service.h

  // If the path is valid (i.e., registered), update the pref value in the user
  // prefs.
  // To set the value of dictionary or list values in the pref tree use
  // Set(), but to modify the value of a dictionary or list use either
  // ListPrefUpdate or DictionaryPrefUpdate from scoped_user_pref_update.h.
  void Set(const char* path, const base::Value& value);
  void SetBoolean(const char* path, bool value);
  void SetInteger(const char* path, int value);
  void SetDouble(const char* path, double value);
  void SetString(const char* path, const std::string& value);
  void SetFilePath(const char* path, const FilePath& value);

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros_settings_provider.h (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros_settings_provider.h:23: // Note that this takes
ownership of |in_value|.
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> comment is wrong.

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:206: static const char*
kVeritas[];
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> This should be unused, so remove it.

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:617:
allow->set_allow_new_users(allow_value);
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> else NOTREACHED() (here and below?)
> 
> Actually, it might be quite helpful to write a small helpers for the common
> boolean and string cases.

Given the fact that we can only abstract the extraction step  it will bake less
sense to extract helpers. But NOTREACHED has been done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:651: } else if (prop ==
kAccountsPrefUsers) {
On 2011/10/06 16:13:06, Chris Masone wrote:
> so, this essentially means that WhitelistOp is obsolete, right?  Can we get a
> comment to that effect on the WhitelistOp class declaration?

I don't think this is much needed. The code still uses the Whitelist ops as of
this CL and the next CLs actually remove the WhitelistOp altogether.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings.cc:758: return
(base::Value::CreateBooleanValue(
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> don't need outer parentheses.

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc
(right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc:24:
ref_map_.Set("some_stuff", new base::StringValue("a=35;code=64"));
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> You should rather use Value::CreateStringValue() (same in the lines below).

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_temp_storage_unittest.cc:41:
//std::map<std::string, std::string> ref_map_;
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> remove.

Done.

P.S This file will be changed heavily when you see it in the next revision due
to ToT rebase.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_unittest.cc:85: // Speicalize the
template for base::Value obects because these compare
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> typos!

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_unittest.cc:507:
TEST_F(SignedSettingsTest, RetrieveProperty) {
On 2011/10/06 16:13:06, Chris Masone wrote:
> Could you put in a new test to validate the whitelist getting/setting you
added?

I'd rather not do in this CL. A latter step of this CLs remove all those tests
that use the RetrievePropertyOp because we remove it too. However I will move
those tests in a new CrosSettings test suite with the CL that removes the
RetrievePropertyOp from the picture.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/signed_settings_unittest.cc:650:
string_fake_value.c_str() + string_fake_value.length());
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> seems like a helper for this would be nice now that it's longer and occurring
> already 4 times.

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/user...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:78:
migration_values_[path] = value;
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> this may leak btw. if called twice for the same value. Consider using
> PrefValueMap.

Done.

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/8091002/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:221: }
On 2011/10/07 11:02:57, Mattias Nissler wrote:
> unrelated?

Yes. Cleared.

[Denis Lagno, 2011-10-13 13:43:10]

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:62: Set(path, value);
nit: why not pass const reference to temporary object?  Like:

Set(path, base::FundamentalValue(in_value));

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:68: Set(path, value);
ditto

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:74: Set(path, value);
ditto

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:81: Set(path, value);
ditto

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings_provider.h (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_provider.h:31: // true and win't call the
|callback|.
won't

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings.h:12: #include "base/values.h"
nit: sort

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_temp_storage.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_temp_storage.cc:65:
temp_storage->GetWithoutPathExpansion(*it, &value);
value contains garbage initially, and you do not check return value of
GetWithoutPathExpansion

[Mattias Nissler (ping if slow), 2011-10-13 14:49:52]

Due to the issues we discussed I feel that switching all  Value& parameters to
Value* with ownership transfer might be the better solution after all (as
pointed out by Denis initially).

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:62: Set(path, value);
On 2011/10/13 13:43:10, Denis Lagno wrote:
> nit: why not pass const reference to temporary object?  Like:
> 
> Set(path, base::FundamentalValue(in_value));
> 

clang won't like it.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings.cc:721: code,
base::ConstRef(*value_)));
This looks dangerous in that you need to guarantee that RetrievePropertyOp
outlives the references you take here. That makes me lean towards switching back
to passing Values as pointers after all.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_unittest.cc:86: // differently.
Why not just make a subclass as is done below for ProtoDelegate? What you have
now is inconsistent.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:135: base::Closure
callback_;
DISALLOW_COPY_AND_ASSIGN?

[Chris Masone, 2011-10-13 23:47:27]

Once Mattias and Denis are happy, I'm happy :-)

On 2011/10/13 14:49:52, Mattias Nissler wrote:
> Due to the issues we discussed I feel that switching all  Value& parameters to
> Value* with ownership transfer might be the better solution after all (as
> pointed out by Denis initially).
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
> File chrome/browser/chromeos/cros_settings.cc (right):
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros_settings.cc:62: Set(path, value);
> On 2011/10/13 13:43:10, Denis Lagno wrote:
> > nit: why not pass const reference to temporary object?  Like:
> > 
> > Set(path, base::FundamentalValue(in_value));
> > 
> 
> clang won't like it.
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/signed_settings.cc (right):
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/signed_settings.cc:721: code,
> base::ConstRef(*value_)));
> This looks dangerous in that you need to guarantee that RetrievePropertyOp
> outlives the references you take here. That makes me lean towards switching
back
> to passing Values as pointers after all.
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/signed_settings_unittest.cc:86: // differently.
> Why not just make a subclass as is done below for ProtoDelegate? What you have
> now is inconsistent.
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/use...
> File chrome/browser/chromeos/user_cros_settings_provider.cc (right):
> 
>
http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/use...
> chrome/browser/chromeos/user_cros_settings_provider.cc:135: base::Closure
> callback_;
> DISALLOW_COPY_AND_ASSIGN?

[pastarmovj, 2011-10-26 15:44:59]

Addressed all comments. Any other changes are collateral damage from rebasing to
ToT.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:62: Set(path, value);
On 2011/10/13 14:49:52, Mattias Nissler wrote:
> On 2011/10/13 13:43:10, Denis Lagno wrote:
> > nit: why not pass const reference to temporary object?  Like:
> > 
> > Set(path, base::FundamentalValue(in_value));
> > 
> 
> clang won't like it.

Exactly, sad but true fact :(

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings_provider.h (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings_provider.h:31: // true and win't call the
|callback|.
On 2011/10/13 13:43:10, Denis Lagno wrote:
> won't

Done.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings.cc:721: code,
base::ConstRef(*value_)));
On 2011/10/13 14:49:52, Mattias Nissler wrote:
> This looks dangerous in that you need to guarantee that RetrievePropertyOp
> outlives the references you take here. That makes me lean towards switching
back
> to passing Values as pointers after all.

Done. This made the mem management a little bit clumsier but this is
disappearing a few version later anyway completely.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings.h:12: #include "base/values.h"
On 2011/10/13 13:43:10, Denis Lagno wrote:
> nit: sort

Done.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_temp_storage.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_temp_storage.cc:65:
temp_storage->GetWithoutPathExpansion(*it, &value);
On 2011/10/13 13:43:10, Denis Lagno wrote:
> value contains garbage initially, and you do not check return value of
> GetWithoutPathExpansion

Done.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_unittest.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_unittest.cc:86: // differently.
On 2011/10/13 14:49:52, Mattias Nissler wrote:
> Why not just make a subclass as is done below for ProtoDelegate? What you have
> now is inconsistent.

Done.

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/8091002/diff/23001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:135: base::Closure
callback_;
On 2011/10/13 14:49:52, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

[Mattias Nissler (ping if slow), 2011-10-27 10:14:41]

Summing up our offline discussion: It seems like it'd be nicer for client code
if we changed Set() to pass a pointer and also pass in ownership.

http://codereview.chromium.org/8091002/diff/41001/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/8091002/diff/41001/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:394: const base::Value*
value) {
Seems like it'd be nicer for the users of OnRetrievePropertyCompleted if you
don't pass ownership of |value|. You can use base::Owned() for the
RetrievePropertyOp then.

[pastarmovj, 2011-10-28 13:35:33]

Addressed the comments about memory management. Didn't change the
StorePropertyOp as discussed offline it makes the code rather clumsier.

Also introduced one helper function in CrosSettings to check if a mail is in a
list with wildcards support. This was forgotten when removing the whitelist ops
and not only fixed the issue that wildcards were not working but also made the
usage of this feature so much cleaner as the diff to the previous version shows.

[Mattias Nissler (ping if slow), 2011-10-28 14:44:06]

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:115:
std::string("*").append(email.substr(email.find('@'))));
you call email.find twice. Create local variable?

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:74: const base::ListValue** out_value)
const;
newline before comment

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:77: // It will respect whitelists too so
foo@bar.baz will match *@bar.baz too.
s/will respect/respects/
too...too?

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:112:
users[i].email())) {
This is actually changing semantics to also show user pods if you have
*@domain.com, correct? I'm fine with that, just checking.

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:284: if
(cros_settings->FindEmailInList(kAccountsPrefUsers, username)) {
why has this worked before if we didn't check *@domain.com?

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:124: class
TrustedEntryFetcher : public Task {
Seems unused.

[pastarmovj, 2011-11-18 13:18:42]

Addressed comments the rest is collateral damage from rebasing to ToT.

Running a whole fleet of trybots already. Local tests seem to be ok too.

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.cc:115:
std::string("*").append(email.substr(email.find('@'))));
On 2011/10/28 14:44:07, Mattias Nissler wrote:
> you call email.find twice. Create local variable?

Done.

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros_settings.h (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:74: const base::ListValue** out_value)
const;
On 2011/10/28 14:44:07, Mattias Nissler wrote:
> newline before comment

Done.

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros_settings.h:77: // It will respect whitelists too so
foo@bar.baz will match *@bar.baz too.
On 2011/10/28 14:44:07, Mattias Nissler wrote:
> s/will respect/respects/
> too...too?

Too...too...the train is coming :)

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller.cc:112:
users[i].email())) {
On 2011/10/28 14:44:07, Mattias Nissler wrote:
> This is actually changing semantics to also show user pods if you have
> mailto:*@domain.com, correct? I'm fine with that, just checking.

Yes this is going to leave emails of people whitelisted by wildcards which I
think is correct.

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_performer.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_performer.cc:284: if
(cros_settings->FindEmailInList(kAccountsPrefUsers, username)) {
On 2011/10/28 14:44:07, Mattias Nissler wrote:
> why has this worked before if we didn't check *@domain.com?

Because there is some code that whitelists existing users when the checkbox to
restrict users is clicked. However I haven't seen code that does this for cloud
policy so we might have had some issues there with that. Moreover this code will
not get run for new users logging for the first time so no issues there too.

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/use...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/8091002/diff/44002/chrome/browser/chromeos/use...
chrome/browser/chromeos/user_cros_settings_provider.cc:124: class
TrustedEntryFetcher : public Task {
On 2011/10/28 14:44:07, Mattias Nissler wrote:
> Seems unused.

Yep obsolete since the base::Bind refactor of Tasks :) removed.

[Mattias Nissler (ping if slow), 2011-11-18 14:24:42]

LGTM with nits.

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings.h:13: #include "base/values.h"
forward declaration should suffice?

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_helper.h (right):

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_helper.h:11: #include
"base/values.h"
forward decl?

[pastarmovj, 2011-11-22 12:13:58]

Addressed the nits and rebased on the committed Part1. Will land once some more
bots cycle green on the first part.

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings.h (right):

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings.h:13: #include "base/values.h"
On 2011/11/18 14:24:42, Mattias Nissler wrote:
> forward declaration should suffice?

Done.

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/signed_settings_helper.h (right):

http://codereview.chromium.org/8091002/diff/51001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/signed_settings_helper.h:11: #include
"base/values.h"
On 2011/11/18 14:24:42, Mattias Nissler wrote:
> forward decl?

Done.