Callback to function InitializeRegisteredDeviceWallpaper after cros settings can be trusted

chrome/browser/chromeos/login/wallpaper_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/wallpaper_manager.h"
 
 #include <vector>
 
 #include "ash/shell.h"
 #include "base/command_line.h"
@@ skipping 226 matching lines @@
   bool disable_new_oobe = CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kDisableNewOobe);
   bool disable_boot_animation = CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kDisableBootAnimation);
 
   if (!user_manager->IsUserLoggedIn()) {
     if (!disable_new_oobe) {
       if (!WizardController::IsDeviceRegistered()) {
         SetDefaultWallpaper();
       } else {
+        if (CrosSettingsProvider::TRUSTED !=
+            CrosSettings::Get()->PrepareTrustedValues(
+                base::Bind(&WallpaperManager::InitializeWallpaper,
+                           base::Unretained(this)))) {
+          return;
+        }

[Mattias Nissler (ping if slow), 2012/12/04 09:31:18]

should this be moved to the top of the function? Otherwise you re-do everything
that happens between function entry and here (which may be OK).

Also, you might want to think about watching for changes to
kAccountsPrefShowUserNamesOnSignIn (it may flip at any time) instead of just
waiting until the policy is loaded from disk.

[bartfab (slow), 2012/12/04 09:59:39]

You will also run into trouble when settings are permanently untrusted, as
happens at certain points during enrollment. If the settings are permanently
untrusted, you get neither a CrosSettingsProvider::TRUSTED return value nor a
callback.

Mattias' suggestion should work around this. If you set up an observer for
kAccountPrefShowUserNamesOnSignIn, you will be informed when policy kicks in,
the setting gets updated and trustedness goes from PERMANENTLY_UNTRUSTED to
TRUSTED without notifying you.

[bshe, 2012/12/04 18:32:37]

I see. How often does the PERMANENTLY_UNTRUSTED situation happen? I saw the
RetrieveTrustedDevicePolicies uses the callback pattern as well. Sorry I may
understood it wrong, but won't it have the same issue for PERMANENTLY_UNTRUSTED
status?

On 2012/12/04 09:59:39, bartfab wrote:

[bshe, 2012/12/04 18:32:37]

Some of the code path is not depend on the setting. If I move this to the top of
the function, these code path will only be executed once the value is ready. I
was hoping to unblock these code paths so I add it here. I should probably wrap
everything in the else block after line 252 to a new function and callback the
new function so I can avoid redo everything above. But since this is targeting
as m23, and some of the code has modified since m23. I would really like to
avoid merge conflict if possible and increase the chance to be merged. And redo
everything above doesn't look like a big overhead either. So probably keep it
this way for now? Happy to do the change if you disagree. 

For add observers, it seems that we already observe it here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/lo...

So if the enterprise administrator flip the setting on the server, it should be
observed by that code and the "correct" wallpaper should be set by it as well. I
believe it is not necessary to add another observer for the setting again on
TOT?

There is one problem though. The code that guarantee wallpaper is set correctly
(https://chromiumcodereview.appspot.com/11190022/) is not in M23. So on m23, we
will have the correct wallpaper not being set issue. eg. when switch to GAIA
signin form, we stuck to previous wallpaper instead of default wallpaper. This
should be acceptable in M23 as far as I know.  

On 2012/12/04 09:31:18, Mattias Nissler wrote:

[bshe, 2012/12/05 14:58:36]

bartfab@ Sorry. Please ignore my comment about RetrieveTrustedDevicePolicies. It
looks like do nothing for PERMANENTLY_UNTRUSTED is intended for that function.

On 2012/12/04 18:32:37, bshe wrote:

         bool show_users = true;
         bool result = CrosSettings::Get()->GetBoolean(
             kAccountsPrefShowUserNamesOnSignIn, &show_users);
         DCHECK(result) << "Unable to fetch setting "
                        << kAccountsPrefShowUserNamesOnSignIn;
         const chromeos::UserList& users = user_manager->GetUsers();
         if (!show_users || users.empty()) {
           // Boot into sign in form, preload default wallpaper.
           SetDefaultWallpaper();
           return;
@@ skipping 694 matching lines @@
                           FROM_HERE,
                           base::Bind(&WallpaperManager::StartLoad,
                                      base::Unretained(this),
                                      email,
                                      info,
                                      update_wallpaper,
                                      valid_path));
 }
 
 }  // chromeos