Getting rid of GetDefaultProfile & fixing multi user issues with accessibility

Getting rid of GetDefaultProfile & fixing multi user issues with accessibility

As discussed: I removed the occurances, switched the accessibility functionality with a user change and made sure that the accessibility menu is presented when at least one user has one accessibility feature turned on.

Not done:
- ChromeVox - this will most likely only partially work in multi user scenarios. It makes no sense to get this 80% working if we are in the midst of overhauling the use of ChromeVox. This needs to be re-visited once the changes are done.
- BrailleDisplayAPI - Since this is coupled with ChromeVox I guess that this falls into the same problem.
- MagnifierManager - A refactor should be done which merges this into the AccessibilityManager, but that would go beyond what we currently have to do. Besides I have found a comment which indicates that this might even be underway.


BUG=322682
TEST=unit tests (and multi profile by visual tests)

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

[Mr4D (OOO till 08-26), 2013-12-12 19:28:32]

Please have a look!

[Peter Lundblad, 2013-12-12 20:53:00]

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

https://codereview.chromium.org/102483006/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:318: // The
details are a chromeos::User object.
Misplaced comment?

https://codereview.chromium.org/102483006/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:351:
std::vector<Profile*> list =
nit: slightly confusing to call a vector a list:-)  Call profiles?

[Mr4D (OOO till 08-26), 2013-12-12 21:01:30]

Please have another look! (Added oshima for owners review)

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

https://codereview.chromium.org/102483006/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:318: // The
details are a chromeos::User object.
In deed. Done.

https://codereview.chromium.org/102483006/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:351:
std::vector<Profile*> list =
On 2013/12/12 20:53:00, Peter Lundblad wrote:
> nit: slightly confusing to call a vector a list:-)  Call profiles?

Done.

[dmazzoni, 2013-12-13 16:15:06]

lgtm with a few suggestions

https://codereview.chromium.org/102483006/diff/20001/ash/accessibility_delega...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/102483006/diff/20001/ash/accessibility_delega...
ash/accessibility_delegate.h:66: // Returns true if the user wants to show
accessibility menu.
I think it's misleading to say "if the user wants..."

Previously this function only returned true if the user had chosen to always
show the accessibility menu by a preference. It looks like now you're using this
function to compute if it should be shown for other reason.

So I'd comment "Returns true if the accessibility menu should be shown, which
may be because of an explicit user preference or because one of the logged-in
users has an accessibility setting on."

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:343:
RemoveSessionStateObserver(this);
I think we should have a ScopedSessionStateObserver class to make this more
foolproof. It'd eliminate the need for a boolean and code in the destructor.

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:347: // If any of
the loaded profiles has an accessibility feature turned on, we
If they have an accessibility feature turned on, *or* if they chose the checkbox
to always show the accessibility menu.

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:756:
SetProfile(ProfileManager::GetActiveUserProfile());
Does this need to take login / lock screen into account? Just making sure
AcitveUserChanged wouldn't get called when opening the login screen, for
example.

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/magnification_manager.cc:64: if
(session_state_observer_installed_)
Nit: {}

[Mr4D (OOO till 08-26), 2013-12-13 17:21:12]

Addressed! Many thanks!

Oshima - could you please do then the owners review?

https://codereview.chromium.org/102483006/diff/20001/ash/accessibility_delega...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/102483006/diff/20001/ash/accessibility_delega...
ash/accessibility_delegate.h:66: // Returns true if the user wants to show
accessibility menu.
On 2013/12/13 16:15:07, Dominic Mazzoni wrote:
> I think it's misleading to say "if the user wants..."
> 
> Previously this function only returned true if the user had chosen to always
> show the accessibility menu by a preference. It looks like now you're using
this
> function to compute if it should be shown for other reason.
> 
> So I'd comment "Returns true if the accessibility menu should be shown, which
> may be because of an explicit user preference or because one of the logged-in
> users has an accessibility setting on."

Done.

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:343:
RemoveSessionStateObserver(this);
On 2013/12/13 16:15:07, Dominic Mazzoni wrote:
> I think we should have a ScopedSessionStateObserver class to make this more
> foolproof. It'd eliminate the need for a boolean and code in the destructor.

Done.

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:347: // If any of
the loaded profiles has an accessibility feature turned on, we
On 2013/12/13 16:15:07, Dominic Mazzoni wrote:
> If they have an accessibility feature turned on, *or* if they chose the
checkbox
> to always show the accessibility menu.

Done.

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:756:
SetProfile(ProfileManager::GetActiveUserProfile());
This only gets called when a user switches actively to another (logged in) user.
So to answer your question: No the login screen is not able to call this
function - and no login will call this function either.

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/accessibility/magnification_manager.cc (right):

https://codereview.chromium.org/102483006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/magnification_manager.cc:64: if
(session_state_observer_installed_)
On 2013/12/13 16:15:07, Dominic Mazzoni wrote:
> Nit: {}

Done.

[dmazzoni, 2013-12-13 17:30:06]

lgtm

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

https://codereview.chromium.org/102483006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.h:41: class
ScopedSessionStateObserver {
Could you move this to session_state_observer.h?

I see lots of other places where it could be used, like
message_center_settings_controller.cc. You don't have to refactor all of them
now, just put it somewhere where it's easy for others to use.

[Mr4D (OOO till 08-26), 2013-12-13 17:58:32]

Addressed.

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

https://codereview.chromium.org/102483006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/accessibility/accessibility_manager.h:41: class
ScopedSessionStateObserver {
On 2013/12/13 17:30:07, Dominic Mazzoni wrote:
> Could you move this to session_state_observer.h?
> 
> I see lots of other places where it could be used, like
> message_center_settings_controller.cc. You don't have to refactor all of them
> now, just put it somewhere where it's easy for others to use.

Done.

[Mr4D (OOO till 08-26), 2013-12-13 18:41:22]

Side note: Not quite sure why all try bots are failing (since I can rebase and
build properly locally).
Looking at the logs it appears that there must be other patches which cannot be
applied (nacl, webkit, webrtc).

[oshima, 2013-12-13 22:40:26]

https://codereview.chromium.org/102483006/diff/60001/ash/session_state_observ...
File ash/session_state_observer.h (right):

https://codereview.chromium.org/102483006/diff/60001/ash/session_state_observ...
ash/session_state_observer.h:38: };
is it possible to use base/scoped_observer.h ?

[oshima, 2013-12-13 23:20:38]

On 2013/12/13 22:40:26, oshima wrote:
>
https://codereview.chromium.org/102483006/diff/60001/ash/session_state_observ...
> File ash/session_state_observer.h (right):
> 
>
https://codereview.chromium.org/102483006/diff/60001/ash/session_state_observ...
> ash/session_state_observer.h:38: };
> is it possible to use base/scoped_observer.h ?

Ok, SessionStateDelegate isn't using proper method name, so this LGTM.
I may change this later, but only if my change to scoped_observer.h is approved
:)

[commit-bot: I haz the power, 2013-12-13 23:31:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/102483006/60001

[commit-bot: I haz the power, 2013-12-14 02:41:14]

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

[commit-bot: I haz the power, 2013-12-14 03:02:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/102483006/60001

[commit-bot: I haz the power, 2013-12-14 06:59:13]

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

[commit-bot: I haz the power, 2013-12-14 19:01:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/102483006/80001

[commit-bot: I haz the power, 2013-12-14 20:03:19]

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

[commit-bot: I haz the power, 2013-12-14 20:13:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/102483006/80001

[commit-bot: I haz the power, 2013-12-14 21:50:51]

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

[commit-bot: I haz the power, 2013-12-16 15:33:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/102483006/80001

[commit-bot: I haz the power, 2013-12-16 18:11:00]

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

[commit-bot: I haz the power, 2013-12-16 18:28:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/102483006/80001

[commit-bot: I haz the power, 2013-12-16 20:36:04]

Change committed as 241010