Fix login visual hitch on chromebook

chrome/browser/chromeos/login/wallpaper_manager.h

Index: chrome/browser/chromeos/login/wallpaper_manager.h
diff --git a/chrome/browser/chromeos/login/wallpaper_manager.h b/chrome/browser/chromeos/login/wallpaper_manager.h
index 7e6c375852630e83ba564f1d48306bad56a1bb4d..6d92a2711086b5b1c1686e44b9c8018a46d4f59c 100644
--- a/chrome/browser/chromeos/login/wallpaper_manager.h
+++ b/chrome/browser/chromeos/login/wallpaper_manager.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 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.
 
@@ -63,12 +63,16 @@ class WallpaperManager: public system::TimezoneSettings::Observer,
   static WallpaperManager* Get();
 
   WallpaperManager();
+  virtual ~WallpaperManager();
+
+  // Indicates imminent shutdown, allowing the WallpaperManager to remove any
+  // observers it has registered.
+  void Shutdown();
 
   // Registers wallpaper manager preferences.
   static void RegisterPrefs(PrefServiceSimple* local_state);
 

[bshe, 2013/01/23 16:08:34]

Add this function back because some tests (unit tests) use WallpaperManager
instance but do not call Shutdown function. So it is not a good idea to add
these observers in ctor.

[Nikita (slow), 2013/01/23 16:28:17]

Should they call shutdown instead?

[bshe, 2013/01/23 17:11:45]

We can explicitly call the shutdown in the tear down of these tests. But I
forgot to mention another reason to do this.
In unit tests, there are lots of place use wallpaper manager but do not have
DBusThreadManager or CrosSettings instance. If we add observers in ctor, we have
to add these intances to those tests.
So this is probably the easiest in terms of making these tests pass.

On 2013/01/23 16:28:17, Nikita Kostylev wrote:

[Nikita (slow), 2013/01/24 12:03:26]

I think that shutdown should be called in tests, ideally using the same code
path as the production code.

[bshe, 2013/01/24 14:56:54]

Sorry. Just to be clear, do you want me to call this Shutdown method in any test
that use WallpaperManager? I think the shutdown method is called by some tests
already but not all. In the case that it is not called, we cant add observers in
ctor. Otherwise, we will get size of observer_list != 0 error when program
exist.

On 2013/01/24 12:03:26, Nikita Kostylev wrote:

[Nikita (slow), 2013/01/24 15:39:00]

Also you'll have DCHECK failing because shutdown was not called.

My point is that if Shutdown() method is added as a required method for
WallpaperManager it has to be always called, including those tests. Ideally you
should not add ad-hoc solution but figure out why some tests do not call that
method. Those are browser_tests, right?

[bshe, 2013/01/24 16:25:09]

Good point on the DCHECK fail. And yes they are browser_tests. Somehow I am not
seeing the dcheck fail on the test results. It looks the wallpaper manager
desctructor is not being called somehow? If this is the case, I will address it
in issue 171694.

Also, the Shutdown is probably no longer a required method. I am guessing by
saying required, you mean the DCHECK in desctructor makes Shutdown required. The
reason we added the DCHECK is because we previously adding observer in ctor. So
we have to make sure Shutdown is called in dtor.

But now we are not doing that. Observers is not always added. So the DCHECK is
probably wrong? And AddObservers and Shutdown is probably not always needed for
all browser tests?

In the latest patch, I have updated the DCHECK to make sure if AddObservers is
called, we have to call Shutdown when program exist.

On 2013/01/24 15:39:00, Nikita Kostylev wrote:

-  // Adds PowerManagerClient and TimeZoneSettings observers. It needs to be
-  // added after PowerManagerClient has been initialized.
+  // Adds PowerManagerClient, TimeZoneSettings and CrosSettings observers.
   void AddObservers();
 
   // Loads wallpaper asynchronously if the current wallpaper is not the
@@ -158,8 +162,6 @@ class WallpaperManager: public system::TimezoneSettings::Observer,
   friend class WallpaperManagerBrowserTest;
   typedef std::map<std::string, gfx::ImageSkia> CustomWallpaperMap;
 
-  virtual ~WallpaperManager();
-
   // The number of wallpapers have loaded. For test only.
   int loaded_wallpapers() const { return loaded_wallpapers_; }
 
@@ -267,6 +269,9 @@ class WallpaperManager: public system::TimezoneSettings::Observer,
                                 bool update_wallpaper,
                                 const FilePath& wallpaper_path);
 
+  // True if Shutdown function get called.
+  bool called_shutdown_;
+
   // The number of loaded wallpapers.
   int loaded_wallpapers_;