Move ChromeVox loading out of ComponentLoader.

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}

[dmazzoni, 2016-08-04 06:35:42]

dmazzoni@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[dmazzoni, 2016-08-04 06:35:43]

Devlin, please take a look and let me know what you think.
David: FYI / secondary review

[Devlin, 2016-08-04 16:18:33]

https://codereview.chromium.org/2212853002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2212853002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:661: bool
ComponentLoader::IsNormalSession() {
Rather than exposing this logic to the a11y manager, could we just determine the
manifest file name directly in AddWithManifestFile()?  It'd also make
AddChromeOsSpeechSynthesisExtension a little simpler.

[dmazzoni, 2016-08-04 17:54:28]

The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run

[dmazzoni, 2016-08-04 17:54:38]

https://codereview.chromium.org/2212853002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/component_loader.cc (right):

https://codereview.chromium.org/2212853002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/component_loader.cc:661: bool
ComponentLoader::IsNormalSession() {
On 2016/08/04 at 16:18:33, Devlin wrote:
> Rather than exposing this logic to the a11y manager, could we just determine
the manifest file name directly in AddWithManifestFile()?  It'd also make
AddChromeOsSpeechSynthesisExtension a little simpler.

Sounds good.

How about calling it something like AddComponentWithGuestManifest? Take a look.

[commit-bot: I haz the power, 2016-08-04 17:55:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-08-04 18:06:44]

lgtm with nit

https://codereview.chromium.org/2212853002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/2212853002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.h:103: // this is a guest session. 
Calls |done_cb| on success, unless the
To me, this implies that it only adds it during guest sessions.  How about
something like AddComponentFromDir()?

[commit-bot: I haz the power, 2016-08-04 18:44:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 18:44:34]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2016-08-04 21:35:09]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-08-04 21:35:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2212853002/#ps40001
(title: "Rename to AddComponentFromDir")

[dmazzoni, 2016-08-04 21:35:27]

https://codereview.chromium.org/2212853002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/component_loader.h (right):

https://codereview.chromium.org/2212853002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/component_loader.h:103: // this is a guest session. 
Calls |done_cb| on success, unless the
On 2016/08/04 at 18:06:44, Devlin wrote:
> To me, this implies that it only adds it during guest sessions.  How about
something like AddComponentFromDir()?

Sure, done

[commit-bot: I haz the power, 2016-08-04 21:35:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 22:24:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-04 22:27:12]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-08-04 22:27:13]

Patchset 3 (id:??) landed as
https://crrev.com/18f89cf0b120afff95e064f437140bdbfb33ef36
Cr-Commit-Position: refs/heads/master@{#409916}

[David Tseng, 2016-08-09 15:02:44]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2229833002/ by dtseng@chromium.org.

The reason for reverting is: 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.
.