Magnifier: Prevent useless operation in enabling/disabling magnifier.

chrome/browser/chromeos/accessibility/magnification_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/accessibility/magnification_manager.h"
 
 #include "ash/magnifier/magnification_controller.h"
 #include "ash/magnifier/partial_magnification_controller.h"
 #include "ash/shell.h"
 #include "ash/system/tray/system_tray_notifier.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "chrome/browser/api/prefs/pref_member.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
 
 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());
     registrar_.Add(this,
                   chrome::NOTIFICATION_PROFILE_CREATED,
                   content::NotificationService::AllSources());
     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;
 
     PrefService* prefs = profile_->GetPrefs();
     if (!prefs)
       return ash::MAGNIFIER_OFF;
 
     return accessibility::MagnifierTypeFromName(
         prefs->GetString(prefs::kMagnifierType).c_str());
   }
 
   void SetMagnifier(ash::MagnifierType type) OVERRIDE {
+    if (type_ == type)
+      return;
+
+    type_ = type;
+
     PrefService* prefs = profile_->GetPrefs();
     if (prefs) {
       std::string typeString = accessibility::ScreenMagnifierNameFromType(type);
       if (typeString != prefs->GetString(prefs::kMagnifierType)) {
         prefs->SetString(prefs::kMagnifierType, typeString);
         prefs->CommitPendingWrite();
       }
     }
 
-    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();
     }
 
     if (profile) {
       pref_change_registrar_.reset(new PrefChangeRegistrar);
       pref_change_registrar_->Init(profile->GetPrefs());
       pref_change_registrar_->Add(
           prefs::kMagnifierType,
           base::Bind(&MagnificationManagerImpl::UpdateMagnifierStatus,
                      base::Unretained(this)));
     }
 
     profile_ = profile;
     UpdateMagnifierStatus();
   }
 
   void UpdateMagnifierStatus() {
     UserManager* manager = UserManager::Get();
     if (!profile_) {
       SetMagnifier(ash::MAGNIFIER_OFF);
     } 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 {
     switch (type) {
       case chrome::NOTIFICATION_LOGIN_WEBUI_VISIBLE:
       case chrome::NOTIFICATION_SESSION_STARTED: {
         Profile* profile = ProfileManager::GetDefaultProfileOrOffTheRecord();
         SetProfile(profile);
         break;
       }
     }
   }
 
-  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