New issue
Advanced search Search tips

Issue 634233 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Sep 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug
Team-Accessibility

Blocking:
issue 593887



Sign in to add a comment

Accessibility Manager should support multiple accessibility extensions

Project Member Reported by dmazz...@chromium.org, Aug 4 2016

Issue description

Right now a lot of stuff is hard-coded for ChromeVox, we should refactor it so that we can implement more accessibility features using component extensions where that makes sense.

 
Blocking: 593887
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 4 2016

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

commit 18f89cf0b120afff95e064f437140bdbfb33ef36
Author: dmazzoni <dmazzoni@chromium.org>
Date: Thu Aug 04 22:23:50 2016

Move ChromeVox loading out of ComponentLoader.

First, it isn't actually necessary to load ChromeVox from
ComponentLoader. This code is probably historical. For quite some time,
AccessibilityManager automatically loads ChromeVox on startup if the
flag is set (and automatically loads/unloads when the flag changes).
We should load it from only one place. Tested manually during startup
and when logging into a session, both work fine without this call.

Second, I intend to refactor AccessibilityManager so that it can support
multiple accessibility component extensions. Rather than a separate function
in ComponentLoader to load each one, I think it'd make more sense for
AccessibilityManager to just call ComponentLoader with the appropriate
parameters like the extension id and path.

BUG= 634233 

Review-Url: https://codereview.chromium.org/2212853002
Cr-Commit-Position: refs/heads/master@{#409916}

[modify] https://crrev.com/18f89cf0b120afff95e064f437140bdbfb33ef36/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/18f89cf0b120afff95e064f437140bdbfb33ef36/chrome/browser/extensions/component_loader.cc
[modify] https://crrev.com/18f89cf0b120afff95e064f437140bdbfb33ef36/chrome/browser/extensions/component_loader.h

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 9 2016

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

commit 03bc1f9c36c253bfd2ef0b53cf621a52128198f3
Author: dtseng <dtseng@chromium.org>
Date: Tue Aug 09 15:50:06 2016

Revert of Move ChromeVox loading out of ComponentLoader. (patchset #3 id:40001 of https://codereview.chromium.org/2212853002/ )

Reason for revert:
BUG= 635294 

Repro:
- run Chrome OS on linux with an existing profile and with the flag --login-manager
- sign in or go through OOBE with ChromeVox on (and working)

result:
content scripts appear to be broken when logged in. I suspect user ChromeVox isn't loading.

Original issue's description:
> Move ChromeVox loading out of ComponentLoader.
>
> First, it isn't actually necessary to load ChromeVox from
> ComponentLoader. This code is probably historical. For quite some time,
> AccessibilityManager automatically loads ChromeVox on startup if the
> flag is set (and automatically loads/unloads when the flag changes).
> We should load it from only one place. Tested manually during startup
> and when logging into a session, both work fine without this call.
>
> Second, I intend to refactor AccessibilityManager so that it can support
> multiple accessibility component extensions. Rather than a separate function
> in ComponentLoader to load each one, I think it'd make more sense for
> AccessibilityManager to just call ComponentLoader with the appropriate
> parameters like the extension id and path.
>
> BUG= 634233 
>
> Committed: https://crrev.com/18f89cf0b120afff95e064f437140bdbfb33ef36
> Cr-Commit-Position: refs/heads/master@{#409916}

TBR=rdevlin.cronin@chromium.org,dmazzoni@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 634233 

Review-Url: https://codereview.chromium.org/2229833002
Cr-Commit-Position: refs/heads/master@{#410685}

[modify] https://crrev.com/03bc1f9c36c253bfd2ef0b53cf621a52128198f3/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/03bc1f9c36c253bfd2ef0b53cf621a52128198f3/chrome/browser/extensions/component_loader.cc
[modify] https://crrev.com/03bc1f9c36c253bfd2ef0b53cf621a52128198f3/chrome/browser/extensions/component_loader.h

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 7 2016

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

commit 2c0132079532714ac3628568ef3ecd3826a39c18
Author: dmazzoni <dmazzoni@chromium.org>
Date: Wed Sep 07 19:59:37 2016

Re-land: Move ChromeVox loading out of ComponentLoader.

Original changelist: http://crrev.com/2212853002

This change landed and was then reverted because it broke ChromeVox when
logging in when it was already enabled on the login screen. The issue was
that AccessibilityManager needed to add the component extension to each
profile.

The immediate benefit is that ComponentLoader doesn't need to depend on
AccessibilityManager - previously they both depended on each other.

The end goal is to refactor AccessibilityManager so that it can support
multiple accessibility component extensions. Rather than a separate function
in ComponentLoader to load each one, I think it'd make more sense for
AccessibilityManager to just call ComponentLoader with the appropriate
parameters like the extension id and path.

BUG= 634233 
TEST=Toggle ChromeVox on/off from login screen, logged-in session, and guest session. Enable ChromeVox in login screen and log in, ensure ChromeVox remains on. Try with both ChromeVox Classic and Next.

Review-Url: https://codereview.chromium.org/2291133005
Cr-Commit-Position: refs/heads/master@{#417038}

[modify] https://crrev.com/2c0132079532714ac3628568ef3ecd3826a39c18/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/2c0132079532714ac3628568ef3ecd3826a39c18/chrome/browser/extensions/component_loader.cc
[modify] https://crrev.com/2c0132079532714ac3628568ef3ecd3826a39c18/chrome/browser/extensions/component_loader.h

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 13 2016

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

commit 337c5afe3e3c93faa54c445355dc95299d0d73ef
Author: dmazzoni <dmazzoni@chromium.org>
Date: Tue Sep 13 23:08:57 2016

Factor the extension loading code out of AccessibilityManager

Create a new class, AccessibilityExtensionLoader, and pull all of the code
that currently loads ChromeVox into it.

This is meant to be a pure refactoring change with no user-perceivable
change in functionality. If any change in behavior traces back to this
change, that's a bug.

The purpose of this change is so that AccessibilityManager can be
extended to load additional accessibility component extensions other
than ChromeVox.

BUG= 634233 
TBR=oshima@chromium.org
TEST=Toggle ChromeVox on login screen, logged-in screen, guest screen, and lock screen. Test logging in with ChromeVox disabled and enabled.

Review-Url: https://codereview.chromium.org/2212863002
Cr-Commit-Position: refs/heads/master@{#418401}

[modify] https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef/chrome/browser/chromeos/BUILD.gn
[add] https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef/chrome/browser/chromeos/accessibility/accessibility_extension_loader.cc
[add] https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef/chrome/browser/chromeos/accessibility/accessibility_extension_loader.h
[modify] https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef/chrome/browser/chromeos/accessibility/accessibility_manager.cc
[modify] https://crrev.com/337c5afe3e3c93faa54c445355dc95299d0d73ef/chrome/browser/chromeos/accessibility/accessibility_manager.h

Status: Fixed (was: Started)

Comment 7 by dchan@chromium.org, Oct 7 2016

Labels: VerifyIn-55
Status: Verified (was: Fixed)
As per #6

Sign in to add a comment