Delay the metrics policy migration call to make sure ownership has been taken.

Delay the metrics policy migration call to make sure ownership has been taken.

This solved the issue that the UI does not reflect the OOBE screen setting
on new machines. There will be now a window of 30secs where this value will
still be wrong but this is still better than not migrating correctly at all.

BUG=chromium-os:19427
TEST=Check the metrics reporting on the EULA screen and see if the settings UI is checked too.


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

[pastarmovj, 2011-08-26 16:36:13]

Please take a look at this fix for the issue with migrating the metrics
reporting policy from the EULA screen after ownership is established. Thanks!

[Mattias Nissler (ping if slow), 2011-08-29 10:55:26]

I must say I don't really like this fix. Why can't we just act on the owner key
set signal, it already calls SignedSettingsTempStorage::Finalize()? Or just
listen for NOTIFICATION_OWNER_KEY_FETCH_ATTEMPT_SUCCEEDED.

http://codereview.chromium.org/7741045/diff/1/chrome/browser/chromeos/user_cr...
File chrome/browser/chromeos/user_cros_settings_provider.cc (right):

http://codereview.chromium.org/7741045/diff/1/chrome/browser/chromeos/user_cr...
chrome/browser/chromeos/user_cros_settings_provider.cc:66: // Delay this task by
30 seconds to ensure the ownership has been taken.
If you really want to do this, then please put the delay into the other PostTask
call that schedules StartStorePropertyOpIfOwner().

http://codereview.chromium.org/7741045/diff/1/chrome/browser/chromeos/user_cr...
chrome/browser/chromeos/user_cros_settings_provider.cc:75: 1000 * 30);
declare a constant?

[rkc, 2011-08-29 16:38:57]

If this is going in as a short term hack, please create a crosbug to fix it and
add a TODO pointing to it.

On 2011/08/29 10:55:26, Mattias Nissler wrote:
> I must say I don't really like this fix. Why can't we just act on the owner
key
> set signal, it already calls SignedSettingsTempStorage::Finalize()? Or just
> listen for NOTIFICATION_OWNER_KEY_FETCH_ATTEMPT_SUCCEEDED.
> 
>
http://codereview.chromium.org/7741045/diff/1/chrome/browser/chromeos/user_cr...
> File chrome/browser/chromeos/user_cros_settings_provider.cc (right):
> 
>
http://codereview.chromium.org/7741045/diff/1/chrome/browser/chromeos/user_cr...
> chrome/browser/chromeos/user_cros_settings_provider.cc:66: // Delay this task
by
> 30 seconds to ensure the ownership has been taken.
> If you really want to do this, then please put the delay into the other
PostTask
> call that schedules StartStorePropertyOpIfOwner().
> 
>
http://codereview.chromium.org/7741045/diff/1/chrome/browser/chromeos/user_cr...
> chrome/browser/chromeos/user_cros_settings_provider.cc:75: 1000 * 30);
> declare a constant?

[pastarmovj, 2011-08-31 10:22:14]

Completely overhauled the fix to actually react to the proper notification and
extended it to a more generic migration framework which we might need in the
future too.

[rkc, 2011-08-31 11:18:11]

Except the nits, looks good.

I'm concerned about the parameter passing to the StartStorePropertyOp task
though.

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

http://codereview.chromium.org/7741045/diff/5001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:65: //
SignedSettingsTempStorage if there is no owner yet. Or
Nit: Incomplete comment?

http://codereview.chromium.org/7741045/diff/5001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:108: i->first.c_str(),
value,
Why the difference between the passing of these two parameters? They seem to be
identical in the function declaration and the map template.

[pastarmovj, 2011-08-31 11:50:58]

Addressed Rahul's comments.

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

http://codereview.chromium.org/7741045/diff/5001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:65: //
SignedSettingsTempStorage if there is no owner yet. Or
On 2011/08/31 11:18:11, Rahul Chaturvedi wrote:
> Nit: Incomplete comment?

Ooops true.

http://codereview.chromium.org/7741045/diff/5001/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:108: i->first.c_str(),
value,
On 2011/08/31 11:18:11, Rahul Chaturvedi wrote:
> Why the difference between the passing of these two parameters? They seem to
be
> identical in the function declaration and the map template.

Underneath they are not the same because the key(first) is stored as const
string and the value second as a non-const string. And the compiler is clever
enough to extend the lifetime of the const object due to the const ref argument
but the value is not and the clear below deletes it.

For the sake of clarity I decided to go for unifying them.

[rkc, 2011-08-31 11:55:50]

LGTM
mnissler should give it a look over before commit though.

On 2011/08/31 11:50:58, pastarmovj wrote:
> Addressed Rahul's comments.
> 
>
http://codereview.chromium.org/7741045/diff/5001/chrome/browser/chromeos/user...
> File chrome/browser/chromeos/user_cros_settings_provider.cc (right):
> 
>
http://codereview.chromium.org/7741045/diff/5001/chrome/browser/chromeos/user...
> chrome/browser/chromeos/user_cros_settings_provider.cc:65: //
> SignedSettingsTempStorage if there is no owner yet. Or
> On 2011/08/31 11:18:11, Rahul Chaturvedi wrote:
> > Nit: Incomplete comment?
> 
> Ooops true.
> 
>
http://codereview.chromium.org/7741045/diff/5001/chrome/browser/chromeos/user...
> chrome/browser/chromeos/user_cros_settings_provider.cc:108: i->first.c_str(),
> value,
> On 2011/08/31 11:18:11, Rahul Chaturvedi wrote:
> > Why the difference between the passing of these two parameters? They seem to
> be
> > identical in the function declaration and the map template.
> 
> Underneath they are not the same because the key(first) is stored as const
> string and the value second as a non-const string. And the compiler is clever
> enough to extend the lifetime of the const object due to the const ref
argument
> but the value is not and the clear below deletes it.
> 
> For the sake of clarity I decided to go for unifying them.

[Mattias Nissler (ping if slow), 2011-08-31 12:47:23]

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

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:63: // store. It does one
of three things - stored the settings in the policy blob
s/stored/store/

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:80: // Migration value is
not thread safe so take care if you call this function
what is not thread safe? Isn't it the case that you want to call MigrateValues()
every time you call AddMigrationValue()? If that is true, let's just do the
thread switching in AddMigrationValue().

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:88: // there jump on the
right thread.
s/jump on/jump to/

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:100:
migration_values_.begin();
maybe move the i = migration_values_.begin() into the loop header if it fits?

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:107:
BrowserThread::PostTask(
It seems like you should only do the ownership checks on the file thread and
everything else on the UI thread. This also avoids accessing |migration_values_|
from different threads. Just post a task to the FILE thread to make the check,
then post a task back for the result. Bonus points if you make this into a
reusable component in the spirit of OwnershipStatusChecker.

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:125: const
NotificationSource& source,
indentation.

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:128: if
(!migration_values_.empty())
it's not safe to check from the UI thread, just don't.

[pastarmovj, 2011-08-31 14:29:36]

Addressed the new round of comments.

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

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:63: // store. It does one
of three things - stored the settings in the policy blob
On 2011/08/31 12:47:24, Mattias Nissler wrote:
> s/stored/store/

Done.

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:80: // Migration value is
not thread safe so take care if you call this function
This comment is obsolete now that we don't have to jump around different
threads.

On 2011/08/31 12:47:24, Mattias Nissler wrote:
> what is not thread safe? Isn't it the case that you want to call
MigrateValues()
> every time you call AddMigrationValue()? If that is true, let's just do the
> thread switching in AddMigrationValue().

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:88: // there jump on the
right thread.
On 2011/08/31 12:47:24, Mattias Nissler wrote:
> s/jump on/jump to/

Done.

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:100:
migration_values_.begin();
On 2011/08/31 12:47:24, Mattias Nissler wrote:
> maybe move the i = migration_values_.begin() into the loop header if it fits?

Done.

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:107:
BrowserThread::PostTask(
Tuned the OwnershipStatusChecker to supply all the info I needed.

On 2011/08/31 12:47:24, Mattias Nissler wrote:
> It seems like you should only do the ownership checks on the file thread and
> everything else on the UI thread. This also avoids accessing
|migration_values_|
> from different threads. Just post a task to the FILE thread to make the check,
> then post a task back for the result. Bonus points if you make this into a
> reusable component in the spirit of OwnershipStatusChecker.

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:125: const
NotificationSource& source,
On 2011/08/31 12:47:24, Mattias Nissler wrote:
> indentation.

Done.

http://codereview.chromium.org/7741045/diff/6002/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:128: if
(!migration_values_.empty())
On 2011/08/31 12:47:24, Mattias Nissler wrote:
> it's not safe to check from the UI thread, just don't.

Done.

[Mattias Nissler (ping if slow), 2011-08-31 15:08:33]

LGTM if you fix my nits and pending trybots.

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:253:
OwnershipService::Status status, bool current_user_is_owner) {
line break after comma

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/ownership_status_checker.h (right):

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/ownership_status_checker.h:26: //
OWNERSHIP_UNKNOWN. The bool parameter is true when the current logged in
s/when/if/

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

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:87: void
DoMigrateValues(OwnershipService::Status status,
make this private

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:91: if
(current_user_is_owner || status != OwnershipService::OWNERSHIP_TAKEN) {
Put a comment why we do this if ownership hasn't been taken yet (maybe move the
comment below in the empty else block up here)?

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:94: // This is needed to
avoid loosing the value after the clear below.
Nope, this is not needed. A copy is created when constructing the
StorePropertyOpContext.

[pastarmovj, 2011-08-31 15:26:36]

Addressed the nits and ran trybots. Will commit tomorrow if no objections or try
bot issues arise.

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/existing_user_controller.cc:253:
OwnershipService::Status status, bool current_user_is_owner) {
On 2011/08/31 15:08:33, Mattias Nissler wrote:
> line break after comma

Done.

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/ownership_status_checker.h (right):

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/ownership_status_checker.h:26: //
OWNERSHIP_UNKNOWN. The bool parameter is true when the current logged in
On 2011/08/31 15:08:33, Mattias Nissler wrote:
> s/when/if/

Done.

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

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:87: void
DoMigrateValues(OwnershipService::Status status,
On 2011/08/31 15:08:33, Mattias Nissler wrote:
> make this private

Done.

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:91: if
(current_user_is_owner || status != OwnershipService::OWNERSHIP_TAKEN) {
On 2011/08/31 15:08:33, Mattias Nissler wrote:
> Put a comment why we do this if ownership hasn't been taken yet (maybe move
the
> comment below in the empty else block up here)?

Done.

http://codereview.chromium.org/7741045/diff/8007/chrome/browser/chromeos/user...
chrome/browser/chromeos/user_cros_settings_provider.cc:94: // This is needed to
avoid loosing the value after the clear below.
Before I did need it because I was passing the result of a c_str instead of the
object and it was destroyed but not done anymore so removed. (Dunno why I
thought I need to pass pure char* instead of a string object).

On 2011/08/31 15:08:33, Mattias Nissler wrote:
> Nope, this is not needed. A copy is created when constructing the
> StorePropertyOpContext.

[commit-bot: I haz the power, 2011-09-01 12:05:27]

Change committed as 99169