Remove dependency on NotificationService for cros a11y settings.

Remove dependency on NotificationService for cros a11y settings.

BUG=338387, 268984
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253033

[kevers, 2014-02-19 16:55:23]

Hey David and Nikita,

Please take a look at this CL. Part of a cleanup effort to phase out
notification services.

[kevers, 2014-02-19 19:36:24]

Found a problem with the magnification manager unit tests.  The compiler error
was easy enough to fix, but there is a problem with notifications as the failing
unit tests instantiate a magnification manager in the absence of an
accessibility manager.

[Nikita (slow), 2014-02-20 10:39:04]

chrome/browser/chromeos/login/*
chrome/browser/ui/webui/chromeos/login/*

lgtm with nits

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.h:10: #include
"base/callback_list.h"
I think this include is not needed here.

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.h:231: const
AccessibilityStatusEventDetails& details);
nit: 4 spaces indent.

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h (right):

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:119:
content::NotificationRegistrar registrar_;
NotificationRegistrar not needed anymore.

[kevers, 2014-02-20 17:00:49]

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.h (right):

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.h:10: #include
"base/callback_list.h"
On 2014/02/20 10:39:04, Nikita Kostylev wrote:
> I think this include is not needed here.

Removed.

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.h:231: const
AccessibilityStatusEventDetails& details);
On 2014/02/20 10:39:04, Nikita Kostylev wrote:
> nit: 4 spaces indent.

Done.

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h (right):

https://codereview.chromium.org/166273014/diff/50001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:119:
content::NotificationRegistrar registrar_;
On 2014/02/20 10:39:04, Nikita Kostylev wrote:
> NotificationRegistrar not needed anymore.

Removed

https://codereview.chromium.org/166273014/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/magnification_manager_unittest.cc
(left):

https://codereview.chromium.org/166273014/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/magnification_manager_unittest.cc:101:
TEST_F(MagnificationManagerTest, MagnificationObserver) {
Moved to browser test since notifications are via the AccessibilityManager,
which in turn depends on other resources.

[kevers, 2014-02-21 16:19:35]

David can you please take a look at the CL.

+sky for chrome_notification_types and system_tray_delegate.
+fsamuel for webview.

[Fady Samuel, 2014-02-21 16:21:39]

old chunk mismatch. Please reupload.

[kevers, 2014-02-21 16:50:59]

On 2014/02/21 16:21:39, Fady Samuel wrote:
> old chunk mismatch. Please reupload.

Done.

[Fady Samuel, 2014-02-21 16:53:31]

webview_guest LGTM

[sky, 2014-02-21 17:10:32]

LGTM

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:283:
base::Unretained(this)));
Add a comment as to why Unretained is safe here.

[David Tseng, 2014-02-21 17:42:58]

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.h:255:
AccessibilityStatusCallbackList callback_list_;
I'm unclear as to the ownership of this list. It's returned by RegisterCallback
and appears to be reset by external classes?

[kevers, 2014-02-21 17:52:30]

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.h:255:
AccessibilityStatusCallbackList callback_list_;
On 2014/02/21 17:42:59, David Tseng wrote:
> I'm unclear as to the ownership of this list. It's returned by
RegisterCallback
> and appears to be reset by external classes?

External classes reset/cancel their subscriptions when deleted. Subscribers are
also told when the accessibility manager is being shut down so that they can
release their subscriptions, which alleviates potential ordering problems on
shutdown.

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:283:
base::Unretained(this)));
On 2014/02/21 17:10:32, sky wrote:
> Add a comment as to why Unretained is safe here.

The destructor for the subscription removes itself from the callback list.

[David Tseng, 2014-02-21 19:44:50]

LGTM accessibility.

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/166273014/diff/360020/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.h:255:
AccessibilityStatusCallbackList callback_list_;
On 2014/02/21 17:52:31, kevers wrote:
> On 2014/02/21 17:42:59, David Tseng wrote:
> > I'm unclear as to the ownership of this list. It's returned by
> RegisterCallback
> > and appears to be reset by external classes?
> 
> External classes reset/cancel their subscriptions when deleted. Subscribers
are
> also told when the accessibility manager is being shut down so that they can
> release their subscriptions, which alleviates potential ordering problems on
> shutdown.

Ok; thanks for clarifying.

[kevers, 2014-02-24 14:30:56]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-02-24 14:31:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/166273014/360020

[commit-bot: I haz the power, 2014-02-24 15:50:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-24 15:50:33]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kevers, 2014-02-24 17:10:11]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-02-24 17:10:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/166273014/360020

[commit-bot: I haz the power, 2014-02-24 17:56:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-24 17:56:28]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[kevers, 2014-02-24 22:24:09]

The CQ bit was checked by kevers@chromium.org

[commit-bot: I haz the power, 2014-02-24 22:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kevers@chromium.org/166273014/360020

[commit-bot: I haz the power, 2014-02-25 00:06:08]

Change committed as 253033

[Avi (use Gerrit), 2014-02-25 03:39:48]

Woot. Thank you for your help!