Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(32)

Issue 2291133005: Re-land: Move ChromeVox loading out of ComponentLoader. (Closed)

Created:
4 years, 3 months ago by dmazzoni
Modified:
4 years, 3 months ago
Reviewers:
Devlin, David Tseng
CC:
aboxhall+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, davemoore+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, extensions-reviews_chromium.org, je_julie, nektar+watch_chromium.org, oshima+watch_chromium.org, yuzo+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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. Committed: https://crrev.com/2c0132079532714ac3628568ef3ecd3826a39c18 Cr-Commit-Position: refs/heads/master@{#417038}

Patch Set 1 #

Patch Set 2 : Add ChromeVox to this profile if needed #

Total comments: 2

Patch Set 3 : Update component_loader comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -49 lines) Patch
M chrome/browser/chromeos/accessibility/accessibility_manager.cc View 1 2 3 chunks +20 lines, -1 line 0 comments Download
M chrome/browser/extensions/component_loader.h View 1 2 2 chunks +11 lines, -14 lines 0 comments Download
M chrome/browser/extensions/component_loader.cc View 1 2 4 chunks +7 lines, -34 lines 0 comments Download

Messages

Total messages: 17 (10 generated)
dmazzoni
Hi David and Devlin, Please take a fresh look at this change. The bug before ...
4 years, 3 months ago (2016-08-31 20:19:01 UTC) #2
Devlin
re-lgtm.
4 years, 3 months ago (2016-08-31 22:55:23 UTC) #8
David Tseng
Please add a TEST= with the specifics of how you tested this cl. https://codereview.chromium.org/2291133005/diff/20001/chrome/browser/extensions/component_loader.h File ...
4 years, 3 months ago (2016-09-01 19:46:41 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2291133005/40001
4 years, 3 months ago (2016-09-07 19:04:41 UTC) #13
dmazzoni
Did some more manual tests and added TEST line to reflect what I tested. https://codereview.chromium.org/2291133005/diff/20001/chrome/browser/extensions/component_loader.h ...
4 years, 3 months ago (2016-09-07 19:05:03 UTC) #14
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 3 months ago (2016-09-07 20:00:00 UTC) #15
commit-bot: I haz the power
4 years, 3 months ago (2016-09-07 20:05:48 UTC) #17
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/2c0132079532714ac3628568ef3ecd3826a39c18
Cr-Commit-Position: refs/heads/master@{#417038}

Powered by Google App Engine
This is Rietveld 408576698