Track all profiles ChromeVox loads to.

Track all profiles ChromeVox loads to.

- keeps track of all profile event routers ChromeVox registers observers on.
- ensures AccessibilityManager unregisters observers on all profile event routers when Shutdown occurs.
- always plays startup/shutdown sounds regardless of system preference. (This is the only accessible notification that ChromeVox has started/shutdown when toggling).

BUG=323030, 322793
TEST=Manual.

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

[David Tseng, 2014-01-06 19:55:07]

+ jamescook. Please take a look. Thanks.

[James Cook, 2014-01-06 20:38:19]

Also, one higher level question - do you still need to cache profile_ as well as
a set of profiles? Or could you look up the profile you need from
ProfileManager::GetActiveUserProfile()? I don't really know what happens here
when a user is using multi-profile.

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:752:
chromevox_profiles_.erase(item);
I think you can just do chromevox_profiles_.erase() without checking if it is
there.

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:862: if (profile_
== profile)
Should this clean up observers if *any* profile in the set is destroyed?  I'm
not sure if that can happen in practice (maybe with multi-profile) but would
probably be safer.

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:962: if (item !=
chromevox_profiles_.end())
same thing, I don't think you need to check if it is there

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.h:179: // Plays
sound identified by |sound_key| if |system_sounds_enabled_|.
This comment isn't true anymore.

[David Tseng, 2014-01-06 20:40:30]

+ dmazzoni for OWNERS.

[dmazzoni, 2014-01-06 20:41:25]

lgtm

[David Tseng, 2014-01-06 21:58:25]

On Mon, Jan 6, 2014 at 12:38 PM, <jamescook@chromium.org> wrote:
Also, one higher level question - do you still need to cache profile_ as
well as
a set of profiles? Or could you look up the profile you need from
ProfileManager::GetActiveUserProfile()? I don't really know what happens
here
when a user is using multi-profile.

I'd prefer to leave profile_ as it's used by other accessibility services
and the cached set of profiles is specific to ChromeVox only.


https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc
(right):

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:752:
chromevox_profiles_.erase(item);
I think you can just do chromevox_profiles_.erase() without checking if
it is there.

Done.
https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:862: if
(profile_ == profile)
Should this clean up observers if *any* profile in the set is destroyed?
 I'm not sure if that can happen in practice (maybe with multi-profile)
but would probably be safer.

Done.
https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:962: if
(item != chromevox_profiles_.end())
same thing, I don't think you need to check if it is there

Done.
https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
File chrome/browser/chromeos/accessibility/accessibility_manager.h
(right):

https://codereview.chromium.org/123443002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.h:179: //
Plays sound identified by |sound_key| if |system_sounds_enabled_|.
This comment isn't true anymore.

Removed.
https://codereview.chromium.org/123443002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[James Cook, 2014-01-06 22:06:21]

LGTM

[commit-bot: I haz the power, 2014-01-06 22:25:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dtseng@chromium.org/123443002/180001

[commit-bot: I haz the power, 2014-01-07 10:57:37]

Change committed as 243249