Magnifier: Prevent useless operation in enabling/disabling magnifier.

Magnifier: Prevent useless operation in enabling/disabling magnifier. 

On login, although the magnifier is disabled, the manager/controller try to disable it again. This CL prevents this useless operation.

Changes: 
- In MagnificationManager::SetMagnifier(), if the given type is same as the current type, do nothing. 
- In MagnificationControllerImpl::SetEnabled(), if the given |enabled| is same as current, do nothing. 
- Use Initialize/Shutdown pattern instead of scoped_ptr in ChromeBrowserMainPartsChromeos. 
- Initialize in PreProfileInit() instead of PostProfileInit().
- Add MagnificationObserver, and use it instead of watching pref changes. 
- GetMagnifierType() returns the cached value. 

BUG=163941
TEST=manual, browser_tests passes

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171401

[yoshiki, 2012-12-04 00:56:08]

Could you review this? Thanks,

@zork: 
- chrome/browser/chromeos/accessibility/*

@derat:
- ash/*
- chrome/browser/chromeos/* (except accessibility/*)

[Daniel Erat, 2012-12-04 01:06:35]

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
chrome/browser/chromeos/accessibility/magnification_manager.cc:52: return
Singleton<MagnificationManagerImpl>::get();
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.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:1115: LOG(ERROR) <<
"OnAccessibilityModeChanged";
nit: delete this

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

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
chrome/browser/chromeos/accessibility/magnification_manager.cc:52: return
Singleton<MagnificationManagerImpl>::get();
On 2012/12/04 01:06:36, Daniel Erat wrote:
> 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.

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;

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
File chrome/browser/chromeos/accessibility/magnification_manager.h (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
chrome/browser/chromeos/accessibility/magnification_manager.h:32: virtual
~MagnificationManager() {}
nit: This should be protected

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/chrom...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/chrom...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:628:
output_observer_.reset();
Having a MagnificationManager instance live past here could cause a number of
subtle shutdown bugs (e.g. accessing a destroyed Profile or ash::Shell). Much
better for code that might accidentally call MagnificationManager::GetInstance()
after this point to crash immediately on a NULL reference.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:223:
MagnificationManager::GetInstance()->AddObserver(this);
We will need to ensure that we create MagnificationManager before this gets
called.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:260:
MagnificationManager::GetInstance()->RemoveObserver(this);
This can be:
if (MagnificationManager::GetInstance())
  MagnificationManager::GetInstance()->RemoveObserver(this)

That way shutdown order doesn't matter.

[Zachary Kuznia, 2012-12-04 01:45:30]

lgtm after comments are addressed.

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

I've updated the code and the description. PTAL.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
chrome/browser/chromeos/accessibility/magnification_manager.cc:52: return
Singleton<MagnificationManagerImpl>::get();
Thanks, I removed Singleton and and use Initialize()/Shutdown() pattern. 

On 2012/12/04 01:35:34, stevenjb (chromium) wrote:
> On 2012/12/04 01:06:36, Daniel Erat wrote:
> > 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.
> 
> 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;
>

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
File chrome/browser/chromeos/accessibility/magnification_manager.h (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/acces...
chrome/browser/chromeos/accessibility/magnification_manager.h:32: virtual
~MagnificationManager() {}
On 2012/12/04 01:35:34, stevenjb (chromium) wrote:
> nit: This should be protected

Done.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/chrom...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/chrom...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:628:
output_observer_.reset();
I see, I added back removal of the instance.

On 2012/12/04 01:35:34, stevenjb (chromium) wrote:
> Having a MagnificationManager instance live past here could cause a number of
> subtle shutdown bugs (e.g. accessing a destroyed Profile or ash::Shell). Much
> better for code that might accidentally call
MagnificationManager::GetInstance()
> after this point to crash immediately on a NULL reference.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
File chrome/browser/chromeos/system/ash_system_tray_delegate.cc (right):

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:223:
MagnificationManager::GetInstance()->AddObserver(this);
On 2012/12/04 01:35:34, stevenjb (chromium) wrote:
> We will need to ensure that we create MagnificationManager before this gets
> called.

Done.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:260:
MagnificationManager::GetInstance()->RemoveObserver(this);
On 2012/12/04 01:35:34, stevenjb (chromium) wrote:
> This can be:
> if (MagnificationManager::GetInstance())
>   MagnificationManager::GetInstance()->RemoveObserver(this)
> 
> That way shutdown order doesn't matter.

Done.

https://codereview.chromium.org/11280287/diff/1/chrome/browser/chromeos/syste...
chrome/browser/chromeos/system/ash_system_tray_delegate.cc:1115: LOG(ERROR) <<
"OnAccessibilityModeChanged";
On 2012/12/04 01:06:36, Daniel Erat wrote:
> nit: delete this

Done.

[Daniel Erat, 2012-12-04 17:02:33]

(i'm out today and most of tomorrow, so i'll let steven give review this)

[stevenjb, 2012-12-04 18:06:26]

lgtm with one change.

https://codereview.chromium.org/11280287/diff/12019/chrome/browser/chromeos/a...
File chrome/browser/chromeos/accessibility/magnification_manager.h (right):

https://codereview.chromium.org/11280287/diff/12019/chrome/browser/chromeos/a...
chrome/browser/chromeos/accessibility/magnification_manager.h:29: virtual
~MagnificationManager() {}
protected section needs to follow public. Only the destructor really needs to be
protected, and since the constructor does nothing it can be omitted.

[commit-bot: I haz the power, 2012-12-05 06:16:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11280287/24002

[commit-bot: I haz the power, 2012-12-05 06:16:29]

Presubmit check for 11280287-24002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ash

Presubmit checks took 1.1s to calculate.

[commit-bot: I haz the power, 2012-12-05 06:19:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11280287/24002

[commit-bot: I haz the power, 2012-12-05 10:03:38]

Retried try job too often on linux_chromeos for step(s) browser_tests

[Daniel Erat, 2012-12-05 20:52:04]

LGTM for ash/

[commit-bot: I haz the power, 2012-12-05 23:53:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/11280287/28008