It will never work on signin screen because there are no user prefs yet. We discussed this with UI review and it seems there needs to be a decision about features that are per-user preferences (as opposed to per-device preferences). 

I can hide it from the menu on sign in screen if that's desired.

We have signin pref for signin screen behaviors: https://cs.chromium.org/chromium/src/ash/accessibility/accessibility_controller.h?q=accessibilitycontroller&dr=CSs&l=142. That is what we do for other signin screen a11y behaviors.

Yeah, for a lot of signin screen stuff we set the pref in the signin-screen profile, then there's code in chrome that copies the pref to the user profile on first login.

The code is ugly, but I think it's there to support features like this.

Re#3: As suggested, I added the code to make it possible to enable the docked magnifier on the sign in screen. However, I don't see that pref copying happening on first login. Is that a regression?

Think you need to have code in 

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/accessibility/accessibility_manager.cc?rcl=013076d59e1fba8ceb75eebc3306a4669d4d27b0&l=993

to copy to new profile.

Oh, I see. The problem is that it wasn't being used for the existing fullscreen magnifier.

The code in AccessibilityManager::PrefHandler::HandleProfileChanged() seem to be very useful to share and make available for other prefs to use. Any suggestions about where it might be better extracted? /ash/public/<something>? something else?

 Issue 820365  has been merged into this issue.

Not sure about long term plan. But can you add AccessibilityManager::PrefHandler for docked magnifier prefs?  It seems we already have some handler for ash prefs, e.g.

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/accessibility/accessibility_manager.cc?rcl=27761e38bec7cf9179ba5a7c78c86ca182fe2729&l=223

I can do that, but the new design James and I agreed on initially was to keep the docked magnifier fully implemented in Ash without any dependency on chrome.

Actually, the fullscreen magnifier should handle signin profile to user profile change: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/accessibility/magnification_manager.cc?q=%22magnifier_enabled_pref_handler_.HandleProfileChanged%22&l=128

I wonder why it doesn't work. I will investigate ...

It fails because of this check: current_profile->IsNewProfile() [1]

Is that expected? Should we really copy the prefs to a newly created profile only? What if the user makes changes to the prefs on the signin screen, then logs in, and his/her profile isn't new, shouldn't those changes be migrated to the actual user profile, or should the user be forced to redo these changes?


[1]: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/accessibility/accessibility_manager.cc?q=%22current_profile-%3EIsNewProfile()%22&l=174

Yes, my impression is that the prefs copying happens only for new user to the device.

It is implemented this way because we don't want to change an existing user profile automatically. For a single user device, it might make senses to copy the prefs. But it might not be case for a shared device.

Got it. I'll keep the behavior as is then.

For long term, if we want to move the AccessibilityManager::PrefHandler to ash, here are a few things we'll need in ash:

- Profile::IsNewProfile(): can easily be implemented in ash, since all it depends on is the PrefService [1].
- ProfileHelper::IsSigninProfile(): We'll need access to the profile itself to check its path [2] which I think we can't do in ash at the moment. Maybe we can store this info inside the PrefService, or make ash::SessionController able to tell if a PrefService is that of a signin profile.
- Profile::IsGuestSession(): This can be easily checked in ash. All we need to do is check for a command-line flag [3].


[1]: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile.cc?type=cs&q=Profile::IsNewProfile&l=266
[2]: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/profiles/profile_helper.cc?type=cs&q=ProfileHelper::IsSigninProfile&l=168
[3]: https://cs.chromium.org/chromium/src/chrome/browser/profiles/profile.cc?type=cs&q=Profile::IsGuestSession&l=236

For the second point, SessionController has that knowledge already in SessionController::GetSigninScreenPrefService()

We sort of have the IsNewProfile info [1] but it was put in user info.

[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/session_controller_client.cc?rcl=b140562cdcac6b2f7d34c95b0a1d43e6add0fd83&l=95

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07892721d37c6923fb55e0f7e37cab6a71b379f5

commit 07892721d37c6923fb55e0f7e37cab6a71b379f5
Author: Ahmed Fakhry <afakhry@google.com>
Date: Fri Apr 06 20:59:09 2018

Docked Magnifier: Enable using it on Signin Screen

- Enable using the docked magnifier on sign in screen.
- Refactor copying the accessibility prefs from the
  signin profile to a newly created profile out of Chrome
  into ash

BUG= 825969 
TEST=new test was added

Change-Id: Ie5d813832be203b8994b2bd8e4eb57d6744e0dd5
Reviewed-on: https://chromium-review.googlesource.com/993156
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548922}
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/accessibility/accessibility_controller.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/accessibility/accessibility_controller.h
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/accessibility/accessibility_controller_unittest.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/magnifier/docked_magnifier_controller.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/magnifier/docked_magnifier_controller.h
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/magnifier/docked_magnifier_controller_unittest.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/session/session_controller.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/session/session_controller.h
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/test/ash_test_base.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/ash/test/ash_test_base.h
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/chrome/browser/chromeos/accessibility/accessibility_manager.h
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/chrome/browser/chromeos/accessibility/magnification_manager.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/chrome/browser/chromeos/accessibility/magnification_manager_browsertest.cc
[modify] https://crrev.com/07892721d37c6923fb55e0f7e37cab6a71b379f5/testing/buildbot/filters/mash.ash_unittests.filter

lpalmaro@ Merging the CL in #16 to M-66 is not a good idea. The code in M-67 changed heavily from M-66 due to various refactorings. Merging this CL to M-66 introduces a lot of merge conflicts and I think we shouldn't do it.
What do you think?

Fixed in M-67+.