Comb up ownership things.

Comb up ownership things.

Current codebase contains race in using UserManager::current_user_is_owner() method.
UserManager checks ownership asynchronously, so there is lapse of time when owner is logged in
but UserManager::current_user_is_owner() returns false.
In this CL this race is fixed for purpose of setting kOwnerLocale.

Also: shorten lapse of time during which UserManager::current_user_is_owner() returns false for owner session:
do this by making multi-threaded access to variable instead of posting another Task to message queue.

Also: in r75988 http://codereview.chromium.org/6538098 OWNERSHIP_TAKEN notification
was removed but observers of this notification were left as is.  Fix it, convert observers to another notification.

BUG=chromium-os:11212
TEST=Manual

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

[Denis Lagno, 2011-03-15 18:51:28]

please take a look

[Chris Masone, 2011-03-15 19:17:38]

user manager/owner manager chnages lgtm

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/ownership_service.h (right):

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/ownership_service.h:12: #include
"base/synchronization/lock.h"
Is this used?  Maybe a remnant of an intermediate stage of the CL?

[Denis Lagno, 2011-03-15 19:31:50]

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/ownership_service.h (right):

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/ownership_service.h:12: #include
"base/synchronization/lock.h"
On 2011/03/15 19:17:38, Chris Masone wrote:
> Is this used?  Maybe a remnant of an intermediate stage of the CL?

yes, this is used: there is base::Lock ownership_status_lock_; variable, so this
header should be included.

(The code works without this header because it includes
content/browser/browser_thread.h and it in its turn includes lock.h  But good
style is still to include lock.h explicitly.)

[Chris Masone, 2011-03-15 19:34:04]

Then, thanks for fixing my code :-)

On Tue, Mar 15, 2011 at 12:31 PM,  <dilmah@chromium.org> wrote:
>
>
http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/logi...
> File chrome/browser/chromeos/login/ownership_service.h (right):
>
>
http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/logi...
> chrome/browser/chromeos/login/ownership_service.h:12: #include
> "base/synchronization/lock.h"
> On 2011/03/15 19:17:38, Chris Masone wrote:
>>
>> Is this used?  Maybe a remnant of an intermediate stage of the CL?
>
> yes, this is used: there is base::Lock ownership_status_lock_; variable,
> so this header should be included.
>
> (The code works without this header because it includes
> content/browser/browser_thread.h and it in its turn includes lock.h  But
> good style is still to include lock.h explicitly.)
>
> http://codereview.chromium.org/6677049/
>

[altimofeev, 2011-03-16 11:35:54]

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/loca...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:91: if (prefs == NULL)
You can add NOTREACHED() here, since GetPrefs() should init them for you.

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

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1499:
local_state->SetString(prefs::kOwnerLocale, new_locale);
What about the case, when the owner changes locale from the settings? AFAIU, you
still need similar code here.

[Denis Lagno, 2011-03-16 11:55:14]

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/loca...
File chrome/browser/chromeos/locale_change_guard.cc (right):

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/chromeos/loca...
chrome/browser/chromeos/locale_change_guard.cc:91: if (prefs == NULL)
On 2011/03/16 11:35:54, altimofeev wrote:
> You can add NOTREACHED() here, since GetPrefs() should init them for you.

Done.

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

http://codereview.chromium.org/6677049/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1499:
local_state->SetString(prefs::kOwnerLocale, new_locale);
On 2011/03/16 11:35:54, altimofeev wrote:
> What about the case, when the owner changes locale from the settings? AFAIU,
you
> still need similar code here.

Good catch.  Done.

[altimofeev, 2011-03-16 12:04:25]

Thanks, LGTM!

Also, I would prefer to remove file "chrome/browser/profiles/profile_impl.cc"
from this CL (git reset, git checkout) to keep the history clean.