Magnifier: Prevent useless operation in enabling/disabling magnifier.

chrome/browser/chromeos/accessibility/magnification_manager.cc

Index: chrome/browser/chromeos/accessibility/magnification_manager.cc
diff --git a/chrome/browser/chromeos/accessibility/magnification_manager.cc b/chrome/browser/chromeos/accessibility/magnification_manager.cc
index 56819a49185111854b35d3347b13cb7ddf6a0dbe..e5b84cc3cedc787959e47b2b354ffc073ab04207 100644
--- a/chrome/browser/chromeos/accessibility/magnification_manager.cc
+++ b/chrome/browser/chromeos/accessibility/magnification_manager.cc
@@ -27,10 +27,8 @@ namespace chromeos {
 class MagnificationManagerImpl : public MagnificationManager,
                                  public content::NotificationObserver {
  public:
-  MagnificationManagerImpl() {
-    DCHECK(!instance_);
-    instance_ = this;
-
+  MagnificationManagerImpl() : profile_(NULL),
+                               type_(ash::MAGNIFIER_OFF) {
     registrar_.Add(this,
                   chrome::NOTIFICATION_SESSION_STARTED,
                   content::NotificationService::AllSources());
@@ -40,18 +38,26 @@ class MagnificationManagerImpl : public MagnificationManager,
     registrar_.Add(this,
                   chrome::NOTIFICATION_LOGIN_WEBUI_VISIBLE,
                   content::NotificationService::AllSources());
+  }
 
+  virtual void Initialize() OVERRIDE {
     Profile* profile = ProfileManager::GetDefaultProfileOrOffTheRecord();
     SetProfile(profile);
   }
-  virtual ~MagnificationManagerImpl() {}
+
+  virtual ~MagnificationManagerImpl() {
+  }
 
   static MagnificationManagerImpl* GetInstance() {
-    return instance_;
+    return Singleton<MagnificationManagerImpl>::get();

[Daniel Erat, 2012/12/04 01:06:36]

the Singleton template causes more trouble than it's worth, in my experience --
we've had a huge number of problems caused by these getting created or destroyed
in the wrong order.  please consider instead following the
Initialize()/Shutdown() pattern used by other singleton objects to keep creation
and destruction explicit.  see /chrome/browser/chromeos/audio/audio_handler.h or
/chromeos/dbus/dbus_thread_manager.h, for instance.

[stevenjb, 2012/12/04 01:35:34]

I would agree. Specifically, this class has a Profile member pointer and needs
to be explicitly destroyed before we destroy Profile (as it is currently) or
register the chrome::NOTIFICATION_PROFILE_DESTROYED message and clear profile_
(not a bad idea regardless).

The way this is currently constructed looks fine, and the fact that it is
explicitly owned and destroyed by ChromeBrowserMainPartsChromeos is nice. We
should however add the following code to the destructor:

DCHECK(this == instance_);
instance_ = NULL;

[yoshiki, 2012/12/04 08:30:20]

Thanks, I removed Singleton and and use Initialize()/Shutdown() pattern. 

On 2012/12/04 01:35:34, stevenjb (chromium) wrote:

   }
 
   // MagnificationManager implimentation:
   ash::MagnifierType GetMagnifierType() OVERRIDE {
+    return type_;
+  }
+
+  ash::MagnifierType GetMagnifierTypeFromPref() {
     if (!profile_)
       return ash::MAGNIFIER_OFF;
 
@@ -64,6 +70,11 @@ class MagnificationManagerImpl : public MagnificationManager,
   }
 
   void SetMagnifier(ash::MagnifierType type) OVERRIDE {
+    if (type_ == type)
+      return;
+
+    type_ = type;
+
     PrefService* prefs = profile_->GetPrefs();
     if (prefs) {
       std::string typeString = accessibility::ScreenMagnifierNameFromType(type);
@@ -73,16 +84,28 @@ class MagnificationManagerImpl : public MagnificationManager,
       }
     }
 
-    ash::Shell::GetInstance()->system_tray_notifier()->
-        NotifyAccessibilityModeChanged();
-
     ash::Shell::GetInstance()->magnification_controller()->SetEnabled(
         type == ash::MAGNIFIER_FULL);
     ash::Shell::GetInstance()->partial_magnification_controller()->SetEnabled(
         type == ash::MAGNIFIER_PARTIAL);
+
+    NotifyMagnifierTypeChanged(type);
+  }
+
+  void AddObserver(MagnificationObserver* observer) OVERRIDE {
+    observers_.AddObserver(observer);
+  }
+
+  void RemoveObserver(MagnificationObserver* observer) OVERRIDE {
+    observers_.RemoveObserver(observer);
   }
 
  private:
+  void NotifyMagnifierTypeChanged(ash::MagnifierType new_type) {
+    FOR_EACH_OBSERVER(MagnificationObserver, observers_,
+                      OnMagnifierTypeChanged(new_type));
+  }
+
   void SetProfile(Profile* profile) {
     if (pref_change_registrar_) {
       pref_change_registrar_.reset();
@@ -108,11 +131,12 @@ class MagnificationManagerImpl : public MagnificationManager,
     } else if (manager && !manager->IsSessionStarted()) {
       SetMagnifier(ash::MAGNIFIER_FULL);
     } else {
-      ash::MagnifierType type = GetMagnifierType();
+      ash::MagnifierType type = GetMagnifierTypeFromPref();
       SetMagnifier(type);
     }
   }
 
+  // content::NotificationObserver implimentation:
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE {
@@ -126,27 +150,19 @@ class MagnificationManagerImpl : public MagnificationManager,
     }
   }
 
-  static MagnificationManagerImpl* instance_;
-
   Profile* profile_;
+  ash::MagnifierType type_;
   content::NotificationRegistrar registrar_;
   scoped_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
+  ObserverList<MagnificationObserver> observers_;
+
   friend struct DefaultSingletonTraits<MagnificationManagerImpl>;
   DISALLOW_COPY_AND_ASSIGN(MagnificationManagerImpl);
 };
 
-MagnificationManagerImpl* MagnificationManagerImpl::instance_ = NULL;
-
 MagnificationManager* MagnificationManager::GetInstance() {
   return MagnificationManagerImpl::GetInstance();
 }
 
-MagnificationManager* MagnificationManager::CreateInstance() {
-  // Makes sure that this is not called more than once.
-  CHECK(!GetInstance());
-
-  return new MagnificationManagerImpl();
-}
-
 }  // namespace chromeos