New issue
Advanced search Search tips

Issue 817157 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Team-Accessibility



Sign in to add a comment

Refactor Fullscreen Magnifier

Project Member Reported by afakhry@chromium.org, Feb 28 2018

Issue description

- Make MagnificationController a concrete class.
- Keep the state in one place (user prefs).
- Remove MagnificationManager entirely.
- Reuse the same DockedControllerClient to observe NOTIFICATION_FOCUS_CHANGED_IN_PAGE when either of the magnifiers is enabled.
 

Comment 1 by warx@chromium.org, Apr 18 2018

Cc: afakhry@chromium.org
Owner: warx@chromium.org
Let me take this one as part of the work of removing AccessibilityDelegate.

- Make MagnificationController a concrete class.
--> This is done.

- Keep the state in one place (user prefs).
--> Since ash::DockedMagnifierController registers/observes prefs change, I will also make ash::MagnifierController registers/observes prefs change.

- Remove MagnificationManager entirely.
- Reuse the same DockedControllerClient to observe NOTIFICATION_FOCUS_CHANGED_IN_PAGE when either of the magnifiers is enabled.

--> For chrome side, I am inclined to remove DockedControllerClient and merge it into chromeos::MagnificationManager. Then evaluate whether chromeos::MagnificationManager should be merged into chromeos::AccessibilityManager, as chromeos::ManificationManager will be a very thin class which just observes NOTIFICATION_FOCUS_CHANGED_IN_PAGE, which is something chromeos::AccessibilityManager did too.

For removing DockedMagnifierClient, I feel we can remove OnEnableStatusChanged:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/docked_magnifier/docked_magnifier_client.cc?l=40
(1) I think We can keep observing and call docked_magnifier_controller_->CenterOnPoint(), on ash side it early returns if feature is not enabled.
(2) on registrar_ destructing, it will automatically remove observers: https://cs.chromium.org/chromium/src/content/public/browser/notification_registrar.cc?l=44
Thus it doesn't need to be a client of DockedMagnifierController.
Also please rename MagnificationController to FullscreenMagnifier.

Regarding always observing, I feel this would be a waste to keep making these mojo calls to ash unnecessarily when neither of the magnifiers is enabled.

Comment 3 by warx@chromium.org, Apr 18 2018

James may have an answer whether OnEnableStatusChanged can be removed. For focus highlight, I already kept making mojo calls when focus highlight is not enabled: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/accessibility/accessibility_manager.cc?q=accessibility_manager.cc&dr&l=1091-1095
Since accessibility features can be rarely used, I still think it's wasteful to keep making mojo calls unnecessarily; especially for focus change events that happen all the time whilst the device is being used.
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 18 2018

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

commit bd0c79c3a69e07d6cb2dabef2a3d3bc4fea3a7da
Author: Qiang Xu <warx@google.com>
Date: Wed Apr 18 19:26:38 2018

cros: remove MagnificationManagerImpl

Changes:
This is a preparation CL. MagnificationManager could just be a concrete
class.

Bug: 817157
Test: compiles
Change-Id: If2d1e94e2679d220df142d115eaf36c93d456b6b
Reviewed-on: https://chromium-review.googlesource.com/1016321
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/heads/master@{#551779}
[modify] https://crrev.com/bd0c79c3a69e07d6cb2dabef2a3d3bc4fea3a7da/chrome/browser/chromeos/accessibility/magnification_manager.cc
[modify] https://crrev.com/bd0c79c3a69e07d6cb2dabef2a3d3bc4fea3a7da/chrome/browser/chromeos/accessibility/magnification_manager.h

Comment 6 by warx@chromium.org, Apr 18 2018

Regarding comment #4, I think we can check on chrome side, whether a11y feature is enabled or not, if not enabled, then not making mojo calls.
Comment 6 (check on chrome side if a11y feature enabled) sounds OK to me. Just be sure to document - otherwise the next person who adds a feature in ash that depends on focus changes may be confused that ash isn't always receiving them. :-)

Project Member

Comment 8 by bugdroid1@chromium.org, Apr 21 2018

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

commit c75e75dcf24f25f75d20075d5107501273a3b629
Author: Qiang Xu <warx@google.com>
Date: Sat Apr 21 07:13:39 2018

cros: remove DockedMagnifierClient

changes:
The plan is to make MagnificationManager a class to make mojo calls to
ash for FullscreenMagnifier or DockedMagnifier when
- Focus changed in page.
- Corresponding feature is enabled.
This CL removes DockedMagnifierClient and merges it into
MagnificationManager.

Bug: 817157
Test: existing tests and played on device working fine.
Change-Id: Idae6d108fd12a19a17e5d6d75fd89707e5ebf815
Reviewed-on: https://chromium-review.googlesource.com/1018823
Commit-Queue: Qiang Xu <warx@google.com>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552583}
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/ash/magnifier/docked_magnifier_controller.cc
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/ash/magnifier/docked_magnifier_controller.h
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/ash/magnifier/docked_magnifier_controller_unittest.cc
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/ash/magnifier/magnification_controller.h
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/ash/magnifier/magnification_controller_unittest.cc
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/ash/public/interfaces/docked_magnifier_controller.mojom
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/chrome/browser/chromeos/accessibility/magnification_manager.cc
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/chrome/browser/chromeos/accessibility/magnification_manager.h
[delete] https://crrev.com/ad680907edd75864926df6f07e7485840aa6bffa/chrome/browser/chromeos/accessibility/magnification_manager_unittest.cc
[delete] https://crrev.com/ad680907edd75864926df6f07e7485840aa6bffa/chrome/browser/chromeos/docked_magnifier/docked_magnifier_client.cc
[delete] https://crrev.com/ad680907edd75864926df6f07e7485840aa6bffa/chrome/browser/chromeos/docked_magnifier/docked_magnifier_client.h
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
[modify] https://crrev.com/c75e75dcf24f25f75d20075d5107501273a3b629/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h

Project Member

Comment 9 by bugdroid1@chromium.org, Apr 23 2018

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

commit 40fcdc2af365e3845ad683ea30e925d7d9eb8381
Author: Camille Lamy <clamy@chromium.org>
Date: Mon Apr 23 11:43:54 2018

Revert "cros: remove DockedMagnifierClient"

This reverts commit c75e75dcf24f25f75d20075d5107501273a3b629.

Reason for revert: This is causing AccessibilityManagerLoginTest.Login (browser_test) to fail on Linux ChromiumOS ASan LSan Test (https://uberchromegw.corp.google.com/i/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/27133)

Test failure:
[16199:16199:0421/024853.476320:FATAL:pref_service.cc(121)] Check failed: false. Trying to read an unregistered pref: ash.docked_magnifier.enabled

Original change's description:
> cros: remove DockedMagnifierClient
> 
> changes:
> The plan is to make MagnificationManager a class to make mojo calls to
> ash for FullscreenMagnifier or DockedMagnifier when
> - Focus changed in page.
> - Corresponding feature is enabled.
> This CL removes DockedMagnifierClient and merges it into
> MagnificationManager.
> 
> Bug: 817157
> Test: existing tests and played on device working fine.
> Change-Id: Idae6d108fd12a19a17e5d6d75fd89707e5ebf815
> Reviewed-on: https://chromium-review.googlesource.com/1018823
> Commit-Queue: Qiang Xu <warx@google.com>
> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#552583}

TBR=jamescook@chromium.org,dcheng@chromium.org,afakhry@chromium.org,warx@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 817157
Change-Id: I3663748ef764e0b70787d93f212d87948b7d7286
Reviewed-on: https://chromium-review.googlesource.com/1023633
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552671}
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/ash/magnifier/docked_magnifier_controller.cc
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/ash/magnifier/docked_magnifier_controller.h
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/ash/magnifier/docked_magnifier_controller_unittest.cc
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/ash/magnifier/magnification_controller.h
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/ash/magnifier/magnification_controller_unittest.cc
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/ash/public/interfaces/docked_magnifier_controller.mojom
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/chromeos/accessibility/magnification_manager.cc
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/chromeos/accessibility/magnification_manager.h
[add] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/chromeos/accessibility/magnification_manager_unittest.cc
[add] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/chromeos/docked_magnifier/docked_magnifier_client.cc
[add] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/chromeos/docked_magnifier/docked_magnifier_client.h
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
[modify] https://crrev.com/40fcdc2af365e3845ad683ea30e925d7d9eb8381/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 25 2018

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

commit dcf11e3e2f37233268e354ad7cbc3ca368b5786d
Author: Qiang Xu <warx@google.com>
Date: Wed Apr 25 17:10:17 2018

reland: cros: remove DockedMagnifierClient

this cl changes:
- Move ash::prefs::kDockedMagnifierEnabled registration to chrome to
  avoid ShouldShowAccessibilityMenu() crash.

original changes:
The plan is to make MagnificationManager a class to make mojo calls to
ash for FullscreenMagnifier or DockedMagnifier when
- Focus changed in page.
- Corresponding feature is enabled.
This CL removes DockedMagnifierClient and merges it into
MagnificationManager.

TBR=dcheng@chromium.org

Bug: 817157,  831258 
Test: existing tests and played on device working fine.
Change-Id: Ie8346291f03cc50b1b2580489413c3e45f6e38c8
Reviewed-on: https://chromium-review.googlesource.com/1018823
Commit-Queue: Qiang Xu <warx@google.com>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552583}
Reviewed-on: https://chromium-review.googlesource.com/1027080
Reviewed-by: Qiang Xu <warx@google.com>
Cr-Commit-Position: refs/heads/master@{#553606}
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/ash/magnifier/docked_magnifier_controller.cc
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/ash/magnifier/docked_magnifier_controller.h
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/ash/magnifier/docked_magnifier_controller_unittest.cc
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/ash/magnifier/magnification_controller.h
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/ash/magnifier/magnification_controller_unittest.cc
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/ash/public/interfaces/docked_magnifier_controller.mojom
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/ash/shell.cc
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/chrome/browser/chromeos/accessibility/magnification_manager.cc
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/chrome/browser/chromeos/accessibility/magnification_manager.h
[delete] https://crrev.com/540a25f935cbb605cdce15892cd2c6f7dd91f148/chrome/browser/chromeos/accessibility/magnification_manager_unittest.cc
[delete] https://crrev.com/540a25f935cbb605cdce15892cd2c6f7dd91f148/chrome/browser/chromeos/docked_magnifier/docked_magnifier_client.cc
[delete] https://crrev.com/540a25f935cbb605cdce15892cd2c6f7dd91f148/chrome/browser/chromeos/docked_magnifier/docked_magnifier_client.h
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/chrome/browser/chromeos/preferences.cc
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc
[modify] https://crrev.com/dcf11e3e2f37233268e354ad7cbc3ca368b5786d/chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.h

Comment 11 by warx@chromium.org, Jun 26 2018

Cc: -afakhry@chromium.org
Owner: afakhry@chromium.org
Labels: MagnifierCrOS
Labels: Proj-Mustash
Labels: -Proj-Mustash Proj-Mash-MultiProcess

Sign in to add a comment